Community
Participate
Working Groups
When clicking https://gerrit.example.com/#/c/411/3 patch set 3 should be expanded.
You mean *instead* of opening the relevant web page?
If you have the connector installed and the repository set up, it doesn't open a web page, it opens the review editor. This bug is about also expanding the patch set.
Thanks for the clarification. I had actually clicked on your example review url above. ;)
Proposed solution: https://git.eclipse.org/r/#/c/10057/
This is a great suggestion. Miles, the forms API provides FormPage.selectReveal() for this purpose. You can take a look at TaskEditorAttachmentPart for instance to see an example implementation.
(In reply to comment #5) > This is a great suggestion. Miles, the forms API provides > FormPage.selectReveal() for this purpose. You can take a look at > TaskEditorAttachmentPart for instance to see an example implementation. Thanks for the reference. Looks like TaskEditorAttachmentPart uses it's own implementation. It's also using a CommonFormUtil#setExpanded hack because (as I also discovered) expandable composite doesn't seem to provide a good way to programmatically alter the expansion state *and* fire the notification, which the PatchSetSection relies on to lazily create the contents. I went with another approach which was to simply call the same code when we expanded programmatically. I missed the "selection" part of the reveal. I'm going to add that. Otherwise, do you see any problem w/ the way it is implemented as is?
(In reply to comment #6) > (In reply to comment #5) > > This is a great suggestion. Miles, the forms API provides > > FormPage.selectReveal() for this purpose. You can take a look at > > TaskEditorAttachmentPart for instance to see an example implementation. > > Thanks for the reference. Looks like TaskEditorAttachmentPart uses it's own > implementation. It's also using a CommonFormUtil#setExpanded hack because (as I > also discovered) expandable composite doesn't seem to provide a good way to > programmatically alter the expansion state *and* fire the notification, which > the PatchSetSection relies on to lazily create the contents. Yes, I would prefer if we used the same approach consistently. > I missed the "selection" part of the reveal. I'm going to add that. Otherwise, > do you see any problem w/ the way it is implemented as is? I am not sure we need the section manager abstraction. I would prefer if the section simply expanded itself.
(In reply to comment #7) > I am not sure we need the section manager abstraction. I would prefer if the > section simply expanded itself. Yep. By using the CommonFormUtil hack that forces the toggle notification, we're able to do away with all of that. Please see https://git.eclipse.org/r/#/c/10057/4.
It looks like the code doesn't use FormPage.selectReveal(). Can you comment why you decided against using that?
(In reply to comment #9) > It looks like the code doesn't use FormPage.selectReveal(). Can you comment why > you decided against using that? Tried it, but it didn't work. I don't think setting the input is the appropriate thing to do here. Then I looked at the pattern in AbstractTaskEditorAttributeSection#selectReveal that used CommonFormUtil.setExpanded(getSection(), true); it worked perfectly, so I went with that. I didn't use the EditorUtil.reveal(..) part of that because there are some unneccesary calls and the scroll to sport was off -- not sure why, but I'm thinking maybe because we're actually revealing a sub section.. (?) I want to change the name of the method to reveal at Sam's suggestion but I'll hold on that to see if there are any other changes. I'm also going to check if the patch set is already expanded.
Okay please see latest review. Steffen, there are some additional notes on why I chose the reveal strategy I did.
(In reply to comment #11) > Okay please see latest review. Steffen, there are some additional notes on why I > chose the reveal strategy I did. Okay, sounds reasonable. I have rebased the change against the latest master. Under 4.3 the scrolling to the section didn't work but this might be a bug in 4.3 which is behaving in funny ways with the editors (haven't verified on other versions). Let me know if it works for you and we can merge.
(In reply to comment #12) > Okay, sounds reasonable. I have rebased the change against the latest master. > Under 4.3 the scrolling to the section didn't work but this might be a bug in > 4.3 which is behaving in funny ways with the editors (haven't verified on other > versions). Let me know if it works for you and we can merge. It works fine here, but I'm running 3.8. :) I can get a 4.2/4.3 setup going if you think that makes sense. I'm also wondering if it might be a Linux thing, in which case we obviously need to address it. Do you see it on earlier versions? I'm also getting an annoying thing happening. Originally, I had the scroll to scroll the section such that it was at the top of the view port, but this code just scrolls the minimum extent needed to reveal. The scroll to works fine initially - it scrolls up to uncover the complete patch set, but then after the patch set is actually downloaded the controls look like they are layed out again, adding an additional 10pxs or so. That then puts the button bottoms slightly below the view port, which is kind of ugly. I'm not quite ready to merge yet in any case. I want to add a bit of test coverage for the URL handling.
(In reply to comment #13) > I'm not quite ready to merge yet in any case. I want to add a bit of test > coverage for the URL handling. I've taken care of that. Note that I have put a small fix into the connector getTaskUrl code as well. I hope it's okay to put that into this patch, but it is neccessary to make the tests work properly.
The change was merged into master. Thanks very much for the contribution!
