Relax HarwareKeyboard assert on first received event#131258
Relax HarwareKeyboard assert on first received event#131258bleroux wants to merge 1 commit intoflutter:masterfrom
Conversation
github-actions
bot
added
the
framework
bleroux
force-pushed
the
relax_hardware_keyboard_assert_on_first_event
branch
2 times, most recently
from
2ebc92e to
d023fe3
Compare
bleroux
force-pushed
the
relax_hardware_keyboard_assert_on_first_event
branch
2 times, most recently
from
8e07e5d to
c1da590
Compare
bleroux
force-pushed
the
relax_hardware_keyboard_assert_on_first_event
branch
from
c1da590 to
19db1c6
Compare
|
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! |
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:
|
|
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. |
|
Great! Let's embrace the yak shave 😄 Closing this PR and I will try to file several engine PRs. |
2 participants
Description
This PR updates the
HardwareKeyboardto not assert for the first up or down event.Since #122885 (and the related PRs on engine side), the
HardwareKeyboardis 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:
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.