Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement repeat filtering logic in Android Embedder #17509

Merged
merged 13 commits into from
Apr 8, 2020

Conversation

GaryQian
Copy link
Contributor

@GaryQian GaryQian commented Apr 4, 2020

There is a longstanding problem where some keyboards, via calls to endBatchEdit(), inadvertently send multiple copies of the same editing state to the framework. This has caused many issues that stem from the framework treating these repeat invocations as new instances of data or new keypresses.

In this PR, I add filtering to prevent the embedder from sending any state more than once unless it is marked as dirty via making any change to the state. This ensures that only valid changes to the state are sent to the framework.

The extra calls to updateEdtingState() may be removed for any override that calls the super version of itself since the default android implementation all include calls to endBatchEdit() which includes a gated call to updateEditingState as needed.

Paired with flutter/flutter#53974 to resolve b/152733348

@GaryQian GaryQian removed the Work in progress (WIP) Not ready (yet) for review! label Apr 7, 2020
@GaryQian
Copy link
Contributor Author

GaryQian commented Apr 7, 2020

cc @jason-simmons Ready for review

Copy link
Member

@jason-simmons jason-simmons left a comment

Choose a reason for hiding this comment

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

LGTM

// Used to store the last-sent values via updateEditingState to the framework.
// These are then compared against to prevent redundant messages with the same
// data before any valid operations were made to the contents.
private int mPreviousSelectionStart;
Copy link
Member

Choose a reason for hiding this comment

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

Consider moving the selection start/end, composing start/end, and text fields into a class. updateEditingState would construct an instance of the class from the mEditable and call its equals method to check whether it matches the previous instance.

@GaryQian
Copy link
Contributor Author

GaryQian commented Apr 8, 2020

LUCI is actually green. Merging.

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 9, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 9, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 9, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 9, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 9, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 9, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 10, 2020
goderbauer pushed a commit to goderbauer/engine that referenced this pull request Apr 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants