Community
Participate
Working Groups
When I use the compare editor with a Gerrit review, I have no button selection to navigate through the comments. 2 Buttons should be added to navigate through next/previous comments
Would you please review the code associated with the bug. https://git.eclipse.org/r/#/c/19327/
Hello, all comments were addressed. If you have any further comments, please look at patch set 3 (https://git.eclipse.org/r/#/c/19327/) Thanks Guy Perron
The comments were addressed more than a week ago and no second round of comments were raised since. Should we expect a push to the master branch soon for this new functionality ! The latest Gerrit review: (https://git.eclipse.org/r/#/c/19327/)
I do have the patch in my workspace. However, it needs some major adjustment for getting it to work. Main reason is that there are some reflection hacks in, which I'd like to centralize into a common place. Also, you are saving details in a static variable which is broken if you have more than two files open.
The reflection hack is centralized in the UIUtils.java class, where would be a better place ? Also, thanks for noticing the static variable. Is it best to fix now or add in the next delivery that will include the dialogs (423242) for editing comments ? Let me know Thanks
I updated the review with an alternate implementation that does not require UIUtils. No action necessary from your side. :)
This is very nice! One thing that I noticed while trying the feature was that the annotations weren't shown (i.e. the tooltip wasn't coming up automatically) and buttons were always enabled. It would be nice if the next/previous button were only enabled when the user can actually navigate.
Hi, I am baffled by the sudden shift with the Gerrit review (https://git.eclipse.org/r/#/c/19327/5) on this bug. After pushing, then addressing your comments and waiting approx. 10 days for the next round of inputs, now I see that the code has been swapped by an almost totalling different set of files. What I pushed was working and demoed last week in a meeting ! Based on comment #4 all that was left to do is fix a bug and a comment… which was almost immediately followed by your file swap ?! Is this the way to go ? It looks more to me like you are suddenly taking ownership. Please advise Thanks Guy
Guy, sorry for the confusion. As I mentioned in comment #6, I updated the review with an alternate implementation. The code you wrote initially was a good start and worked for your meeting. However, it had a few issues so I did some research on how to best approach the problem. I turns out that the "navigate to an annotation" problem has been solved previously by the Platform Compare team. Thus, I followed their approach. It's a bit different from the original concept but it still delivers the same functionality. The main benefits are that it uses well working code from Platform Compare and also re-uses the ReviewCompareAnnotationSupport which avoids any hacks in UIUtils. It's also a clearer architecture/code design to have the navigation centralized into the ReviewCompareAnnotationSupport class instead of being separated into different command handlers. FWIW, the only visible change in the UI from the original concept is the integration of the buttons into the "navigation" group in the toolbar. I thought this might be better because screen readers might use this grouping which makes it more accessible. However, I don't have strong opinions on this one. We can move it back into it's own group if you like. Again, sorry for the confusion and please don't take that as an offense. I did this while being on a train so I though having an alternate patch is a good idea for a broader discussion.
I think it's great that we got this functionality merged. It's very common that we iterate over changes and have multiple people involved. Sorry, if I merged this too quickly without waiting for your feedback. Please feel free to follow up with any additional changes that you would like see.
It is nice to have this functionality merged to master. It will be beneficial to all users. You proposed an alternative which is fine, but I think it would have been nice to notify GUY about it. Also, since Guy initiated the modification for this new feature, I think that GUY should have some credit about it, so it will help him to become a committer eventually to mylyn review.
Understood and I'm not claiming credibility for this one. Please also notice the "Also-by: Guy Perron <guy.perron@ericsson.com>" line in the commit comment which is an official Eclipse practice to mention co-authors. I also tried pushing the changed patch with Guy as the original author and myself in the "Also-by" line but Gerrit didn't allow that because I'm not a Mylyn Reviews committer myself. I'll open a bug to discuss this with the Eclipse Webmaster team.
MArking fixed.
FYI: I opened bug 424469 to discuss the upload issue with the Eclipse Webmaster team.
BTW, the owner of the review is still Guy Perron. Thus, he should be able to take full credibility in case of a committer nomination.
This is a great enhancement! There is one thing I'd like to see improved, though: * The icons look a bit pixelated. Could we change them to combine the standard arrow and glasses icons without resizing them? Right now they look like they were scaled up slightly. I can open a new bug for this if that's preferable.
Please open a new bug with the icons.
I've created 425218: improve next/previous comment icons https://bugs.eclipse.org/bugs/show_bug.cgi?id=425218
This feature works correctly if I initiate the compare by clicking "Compare With Base". If I initiate the compare by double clicking a file, however, the comment navigation icons are disabled and nothing I do within the compare editor causes them to become enabled. Is this a bug? User error? Thanks.
It works for me. Could you try refreshing the review? Actually, those buttons are never disabled for me, even when there are no comments. I have noticed the following though: 428038: next/previous comment buttons not shown on plugin.xml https://bugs.eclipse.org/bugs/show_bug.cgi?id=428038
