Fix Korean IME on Android

[kerams]

What does the pull request do?

Fixes #16956 on Android.

What is the current behavior?

AndroidInputMethod subscribes to TextInputMethodClient's SurroundingTextChanged, which ends up calling AvaloniaInputConnection.UpdateState several times on each key press. AvaloniaInputConnection also calls this method internally as part of its implementation of Android's IInputConnection, just after raising the previously mentioned SurroundingTextChanged.

I am not exactly sure what the relationship between AndroidInputMethod and AvaloniaInputConnection is, but this behavior does not seem right. These UpdateState calls are interfering with one another, messing up composition state when typing Korean. As soon as a 2-letter block is created, FinishComposingText is called by the OS, making it impossible to create 3- and 4-letter blocks.

What is the updated/expected behavior with this PR?

FinishComposingText is not called when typing Korean. The user can keep typing without breaking composition, ultimately being able to create any syllable block. Deletes also work as expected - only the final letter part is removed, not the entire block.

qemu-system-x86_64_VNePyHcDnI.mp4

How was the solution implemented (if it's not obvious)?

AndroidInputMethod does not react to SurroundingTextChanged, letting AvaloniaInputConnection handle its state. No idea whether anything else breaks as a result though.

Checklist

• Added unit tests (if possible)?
• Added XML documentation to any related classes?
• Consider submitting a PR to https://github.com/AvaloniaUI/avalonia-docs with user documentation

Breaking changes

Don't know.

[avaloniaui-bot]

You can test this PR using the following package version. 11.3.999-cibuild0054461-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

[Gillibald]

SurroundingText always represents the current part of the text being edited, usually the current line. Removing the synchronization makes it impossible to update the position of the edit context. Just select a new line, and it should break. We need a way to detect that the SurroundingText isn't different from the buffer that the current InputConnection is aware of. SurroundingText is required to be able to virtualize the text buffer. We need to find a solution that makes this possible.

Maybe we can sync the SurroundingText when the user explicitly changes the selection or a new line is selected.

We might also need to use absolute indices to be able to determine the current SurroundingText is at a different position within the text buffer.

Avalonia/src/Avalonia.Controls/TextBoxTextInputMethodClient.cs

Line 69 in 3850b80

public override TextSelection Selection

We could also try to compare the current text edit buffer with the new SurroundingText. And if they match we ignore that change.

[kerams]

qemu-system-x86_64_0czKeYCk3z.mp4

Just select a new line, and it should break

Do you mean like this where I move the caret to a different line with arrow keys? I switched to master and the bug is present there too.

In case you had something else in mind, what about adding _inputConnection.IsInUpdate as another check into _client_SurroundingTextChanged, so that the guard condition is the same as in _client_SelectionChanged?

The flag is set here

public bool EndBatchEdit()
{
    _batchLevel = Interlocked.Decrement(ref _batchLevel);

    if (!IsInBatchEdit)
    {
        IsInUpdate = true;
        while (_commandQueue.TryDequeue(out var command))
        {
            command.Apply(_editBuffer);
        }
        IsInUpdate = false;
    }

    UpdateState();
    return IsInBatchEdit;
}

meaning just around the code that gives rise to the unwanted UpdateState call in the stack in the picture above. Would that allow the synchronization to continue working?

[Gillibald]

Yes, that is what I meant. If that is still working there is no regression and we can keep the change as is.

[kerams]

No, it's broken both in master and in this PR - the ㅎ component on the first line is overwritten with ㄹ if you look closely. Maybe the guard condition change would be safer in any case?

[Gillibald]

Y changes to SurroundingText should not affect the text buffer if it is being edited. The buffer already has all the information. Changes to SurroundingText are also propagated when the text is changed externally. In that case, the current text buffer should be reset.

[kerams]

Then I think the condition is the only required change. Gboard and SwiftKey both work for Korean now.

I don't suppose there's an integration test suite for Android text input? This all seems very prone to subtle bugs and regressions.

[avaloniaui-bot]

You can test this PR using the following package version. 11.3.999-cibuild0054479-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

[Gillibald]

We need proper automation infrastructure to be able to have integration tests

[avaloniaui-bot]

You can test this PR using the following package version. 11.3.999-cibuild0054481-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

[timunie]

Does it also help for Japanese? If probably yes there are some recent issues where we can ask the OP to test this PR

[kerams]

Yeah, I have already posted a comment in that thread.

[Gillibald]

LGTM Thank you