Community
Participate
Working Groups
* open a review with no connectivity to the server * open the review explorer * push refresh button in review explorer ** an error dialog is displayed (NPE), and a message about the connection error is displayed next to each patch set in the task editor * click compare with base ** get another NPE [1] * close review explorer ** get IllegalStateException [2] fn1. java.lang.NullPointerException at org.eclipse.mylyn.internal.gerrit.core.remote.PatchSetContentRemoteFactory.pull(PatchSetContentRemoteFactory.java:63) at org.eclipse.mylyn.internal.gerrit.core.remote.PatchSetContentCompareRemoteFactory.pull(PatchSetContentCompareRemoteFactory.java:38) at org.eclipse.mylyn.internal.gerrit.core.remote.PatchSetContentCompareRemoteFactory.pull(PatchSetContentCompareRemoteFactory.java:1) fn2. java.lang.IllegalStateException: Need an underlying widget to be able to set the input.(Has the widget been disposed?) at org.eclipse.jface.viewers.StructuredViewer.setInput(StructuredViewer.java:1681) at org.eclipse.mylyn.internal.reviews.ui.views.ReviewExplorer.update(ReviewExplorer.java:478) at org.eclipse.mylyn.internal.reviews.ui.views.ReviewExplorer.setReview(ReviewExplorer.java:535) at org.eclipse.mylyn.internal.reviews.ui.views.ReviewExplorer$2.partActivated(ReviewExplorer.java:172) at org.eclipse.ui.internal.PartListenerList$1.run(PartListenerList.java:72)
Created attachment 232765 [details] errors in task editor I wonder if it is right to show the same message on each patch set instead of using the header of the task editor. Also, this message is displayed even when the patch set contents are available.
I should mention that I got these errors running on 376dae4330d84b7d28a26d1b9e93e16ba6cb5206.
Created attachment 232766 [details] blank task editor After restoring connectivity, when I refresh the review, I get the following error about 9 times, and the refresh operation never completes. After stopping the operation, the attributes and summary in the task editor are blank. Caused by: org.eclipse.swt.SWTException: Widget is disposed at org.eclipse.swt.SWT.error(SWT.java:4361) at org.eclipse.swt.SWT.error(SWT.java:4276) at org.eclipse.swt.SWT.error(SWT.java:4247) at org.eclipse.swt.widgets.Widget.error(Widget.java:468) at org.eclipse.swt.widgets.Widget.checkWidget(Widget.java:340) at org.eclipse.swt.widgets.Tree.getItems(Tree.java:3251) at org.eclipse.mylyn.internal.reviews.ui.views.ReviewExplorer$RemoteItemSetContentObserver.updated(ReviewExplorer.java:128) at org.eclipse.mylyn.internal.reviews.ui.views.ReviewExplorer$RemoteItemSetContentObserver.updated(ReviewExplorer.java:1) at org.eclipse.mylyn.reviews.core.spi.remote.emf.RemoteEmfConsumer$ConsumerAdapter.notifyChanged(RemoteEmfConsumer.java:92) at org.eclipse.emf.common.notify.impl.BasicNotifierImpl.eNotify(BasicNotifierImpl.java:374)
Closing the review editor and refreshing the review in the task list restores the task data, but also triggers about a dozen jobs that all say they are retrieving the latest patch set.
It's important that errors are handled gracefully when offline. Particularly the widget disposed error doesn't look right. I think we still have another bug open to track general improvements to error presentation.
Sam, the change you've been using is almost a week old. ;) Since then I know that we've already fixed at least one of those issues. Can you please run on either master or https://git.eclipse.org/r/#/c/13998/7 and verify that you still see those issues.
(In reply to comment #1) > Created attachment 232765 [details] > errors in task editor > > I wonder if it is right to show the same message on each patch set instead of > using the header of the task editor. Yeah, I'm not sure. Steffen made a similar suggestion. But note that it is possible that you might be able to retrieve patch set but not another. This is important because you could see an older patch set if it had been opened at some point in the past. The reason you're getting so many notices is that those had already been retrieved before. In typical usage where all patch sets are not retrieved, that would not be the case. (You only get the error on demand.)
(In reply to comment #4) > Closing the review editor and refreshing the review in the task list restores > the task data, but also triggers about a dozen jobs that all say they are > retrieving the latest patch set. Are you sure they're the same job, or is it a job for each available patch set? If the latter, I think I have an idea of what is going on here. Investigating.
(In reply to comment #0) > * click compare with base > ** get another NPE [1] bug 411708
(In reply to comment #0) > * close review explorer > ** get IllegalStateException [2] Already fixed in bug 410541.
(In reply to comment #5) > It's important that errors are handled gracefully when offline. Particularly the > widget disposed error doesn't look right. Already fixed. Note that these issues all seem to be for the case where connection did exist when the review was open at some point, and *then* connectivity was lost. I'm not able to reproduce bug 411708 in the case where the was no connectivity before editor was opened. > I think we still have another bug open > to track general improvements to error presentation. Yes, that's bug bug 4110443 (scheduled for 2.1)
(In reply to comment #8) > (In reply to comment #4) > > Closing the review editor and refreshing the review in the task list restores > > the task data, but also triggers about a dozen jobs that all say they are > > retrieving the latest patch set. > > Are you sure they're the same job, or is it a job for each available patch set? > If the latter, I think I have an idea of what is going on here. Investigating. They all said "patch set 6"
(In reply to comment #12) > They all said "patch set 6" OK -- based on that and your previous comment 3 about exception appearing 9 times, I think the issue here is that because of connectivity loss, we aren't properly disposing existing review content set observers on a network failure.
(In reply to comment #13) > (In reply to comment #12) > > They all said "patch set 6" > > OK -- based on that and your previous comment 3 about exception appearing 9 > times, I think the issue here is that because of connectivity loss, we aren't > properly disposing existing review content set observers on a network failure. I'm pretty sure now that all of the bad patch set behaviour happened as a result of the IllegalStateException, but I can't say for sure. In any case, I'm not able to reproduce it. Please let me know if you still see it with the latest changes.
(In reply to comment #13) > (In reply to comment #12) > > They all said "patch set 6" > > OK -- based on that and your previous comment 3 about exception appearing 9 > times, I think the issue here is that because of connectivity loss, we aren't > properly disposing existing review content set observers on a network failure. This should also be fixed by https://git.eclipse.org/r/#/c/13998/11. Tomek, can you verify?
I'll mark this as resolved to get a better sense what's still left to do. Tomek, please reopen if Miles' fix doesn't address the problem.
