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

This PR refactors keyboard_win32_unittests.cc so that, dispatching win32 messages and setting key states can be put into a single sequence, which is injected into the tester.

This is required for accurate keyboard simulation. It's already possible to queue multiple messages to inject at the same time in order to allow peeking, but the key state changes must be applied either before all of them or after. However, when multiple modifier events must be injected with a single sequence, the key state changes must be interlaced within the messages. For example, when dispatching an AltGr key down (for which Win32 dispatches a ControlLeft down then a AltRight down), the correct sequence must be:

SetState(ControlLeft down) > Message(ControlLeft down) > SetState(AltRight down) > Message(AltRight down)

The current tests have been working fine because we haven't added Alt keys into critical keys yet, i.e. their key state don't matter (see flutter/flutter#76736). But as I'm about to resolve this issue, we must have a way to interlace the key changes.

Nevertheless, no production code or the semantic of tests are changed by this PR. This is only a refactor.

Another design was considered, where testing::Win32Message is appended with a key_state_change field. But this mixes several concepts together, and isn't flexible enough for more complicated scenarios, such as key state changes that accompany key messages that are synthesized by Flutter (the ControlLeft key up synthesized right before AltGr key up).

This is a prerequisite to a fix to flutter/flutter#76736.

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

va_end(args);
window_->InjectMessageList(count, messages);
void InjectKeyboardChanges(std::vector<KeyboardChange> changes) {
window_->InjectKeyboardChanges(changes);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe assert that window_ is initialized here.

@dkwingsmt dkwingsmt added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Feb 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

affects: text input platform-windows waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants