#2178 Inline IME: app receives toggled save point states for each tentative added character

[Zufu Liu]

When using inline mode IME typing word, save point state is toggled for each tentative added character, which may cause undesired behavior in host app, such as flash on title bar, like https://github.com/zufuliu/notepad2/issues/203 (reproducible with SciTE).

for win32 case, in ScintillaWin::HandleCompositionInline(), pdoc->TentativeUndo() leads to NotifySavePoint(true)/SCN_SAVEPOINTREACHED; InsertCharacter() leads to Document::InsertString(), then NotifySavePoint(false)/SCN_SAVEPOINTLEFT.

[johnsonj]

I revoke the patch.
sorry! misunderstood.

[Zufu Liu]

Here is the workaround I currently use:

// ScintillaWin::HandleCompositionInline()
if (pdoc->TentativeActive()) {
    // GCS_COMPSTR is set on pressing Esc, but without composition string.
    const bool pending = (lParam & GCS_COMPSTR) && imc.HasCompositionString(GCS_COMPSTR);
    pdoc->TentativeUndo(pending);
}

// void Document::TentativeUndo(bool pendingUpdate)
const bool endSavePoint = cb.IsSavePoint();
if (startSavePoint != endSavePoint && !pendingUpdate) {
    NotifySavePoint(endSavePoint);
}

[johnsonj]

Is it already fixed?
Try it without your patch.

[Neil Hodgson]

There's a comment on https://github.com/zufuliu/notepad2/issues/203 :

Current changes doesn't fix this, it may cause other bugs under some rarely circumstance.

If the change can cause the application to not know that the document is saved/unsaved then its too dangerous.

[Zufu Liu]

My patch relies on GCS_COMPSTR call InsertCharacter() , however from the code it's not always true, then application will in a dirty state.

[johnsonj]

It seems reasonable that pendingUpdate be renamed to IsCompStrExist.

You apply !pendingUpdate
so NotifySavePoint(endSavePoint) always is performed
only when !IsCompStrExist.
That is, while in composition No NotifySavePoint except for empty compStr.

[Zufu Liu]

some word contains composition (e.g. startComposition) might be than IsCompStrExist.

[johnsonj]

sorry! Test to be required patch attached

[johnsonj]

Composing 'ㅎ'
tentativeSteps: 0, startSavePoint: 0, endSavePoint: 1
:::NotifySavePoint:::
Composing '하'
tentativeSteps: 0, startSavePoint: 0, endSavePoint: 1
:::NotifySavePoint:::
Composing '한'
tentativeSteps: 0, startSavePoint: 0, endSavePoint: 1
:::NotifySavePoint:::
Composing '한ㄱ'
tentativeSteps: 0, startSavePoint: 0, endSavePoint: 1
:::NotifySavePoint:::
Composing '한그'
tentativeSteps: 0, startSavePoint: 0, endSavePoint: 1
:::NotifySavePoint:::
Composing '한글'
-----------------------Commit :'한글'----------------------

tentativeSteps: 0, startSavePoint: 0, endSavePoint: 1
:::NotifySavePoint:::
-----------------------Commit :' '----------------------

Composing 'ㅁ'
tentativeSteps: -1, startSavePoint: 0, endSavePoint: 0
Composing '마'
tentativeSteps: 0, startSavePoint: 0, endSavePoint: 0
Composing '만'
tentativeSteps: 0, startSavePoint: 0, endSavePoint: 0
Composing '만ㅅ'
tentativeSteps: 0, startSavePoint: 0, endSavePoint: 0
Composing '만세'
-----------------------Commit :'만세'----------------------

tentativeSteps: 0, startSavePoint: 0, endSavePoint: 0
-----------------------Commit :' '----------------------

[johnsonj]

It turns out NotifySavePoint() never be performed except for initial TentativeUndo.
It seems Ok to Remove NotifySavePoint() from TentativeUndo().
initial TentativeUndo() compStr should not influence on title bar since that is tentative and should be treated none.

[Zufu Liu]

No, please test other IMEs (e.g. MS Pinyin) and continue type letters, the composition string will continue to grow, each letter will cause TentativeUndo + InsertCharacter.

[johnsonj]

I use SciTEGtk to test this.
Is this something different?

--- preeditStr:'w' ---

savePoint: 0, currentAction: 0
savePoint: 0, currentAction: 2
savePoint: 0, currentAction: 0
tentativeSteps: 0, startSavePoint: 0, endSavePoint: 1
:Tentative NotifySavePoint:
--- preeditStr:'wo' ---

savePoint: 0, currentAction: 0
savePoint: 0, currentAction: 2
savePoint: 0, currentAction: 3
savePoint: 0, currentAction: 0
tentativeSteps: 0, startSavePoint: 0, endSavePoint: 1
:Tentative NotifySavePoint:
--- preeditStr:'wo a' ---

savePoint: 0, currentAction: 0
savePoint: 0, currentAction: 2
savePoint: 0, currentAction: 3
savePoint: 0, currentAction: 4
savePoint: 0, currentAction: 5
savePoint: 0, currentAction: 0
tentativeSteps: 0, startSavePoint: 0, endSavePoint: 1
:Tentative NotifySavePoint:
--- preeditStr:'wo ai' ---

savePoint: 0, currentAction: 0
savePoint: 0, currentAction: 2
savePoint: 0, currentAction: 3
savePoint: 0, currentAction: 4
savePoint: 0, currentAction: 5
savePoint: 0, currentAction: 6
savePoint: 0, currentAction: 0
tentativeSteps: 0, startSavePoint: 0, endSavePoint: 1
:Tentative NotifySavePoint:
--- preeditStr:'wo ai n' ---

savePoint: 0, currentAction: 0
savePoint: 0, currentAction: 2
savePoint: 0, currentAction: 3
savePoint: 0, currentAction: 4
savePoint: 0, currentAction: 5
savePoint: 0, currentAction: 6
savePoint: 0, currentAction: 7
savePoint: 0, currentAction: 8
savePoint: 0, currentAction: 0
tentativeSteps: 0, startSavePoint: 0, endSavePoint: 1
:Tentative NotifySavePoint:
--- preeditStr:'wo ai ni' ---

savePoint: 0, currentAction: 0
savePoint: 0, currentAction: 2
savePoint: 0, currentAction: 3
savePoint: 0, currentAction: 4
savePoint: 0, currentAction: 5
savePoint: 0, currentAction: 6
savePoint: 0, currentAction: 7
savePoint: 0, currentAction: 8
--- CommitStr :'我爱你'----

savePoint: 0, currentAction: 9
savePoint: 0, currentAction: 0
tentativeSteps: 0, startSavePoint: 0, endSavePoint: 1
:Tentative NotifySavePoint:
savePoint: 0, currentAction: 0
savePoint: 0, currentAction: 2
savePoint: 0, currentAction: 3
--- CommitStr :'a'----

savePoint: 0, currentAction: 4
--- CommitStr :'b'----

savePoint: 0, currentAction: 5
--- CommitStr :'c'----

savePoint: 0, currentAction: 6
savePoint: 0, currentAction: 7
savePoint: 0, currentAction: 0
tentativeSteps: -1, startSP: 0, endSP: 1
:Ctrl+Z Undo NotifySavePoint:

[Zufu Liu]

Tested again on Win 10, indeed only first (when document unmodified) compositing has the behavior (save point change on each tentative added character). if document is already modified no save point change is notified.

[johnsonj]

do not redraw while TentativeActive as redrawPendingText.
patch proposed

[Zufu Liu]

NotifySavePoint() in TentativeUndo() is needed at least when press Esc key to cancel.

[johnsonj]

So I follow your idea redrawPendingText.

[Zufu Liu]

I got another idea for this: delay NotifySavePoint() to the end of ScintillaWin::HandleCompositionInline().

pseudo code:

class DelayedSavePoint {
    const bool startSavePoint;
public:
    DelayedSavePoint() noexcept : startSavePoint{cb.IsSavePoint()} {
        pdoc->delaySavePointNotify = true;
    }
    ~DelayedSavePoint() {
        pdoc->delaySavePointNotify = false;
        const bool endSavePoint = cb.IsSavePoint();
        if (startSavePoint != endSavePoint) {
            NotifySavePoint(endSavePoint);
        }
    }
};
const DelayedSavePoint dummy;
bool initialCompose = false;
...

[Neil Hodgson]

Delaying to the end of HandleCompositionInline would avoid the title flash but it may conflict with application logic that closely follows events.

The application could implement a coalescing mechanism similar to window invalidation & painting. When the app receives a SCN_SAVEPOINTREACHED or SCN_SAVEPOINTLEFT, it posts itself an 'update title' message. When it receives an 'update title' it calls SetWindowName to update. Since the 'update title' won't be received until all the changes caused by HandleCompositionInline have finished, it won't display the temporary state.

[Zufu Liu]

It seems delayed idea can be used to fix dirty state bug in my workaround: tells TentativeUndo() that current composition string is likely to be replaced with new composition string, and save the changed save point. at end of HandleCompositionInline() if the replacement does not actually happen, then notify the saved save point.

pseudo code:

struct DelayedSavePoint {
    std::optional<bool> savePoint;
    ~DelayedSavePoint() {
        if (savePoint.has_value() && savePoint.value() == cb.IsSavePoint()) {
            NotifySavePoint(savePoint.value());
        }
    }
};

// ScintillaWin::HandleCompositionInline(uptr_t, sptr_t lParam)
bool initialCompose = false;
DelayedSavePoint delayedSavePoint;
if (pdoc->TentativeActive()) {
    // GCS_COMPSTR is set on pressing Esc, but without composition string.
    const bool continueComposition = (lParam & GCS_COMPSTR) && imc.HasCompositionString(GCS_COMPSTR);
    pdoc->TentativeUndo(continueComposition, &delayedSavePoint);
}

//Document::TentativeUndo(bool continueComposition, DelayedSavePoint *delayedSavePoint)
const bool endSavePoint = cb.IsSavePoint();
if (startSavePoint != endSavePoint) {
    if (continueComposition) {
        delayedSavePoint->savePoint = endSavePoint;
    } else {
        NotifySavePoint(endSavePoint);
    }
}

[Neil Hodgson]

A potential problem is that the application won't be receiving the save-point notifications at the time when it can examine the state of the document that corresponds to that save-point.

The motivating 'title asterisk flash' problem does not appear important enough to break compatibility with previous behaviour.

If completely pushing the issue to application code is too much then there could be an intermediate approach where Notification::UpdateUI is used to deliver a new Update::SavePoint asynchronously. An application may then choose whether to respond to Notification::SavePointReached / Notification::SavePointLeft or to Update::SavePoint or to a combination such as setting the dirty flag in Notification::SavePoint* and updating the title in Update::SavePoint.

[Zufu Liu]

Patch that implements TentativeSavePoint , the save point will only be notified/delayed when current composition string not been replaced with new composition string or composition result (GCS_RESULTSTR also cause toggled states), e.g. when press Esc. This can treated as model HandleCompositionInline() as a string replace operation, and drop the tentative save point when operating succeeded .

it can be used for other platforms in a simple way:

    bool initialCompose = false;

+   TentativeSavePoint tentativeSavePoint;
    if (pdoc->TentativeActive()) {
-       pdoc->TentativeUndo();
+       pdoc->TentativeUndo(&tentativeSavePoint);
    } else {
        // No tentative undo means start of this composition so
        // fill in any virtual spaces.

[Zufu Liu]

Still need to dig more, as it seems better to fire NotifySavePoint() with zero once (just like direct input) instead of for each tentative character.

[Zufu Liu]

Notepad2 switched to delay save point approach:
https://github.com/zufuliu/notepad2/commit/e10af5ffcd7103c047f01ea155413cdf5f5ed37c

This reduce inline mode IME save point notification (to application) to just 2: NotifySavePoint(0) on start and NotifySavePoint(1) on cancel, none on typing more tentative characters, whereas window mode IME there is only NotifySavePoint(0) on complete. Maybe an option ("disable inline IME tentative save point change notification during compositing"?) can be added to enable this experimental workaround if application think it's OK for them.

[Neil Hodgson]

Have you checked whether this will work if there is a save during IME entry? Saves may be performed automatically on a timer in some applications.

Shouldn't have ~DelaySavePoint call non-noexcept method.

C:\u\hg\scintilla\src\Document.h(571): warning C26447:
The function is declared 'noexcept' but calls function 
'EndDelaySavePoint()' which may throw exceptions (f.6).

void Document::EndDelaySavePoint() noexcept {
    const bool startSavePoint = *delaySavePoint;
    delaySavePoint.reset();
    const bool endSavePoint = cb.IsSavePoint();
    if (startSavePoint != endSavePoint) {
        try {
            NotifySavePoint(endSavePoint);
        } catch (...) {
            // Ignore exceptions at this point
        }
    }
}