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 Jan 6, 2021

Description

This PR contains the Windows changes for the Hardware Keyboard project.

Classes has been adjusted as follows:

Before After Explain
KeyboardHookHandler KeyboardHandlerBase Abstract class for a handler to be added to FlutterWindowsView. Base class to KeyboardKeyHandler and TextInputPlugin.
KeyEventHandler KeyboardKeyHandler Handles redispatching and detects redispatched events.
KeyboardKeyChannelHandler Sends raw events through method channel and collect responses.
- KeyboardKeyEmbedderHandler Converts raw events to FlutterKeyEvent, sends them through embedder API and collect responses.

Other changes

The code for skipping VK_PROCESSKEY events have been moved from Win32Window to embedder delegates. This is because the key up events for these presses have normal virtual keys, but also need filtering, which requires more delicate key press tracking.

The logic for handling asynchronous event redispatching has been refactored to handle crashing in several cases. Now the two phases have different identifiers.

  • Response phase:
    • Events are sent to platform, waiting for response.
    • Such events are stored in pending_responses_.
    • Such events are identified by self-incrementing sequence_ids.
  • Redispatching phase:
    • Events are sent to the system, waiting for the redispatch.
    • Such events are stored in pending_redispatches_.
    • Such events are identified by hash.

The crash that lead to this refactor is explained in test KeyboardKeyHandlerTest/WithSlowFrameworkResponse.

Related Issues

Tests

I added the following tests:

  • keyboard_key_handler_unittests.cc
    • SingleDelegateWithAsyncResponds
    • SingleDelegateWithSyncResponds
    • WithTwoAsyncDelegates
    • WithSlowFrameworkResponse
  • keyboard_key_embedder_handler_unittests.cc
    • BasicKeyPressingAndHolding
    • ToggleNumLockDuringNumpadPress
    • ImeEventsAreIgnored
    • ModifierKeysByExtendedBit
    • ModifierKeysByVirtualKey
    • AbruptRepeatIsConvertedtoDown
    • SynthesizeForDesyncPressingState
    • SynthesizeForDesyncToggledState
    • SynthesizeForDesyncToggledStateByItself
    • SynthesizeWithInitialTogglingState

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the contributor guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the C++, Objective-C, Java style guides for the engine.
  • I read the tree hygiene wiki page, which explains my responsibilities.
  • I updated/added relevant documentation.
  • All existing and new tests are passing.
  • I am willing to follow-up on review comments in a timely manner.

Reviewer Checklist

Breaking Change

Did any tests fail when you ran them? Please read handling breaking changes.

@dkwingsmt dkwingsmt marked this pull request as draft January 7, 2021 01:32
@clarkezone
Copy link

I can't see any conflicts but would appreicate it if you could take a look at #23573 to ensure this will work with / not break the forthcoming UWP embedder. Thx!

@dkwingsmt
Copy link
Contributor Author

dkwingsmt commented Feb 18, 2021

@gspencergoog @cbracken I've added the feature to synthesize events to keep synchronized with the true states from Win32's GetKeyState, as well as more tests. Can you make a full review? Thanks!

@chinmaygarde
Copy link
Member

Ping @gspencergoog. This is also on @cbracken s backlog. But I know both are super busy ATM, so not sure this is will be reviewed immediately. Are there other reviewers we can find for this?

@dkwingsmt
Copy link
Contributor Author

@gspencergoog I addressed all comments, PTAL

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

@dkwingsmt dkwingsmt merged commit 81701db into flutter:master Feb 27, 2021
@dkwingsmt dkwingsmt deleted the keyboard-win branch February 27, 2021 00:11
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 27, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 27, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 27, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 27, 2021
hjfreyer pushed a commit to hjfreyer/engine that referenced this pull request Mar 22, 2021
Windows changes for the Hardware Keyboard project.
chriscraws pushed a commit to chriscraws/engine that referenced this pull request Mar 23, 2021
Windows changes for the Hardware Keyboard project.
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.

4 participants