#1924 Wrong cursor position when selecting text with block caret

[Semyon]

I'm using Geany text editor, which is Scintilla-based. Recently text selection behavior has changed, when using themes with block-style caret.

Selecting a text with shift+arrow or shift+home/end leaves the last symbol under the cursor selected. For example, pressing shift+right arrow leaves the cursor at the same position, while the symbol under the cursor gets selected. Expected behavior is both selecting the current symbol and moving the cursor.

Unfortunately, I can't test this in SciTE, as there is no support for block caret.

[Neil Hodgson]

This change was made deliberately. It was discussed on the mailing list.
The change was implemented by [d29f1d].

[Semyon]

Sorry to hear that. Hope there will be an option to bring the old behavior back. The new one is not apparent for me at all.

[Alberto González Palomo]

Hi, I've read the mailing list discussion and the commit. I see the point.

However, I think it might be good to reconsider. I'm sorry I didn't participate in the discussion at the time but I've only recently started using a text editor affected by this.

The entry in ScintillaHistory.html says:
"Display block caret over the character at the end of a selection to be similar to other editors."

Using the plural seems inaccurate: I've been testing several common editors and there is only one that implements that behaviour (apart from newer Scintilla-based editors, of course).

That one editor is GVim, but please note that Vim works otherwise and matches all other editors I've tested so far.

Editors with selection extending to right edge of block cursor when extending rightwards:
- Scintilla post-[d29f1d], 2017-01-22
- GVim

Editors with selection extending to left edge of block cursor when extending rightwards:
- Vim: Esc,v (first Esc, then "v") to enter visual mode, then extend the selection with the cursor keys or "h"/"l".
- Emacs (both GUI and console)
- Visual Studio Code: { "editor.cursorStyle": "block" }
- mcedit (part of Midnight Commander)
- Joe: Ctrl-k,b to start marking, move with cursor keys, Ctrl-k,k to mark end.
- Nano: Ctrl,Ctrl or Alt-A to start marking, move with cursor keys, Ctrl-k cuts text.
- Komodo Edit 10.2 (current), uses Scintilla but maybe an older or patched version.

This is a big issue for me because the Scintilla-based editor I've started using is Textadept which does have a console mode, and in that mode the only cursor style available is "block". Otherwise I would be able to ignore the block behaviour.

I propose making the new behaviour configurable under something like "CARETSTYLE_GVIM", with the old behaviour as default.

If this would be acceptable, I'm ready to spend some time writing the code and submitting a patch.
I've also submitted a subscription request to scintilla-interest in case you think we should discuss it there.

Cheers,
Alberto González Palomo https://sentido-labs.com

[Semyon]

It would be really great if you could do this. I prefer the block caret and that change is annoying for me.

[Neil Hodgson]

An option would be sensible. If added to CARETSTYLE_ it should be a separate bit, probably with value 4 in case it needs to be combined with other options in the future.

CARETSTYLE_GVIM doesn't seem very explanatory and something that mentioned the placing of the block caret would make more sense. The default should be the behaviour of Scintilla 4.0.

[Alberto González Palomo]

I'm working on the code and it seems quite straightforward.
To test it I set the caret style in "gtk/SciTEGTK.cxx" at the end of SciTEGTK::CreateUI(), right before "UIAvailable()": wEditor.Call(SCI_SETCARETSTYLE, CARETSTYLE_BLOCK);

Adding CARETSTYLE_BLOCKEND is simple, but it would require duplicating each check for block-style carets like this one:

if ((vsDraw.caretStyle == CARETSTYLE_BLOCK) || imeCaretBlockOverride) { ... }

If we defined CARETSTYLE_BLOCKEND as 4, each such comparison would have to be exteded as:

if ((vsDraw.caretStyle == CARETSTYLE_BLOCK || vsDraw.caretStyle == CARETSTYLE_BLOCKEND) || imeCaretBlockOverride) { ... }

If instead we split that into CARETSTYLE_END as 4 and then CARETSTYLE_BLOCKEND as CARETSTYLE_BLOCK | CARETSTYLE_END (that is 6), we just need to replace the == operator with binary &:

if ((vsDraw.caretStyle & CARETSTYLE_BLOCK) || imeCaretBlockOverride) { ... }

There are 4 such lines, the first one is from [d29f1d]:

src/EditView.cxx:       if (vsDraw.caretStyle == CARETSTYLE_BLOCK && !drawDrag && posCaret > model.sel.Range(r).anchor) {
src/EditView.cxx:               } else if ((vsDraw.caretStyle == CARETSTYLE_BLOCK) || imeCaretBlockOverride) {
src/Editor.cxx:         if ((vs.caretStyle == CARETSTYLE_BLOCK) || view.imeCaretBlockOverride) {
src/Editor.cxx:     if (wParam <= CARETSTYLE_BLOCK)

I propose the following interface definition in "include/Scintilla.iface":

enu CaretStyle=CARETSTYLE_
val CARETSTYLE_INVISIBLE=0
val CARETSTYLE_LINE=1
val CARETSTYLE_BLOCK=2
val CARETSTYLE_END=4

Maybe add val CARETSTYLE_BLOCKEND=6 for convenience.

With it, those 4 lines become:

src/EditView.cxx:       if ((vsDraw.caretStyle & CARETSTYLE_END) && !drawDrag && posCaret > model.sel.Range(r).anchor) {
src/EditView.cxx:               } else if ((vsDraw.caretStyle & CARETSTYLE_BLOCK) || imeCaretBlockOverride) {
src/Editor.cxx:         if ((vs.caretStyle & CARETSTYLE_BLOCK) || view.imeCaretBlockOverride) {
src/Editor.cxx:     if (wParam < (CARETSTYLE_END << 1))

The calls to set block cursor mode are then:

// Common block cursor behaviour:
wEditor.Call(SCI_SETCARETSTYLE, CARETSTYLE_BLOCK);
// GVim-style behaviour:
wEditor.Call(SCI_SETCARETSTYLE, CARETSTYLE_BLOCK | CARETSTYLE_END);
// or, if we define CARETSTYLE_BLOCKEND=6:
// wEditor.Call(SCI_SETCARETSTYLE, CARETSTYLE_BLOCKEND);

I've tested this with SciTE (Scintilla 4.0, current hg) and with Textadept by patching manually the local copy of Scintilla 3.7 that it uses. Textadept will keep using the LongTerm3 branch [1] so I'd like this change to be put in both branches, default and LongTerm3.

[1] https://foicica.com/lists/code/201708/4036.html

If that sounds fine to you I'll prepare a patch.

Should I also modify "doc/ScintillaHistory.html" and "doc/ScintillaDoc.html" or would you prefer to do that?

Do you prefer a patch file here, or in the mailing list, or a merge request using SourceForge's repo forking mechanism?
For me a patch is the simplest option.

Cheers,
Alberto González Palomo https://sentido-labs.com

[Neil Hodgson]

// Common block cursor behaviour:
wEditor.Call(SCI_SETCARETSTYLE, CARETSTYLE_BLOCK);
// GVim-style behaviour:
wEditor.Call(SCI_SETCARETSTYLE, CARETSTYLE_BLOCK | CARETSTYLE_END);

API churn should be minimized. Setting CARETSTYLE_BLOCK should behave as Scintilla 4.0. A new enumeration value should be added for what you see as common behaviour.

The patch should include changes to ScintillaDoc.html. Its worthwhile including an update in ScintillaHistory.html.

Changes are best published as a patch attached to this tracker item.

[Alberto González Palomo]

Well, I think it makes sense to call the original block cursor behaviour "common", but that's beside the point as you prefer the new behaviour. What gave me trouble was the unexpected API change.

In the meantime, I've found the documentation for that setting in Vim which is at the root of this issue:
http://vimhelp.appspot.com/options.txt.html#%27selection%27

I understand that the new behaviour will stay at value 2.
What I propose now is to follow Vim's terminology: would the following be acceptable?

enu CaretStyle=CARETSTYLE_
val CARETSTYLE_INVISIBLE=0
val CARETSTYLE_LINE=1
val CARETSTYLE_BLOCK_INCLUSIVE=2
val CARETSTYLE_BLOCK_EXCLUSIVE=4

This gives other developers a hint that something changed, and allows them to make a explicit choice. Still, the new behaviour remains the default until they update "Scintilla.h".

Would you consider applying such a patch? If so, I'll prepare patches for the LongTerm3 (the most relevant for me) and default branches.

Cheers,
Alberto González Palomo https://sentido-labs.com

[Neil Hodgson]

I do not 'prefer' either option as I do not use block carets. I'm just bridging a communication gap and ensuring stability.

val CARETSTYLE_BLOCK_INCLUSIVE=2

That change will cause current applications to fail compilation. To allow applications written to the current API to continue working, there should be a CARETSTYLE_BLOCK with value 2.

Hinting should be done through documentation, not with a header change that will cause a build failure.

Add CARETSTYLE_BLOCK_EXCLUSIVE. Use the value 3 as I haven't been able to think of a reason for applying an exclusive option to other styles.

[Zufu Liu]

I think it's better to add a bit value (or another option) like

val CARETSTYLE_BLOCK_EXCLUSIVE=32, 64 or larger value

we have block caret in INS and OVR mode, this bit controls whether the block caret is inside selection (current behavior), or outside selection (past behavior).

This only applies to forward selection.

http://vimdoc.sourceforge.net/htmldoc/options.html#%27selection%27

[Neil Hodgson]

With CARETSTYLE_OVERSTRIKE_* used for behaviour that differs between overstrike and insertion modes, there may be a later need for more ovr/ins dependent flags. These should be contiguous if possible and allocating 4 bits seems reasonable and sufficient.

Its possible there could be other inclusive/exclusive options so it would be OK to start another set of options at 0x100. Since this could be modifying new shapes (HALF_BLOCK_LOWER, for example), the name should not start with the name of a particular shape. Perhaps starting with CARETSTYLE_EXCLUSIVE or CARETSTYLE_INCLUSIVE.

[Zufu Liu]

Currently the three caret (line in INS mode, bar in OVR mode and block in both mode) behaves differently on selecting, only block caret is inside the selection.

Set either CARETSTYLE_EXCLUSIVE or CARETSTYLE_INCLUSIVE to default will change current behavior.

[Neil Hodgson]

Potential fix attached. Adds new option flag CARETSTYLE_BLOCK_MAY_TRAIL_RANGE to stop moving carets inside the selection.

Depends on 2106 patch on [#2106] being applied first.

[Zufu Liu]

IsBlockCaretStyle need update

 bool ViewStyle::IsBlockCaretStyle() const noexcept {
    return (caretStyle == CARETSTYLE_BLOCK) || (caretStyle & CARETSTYLE_OVERSTRIKE_BLOCK) != 0;
 }

(caretStyle & CARETSTYLE_INS_MASK) == CARETSTYLE_BLOCK

CARETSTYLE_BLOCK_OUTSIDE_RANGE not defined in SCI_SETCARETSTYLE.

[Neil Hodgson]

The remaining use is in XYScrollToMakeVisible and what it is really after is to determine the scrolling needed to make the whole caret visible so is interested in the probable width of the caret. IsBlockCaretStyle does not need to be changed for this bug report 1924 and any enhancements to scrolling should be a separate issue.

[Zufu Liu]

After adding CARETSTYLE_BLOCK_MAY_TRAIL_RANGE, if it's set, old condition for block caret in INS mode no longer true.

[Neil Hodgson]

OK, updated patch also matches other recent changes to Scintilla.iface.

[Zufu Liu]

I think the whole checking can made into DrawCaretInsideSelection() (not very readable):

bool ViewStyle::DrawCaretInsideSelection(bool inOverstrike, bool imeCaretBlockOverride) const noexcept {
    return (caretStyle & CARETSTYLE_BLOCK_MAY_TRAIL_RANGE) == 0 &&
        (((caretStyle & CARETSTYLE_INS_MASK) == CARETSTYLE_BLOCK) ||
        (inOverstrike && (caretStyle & CARETSTYLE_OVERSTRIKE_BLOCK) != 0) ||
        imeCaretBlockOverride);
}

[Zufu Liu]

I think this bug and https://sourceforge.net/p/scintilla/bugs/2106/ can be fixed with above patch and a function like above (to make EditView::DrawCarets() clean).

[Neil Hodgson]

Committed as [340a0f]. Changed CARETSTYLE_BLOCK_MAY_TRAIL_RANGE to CARETSTYLE_BLOCK_AFTER as its shorter. This is using the DrawCaretInsideSelection from Bug1924.patch with two returns as the single return version above is too difficult to understand.