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 Jun 22, 2022

This PR changes how Win32 processes keyboard messages. The messages are now put into a queue, and the next message will not be processed at all until the previous message has been responded by the framework (or if the message does not need a response, such as a single WM_CHAR).

This fixes a racing condition described in flutter/flutter#82673 (comment). A similar strategy has been applied to macOS in #32120.

A previous attempt, #31379, alleviates the problem but failed to handle more complex scenarios.

A few unit tests have to be altered. Previously later key events will be dispatched before the framework responds to the first event. Now they will only be dispatched after the response. No perceptive changes are made.

A unit test "DisorderlyRespondedEvents" is removed, because it will no longer happen. Events will always be responded in order.

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.

@dkwingsmt dkwingsmt changed the title [Windows, Keyboard] Enqurue keyboard message [Windows, Keyboard] Enqueue keyboard message Jun 22, 2022
@dkwingsmt dkwingsmt marked this pull request as ready for review June 23, 2022 23:06
@dkwingsmt dkwingsmt requested a review from gspencergoog June 23, 2022 23:06
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

clear_key_calls();
}

TEST(KeyboardTest, DisorderlyRespondedEvents) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't you still want to test that events that used to arrive before the framework response no longer do? Or are the other test changes sufficient to guarantee that?

Copy link
Contributor Author

@dkwingsmt dkwingsmt Jun 24, 2022

Choose a reason for hiding this comment

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

Other test changes are sufficient to guarantee that even when the platform messages arrive too fast for the framework to handle, nothing will be responded out of order and the events turn out to be normal, such as here.

@dkwingsmt dkwingsmt added affects: text input autosubmit Merge PR when tree becomes green via auto submit App labels Jun 24, 2022
@auto-submit auto-submit bot merged commit eea921c into flutter:main Jun 24, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 25, 2022
@dkwingsmt dkwingsmt deleted the win-key-queue branch June 27, 2022 09:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

affects: text input autosubmit Merge PR when tree becomes green via auto submit App platform-windows

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants