#2289 SCI_REPLACETARGET in 5.1.1 replaces wrong text Windows

[Greg Smith]

In Editor::ReplaceTarget() the following code:

    if (targetRange.Length() > 0)
        pdoc->DeleteChars(targetRange.start.Position(), targetRange.Length());
    targetRange.end = targetRange.start;

Is intended to delete the target (which it does) and then set the target to the (now empty) position at which to insert the replacement. Unfortunately, possibly due to notification enhancements, targetRange is modified during the DeleteChars() call and ends up pointing at the modified line (in my case), so the inserted text ends up at the start of the line, not at the replacement point. To get my program to work with minimum change I have adjusted this code to:

    auto start = targetRange.start;     // Save start as modified by DeleteChars
    if (targetRange.Length() > 0)
        pdoc->DeleteChars(targetRange.start.Position(), targetRange.Length());
    targetRange.end = targetRange.start = start;

I was using the 511 release code. What puzzles me is that sometimes a SCI_REPLACETARGET works and other times it does not. I speculate that this might to relate to whether the buffer space for the editor text is reallocated or not.

[Neil Hodgson]

The most likely culprit here is application code changing the target inside a notification and not restoring it. Using the Visual C++ debugger, when a breakpoint is reached inside ReplaceTarget, add a data breakpoint on &targetRange for 8 bytes (just covers start.position) which should stop in the debugger when targetRange.start.position is changed then check the call stack to see what is changing it.

While the above change may protect against the current specific problem, it doesn't protect against target changes during the RealizeVirtualSpace or InsertString which may also invoke application code.

This is part of a larger problem of notification handlers in application code making unexpected changes which is very difficult to handle well in all cases. An application may decide to change the document text in response to a deletion (perhaps to delete all of a construct when the user's mouse selection may be inaccurate), reformat affected lines, or could even switch to a different document. GetTargetText is a bit of a problem here as its a common way to retrieve portions of the document and the target will generally be changed before calling it.

[Greg Smith]

Thanks for the reply and the very clear explanation.

I was using SearchInTarget in my notification code, (called because of SC_MOD_BEFOREDELETE so I could search the text that was about to vanish), and this has not caused any problems in previous Scintilla versions. I can save and restore the Target, now that I know I have to do it.

Would it make sense to take a copy of the Target before the DeleteChars to remove this dependency? It looks like the Editor code following the DeleteChars does not require the Target, just the position of the start of the deleted text. It could even get the selection start after the delete to remove any dependency on the previous state.

Unless I missed it, the documentation does not state that we cannot use the Target in this situation, so I would suggest that this needs changing to state what can and cannot be changed within a Notification.

[Neil Hodgson]

Would it make sense to take a copy of the Target before the DeleteChars to remove this dependency?

That may then break an application that deliberately used the target to remember something inside a notification in different circumstances.

Unless I missed it, the documentation does not state that we cannot use the Target in this situation, so I would suggest that this needs changing to state what can and cannot be changed within a Notification.

The target is not unique here. Scintilla is a stateful object and modifications to Scintilla's state inside a notification may cause failures. Applications should defer potentially problematic modifications to an idle handler or similar. The set of potential problems is too large to be enumerated in the documentation although some warnings for common areas of difficulty could be added.

Simpler read-only non-stateful access to more of Scintilla would make it easier to write safe code.

[Neil Hodgson]

Applications can wrap GetTargetText in save and restore target to avoid problems. The attached patch implements this.

[Neil Hodgson]

Changed SCI_REPLACETARGET* with [6455b9] which should fix the original problem here.