Community
Participate
Working Groups
Tomek reports issues with fetching from reviews. See bug 411422 comment 6. Note that I'm not able to reproduce this, so not sure how to proceed here. Tomek suggests to maybe add some tracing to try to figure out what's happening here. Perhaps it surfaces on Linux more than Mac? >>Tomasz: On the review I said: "Not sure if this is supposed to be fixed by the patch but I still get an error saying "Cannot Fetch. Try re-synchronizing the review task. If that fails, there may be a problem with your repository connection." if I try to fetch a patch set while the review is being retrieved." And I'm still able to reproduce it. It doesn't happen every time, I guess I had to try 3 reviews to see it. Here are the steps: 1. Open a review in editor 2. Make sure a job retrieving review is running 3. Press Fetch If I close the error dialog and press Fetch again it works as expected.
(In reply to comment #0) > Perhaps it surfaces on Linux more than Mac? I'm sorry to disappoint you but I'm on Windows ;) Running of master (c603cfc349a6deed355ea6388166665d71c8fb96), I'm still get hit by this from time to time. Looking at the code I see that the error is logged in AbstractUiFactory.handleExecutionStateError() [1], so maybe adding some info about 'executable state' would help to trace this one down? Here is some info about tracing facility in Eclipse: * http://help.eclipse.org/kepler/index.jsp?topic=%2Forg.eclipse.pde.doc.user%2Fguide%2Ftools%2Flaunchers%2Ftracing.htm * http://wiki.eclipse.org/FAQ_How_do_I_use_the_platform_debug_tracing_facility%3F [1] https://git.eclipse.org/c/mylyn/org.eclipse.mylyn.reviews.git/tree/org.eclipse.mylyn.reviews.ui/src/org/eclipse/mylyn/reviews/ui/spi/factories/AbstractUiFactory.java#n87
In my experience with this stuff, we often end up w/ observer issues when trying to manually log EMF changes together with UI updates, and I'd prefer not to liter the code w/ tracing calls, but as a last resort we could try that. But before we go that far, could you reproduce under debugger and just set a break point in AbstractUiFactory.handleExecutionStateError()? We could do a pair excercise for this if you'd like. (I have a local Windows vm, but having all kinds of trouble getting my ssh keys to work properly, so naturally I can't test the one part -- fetch -- that I need to.)
(In reply to comment #2) > could you reproduce under debugger and just set a break > point in AbstractUiFactory.handleExecutionStateError()? Sure, I'll give it a try once I'm done with Gerrit 2.6 stuff.
(In reply to comment #3) > (In reply to comment #2) > > could you reproduce under debugger and just set a break > > point in AbstractUiFactory.handleExecutionStateError()? > > Sure, I'll give it a try once I'm done with Gerrit 2.6 stuff. Ok, I'll be gone for a week. If you aren't able to reproduce anymore, please go ahead and close so we can clear out 2.0.1 bugs. Or we can consider moving to 2.1 when we make release decision as I don't htinks should be a blcoker for that.
*** Bug 414464 has been marked as a duplicate of this bug. ***
(In reply to comment #2) > could you reproduce under debugger and just set a break > point in AbstractUiFactory.handleExecutionStateError()? Here is the stack trace when the breakpoint is hit: Thread [main] (Suspended (entry into method handleExecutionStateError in AbstractUiFactory)) FetchUiFactory(AbstractUiFactory<EObjectType>).handleExecutionStateError() line: 89 FetchUiFactory(AbstractUiFactory<EObjectType>).handleExecute() line: 83 AbstractUiFactory<EObjectType>.access$0(AbstractUiFactory) line: 79 AbstractUiFactory$1.widgetSelected(SelectionEvent) line: 72 TypedListener.handleEvent(Event) line: 248 EventTable.sendEvent(Event) line: 84 Button(Widget).sendEvent(Event) line: 1053 Display.runDeferredEvents() line: 4169 Display.readAndDispatch() line: 3758 Workbench.runEventLoop(Window$IExceptionHandler, Display) line: 2701 Workbench.runUI() line: 2665 Workbench.access$4(Workbench) line: 2499 Workbench$7.run() line: 679 Realm.runWithDefault(Realm, Runnable) line: 332 Workbench.createAndRunWorkbench(Display, WorkbenchAdvisor) line: 668 PlatformUI.createAndRunWorkbench(Display, WorkbenchAdvisor) line: 149 IDEApplication.start(IApplicationContext) line: 124 EclipseAppHandle.run(Object) line: 196 EclipseAppLauncher.runApplication(Object) line: 110 EclipseAppLauncher.start(Object) line: 79 EclipseStarter.run(Object) line: 353 EclipseStarter.run(String[], Runnable) line: 180 NativeMethodAccessorImpl.invoke0(Method, Object, Object[]) line: not available [native method] NativeMethodAccessorImpl.invoke(Object, Object[]) line: 57 DelegatingMethodAccessorImpl.invoke(Object, Object[]) line: 43 Method.invoke(Object, Object...) line: 606 Main.invokeFramework(String[], URL[]) line: 629 Main.basicRun(String[]) line: 584 Main.run(String[]) line: 1438 Main.main(String[]) line: 1414
What this means is that we're trying to execute something the we didn't know if we would be able to execute. ;) This would be consistent with the case where a) you have opened a review but the review hasn't actually been fully loaded yet, or fo some reason cannot be loaded, and then b) you try to execute any of actions on it. This state is unavoidable given the current UI constraints, as discussed in bug 410513, and much earlier when we discussed how to deal with the tricky issue of whether to present buttons when we didn't yet know what functions would be available. *** This bug has been marked as a duplicate of bug 410513 ***
Could at minimum instead of the error dialog something be displayed in the task editor? A dialog is always severe, and especially since we know it's going to happen in certain cases we shouldn't make it appear as if everything is broken. Instead, the message in the editor should be explanatory and helpful, stating what's happening and how to resolve ("try again").
(In reply to comment #8) > Could at minimum instead of the error dialog something be displayed in the > task editor? A dialog is always severe, and especially since we know it's > going to happen in certain cases we shouldn't make it appear as if > everything is broken. Instead, the message in the editor should be > explanatory and helpful, stating what's happening and how to resolve ("try > again"). +1. I like that idea. We already have warnings that end up in the Task Editor. We should come up with some decent language for that. But this should be a separate bug.
(In reply to comment #9) > We should come up with some decent language for that. But this > should be a separate bug. To be honest, I really don't care if it's going to be fixed as part of this bug or a separate one, but I don't like the way this is handled: * this bug (reported as a follow-up to bug 411422) has been marked as a dupe of 410513 * bug 410513 has been marked as invalid with a note that error msg is going to be clarified on bug 410433 * bug 410433 is about being offline, marked as minor and targeted for 2.1 For me it looks like the issue is not going to be fixed.
(In reply to comment #10) > For me it looks like the issue is not going to be fixed. It's not going to be fixed because it's not (in a strict sense) *fixable*. See my bug 410513 comment 5. And yeah, I'm not thrilled about it either.
(In reply to comment #10) been marked as invalid with a note that error msg is going to > be clarified on bug 410433 > * bug 410433 is about being offline, marked as minor and targeted for 2.1 That's not the right bug..which one did you mean?
Miles, he meant bug 410443. See bug 410513 comment 2. It seems reasonable to include clarifying this error message in that task but maybe some info about this case should be added to the task, and maybe we should consider it for 2.0.1.
(In reply to comment #13) > Miles, he meant bug 410443. See bug 410513 comment 2. It seems reasonable to > include clarifying this error message in that task but maybe some info about > this case should be added to the task, and maybe we should consider it for > 2.0.1. -1 on 2.0.1. This is a clear annooyance/source of confusion, but doesn't really prevent the tool from being used. At this point given that 2.6n support is now available, I feel like we should focus on the real blockers which to my mind means bug 413480.
Sure. But if this is an easy fix I would still consider it if we have time.
(In reply to comment #15) > Sure. But if this is an easy fix I would still consider it if we have time. It's almost certainly *not* an easy fix. :) It seems likely to involve (potentially risky) changes to the patch set update process or the model, or both.
Just to clarify, I'm only talking about improving the error message.
