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

This PR creates a new concept called "session", which consists of a list of messages (typically 1 to 3) that should be processed together, such as a key down message followed by char messages. The messages of the session are now stored (instead of separate char code or key code) until the end of the session, and are passed along with PendingEvent. This replaces some arbitrary members or static variable with a more meaningful member, and allows redispatching messages in a better way in the future.

This PR also clears up the logic of HandleMessage and adds a few comments.

This PR should not need new tests since it's only a refactor.

This is one of the many steps to refactor the windows keyboard system.

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.

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

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 is a great improvement!


struct PendingEvent {
uint32_t key;
uint16_t key;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you use the Windows type here (i.e. SHORT), and for the others that are derived from Windows struct values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At some point we have to convert the Win32 types to fixed-width types. My idea here is that, the members of Win32Message preserve the original value, since they are to be redispatched, while the members of PendingEvent should be fixed-width values, since they are processed values and will be send to OnKey of the platform-independent FlutterWindowsView.

Copy link
Contributor Author

@dkwingsmt dkwingsmt Jan 25, 2022

Choose a reason for hiding this comment

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

I agree that the change here (or many of the numerical types) is debatable. WPARAM is technically uint64 nowadays in a 64-bit system, but the API dated back to the 32-bit era, and practically the virtual code never exceeds 16 bit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, and I'm sure that practically it makes no difference, but from a correctness standpoint it would be better to use the system defined types instead of making the (yes, perfectly valid) assumptions about the sizes of the types.

I'm OK with making the assumptions, though, if you think that the documentation value of showing the type sizes is important enough to trump the correctness.

Copy link
Contributor

Choose a reason for hiding this comment

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

It might be sufficient to have a compile-time static_assert that sizeof(SHORT) == sizeof(uint16_t).

Copy link
Contributor Author

@dkwingsmt dkwingsmt Jan 26, 2022

Choose a reason for hiding this comment

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

I've replaced the types of key and action to win32 types since they directly come from win32 values. Others are extracted from lparam and have explicitly defined width, so I'm keeping their types.

@dkwingsmt dkwingsmt merged commit 538bb80 into flutter:main Jan 26, 2022
@dkwingsmt dkwingsmt deleted the win-key-session branch January 26, 2022 00:43
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 26, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 26, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 26, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 26, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 26, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 26, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 26, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 26, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 26, 2022
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