Skip to content

Relax HarwareKeyboard assert on first received event #131258

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
bleroux wants to merge 1 commit into from
Closed

Relax HarwareKeyboard assert on first received event #131258

bleroux wants to merge 1 commit into from

Conversation

@bleroux
Copy link
Contributor

@bleroux bleroux commented Jul 25, 2023 •
edited
Loading

Description

This PR updates the HardwareKeyboard to not assert for the first up or down event.

Since #122885 (and the related PRs on engine side), the HardwareKeyboard is initialized with the known keyboard state returned by the engine.
Because the engine sent keyboard event using channel buffers which are created with the default buffer size (which is 1), the first event received on framework side might be an obsolete buffered event.

This PR take a no-risk approach which is to relax two kinds of assert for the very first received event. This logic will only apply to debug mode and apply for only two cases:

  • a buffered up event: if a key is pressed and released before the framework starts, the initial state will be empty but the framework will receive the buffered up event.
  • a buffered down event: when a key is pressed while the framework starts, the initial state (queried by a call from the framework to the engine) already reflects that the key is pressed.

An alternative would be to set buffer size to 0 but it is risky because it will apply to release mode and none of the existing channels does set its buffer size to 0.

Related Issue

Fixes #125975

Tests

Adds 2 tests.

@github-actions github-actions bot added the framework flutter/packages/flutter repository. See also f: labels. label Jul 25, 2023
@bleroux bleroux force-pushed the relax_hardware_keyboard_assert_on_first_event branch 2 times, most recently from 2ebc92e to d023fe3 Compare July 25, 2023 14:59
@bleroux bleroux marked this pull request as draft July 25, 2023 19:25
@bleroux bleroux force-pushed the relax_hardware_keyboard_assert_on_first_event branch 2 times, most recently from 8e07e5d to c1da590 Compare July 25, 2023 20:05
@bleroux bleroux force-pushed the relax_hardware_keyboard_assert_on_first_event branch from c1da590 to 19db1c6 Compare July 25, 2023 20:16
@bleroux bleroux marked this pull request as ready for review July 25, 2023 21:09
@bleroux bleroux requested a review from dkwingsmt July 25, 2023 21:09
@bleroux bleroux mentioned this pull request Jul 26, 2023
Merged
@Hixie
Copy link
Contributor

Hixie commented Jul 27, 2023

Setting the buffer size to zero actually seems like a more resilient solution, since that would also work for other frameworks. With the buffer size set to one, we should document that the first message might be obsolete, and any other frameworks built on the engine would have to know to (maybe!) ignore the first event.

I'm not sure I really understand the risk of setting it to zero? If there's bugs with doing that we should find out!

@bleroux
Copy link
Contributor Author

bleroux commented Jul 28, 2023

I'm not sure I really understand the risk of setting it to zero?

My risk evaluation is not based on known bugs, but simply on the fact that no existing channel buffer is resized to 0. So we might hit bugs or do mistakes, and if we do, it might impact stability in release mode. The proposed solution in this PR is a less satisfying fix but it has no risk on release mode.

Another consideration is also the time needed to implement and review those changes. The 'set the buffer size to 0' way will require a lot more work (PRs on each platforms). For instance, when I tried this, on Android, I noticed that the existing code to ask for a resize was obsolete (and filed flutter/engine#41982).

This is work i'm interested in, but maybe we should first merged this PR with its more simple solution? Otherwise, it will take time before completing this on all platforms and I think we should tackle those warnings as soon as possible because:

  • they are not great for the Flutter development experience.
  • they bring confusion on several keyboard related issues: those assertions are very useful to get bug reports on various keyboard edge cases but because of the assertion being thrown during restart, people hitting the restart issue often comment on other keyboard issues which adds a lot of noise on them.

@Hixie
Copy link
Contributor

Hixie commented Jul 31, 2023

I encourage you to embrace the yak shave. We may find new bugs, but that's always a risk when programming. If we always take the "safe" but les technically-sound route, eventually our entire codebase will be less-technically-sound, and then we will fail.

A few more months of this bug is worth the benefit of having a long-term more stable base for us to build on.

@bleroux
Copy link
Contributor Author

bleroux commented Aug 1, 2023

Great! Let's embrace the yak shave 😄

Closing this PR and I will try to file several engine PRs.

@bleroux bleroux closed this Aug 1, 2023
@bleroux bleroux mentioned this pull request Dec 1, 2023
Open
2 tasks
@bleroux bleroux deleted the relax_hardware_keyboard_assert_on_first_event branch February 24, 2025 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dkwingsmt dkwingsmt Awaiting requested review from dkwingsmt

Assignees

No one assigned

Labels

framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Pressing and releasing a key before the framework has started throws

2 participants

@bleroux @Hixie