Community
Participate
Working Groups
Annotations in source viewers are not painted, if they are at the end of the document. Steps to reproduce: 1. Open a java editor 2. Go to the end of document 3. Type '(' to activate linked mode -> at the end the exit marker is not painted Sometimes: 4. Type backspace to remove '()' -> exit marker is painted under the cursor (should no longer exists)
Created attachment 96431 [details] "overlaps" allows "touching" if length is 0
If it wasn't clear, my attached file, a patch against head, fixes the bug. Stephan Wahlbrink <stephan.wahlbrink@walware.de>
It looks as it fixes this bug but it changes the definition of org.eclipse.jface.text.Position.overlapsWith(int, int) for all clients of AnnotationPainter.overlapsWith(...), hence unexpected other bugs might be caused by this.
1. About severity: It is not important for the java editor, but it is really major issue in editors in which you often work at the end of file and in single line source viewers. In the later case, it is not only a problem at the end of the document, but on other position too. In use cases like the (), the exit marker is the only indicator of linked mode, and therefore not unimportant. 2. Unexpected side effect: It doesn't change, how annotation are painted, it changes only when annotations are painted. The behaviour is only changed, if one of the region has length 0 and is at the end of the other region. Without this change, a region with a length of 0, e.g. exit annotation will never painted/updated if it is a the end of the other region, the document end. The worst case of the patch could be, that a decorator is called twice to paint the annotation, but this is not a real problem.
It can be an issue if you have many annotations of length 0 that are at the end.
I don't understand, it's a feature, that annotations of a length 0 are not painted? Where do we have other annotations of length 0, your case could occurs ? One other note about the definition of the function: if both regions have length zero, it already returns true at moment. But if you want to minimize changes, it seems enough to change the first comparison in overlapsWith (the definition of the overlaps would become more unclear).
My point is: the overlaps(...) method is only copy of the one in Position. They have to be in sync.
Created attachment 96620 [details] Patch retaining overlapsWith() and fixing special cases directly If you want to retain the overlapsWith(), I added a new proposal. The patch retain the overlapsWith and adds special handling of the appropriate situation we have in the AnnotationPainter, so that annotations are also painted at the last visible offset, e.g. document end. As you can see, the overlapsWith of positions is not so fitting for the use case of AnnotationPainter, but perhaps, in this patch, the differences are more clear and exact and less irritating.
Created attachment 96622 [details] Patch allowing other to fix the bug If you absolutely don't want to fix it, so please let at least the other developers fix the bug on its own risk by applying the third proposal.
Created attachment 96647 [details] Patch consistently replacing overlapsWith() In AnnotationPainter, overlapsWith is used to check: 1) if model region is visible (model region ~ visible region) 2) line region to paint is in clipping region 3) annotation decoration is in clipping region In all situations it makes sense to include offsets at the bounds, because they are still visible and to paint. So, I think, this would be the logically more correct solution. The definition of Position#overlapsWith() makes sense, if you have consecutively regions. So you can check, which region contains the position, and an position of 0 at a bound of two regions belongs to the second region. In the use cases of AnnotationPainter, you don't have consecutively regions, but a single visible region or lines without line delimiter. You can rename overlapsWith to avoid confusion with the function of position. In theory, It also solves the situation if a annotation of length 0 is at the first offset of visible region.
Can you fix it in 3.5, please? The patch by Stephan looks correct and works fine for me.
ECF project would like to see this fix in 3.5 if at all possible, as it enables us to introduce much better support for real-time shared editing as per bug #238040
(In reply to comment #12) > ECF project would like to see this fix in 3.5 if at all possible, as it enables > us to introduce much better support for real-time shared editing as per bug > #238040 I believe Scott meant to say bug 237923.
The last patch is still valid for 3.5
>You can rename overlapsWith to avoid confusion with the function of position. Stephan, can you do this and add your copyright info to the header in the following form: name <e-mail> - bugzilla summary - bugzilla URL e.g. Dani Megert <dani@eclipse.org> - this is a bug - https://bugs.eclipse.org... After that I'll commit the patch to HEAD.
Created attachment 156506 [details] Patch (incl. renamed function, javadoc, copyright)
Stephan, the patch looks good except for the optimization to return fReusableRegion: the code can be called from multiple threads and hence one thread could alter the values returned to the other thread. Please provide another patch without that part.
Daniel, I can remove the code. But note, that the old version isn't thread-safe too and the method getWidgetRange can already return the fReusableRegion object through extension.modelRange2WidgetRange(fReusableRegion)[1]! I checked that (I wouldn't introduce such a new behaviour) and added the notice to the javadoc. Doesn't it run in the Display thread? Nevertheless, if you prefer it I change the return value to new Region(...) (the javadoc notice too?) [1] org.eclipse.jface.text.TextViewer#modelRange2WidgetRange(IRegion)
Stephan, you're right. I only reviewed the new code when I noticed that there's a small possibility that the code gets called in the non-UI thread, e.g. when a preference is changed in a non-UI thread. But of course the same issue is already present in code. I committed your patch to HEAD and filed bug 300080 for the potential issue we have. Available in builds >= N20100119-2000. Thanks!
.
*** Bug 140802 has been marked as a duplicate of this bug. ***
