Community
Participate
Working Groups
I20090407-1430. Add a command that allows to show the hover that we currently show when hovering over the vertical ruler icon. This hover already contains all annotations if there's more than one.
Marking as potential 3.6 candidate.
Should behave and be named similar to the "Show Quick Diff Ruler Tooltip" command, see AbstractDecoratedTextEditor#showChangeRulerInformation().
Created attachment 157764 [details] fix
I tried it out and it doesn't seem to work: 0. start with new workspace 1. assign a key binding to the command 2. paste the following snippet into the Package Explorer: public class p { öö } 3. place caret between the two öö 4. press the key binding ==> no hover ==> looks like a bug in showRulerAnnotationInformation() which got triggered A quick review showed that the general direction of the patch is OK. Some minor issues: - use "Tooltip" and not "Hover" in the command name - typo: "Annotaiton" - don't you work with spell checking enabled? ;-) Deepak, please investigate and provide a new patch.
(In reply to comment #4) > I tried it out and it doesn't seem to work: > > 0. start with new workspace > 1. assign a key binding to the command > 2. paste the following snippet into the Package Explorer: > public class p { > öö > } > 3. place caret between the two öö > 4. press the key binding > ==> no hover > ==> looks like a bug in showRulerAnnotationInformation() which got triggered This is strange, a hover does come up for me. Do you have 'Java -> Editor -> Hovers -> Expand vertical ruler icons upon hovering' preference enabled? If this is not enabled then the hover will not come up. The hover comes up, but I dont know how to navigate through icons in it. I realized this after Raksha discussed bug 78522 :). I am looking into this part. > - typo: "Annotaiton" - don't you work with spell checking enabled? ;-) The spell checker is enabled, however it marks 'tooltip', 'bookmark' etc also as incorrect which leads me to ignore it a bit. Will be more careful in future :).
>This is strange, a hover does come up for me. Do you have 'Java -> Editor -> >Hovers -> Expand vertical ruler icons upon hovering' preference enabled? If >this is not enabled then the hover will not come up. The command must also show the same hover as when I hover over the icon in the vertical ruler when that option is off.
Created attachment 158965 [details] reworked patch Now appropriate hover comes up depending on the preference 'Java -> Editor -> Hovers -> Expand vertical ruler icons upon hovering'.
I notice now that although my last patch works the newly introduced method is not good, it has a unused parameter warning :( .
Created attachment 158969 [details] fix
Created attachment 159013 [details] fix Even the last patch was no good.. sigh.. there were API compatibility issues. (In reply to comment #5) > The hover comes up, but I dont know how to navigate through icons in it. As per discussion with Markus over sametime, we may not want to invest effort to make the hover accessible at this point. Dani, what is your opinion on this? Minor issue found and fixed: Javadoc of org.eclipse.jface.text.source.SourceViewer.getCurrentAnnotationHover() was incorrect
> > The hover comes up, but I dont know how to navigate through icons in it. > As per discussion with Markus over sametime, we may not want to invest effort > to make the hover accessible at this point. Dani, what is your opinion on this? I've discussed this with Dani, and we agreed that always showing and giving focus to the summary hover is good enough (regardless of the expand hover preference). In AbstractDecoratedTextEditor, please try to reduce the code duplication between showChangeRulerInformation() and showRulerAnnotationInformation().
Created attachment 159294 [details] fix Created 2 methods getCaretOffset and showFocusedRulerHover to reduce code duplication.
(In reply to comment #12) > Created an attachment (id=159294) [details] [diff] This patch still doesn't work correctly when the "Expand vertical ruler icons..." preference is set. In that case, the command should still show the summary hover (and not show and focus the expansion hover). In AbstractDecoratedTextEditor#showRulerAnnotationInformation() you do a hard cast to SourceViewer. If you still need ISourceViewerExtension5#getAnnotationHoverForKeyboardAction() you should add an instanceof test and cast to the interface. > Created 2 methods getCaretOffset and showFocusedRulerHover to reduce code > duplication. The Javadoc of getCaretOffset() is wrong, and the method does much more than its name suggests. You could try to find a better name, but I would suggest a different approach: Add a method AbstractDecoratedTextEditor#showRulerInformation(boolean changeHover), call it from the show*RulerInformation() methods, and take the annotation hover when changeHover is false.
Created attachment 159881 [details] reworked patch
(In reply to comment #14) > Created an attachment (id=159881) [details] [diff] Sorry, that's not much better. Now, I get a hover that is focused but not resizeable/scrollable, and as soon as I move the mouse, the hover disappears. Furthermore, we again have code duplications in the AbstractDecoratedTextEditor, and new duplications in AnnotationBarHoverManager#showAnnotationHoverForKeyboardAction(). The "keyboard mode" in AbstractHoverInformationControlManager and AnnotationBarHoverManager is not necessary. We already have FocusedInformationPresenter which does that (but correctly). Please go back to the fix from comment 12 and continue from there. To find out where you need to change something to make the expansion-hover work, I would check where that preference is currently used. When I open a call hierarchy on PreferenceConstants.EDITOR_ANNOTATION_ROLL_OVER, I see that JavaEditor#createAnnotationRulerColumn(CompositeRuler) sets a custom hover. So we just have to make sure we don't use that hover. Open the next call hierarchy on AnnotationRulerColumn.fHover and find out that AnnotationBarHoverManager#getHover(MouseEvent) gets the hover. So the trick is to just use AnnotationBarHoverManager#getAnnotationHover() and disregard IVerticalRulerInfoExtension etc. . Make the getter in the manager public and rename the method in ISourceViewerExtension5 to getAnnotationHover().
Created attachment 160032 [details] fix I had interpreted the terms 'summary hover' and 'expansion hover' incorrectly. Got the correct meaning after discussion with Markus over sametime. With the latest patch 'Summary hover' (hover with textual description) is shown in both cases. The 'expansion hover' (hover with icons) is not shown at all.
That's nice, thanks. Released to HEAD. > I had interpreted the terms 'summary hover' and 'expansion hover' incorrectly. Sorry, I could also have spelled this out more clearly. Knowing about that misunderstanding, your patch from comment 15 now makes more sense to me ;-)
Small error in Javadoc for org.eclipse.jface.text.source.AnnotationBarHoverManager.getAnnotationHover() ' * @since 3.6 public, was protected since 3.6'. It should be ' * @since 3.6 public, was protected since 2.1'
Thanks for checking again, fixed the Javadoc in HEAD.
Created attachment 160185 [details] simplified solution Dani revealed that there's a much easier way for the AbstractDecoratedTextEditor to get hold of the annotation hover: Just get it from the source viewer configuration. This has the additional advantage that it does not reuse the annotation hover instance from the AnnotationBarHoverManager, which could also be used concurrently in a background thread. I've changed that in AbstractDecoratedTextEditor#showRulerInformation(boolean) and removed the unnecessary ISourceViewerExtension5 again. By they way: If we had kept the extension interface, we would also have had to mention it in the Javadoc of ISourceViewer. When looking at #showRulerInformation(..) again, I realized that the new action does't work when the line number ruler and the quick diff ruler are not visible. I've removed the unnecessary checks for the new action. Released to HEAD.
Created attachment 161266 [details] Documentation Added a blurb to platform.doc.user plug-in describing the new command.
Created attachment 161267 [details] Documentation Last patch had incorrect command name.
(In reply to comment #22) > Created an attachment (id=161267) [details] [diff] Thanks, released to HEAD.
