[NETBEANS-4940] Workaround for caret-on-TAB drawing issue

[errael]

In ide/editor/lib/BaseCaret there's

newCaretBounds = c.getUI().modelToView(
        c, offset, Position.Bias.Forward);
// [TODO] Temporary fix - impl should remember real bounds computed by paintCustomCaret()
if (newCaretBounds != null) {
    newCaretBounds.width = Math.max(newCaretBounds.width, 2);
}

When offset is sitting on a TAB character, editor.lib2's TabView's modelToViewChecked(int offset, Shape alloc,... does

        mutableBounds.width = 1;
        return mutableBounds;

So the min of 2 applies and the caret drawing code gets a graphics context with clip width of 2.

This causes a drawing problem for the caret which can be ovserved in NetBeans. The problem may be exacerbated for plugins that provide their own caret. This fix provides a workaround that can be used by plugins until (if) the problem is addressed.

[jtulach]

Missing test, proper module versioning, entry in apichanges.xml, description in arch.xml with <api group="property" ... />. When previous requirements are satisfied, there is no need to give the property name starting with HACK. Alternatively you can define real Java type and call it - the Java signature API of editor.lib is open to extensions.

[matthiasblaesing]

Before we add new API/Hack (whatever you want to call it). Why not fix the underlying issue:

// [TODO] Temporary fix - impl should remember real bounds computed by paintCustomCaret()

I don't think that the temporary fix should be turned into a permanent hack.

[errael]

no need to give the property name starting with HACK

There's a phrase lipstick on a pig . Just because it's documented doesn't mean it's not a hack. But "HACK" is gone.

A test? The idea feels ludicrous. Are there many tests for paint code clip regions?

Other than that, I think the rest has been addressed.

[errael]

the underlying issue impl should remember real bounds computed by paintCustomCaret()

Who's to say this doesn't fix the underlying issue? In fact, rather than some hacky code to remember what paint did in the past, this solution allow the paint code to specify what it wants for the current character over which the caret is being painted. At least in my case the value is being provided by an ancient class I've got named "SwingPaintCaret" which is called via BaseCaret's painCustomCaret method.

If I knew how to get hold of Milos, I'd ask him if this addresses his concerns when he put in that comment (knowing he was breaking something) 10 years ago.

[matthiasblaesing]

I tried to reproduce the problem - it is only visible with the "normal" caret. Once I switch to "overwrite mode" (block caret), the update is correctly executed. What is the reason for this behavior?

[matthiasblaesing]

And there are deeper problems. In multi-caret mode + overwrite mode, the whole line blinks. (Edit: happens when one of the carets is placed at the end of line).

[errael]

multi-caret mode + overwrite mode

Where are these settings?

Multicaret mode is handled the EditorCaret class in editor.lib2, not sure about overwrite. The changes in this PR are to the older BaseCaret class in editor.lib.

[matthiasblaesing]

multi-caret mode + overwrite mode

Where are these settings?

I tested on Linux: Switching to multi-caret modes happens by choosing multiple positions by clicking on new positions while CTRL+Shift is held down. You can switch between insert and overwrite mode by pressing "Inset" key.

Multicaret mode is handled the EditorCaret class in editor.lib2, not sure about overwrite. The changes in this PR are to the older BaseCaret class in editor.lib.

My point is, that in multicaret mode to my eye the rendering of the bar cursor and the block cursor is ok. Maybe there is another hack in there, but it seems the problem is fixed there, could that be ported to single cursor mode? Adding new API means, that the API needs a story what happens when "fixes" are introduced.

[errael]

See point in #2482 (comment). This request for changes is to prevent this getting merged accidentally.

@matthiasblaesing While understanding that there may be further issues that may raised to prevent this change from being merged. Do you consider the point raised in https://github.com/apache/netbeans/pull/2482#issuecomment-716127012 still valid?

[matthiasblaesing]

My point is still: The issue seems to be fixed in multicaret mode - why does that not need additional api?

[errael]

In https://github.com/apache/netbeans/pull/2482#issuecomment-716767398 it says

in multicaret mode to my eye the rendering of the bar cursor and the block cursor is ok

I'm not seeing that; the problem exists in multi-caret mode.

Starting with a clean userdir/cachedir, start up NetBeans, in Editor>Formatting>AllLang,Tabs... turn off EnableIndent & ExpandTabtoSpaces, set NumSpacePerIndent to 8 (to match tab size). Then NewFile>Other>EmptyFile. I added a line full of regular chars, two lines of just TABs, then a line of regular chars. Place caret on a TAB in second line, add a second caret on regular char in last line. Hit the right arrow. Observe the rendering artifacts over the TAB.

I'm guessing that the editor.lib2 EditorCaret is being used by default. My plugin uses the old caret. The new caret does not permit a plugin to paint its own caret. That makes it unsuitable. My app draws different caret depending on state.

[matthiasblaesing]

What I'm missing is an analysis what is the underlying the problem and why it can only be solved by providing a new property, that we might need to support for a long time. I read through paintCustomCarent and indeed the caret size is not set there. This would be easier, if the code would even be excercised. In the IDE the BaseCaret#paint method is never hit, so it is difficult to see what is really the problem.

Is it right, that you provide a custom caret and override painCustomCarent?

Am I right that the problem you are seeing is caused by the repaint call in actionPerformed being invoked with a too small area?

[errael]

What I'm missing is an analysis what is the underlying the problem

See the first statement of this github PR thread; and dev list thread Re: API is ... was: https://github.com/apache/netbeans/pull/2482 has some historical information about the NetBeans evolution related to this issue. To summarize: ModelToView returns bounding box with width 1 for a tab (New Editor View Hierarchy introduced in 2010), the BaseCaret changes this to 2. This becomes the clip for drawing the caret in BaseCaret#update which ends with c.repaint(caretBounds).

and why it can only be solved by providing a new property,

Only? It was not my intention to say this PR was the only solution.

that we might need to support for a long time.

This change is marked in arch.xml as "devel" and it's in a "devel" module. No commitment/suggestion of long term support.

I read through paintCustomCarent and indeed the caret size is not set there. This would be easier, if the code would even be exercised. In the IDE the BaseCaret#paint method is never hit, so it is difficult to see what is really the problem.

I didn't realize until this week that BaseCaret was no longer used by the Standard IDE editors. But since you now know that the new (2015) EditorCaret exhibits the same problem, you could probably look there. There are probably other plugins around that use BaseCaret, but if you're really interested in seeing the code action, you could install jVi.

Is it right, that you provide a custom caret and override painCustomCarent?

Yes. And by this time it's too late, the clip is set.

Am I right that the problem you are seeing is caused by the repaint call in actionPerformed being invoked with a too small area?

repaint is called from a variety of places. The setting of the clip region is through BaseCaret#updateCaretBounds, which is primarily called through BaseCaret#update

[errael]

Recapitulation

The problem is that in BaseCaret the caretBounds, which becomes the clip region, is essentially set directly from modelToView without regard to the caret's drawing algorithms. This is usually OK since caret is typically drawn over a "normal" character and there's typically no need to draw outside the character bounds. However for the TAB, and probably for other invisible characters, modelToView provides unreasonable values (width 1).

I believe the caret drawing algorithm should have input into the required bounding box; relying on historical information from paintCustomCaret seems wrong. One possibility is to have a method, for example int getCaretMinWidth(...) or maybe Dimension getCaretSize(...) or ...

The idea of using a property gives considerable flexibility for experimenting with different solutions without having a hard signature-API and breaking client code at compile or runtime. The worst that happens is reverting back to poor choice of clip region.

Using an IntUnaryOperator seems simplest for now (considering the problem has existed for 10 years with BaseCaret and 5 years for EditorCaret). This could be changed in the future to require a Funtion which accepts an API defined input object parameter and returns a Dimension, or a Rectangle, or an Integer. If such a change were made, BaseCaret could continue to support the simple IntUnaryOperator or not (this is marked devel, it can change). All without an API-signature change. If in the fullness of time there's an accepted solution, a method could be added.

Other solutions could involve punting and giving "huge" clip regions. Having a way to get a value from paintCustomCaret could be designed, but it seems wrong and it's hard to see how that would work without major redesign; using an old value doesn't make sense particularly if variable fonts are used. There are undoubtedly other solutions.

[errael]

@jtulach

Missing test, proper module versioning, entry in apichanges.xml, description in arch.xml with <api group="property" ... />.

Are the requirements satisfied (independent of the content which is still under discussion)? And I don't understand what you mean by proper module versioning.

Alternatively you can define real Java type

As discussed in my previous post, Recapitulation, I don't think it's ready for this.

[matthiasblaesing]

Thank you for the summary. If this can be solved without an API change, I would prefer that. You excluded "Other solutions could involve punting and giving "huge" clip regions." from the solution set. How huge do you expect it to be? The block cursor repaints the width of one char, I tested a modification to the EditorCaret, that widens the width to a minimum of 3xheight. We are in 2020, I assume repainting an area of about 400square pixel, will not be measurable.

[errael]

You're suggesting

newCaretBounds.width = Math.max(newCaretBounds.width, 3 * newCaretBounds.height)

I don't have much experience with graphics, I always thought of the clip region as preventing rogue drawing; but it makes a lot of sense as more of a performance thing.

The current PR handles font change, guess this does too. The only problem I see is that it's not a default non-behavior change; not zero risk anymore, but pretty minimal. Every caret, whether over a zero/one width character or not, has this "huge" clip region.

What about

if (newCaretBounds.width < 2) {
    newCaretBounds.width = 3 * newCaretBounds.height;
}

I've seen editors (not in NetBeans) where the entire TAB is highlighted; with standard tab stops this change would work, but not in general.

So my questions are: @matthiasblaesing would you approve one of these changes? @lkishalmi would you include it? @jtulach any comment?

[errael]

Upon reflection, seems there is

1. provide protected int caretMinWidth(int docOffset). This is minimal could be a method or clientproperty as in this PR
2. just make the caret wider. Doesn't fix the problem, but in most cases the problem goes away.
3. provide protected void adjustCaretBounds(Rectangle caretBounds, int docOffset). The caret could be anything, like a circle around the character.

Note that 1 and 3 the associated JTextComponent is implicit and the default implementation of the method would be 2.

[errael]

Closing in the hope that any information contained herein is useful to someone other than myself.

[matthiasblaesing]

I researched a bit more and found, that on Linux with Open JDK 11 + 8, the width property of the returned rectangle from modelToView is fixed to 1 (checked full ASCII range), so the "fix" path is always taken.

My suggestion for a fix would be (untested!):

--- a/ide/editor.lib/src/org/netbeans/editor/BaseCaret.java
+++ b/ide/editor.lib/src/org/netbeans/editor/BaseCaret.java
@@ -399,6 +399,16 @@
                         }
                         newCaretBounds = c.getUI().modelToView(
                                 c, offset, Position.Bias.Forward);
+                        try {
+                            Rectangle nextCaretBounds = c.getUI().modelToView(
+                                c, offset, Position.Bias.Forward);
+                            if (nextCaretBounds.x > (newCaretBounds.x + newCaretBounds.width)) {
+                                newCaretBounds.width = nextCaretBounds.x - newCaretBounds.x;
+                            }
+                        } catch (BadLocationException ex) {
+                            // Should not happen, the last location the cursor can
+                            // reach is still before EOS
+                        }
                         // [TODO] Temporary fix - impl should remember real bounds computed by paintCustomCaret()
                         if (newCaretBounds != null) {
                             newCaretBounds.width = Math.max(newCaretBounds.width, 2);

This assumes, that the area between the left border of the current character and left border of the next character is fair game to draw into, nothing more. To my understanding this should fix the issue described here. Could you test it?

[errael]

I'm only seeing width 1 for TAB, not regular characters (regular chars seem to have font width, 8 in my case for 8x18 fixed width font). jdk1.8, NB repo. The jdk shouldn't matter, since the view is NB code. Surprisingly to me, NL returns width 8. Consider that if 1 was always returned width, the visual glitch would always be present.

For the proposal, do you mean offset+1 for nextCaretBounds? I had briefly considered something like this but it seemed out of bounds (no pun intended). Guess I had an aversion to calling the expensive modelToView twice; though it hardly matters given the caret blink rate. I agree that this should fix it.

In any event, I no longer use BaseCaret and the PR is closed.

[errael]

If the API change is still needed

Any third party module that wants control over caret drawing needs this (or something like this) to avoid visual glitches. The now standard EditorCaret doesn't have much (documented) flexibility. Since EditorCaret has the visual glitches, I'll explicitly mention that in the issue and leave the issue open.

My current workaround (maven module to build BaseCaret as MyBaseCaret, copy jars into library wrapper with same dependencies as maven module, Yenta in wrapper to fix up dependencies, use wrapper in module suite) is OK for now, but if/when NetBeans has a fix would probably use that for simplicity.

Updated PR on the way.

[JaroslavTulach]

This request for changes is to prevent this getting merged accidentally.

I acknowledge I have just seen this comment. It needs a resolution.

[matthiasblaesing]

@errael what is wrong with the "draw till next character" approach? The API addition be it necessary or not, will not fix the existing users. This is the code I used to see what Swing reports for modelToView:

public class JTextComponentWidth {

    public static void main(String[] args) throws BadLocationException {
    JFrame jframe = new JFrame("Test");
    jframe.setSize(1024, 768);
    jframe.setLayout(new BorderLayout());
    jframe.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
    JTextArea textArea = new JTextArea();
    textArea.setCaret(new DefaultCaret() {

    });
    Document doc = textArea.getDocument();
    doc.remove(0, doc.getLength());
    for (char i = 1; i < 128; i++) {
        doc.insertString(doc.getLength(), Character.toString(i), null);
    }
    jframe.add(new JScrollPane(textArea));
    jframe.setVisible(true);
    SwingUtilities.invokeLater(() -> {
        for (char i = 0; i <= doc.getLength(); i++) {
        try {
            Rectangle r1 = textArea.modelToView(i);
            Rectangle r2 = textArea.modelToView(i + 1);
            System.out.println(((int) i + 1) + ": " + r1 + " " + (r2.x - r1.x));
        } catch (BadLocationException ex) {
            Logger.getLogger(JTextComponentWidth.class.getName()).log(Level.SEVERE, null, ex);
        }
        }
    });
    SwingUtilities.invokeLater(() -> {
        try {
        System.out.println(doc.getLength());
        System.out.println(textArea.modelToView(doc.getLength() - 1));
        System.out.println(textArea.modelToView(doc.getLength()));
        System.out.println(textArea.modelToView(doc.getLength() + 1));
        } catch (BadLocationException ex) {
        Logger.getLogger(JTextComponentWidth.class.getName()).log(Level.SEVERE, null, ex);
        }
    });
    }
}

[errael]

@matthiasblaesing

what is wrong with the "draw till next character" approach?

Referring to that suggestion, I said "I agree that this should fix it", Comment in https://github.com/apache/netbeans/pull/2482#issuecomment-720126563 . Don't let me dissuade you from opening a PR to replace this one.I don't particularly like adding another call to modelToView, since it's typically not needed. But I do have a tendency to pre/over-optimize. Simply making it wider, without the second modelToView call, fixing the vast majority of cases and an API for those more adventurous souls is probably the right thing, option 3 in https://github.com/apache/netbeans/pull/2482#issuecomment-719949269

The API addition be it necessary or not, will not fix the existing users.

Recall that my goal with this PR was to sneak something into 12.2 that would be "zero risk", no change to current behavior; that ship has passed And I've said "If in the fullness of time there's an accepted solution..." in https://github.com/apache/netbeans/pull/2482#issuecomment-717312901

This is the code I used to see what Swing reports for modelToView

I've mentioned several times that NetBeans uses it's own ViewHierarchy. What Swing reports is not relevant to this discussion.

I polished up the PR so it was proper. I'm OK if it is not merged. I have a workaround. I'm planning to try some experiments with my caret (based on BaseCaret), like drawing a circle around the character and see if there are other problems. Just crazy stuff that wouldn't work by simply widening the bounding box. This is discussed in https://github.com/apache/netbeans/pull/2482#issuecomment-719949269

The caret drawing problem in the standard NetBeans caret EditorCaret, as discussed in this PR, are probably more important than this.

I'll add the EditorCaret notes to the issue right now, before I forget again.

[lkishalmi]

Well it is documented now. It would be fine with me to have it in 12.2 if rebased on delivery, without the minor version increase (So it should remain at spec.version.base=4.17.0) That version has not been released anyway, so when merging to master it won't cause a conflict.

[lkishalmi]

Also for some weird reason almost all CI tests are broken, those should be passing... (Or at least Travis)

[neilcsmith-net]

@lkishalmi this looks suspicious? - apache/netbeans/ide/editor.lib/apichanges.xml is not a valid XML document.

[errael]

(this is almost getting to be slapstick, I was so used to seeing failures... My bad) I see in nbbuild/javadoctools/apichanges.dtd

<!ELEMENT issue EMPTY>
<!ATTLIST issue
          number CDATA #REQUIRED
>

But I did <issue number="NETBEANS-4940"/>

Is this the problem? How should this be done?

I also did <author login="errael"/>. Is this supposed to be someone with permission to commit?

@lkishalmi I can rebase once I know about the issue.

[lkishalmi]

Well, just ask NetBeans to validate your xml (See the editor tab)

XML validation started.
Checking file:/home/lkishalmi/NetBeansProjects/netbeans/ide/editor.lib/apichanges.xml...
Referenced entity at "file:/home/lkishalmi/NetBeansProjects/netbeans/nbbuild/javadoctools/apichanges.dtd".
Attribute "day" with value "02" must have a value from the list "1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 ". [89] 
XML validation finished.

[errael]

Well, just ask NetBeans to validate your xml (See the editor tab)

Nice. I wonder if I ever used that before.

(adjusted paths to dtd in arch.xml for newer repo layout)

[errael]

I refreshed my local repository, rebased, and after pushing have picked up a bunch of other stuff.

[errael]

To wrap this up? Implemented option 3 from https://github.com/apache/netbeans/pull/2482#issuecomment-719949269 as an experiment. Have a circle as a caret. Just added screen shot in PR's issue.

[matthiasblaesing]

So the new API is already deprecated? If this is still experimental, I strongly suggest to revert the merge of this PR, evaluate both ideas and pursue only one.

[errael]

I gave three possibilities for the future around this issue https://github.com/apache/netbeans/pull/2482#issuecomment-719949269 . My favorite, option 3, I wasn't sure it would work in practice; wanted to report that It does seem to work. I have no plans to open another PR, no need to worry about this PR being deprecated any time soon (at least not by me).