Community
Participate
Working Groups
This bug affects the proper display of the "primary" caret handled by the OS, when multiple carets are active in StyledText. The issue has been encountered on windows, other operating systems unknown. From https://git.eclipse.org/r/c/platform/eclipse.platform.text/+/191500/6/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/texteditor/multiselection/AbstractMultiSelectionHandler.java#301: The reason for the caret not being drawn in the proper place (and probably this might again only happen on windows) is to be found in Caret.setLocationInPixels(): void setLocationInPixels (int x, int y) { if (this.x == x && this.y == y) return; this.x = x; this.y = y; moved = true; if (isVisible && hasFocus ()) move (); } This method does an early return, if the caret hat has not moved. Thus the move()-method is not called, and that results in the OS.SetCaretPos()-method not to be called: void move () { moved = false; if (!OS.SetCaretPos (x, y)) return; resizeIME (); } The OS.SetCaretPos()-method seems to be the one responsible for displaying the blinking cursor. The workaround you don't like did it's trick because of a side effect: it resulted in a new Caret-object to be created, and that didn't have x + y initialized to the old position and resulted in the early return not happening. Three solutions come to mind: 1. Remove the early return -- thus resulting in the OS-caret to be positioned in any case. => easy solution that doesn't break the contract of the method. Unsure if any performance penalties might occur (which might only be the case if a lot of Carets are positioned over and over again, so it might not be a problem at all). 2. Provide some method or parameter to force the drawing on the OS-level. Could be the move()-method. ==> changes the contract, and might be considered as feature envy, as other classes have to take care of Carets business ... 3. Keep somehow track of the actual last set OS-CaretPosition and force an update (by calling move) if that differs from x/y. ==> could be done by either asking OS for the caret position (but no fitting method seems to exist), or having something like a static variable for former values of x/y. Feels brittle to me. I'd currently go for option (1). What do you think?
What I don't get is why would we need to even call setLocationInPixels on the primary caret if it didn't move? What is the actual call stack that triggers the main caret to disappear? How does it differ from the call stack of single caret mode?
(In reply to Mickael Istria from comment #1) > What I don't get is why would we need to even call setLocationInPixels on > the primary caret if it didn't move? What is the actual call stack that > triggers the main caret to disappear? How does it differ from the call stack > of single caret mode? Well ... maybe ask the guy that committed StyledText.setSelectionRanges and StyledText.setSelection(int[] regions, boolean sendEvent, boolean doBlock) in July 2021 :-P When selections change the carets need to update, as carets are tied to selections: if a selection exists a caret is either at the beginning or the end of the selection. That's why StyledText.setCaretLocations() gets called, and that is then propagated to org.eclipse.swt.widget.Caret.
PS: ... and the selection problem was why I put in the original workaround in https://git.eclipse.org/r/c/platform/eclipse.platform.text/+/191500: protected void selectRegions(IRegion[] regions) throws ExecutionException { setBlockSelectionMode(false); ISelection newSelection = new MultiTextSelection(document, regions); textEditor.getSelectionProvider().setSelection(newSelection); // FIXME by calling updateCaret in AbstractTextEditor.doSetSelection updateCaret(); } Adding/removing a multi selection in this case changes the whole selection, which in turn has effects on the carets, that need to be redrawn.
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/191799
(In reply to Mickael Istria from comment #1) > What I don't get is why would we need to even call setLocationInPixels on > the primary caret if it didn't move? What is the actual call stack that > triggers the main caret to disappear? How does it differ from the call stack > of single caret mode? I've giving it some more thought: your question is right to the point. The key problem (and the other problems we discussed in related bugs) probably is in how the Caret class is used. It seems to have been intended to represent only a single caret per widget, but StyledText uses it for multiple carets somehow. If that is true, then the use of an array of Caret[] is probably a use that doesn't fit to this intention. It would mean that Caret should only be used for the "primary" caret, that is directly linked to the Caret on the OS level. Thus we'd need a special class for secondary carets, that would do the drawing of these. I looked into the code, trying to figure out how the additional carets' drawing is actually accomplished, but I couldn't get my head around it. Might it be it somewhat piggybacks on the drawing of the primary OS caret? I compared Caret in the win32 and gtk version -- the gtk implementation seems to draw it's own carets "manually" for gtk < 3.22, and only for gtk >= 3.22 uses "native" gtk. What gtk version are you on, btw? I don't know how to proceed -- the proposed workaround seems to work on win32, yet a proper solution seems to be more like creating an independent StyledText.Caret class, that would either delegate calls to the OS-specific Caret-class for the primary caret, or do the drawing itself for secondary carets. Seems to be lots of work, and probably duplication of code ... Any ideas?
(In reply to Dirk Steinkamp from comment #5) > (In reply to Mickael Istria from comment #1) > > What I don't get is why would we need to even call setLocationInPixels on > > the primary caret if it didn't move? What is the actual call stack that > > triggers the main caret to disappear? How does it differ from the call stack > > of single caret mode? > > I've giving it some more thought: your question is right to the point. The > key problem (and the other problems we discussed in related bugs) probably > is in how the Caret class is used. It seems to have been intended to > represent only a single caret per widget, but StyledText uses it for > multiple carets somehow. If that is true It is true. > then the use of an array of > Caret[] is probably a use that doesn't fit to this intention. It would mean > that Caret should only be used for the "primary" caret, that is directly > linked to the Caret on the OS level. > Thus we'd need a special class for secondary carets, that would do the > drawing of these. Great idea. Some "VirtualCaret" then. The goal in using system's Caret was to leverage existing drawing capabilities only. > I looked into the code, trying to figure out how the additional carets' > drawing is actually accomplished, but I couldn't get my head around it. It's currently done by the way SWT works: when drawing a widget (StyledText here), it does trigger a redraw of all its children controls (Carets here) which have it as parent. So the redraw is implicit. But if it's better to detach additional carets from the widget tree and make their drawing explicit, then the "redraw" method can take care of redrawing exta stuff. > What gtk version are you on, btw? Looking at /proc/${pid}/maps: /usr/lib64/libgtk-3.so.0.2404.27 Then `rpm -qf /usr/lib64/libgtk-3.so.0.2404.27`: gtk3-3.24.31-2.fc35.x86_64 > I don't know how to proceed -- the proposed workaround seems to work on > win32, yet a proper solution seems to be more like creating an independent > StyledText.Caret class, that would either delegate calls to the OS-specific > Caret-class for the primary caret, or do the drawing itself for secondary > carets. Seems to be lots of work, and probably duplication of code ... If current solution is good enough and shows improvement on Windows and no regression on Linux, let's merge it.
(In reply to Mickael Istria from comment #6) > Great idea. Some "VirtualCaret" then. The goal in using system's Caret was > to leverage existing drawing capabilities only. We could go for something like that in some other iteration, it looks worthwhile yet also like work-for-a-while. Would you issue a seperate bug for that? ;-) > > I don't know how to proceed -- the proposed workaround seems to work on > > win32, yet a proper solution seems to be more like creating an independent > > StyledText.Caret class, that would either delegate calls to the OS-specific > > Caret-class for the primary caret, or do the drawing itself for secondary > > carets. Seems to be lots of work, and probably duplication of code ... > > If current solution is good enough and shows improvement on Windows and no > regression on Linux, let's merge it. As the current solution lives in "Eclipse SWT/win32" there should be no regression on Linux...
Gerrit change https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/191799 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=a248547b39ae2984789286005debded9186534f6
Verified on Win10 using Eclipse Build id: I20220407-0240
