Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@dkwingsmt
Copy link
Contributor

@dkwingsmt dkwingsmt commented Feb 10, 2022

This PR fixes issues that have been impacting the Vietnamese IME, UniKeyNT Telex Mode, the windows part of flutter/flutter#82673.

This IME adds or removes diacritics of the entire word when the last letter of the word is typed by forging Backspace keys and new characters. For example,

To get ào, you type aof. The detailed messages are:

  • Tap KeyA, messages are KeyDown(KeyA), Char(a), KeyUp(KeyA).
  • Tap KeyO, messages are KeyDown(KeyO), Char(o), KeyUp(KeyO).
  • Press KeyF, messages are
    • KeyDown(Backspace), Char(Backspace), KeyUp(Backspace).
    • KeyDown(Backspace), Char(Backspace), KeyUp(Backspace).
    • KeyDown(VK_PACKET), Char(à), KeyUp(VK_PACKET).
    • KeyDown(VK_PACKET), Char(o), KeyUp(VK_PACKET).
  • Release KeyF, messages are KeyUp(KeyF).

The first change of this PR is to process the VK_PACKET session, which is easy because we can just ignore the VK_PACKET messages and take the body char messages as standalone char messages.

The second change, which is way harder, is to fix a racing condition. Both the engine and the framework maintains a state of the text input field currently being edited. Sometimes the framework sends an updated state to the engine, sometimes the other way around. More specifically,

  1. When the KeyDown(Backspace) events are received by the framework, the framework processes it with the shortcut system and decides that it affects the text state, deletes the last character of the text, and sends the updated state to the engine.
  2. When the printable Char messages are being processed by the engine, the engine decides that it should update the engine's text state immediately, and sends the updated state to the framework.

However, since all the messages triggered by the KeyF press are so tightly queued, that the first text state update above will usually, but not always, arrive after the second text state update is done locally. The symptom is that after pressing f, the resulting text can be either ào, ao, o, or some other random values.

The best way to fix it might be to make EditableText wait for the text state update to finish (https://github.com/flutter/flutter/blob/master/packages/flutter/lib/src/widgets/editable_text.dart#L2171). However this might be too big of a change.

The fix proposed in this PR is to make unprintable chars (such as Backspace) take up a placeholder char in the char queue, and remove this char only when the corresponding keydown message has arrived. Since the platform messenger maintains message order, and that TextInput._instance._setEditingState(value) synchronously pushes an message to the messenger, it's guaranteed that the text state update arrives at the engine before the response of the key down event. However, this fix does not prevent the case if a key without any characters would like to change the text state.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Copy link
Contributor

@gspencergoog gspencergoog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

32384589-a60f0e74-c078-11e7-9bc1-e5b5287aea9d

This one feels a little scary, since it could block editing updates, but I can't see anything wrong with the logic, it's well tested, and it fixes a problem. I agree that it would be better to make EditableText wait for the update to finish, but this seems like a reasonable workaround.


// Set this flag to enforce prev_state to be 0.
//
// This occurs in rare cases when the message is synthesized.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you maybe describe at least one of the cases?

Copy link
Contributor Author

@dkwingsmt dkwingsmt Feb 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's actually used here and is found during actual device test. It's understandable: The F key never had a key down message, and only got a key up message. It seems that the 3rd party IME blocked its key down message and turned it into the series of backspace and char messages. So the OS probably regularized its state by saying, that although it is a key up message, the key was up somehow.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I should have been more specific: I meant describe it in the comment in the code... It's OK, though.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants