Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[macOS] Return keyboard pressed state #42878

Merged
merged 1 commit into from
Jul 26, 2023
Merged

[macOS] Return keyboard pressed state #42878

merged 1 commit into from
Jul 26, 2023

Conversation

bleroux
Copy link
Contributor

@bleroux bleroux commented Jun 15, 2023

Description

This PR updates the macOS engine in order to answer to keyboard pressed state queries from the framework (as implemented in flutter/flutter#122885).

Related Issue

macOS engine implementation for flutter/flutter#87391
Similar to:

Tests

Adds 2 tests.

@chinmaygarde
Copy link
Member

cc @dkwingsmt

@loic-sharma loic-sharma requested a review from dkwingsmt June 22, 2023 18:14
dkwingsmt
dkwingsmt approved these changes Jul 6, 2023
View reviewed changes
Copy link
Contributor

@dkwingsmt dkwingsmt left a comment

Choose a reason for hiding this comment

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

LGTM

@dkwingsmt
Copy link
Contributor

Is it possible for this one to have the similar issue as Android (message received when the handler is not registered yet)?

@bleroux
Copy link
Contributor Author

bleroux commented Jul 11, 2023

Is it possible for this one to have the similar issue as Android (message received when the handler is not registered yet)?

This won't have the similar issue as my first Android PR. The first Android PR (the one reverted) might creates the flutter/keyboard channel before the KeyboardManager is instantiated which led to the revert.
In the other PRs (the Linux one #42346, the updated Android one #42758 , this one for macOS), the channel is created by the KeyboardManager.

@bleroux
Copy link
Contributor Author

bleroux commented Jul 11, 2023

@dkwingsmt Before merging this, I think it would be great that we discuss further flutter/flutter#125975.

This PR will fix some of the error messages related to keyboard events received by the engine while the framework side is initializing. But it won't fix all cases and it will be common to still get the error message by pressing and releasing a key before the framework initialization is done because the up event is buffered at the channel level (see flutter/flutter#125975).

@bleroux bleroux mentioned this pull request Jul 11, 2023
Closed
@chinmaygarde
Copy link
Member

Triage: Are we closer to land this? If not, perhaps we can mark this as WIP?

@dkwingsmt
Copy link
Contributor

Sorry, I forgot about this. I should discuss with bleroux about the overall design on keyboard state. I'll do it as soon as I have time.

@bleroux bleroux mentioned this pull request Jul 25, 2023
Merged
@bleroux
Copy link
Contributor Author

bleroux commented Jul 26, 2023

Back from vacation.
Let’s land this PR because it helps in some issues related to keyboard state desynchronization after a hot restart.
My concern was about flutter/flutter#125975 which is a remaining issue.
I filed a PR to make progress on that remaining issue, see flutter/flutter#131258, @dkwingsmt and I will discuss on that PR and I hope we can land it soon.

@bleroux bleroux added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 26, 2023
@auto-submit auto-submit bot merged commit b3cd1c5 into flutter:main Jul 26, 2023
@bleroux bleroux deleted the macos_return_keyboard_pressed_state branch July 26, 2023 07:17
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jul 26, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 26, 2023
@engine-flutter-autoroll
b3cd1c599 [macOS] Return keyboard pressed state (flutter/engine#42878)
ba0a240
fluttermirroringbot pushed a commit to flutter/flutter that referenced this pull request Jul 26, 2023
@engine-flutter-autoroll
Roll Flutter Engine from 4bdceccff964 to b3cd1c599abe (1 revision) (#…
35e4e2a 
…131317)

flutter/engine@4bdcecc...b3cd1c5

2023-07-26 [email protected] [macOS] Return keyboard pressed state (flutter/engine#42878)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
LouiseHsu pushed a commit to LouiseHsu/flutter that referenced this pull request Jul 31, 2023
@engine-flutter-autoroll @LouiseHsu
Roll Flutter Engine from 4bdceccff964 to b3cd1c599abe (1 revision) (f…
345c109 
…lutter#131317)

flutter/engine@4bdcecc...b3cd1c5

2023-07-26 [email protected] [macOS] Return keyboard pressed state (flutter/engine#42878)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
vashworth pushed a commit to vashworth/flutter that referenced this pull request Aug 2, 2023
@engine-flutter-autoroll @vashworth
Roll Flutter Engine from 4bdceccff964 to b3cd1c599abe (1 revision) (f…
a73e764 
…lutter#131317)

flutter/engine@4bdcecc...b3cd1c5

2023-07-26 [email protected] [macOS] Return keyboard pressed state (flutter/engine#42878)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
auto-submit bot pushed a commit that referenced this pull request Aug 9, 2023
@bleroux
[Windows] Return keyboard pressed state (#43998)
b291a8e 
## Description

This PR updates the Windows engine in order to answer to keyboard pressed state queries from the framework (as implemented in flutter/flutter#122885).

## Related Issue

Windows engine implementation for flutter/flutter#87391.

Similar to:
- Linux: #42346
- Android: #42758
- macOS: #42878

## Tests

Adds 2 tests.
gaaclarke pushed a commit to gaaclarke/engine that referenced this pull request Aug 30, 2023
@bleroux @gaaclarke
[macOS] Return keyboard pressed state (flutter#42878)
0404a53 
## Description

This PR updates the macOS engine in order to answer to keyboard pressed state queries from the framework (as implemented in flutter/flutter#122885).

## Related Issue

macOS engine implementation for flutter/flutter#87391
Similar to:
- Linux: flutter#42346
- Android: flutter#42758

## Tests

Adds 2 tests.
gaaclarke pushed a commit to gaaclarke/engine that referenced this pull request Aug 30, 2023
@bleroux @gaaclarke
[Windows] Return keyboard pressed state (flutter#43998)
dd9c088 
## Description

This PR updates the Windows engine in order to answer to keyboard pressed state queries from the framework (as implemented in flutter/flutter#122885).

## Related Issue

Windows engine implementation for flutter/flutter#87391.

Similar to:
- Linux: flutter#42346
- Android: flutter#42758
- macOS: flutter#42878

## Tests

Adds 2 tests.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
2 more reviewers

@chinmaygarde chinmaygarde chinmaygarde approved these changes

@dkwingsmt dkwingsmt dkwingsmt approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
affects: desktop autosubmit Merge PR when tree becomes green via auto submit App platform-macos
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bleroux @chinmaygarde @dkwingsmt @loic-sharma