Community
Participate
Working Groups
Currently, all artifacts are shown in list on current patch set. This is really awkward as because all common functions are located at bottom of patch set dialog, it requires user to scroll to bottom of potentially very long list. Especially now that we have Review Explorer, it would be good to be able to collapse these.
Rather than hide the patch set, I simply limited the maximum displayed size. That seems to work well.
https://git.eclipse.org/r/#/c/9512/
The reason the section doesn't have a limit for the vertical size is to avoid nested scroll bars. Usability is a bit odd though for very long patch sets and the buttons can be far down. I'm not a big fan of hard coding the limit to 30 though which doesn't work well for small screens. Have you considered calculating a dynamic size for the table based on the editor size similar to AbstractTaskEditorPart.MaximizePartAction does this?
(In reply to comment #3) > The reason the section doesn't have a limit for the vertical size is to avoid > nested scroll bars. Usability is a bit odd though for very long patch sets and > the buttons can be far down. Yeah, I really hate the current behavior -- for large patch sets it makes the review editor almost unusable. Seemed a compromise worth making even though the nested scroll bars have their own usability issues. > I'm not a big fan of hard coding the limit to 30 > though which doesn't work well for small screens. My thinking was that it would be better than the current design which would works even less well. > Have you considered > calculating a dynamic size for the table based on the editor size similar to > AbstractTaskEditorPart.MaximizePartAction does this? I'd considered doing something like that but thought the code would be too complex. I could take a look at that code but not sure when I'd get to it.
(In reply to comment #4) > > Have you considered > > calculating a dynamic size for the table based on the editor size similar to > > AbstractTaskEditorPart.MaximizePartAction does this? > > I'd considered doing something like that but thought the code would be too > complex. I could take a look at that code but not sure when I'd get to it. Okay. If that approach makes sense we should consider extracting the relevant code into a common utility. Let's leave this open. If we don't get to it we can always merge the current change.
Copying my (edited) review comment here: It doesn't really work to extract the code out -- the usage is just too different. Anyway, it looks like the same approach probably won't work here. It works perfectly for patch sets that aren't expanded, but for patch sets that are expanded at open time (i.e. the last patch set) it doesn't. This is because it requires a call to getManagedForm().getForm().getClientArea(), and apparently that value isn't initialized yet. Perhaps we could perhaps get the height/component in a more indirect way, but I don't see any super obvious way to do that. If we wait and do it after the form is fully layed out, we're going to have to reflow it. An alternative would be to get the entire editor size, but that arguably breaks encapsulation...
I just came across a review with a really large number of changed files (several plugins were moved to a different directory), and noticed that the performance of the Gerrit review editor is terrible in this case. When you scroll, the refresh rate gets really slow. Perhaps this would be fixed by limiting the size. Using the editor size may be the right thing to do. Could we pass that in to the patch set section somehow?
(In reply to comment #7) > I just came across a review with a really large number of changed files > (several plugins were moved to a different directory), and noticed that the > performance of the Gerrit review editor is terrible in this case. When you > scroll, the refresh rate gets really slow. Perhaps this would be fixed by > limiting the size. > > Using the editor size may be the right thing to do. Could we pass that in to > the patch set section somehow? We could, but then again I'm wondering about coupling -- perhaps I'm just being anal about that.
If the section allows clients to pass in the desired size, how does that create coupling?
(In reply to comment #9) > If the section allows clients to pass in the desired size, how does that > create coupling? You'd need to pass in an editor reference itself, because the size of the editor could change. :)
(In reply to comment #10) > (In reply to comment #9) > > If the section allows clients to pass in the desired size, how does that > > create coupling? > > You'd need to pass in an editor reference itself, because the size of the editor > could change. :) I wouldn't worry about that too much. We can add a maximize button to accommodate for dynamic changes of the size. Miles, would that help if we calculated the size outside of the section? I remember that we struggled with the problem described before and I don't think we came up with a good solution that worked for all cases (e.g. editor restore on workbench startup).
I merged the current version of the patch. It's already much better that way and we can iterate and improve later if we come up with a good solution for calculating section size dynamically. Thanks for the contribution, Miles!
(In reply to comment #12) > I merged the current version of the patch. It's already much better that way and > we can iterate and improve later if we come up with a good solution for > calculating section size dynamically. Thanks for the contribution, Miles! Thanks. I just played around w/ the eitor size idea a bit more, but couldn't get it to work very well. I also think this version isn't bad, but the arbitrary maximum size is a bit weird.
This is great Miles! It also solves the performance issue I mentioned.
I spoke too soon. :D When I submit I get an NPE: java.lang.NullPointerException at org.eclipse.mylyn.internal.gerrit.ui.editor.PatchSetSection.dispose(PatchSetSection.java:183) at org.eclipse.ui.forms.ManagedForm.dispose(ManagedForm.java:174) at org.eclipse.ui.forms.editor.FormPage.dispose(FormPage.java:178) at org.eclipse.mylyn.tasks.ui.editors.AbstractTaskEditorPage.dispose(AbstractTaskEditorPage.java:902) at org.eclipse.ui.forms.editor.FormEditor.dispose(FormEditor.java:403) at org.eclipse.ui.forms.editor.SharedHeaderFormEditor.dispose(SharedHeaderFormEditor.java:160) at org.eclipse.mylyn.tasks.ui.editors.TaskEditor.dispose(TaskEditor.java:519) at org.eclipse.ui.internal.WorkbenchPartReference.doDisposePart(WorkbenchPartReference.java:737) at org.eclipse.ui.internal.EditorReference.doDisposePart(EditorReference.java:327) at org.eclipse.ui.internal.WorkbenchPartReference.dispose(WorkbenchPartReference.java:684) at org.eclipse.ui.internal.WorkbenchPage.disposePart(WorkbenchPage.java:1810) at org.eclipse.ui.internal.WorkbenchPage.handleDeferredEvents(WorkbenchPage.java:1514) at org.eclipse.ui.internal.WorkbenchPage.deferUpdates(WorkbenchPage.java:1498) at org.eclipse.ui.internal.WorkbenchPage.closeEditors(WorkbenchPage.java:1472) at org.eclipse.ui.internal.WorkbenchPage.closeEditor(WorkbenchPage.java:1527) at org.eclipse.ui.internal.EditorPane.doHide(EditorPane.java:61) at org.eclipse.ui.internal.PartStack.close(PartStack.java:537) at org.eclipse.ui.internal.EditorStack.close(EditorStack.java:206) at org.eclipse.ui.internal.PartStack$1.close(PartStack.java:120) at org.eclipse.ui.internal.presentations.util.TabbedStackPresentation$1.handleEvent(TabbedStackPresentation.java:83) at org.eclipse.ui.internal.presentations.util.AbstractTabFolder.fireEvent(AbstractTabFolder.java:269) at org.eclipse.ui.internal.presentations.util.AbstractTabFolder.fireEvent(AbstractTabFolder.java:278) at org.eclipse.ui.internal.presentations.defaultpresentation.DefaultTabFolder.access$1(DefaultTabFolder.java:1) at org.eclipse.ui.internal.presentations.defaultpresentation.DefaultTabFolder$1.closeButtonPressed(DefaultTabFolder.java:71) at org.eclipse.ui.internal.presentations.PaneFolder.notifyCloseListeners(PaneFolder.java:631) at org.eclipse.ui.internal.presentations.PaneFolder$4.mouseUp(PaneFolder.java:230) at org.eclipse.swt.widgets.TypedListener.handleEvent(TypedListener.java:220) at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:84) at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1053) at org.eclipse.swt.widgets.Display.runDeferredEvents(Display.java:4169) at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3758) at org.eclipse.ui.internal.Workbench.runEventLoop(Workbench.java:2701) at org.eclipse.ui.internal.Workbench.runUI(Workbench.java:2665) at org.eclipse.ui.internal.Workbench.access$4(Workbench.java:2499) at org.eclipse.ui.internal.Workbench$7.run(Workbench.java:679) at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:332) at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:668) at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:149) at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:124) at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:196) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:110) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:79) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:353) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:180) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:601) at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:629) at org.eclipse.equinox.launcher.Main.basicRun(Main.java:584) at org.eclipse.equinox.launcher.Main.run(Main.java:1438) at org.eclipse.equinox.launcher.Main.main(Main.java:1414)
(In reply to comment #15) > I spoke too soon. :D > > When I submit I get an NPE: > > java.lang.NullPointerException > at > org.eclipse.mylyn.internal.gerrit.ui.editor.PatchSetSection.dispose(PatchSetSection.java:183) :( Hmm...are you on master? The exception doesn't match the line number I'm seeing. The common navigator just got merged (which handles patch sets differently anyway) -- can you run that and see if you get the same issue?
I am on master, but with some changes of my own on top which is why the line numbers are wrong. I did a pull right before I got submitted this. The problem is that labelProvider is null.
This was actually introduced by bug 334967. See bug 399691 for fix. Re-closing.
...
