Bugzilla – Full Text Bug Listing
| Summary: | [rulers] overview ruler annotation arming not correct | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Aaron Digulla <digulla> | ||||||||||||||||||||||
| Component: | Text | Assignee: | Markus Keller <markus.kell.r> | ||||||||||||||||||||||
| Status: | VERIFIED FIXED | QA Contact: | |||||||||||||||||||||||
| Severity: | normal | ||||||||||||||||||||||||
| Priority: | P2 | CC: | bogofilter+eclipse.org, daniel_megert, gabriele.garuglieri, markus.kell.r, noelgrandin, raksha.vasisht | ||||||||||||||||||||||
| Version: | 3.2.1 | ||||||||||||||||||||||||
| Target Milestone: | 3.7 M5 | ||||||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||||||
| OS: | All | ||||||||||||||||||||||||
| Whiteboard: | |||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||
|
Description
Aaron Digulla
There seems to be a bug in computing the activation area which should be the size of the annotation. Same bug can be seen using 3.1. Not an issue for me because I always hit the middle of the annotation with my high-precision laser mouse ;-)
The color can be configured under General > Editors > Text Editors > Annotations. It can also be found by entering "color" or "annotation" into the preference text search field.
>When several markers are below the mouse, show all
>messages
We already show all of them i.e. "Multiple markers at this line".
The idea with having a popup that allows to go directly to a marker if there's more than one is similar to what we offer in the vertical ruler when roll-over is enabled. I suggest to decouple this from here and write a feature request.
>The color can be configured under General > Editors > Text Editors > >Annotations. Ah. Well, I can't see much of an effect when I change the color for warnings. I guess the color which I can select is modified somehow before it is used. You can see this for yourself when you set the color to black. Only the rectangle at the top of the overview bar is now black; the icons in the vertical ruler are unaffected, the squiggles are grey, as are the warning boxes. I suggest that you take the selected color literally for the squiggles and the inside of the marks on the right. The outline of the marks should then be computed to be a darker hue of the inner color. >>When several markers are below the mouse, show all >>messages >We already show all of them i.e. "Multiple markers at this line". I meant the annotations. There is no message when they get pushed into each other. For example, when you have annotations on several consecutive lines and a long source, it's impossible to aim at one of them. I'll add a feature request for this. The popup for overlapping annotations is bug 163813. Created attachment 72050 [details] Screenshot (In reply to comment #1) > [..] Not an issue for me > because I always hit the middle of the annotation with my high-precision laser > mouse ;-) That statement indeed needs a ;-), since this screenshot shows that the middle of the annotation can already be outside of the responsive area (document with 2500 lines, annotation on line 150 => only 2 topmost pixels are responsive). *** Bug 40700 has been marked as a duplicate of this bug. *** *** Bug 34168 has been marked as a duplicate of this bug. *** This bug is now 1.5 years old. For me, this ought to be simple to fix but it wasn't fixed in 3.3. Is there a reason why you push it beyond 3.4 again? >Is there a reason why you push it beyond 3.4 again?
No, we've moved it for no reason. Feel free to provide that simple fix and we will reconsider.
In which plugin and class do you handle the mouse click on the vertical ruler? >In which plugin and class do you handle the mouse click on the vertical ruler?
I assume a typo: this bug is about the Overview ruler.
org.eclipse.jface.text.source.OverviewRuler (in org.eclipse.jface.text).
Can't wait to see the patch ;-)
Another use case of this:
1. paste:
public class foo {
// a comment
}
2. add bookmark on word "comment"
3. mouse hand is activated to early when coming from below (see attached picture)
Besides a bug in the logic this might be an int arithmetic issue.
Created attachment 143870 [details]
Picture with wrong arming
Created attachment 144937 [details]
Patch v01
Attaching Patch v01.
The patch goes into the right direction by removing the rounding errors. I would always switch to double and probably use Math.round(...) instead of casting to int which always floors the value. However, the patch adds code that reduces the hit area and that's not good because that method is not only used when hitting/hovering over the annotations but it is also used to show you the line number and go to a line when clicking in the ruler. Another test case is to use a big font (e.g. 48pt). The hit area should be the whole overview ruler area that corresponds to the line(s) that includes the y coordinate. Also, I'm not convinced that we need the +/-1 code (pixel init code) that's currently there. Created attachment 146934 [details] Patch v02 (In reply to comment #16) > The patch goes into the right direction by removing the rounding errors. I > would always switch to double and probably use Math.round(...) instead of > casting to int which always floors the value How will rounding improve the hit in our case? > However, the patch adds code that > reduces the hit area and that's not good because that method is not only used > when hitting/hovering over the annotations but it is also used to show you the > line number and go to a line when clicking in the ruler. Fixed in the patch v02 > Another test case is to use a big font (e.g. 48pt). Tested. Works > The hit area should be the whole overview ruler area that corresponds to the > line(s) that includes the y coordinate. Also, I'm not convinced that we need > the +/-1 code (pixel init code) that's currently there. This is required when the ruler length is very small than the actual number of lines (like in comment #5). >How will rounding improve the hit in our case?
If 0.8 is rounded to 1.0 instead of floored to 0.0 then it is closer to the original value, isn't it?
(In reply to comment #18) > >How will rounding improve the hit in our case? > If 0.8 is rounded to 1.0 instead of floored to 0.0 then it is closer to the > original value, isn't it? Yes, but that doesn't improve the hit when it is done for pixel0. And similarly when rounding is done for pixel1 and the value tends to be 0.4. If we want to improve the hit applying a similar logic then probably we need to floor the value of pixel0 and ceil the value of pixel1. I did try that, but the difference is hardly noticeable. Updating target Sorry for the delayed feedback, I was really busy. >Yes, but that doesn't improve the hit when it is done for pixel0. Right, I didn't try it - rounding just sounded better when reading the code. The hit area (change of mouse pointer to hand) detection now looks good but the hover is wrong in the case where the mouse is not over the rectangle: it shows the bookmark text instead the line number. Maybe I was not fully clear about this in comment 16 where I missed the line hover when using your first patch. We should - show the annotation hover when over the annotation and show the line number hover otherwise - go to the annotation when the mouse is clicked on the annotation and go to the line otherwise ping ;-) Note that the OverviewRuler code changed in HEAD. Prakash, can I expect a new patch this week for M3? (In reply to comment #23) > Prakash, can I expect a new patch this week for M3? Yes. I'll be working on this bug on Thursday Created attachment 150218 [details]
Patch v03
Attaching Patch v03
Created attachment 150219 [details]
mylyn/context/zip
Prakash, the latest patch does not touch the Overview ruler and hence cannot fix this bug. In addition the patch causes API tooling errors in my workspace because you've set the bundle version to 3.6.100 instead of 3.6.0. I suggest you enable API tooling ;-) The new API doesn't make sense to me. Created attachment 150488 [details] Patch v04 (In reply to comment #27) > Prakash, the latest patch does not touch the Overview ruler and hence cannot > fix this bug. Sorry. Patch v03 is in addition to Patch v02 (which fixes the location of the hand cursor) - not a replacement. Patch v04 combines both and fixes the manifest version also. > In addition the patch causes API tooling errors in my workspace > because you've set the bundle version to 3.6.100 instead of 3.6.0. I suggest > you enable API tooling ;-) I have enabled it. When it showed an error, I changed it in the Overview page of editor manually without realizing there is Quick Fix. (it correctly fixes the version) > The new API doesn't make sense to me. OverviewRulerHoverManager computes the tool tip for a given location. It first converts the cursor location to line number and consults the IAnnotationHover for that line number. IAnnotationHover returns the tooltip, based on whether annotation is available in the line or not. I thought the new API is the right way to find the tooltip on a line, ignoring the annotations in the line. If you have better alternatives, please suggest me. I have tested Patch v04 but the problem described in comment 13 is still present as shown in attachment 143870 [details] :-(. The computation is all done in the DefaultAnnotationHover, so I really can't see any benefit in the new API. >I have tested Patch v04 but the problem described in comment 13 is still >present as shown in attachment 143870 [details] [diff] :-(. This is not 100% correct: the arming is now OK, but the hover and the navigation is still wrong (as mentioned in comment 21). Prakash, I'm reassigning this to Markus because he's currently reworking the Overview ruler with regards to the scroll bar's thumb size. Created attachment 186620 [details]
Fix annotation drawing
Does not yet fix the hover manager and mouse clicks/arming.
Created attachment 186622 [details]
Fix Annotation drawing 2
Eclipse patch creation wizard is broken...
Created attachment 186755 [details]
Drawing and arming
*** Bug 295091 has been marked as a duplicate of this bug. *** Fixed in HEAD of OverviewRuler for source viewers. Compare viewers will be tackled with bug 294454. I have tested fix but the problem described in comment 13 is still present as shown in attachment 143870 [details] :-(. Sample snippet: ------ %< ------ public class foo { // a comment } ------ %< ------ (In reply to comment #37) This only happened in cases where a line was mapped to an area taller than 5px in the overview ruler. In that situation, some pixels before and after the drawn rectangle belong to the same line, so I thought it would be nice to expand the hit area for free. But after testing it a bit more, I now agree that the arming (using the hand cursor) should be aligned with the area of the rectangle. In HEAD, I still use the whole mapped area as click target for the annotation, but I've fixed the cursor. Committed to OverviewRuler with a few other fixes. Verified for 3.7 M5 with I20110124-1800. Filed bug 335282 : overview ruler annotation arming not correct with vertical scroll bars |