#2393 Incorrect selection change when processing SCI_REPLACETARGET message

[cshnik]

The issue appeared in Scintilla 3.11.2+/4.x/5.x (tested with SciTE 5.3.6)
Steps to reproduce:
1. Download SciTE 5.3.6 (https://sourceforge.net/projects/scintilla/files/SciTE/5.3.6/, scite536.zip)
2. Comment SciTE source code line in SciTEBase::DoReplaceAll (line 1306) to remove excess selection change (not a part of Scintilla)
3. Build SciTE
4. Run SciTE, do the following test:
1. Paste text:
abcabc
2. Select the first line
3. Run "Replace All" from a to ZZZ
4. Note result: replace succeeded, selection changed to bcZZZbc (FAILURE)
5. Change document to (add space at the start of the first line):
abcabc
6. Repeat steps 2-3
7. Note result: replace succeeded, selection changed to ZZZbcZZZbc (SUCCESS)

Please consider the following fix:
https://github.com/ProgerXP/Notepad2e/commit/4f39364f367637dc5bdbd4b9b839515eef9dc2c9

[Neil Hodgson]

The Scintilla-bug.gif file shows the first case replacing twice ZZZbcZZZbc so does not show the problem. I also don't see a problem with running the steps myself.

The Notepad2e patch does not show the same preceding code as in current Scintilla: the !insertion clauses are not in Scintilla. So it appears you are using a modified version of Scintilla.

It appears that this is not motivated by the example (which requires disabling SciTE code) so the real motivation should be explained along with how the change achieves this goal.

There are different cases when trying to maintain ranges as the document changes. Changing this code to support some cases may cause problems for other cases. The 'move selection when insertion at start' decision here comes from [#2140].

[Neil Hodgson]

OK, I see you meant that the range selected in (4) omitted the ZZZ not that the a wasn't changed to ZZZ.

If you want to maintain selection ranges in particular ways then it is likely that it will require explicit code to implement your preference.

[cshnik]

Selection ranges are properly supported in other text replacement cases.

Regarding the fix: you are correct, provided fix was rolled over the previous change which was not a part of Scintilla. But the actual fix does not rely on this !insertion statement and actually contains a rollback for it.
The actual fix should look like this:

    const bool caretStart = caret.Position() < anchor.Position();    
    const bool anchorStart = anchor.Position() < caret.Position();

change to:

    const bool caretStart = (caret.Position() > startChange) && (caret.Position() < anchor.Position());    
    const bool anchorStart = (anchor.Position() > startChange) && (anchor.Position() < caret.Position());

[Neil Hodgson]

Isn't that equivalent to removing the moveForEqual case with:

const bool caretStart = false;
const bool anchorStart = false;

moveForEqual is only active when position == startChange and the above patch only sets it when position > startChange.

This change would revert to the [#2140] bug.

[Neil Hodgson]

I am rejecting this as a bug report since the behaviour is intentional.

It would be possible to offer alternative options but they would have to be defined well. In particular, the behaviour with multiple selections that touch should be specified.