Add `KeyEventDeviceType` to `KeyData` #47315

gspencergoog · 2023-10-25T20:03:35Z

Description

Before we deprecate the RawKeyEvent code, it needs to provide parity in functionality. One thing that is missing is the eventSource field from the RawKeyEventDataAndroid class, which provides the device type for the event.

See https://developer.android.com/reference/android/view/InputDevice#SOURCE_KEYBOARD for an example.

This PR implements that support, and sets the source to KeyEventDeviceType.keyboard for platforms that don't provide this information. The main thing it does is add the enum KeyEventDeviceType, and a new field KeyData.deviceType.

Related Issues

RawKeyEvent and RawKeyboard, et al should be deprecated and removed flutter#136419

Tests

Updated tests to also read/write/verify this property.

dkwingsmt · 2023-10-27T21:05:00Z

lib/ui/key.dart

+///
+/// Not all platforms supply an accurate type.
+///
+/// Defaults to [keyboard].


I think default value should belong to a variable/field, not a type.

Yes, good point. I already removed this comment in other places, I just missed this one.

mossmana · 2023-11-03T22:41:03Z

@gspencergoog Before I pull down the branch and test. Do you happen to know if this change will also help to address flutter/flutter#120351 and similar issues? There is a whole class of issues related to the way certain IME key events vs. batch edits are handled.

gspencergoog · 2023-11-03T23:16:24Z

@mossmana No, that has to do with touch keyboards, which don't produce key events. This has to do with events generated by hardware keyboards.

mossmana · 2023-11-06T18:22:06Z

@mossmana No, that has to do with touch keyboards, which don't produce key events. This has to do with events generated by hardware keyboards.

Interesting, I didn't realize only hardware keyboards sent key events. Based on this comment flutter/flutter#120351 (comment), the Samsung soft keyboard does appear to send key events in some cases. Perhaps, the word key event is being overloaded.

gspencergoog · 2023-11-06T18:26:59Z

Yes, some soft keyboards also synthesize hardware key events, sometimes only for specific keys, sometimes for all keys, but it's very dependent upon the soft keyboard and its settings. And the default keyboards don't seem to produce any events (GBoard, iOS keyboard).

gspencergoog · 2023-11-06T21:03:06Z

lib/ui/key.dart

@@ -27,6 +67,7 @@ class KeyData {
    required this.logical,
    required this.character,
    required this.synthesized,
+    this.deviceType = KeyEventDeviceType.keyboard,


In order to pass the smoke test for the framework, this needs to be optional. Should we keep it that way, just to make migration easier? I'm leaning towards "yes", although all the other properties are required.

Yeah I think it's fine. At the end of the day it's PlatformDispatcher that creates this object, right?

Yes, usually, although you could write your own test framework or something that used it.

dkwingsmt

LGTM

…137979) flutter/engine@461d815...b1a5d77 2023-11-07 skia-flutter-autoroll@skia.org Roll Skia from 7e3119240ae4 to 9106e374e08d (1 revision) (flutter/engine#47732) 2023-11-07 skia-flutter-autoroll@skia.org Roll Dart SDK from 82731b940e7f to aded6314af29 (1 revision) (flutter/engine#47731) 2023-11-07 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from HQfFSkurc6-jKM2jh... to wmM4lS2lYcphFSHPV... (flutter/engine#47730) 2023-11-07 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from _vGlgDiKOrtlKrZAP... to NaQb_BU6PSbKXSAoU... (flutter/engine#47728) 2023-11-07 skia-flutter-autoroll@skia.org Roll Skia from bd5f57c9bd1a to 7e3119240ae4 (10 revisions) (flutter/engine#47726) 2023-11-06 gspencergoog@users.noreply.github.com Add `KeyEventDeviceType` to `KeyData` (flutter/engine#47315) 2023-11-06 skia-flutter-autoroll@skia.org Roll Skia from 2b218381e226 to bd5f57c9bd1a (2 revisions) (flutter/engine#47719) 2023-11-06 skia-flutter-autoroll@skia.org Roll Dart SDK from 07d4c9d85a5a to 82731b940e7f (1 revision) (flutter/engine#47717) 2023-11-06 skia-flutter-autoroll@skia.org Roll Skia from 77aeee3b81a5 to 2b218381e226 (1 revision) (flutter/engine#47715) 2023-11-06 30870216+gaaclarke@users.noreply.github.com [Impeller] scales blur coverage to match rendered output (flutter/engine#47621) 2023-11-06 jonahwilliams@google.com [Impeller] Fix EntityPassTarget::Flip with implict MSAA. (flutter/engine#47701) 2023-11-06 skia-flutter-autoroll@skia.org Roll Dart SDK from 9211fc6406bb to 07d4c9d85a5a (1 revision) (flutter/engine#47709) 2023-11-06 jonahwilliams@google.com [Impeller] fix drawVertices dest fast path to apply alpha. (flutter/engine#47695) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from _vGlgDiKOrtl to NaQb_BU6PSbK fuchsia/sdk/core/mac-amd64 from HQfFSkurc6-j to wmM4lS2lYcph 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 rmistry@google.com,zra@google.com 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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

This was introduced in flutter#47315. Internally, this lint breaks the build. See also https://errorprone.info/bugpattern/ImmutableEnumChecker. Fixes: b/309552840

This was introduced in #47315. Internally, this lint breaks the build with the following error: ``` shell/platform/android/io/flutter/embedding/android/KeyData.java:78: Error: DeviceType is an enum, which should be immutable, but field DeviceType.value is not final [ImmutableEnum] private long value; ~~~~~~~~~~~~~~~~~~~ ``` See also https://errorprone.info/bugpattern/ImmutableEnumChecker. Fixes: b/309552840 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

gmackall · 2024-07-11T18:12:49Z

shell/platform/android/io/flutter/embedding/android/KeyEmbedderResponder.java

    output.timestamp = event.getEventTime();
    output.type = type;
    output.logicalKey = logicalKey;
    output.physicalKey = physicalKey;
    output.character = character;
    output.synthesized = false;
+    output.deviceType = KeyData.DeviceType.kKeyboard;


From Android triage (specifically flutter/flutter#151308):

Does this line override the above switch statement in all cases?

Yeah, seems like it does. That line should probably be removed.

github-actions bot added the platform-web Code specifically for the web engine label Oct 25, 2023

gspencergoog force-pushed the key_event_device_type branch from a27fe79 to 50ef672 Compare October 25, 2023 20:09

github-actions bot added the platform-android label Oct 25, 2023

gspencergoog force-pushed the key_event_device_type branch 2 times, most recently from c5256ea to a5384f1 Compare October 25, 2023 23:31

github-actions bot added the platform-ios label Oct 25, 2023

gspencergoog force-pushed the key_event_device_type branch 3 times, most recently from 899ddb4 to d7db7bf Compare October 26, 2023 23:22

gspencergoog marked this pull request as ready for review October 26, 2023 23:29

gspencergoog requested review from cbracken and chinmaygarde as code owners October 26, 2023 23:29

gspencergoog requested review from dkwingsmt and removed request for chinmaygarde and cbracken October 26, 2023 23:29

gspencergoog force-pushed the key_event_device_type branch 3 times, most recently from adaeb93 to 17395ad Compare October 27, 2023 21:24

dkwingsmt reviewed Oct 27, 2023

View reviewed changes

gspencergoog force-pushed the key_event_device_type branch 3 times, most recently from ba2b0d7 to 67cc255 Compare October 27, 2023 23:52

reidbaker requested a review from mossmana November 2, 2023 18:17

gspencergoog added 3 commits November 6, 2023 12:33

Add KeyEventDeviceType to KeyData

0331050

Fix deviceType -> device_type in ObjC files.

cac7a2b

Fix Java null pointer exception

16a728a

gspencergoog added 3 commits November 6, 2023 12:33

Fix Java null pointer exception

20e38bc

Fix more tests

5b38188

Fix Java tests

8bb1eb7

gspencergoog force-pushed the key_event_device_type branch from 67cc255 to 8bb1eb7 Compare November 6, 2023 20:34

Make deviceType optional for now.

00f621a

gspencergoog commented Nov 6, 2023

View reviewed changes

Make deviceType optional

82828cb

dkwingsmt approved these changes Nov 6, 2023

View reviewed changes

gspencergoog added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 6, 2023

auto-submit bot merged commit cc925b0 into flutter:main Nov 6, 2023
24 checks passed

engine-flutter-autoroll mentioned this pull request Nov 7, 2023

Roll Flutter Engine from 461d815b48a0 to b1a5d772792f (13 revisions) flutter/flutter#137979

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 7, 2023

cc925b002 Add KeyEventDeviceType to KeyData (flutter/engine#47315)

34383b6

jiahaog added a commit to jiahaog/flutter-engine that referenced this pull request Nov 7, 2023

Fix ImmutableEnum lint

37e7374

This was introduced in flutter#47315. Internally, this lint breaks the build. See also https://errorprone.info/bugpattern/ImmutableEnumChecker. Fixes: b/309552840

jiahaog mentioned this pull request Nov 7, 2023

Fix ImmutableEnum lint #47739

Merged

8 tasks

gmackall reviewed Jul 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `KeyEventDeviceType` to `KeyData` #47315

Add `KeyEventDeviceType` to `KeyData` #47315

gspencergoog commented Oct 25, 2023 •

edited

Loading

dkwingsmt Oct 27, 2023

gspencergoog Oct 27, 2023

mossmana commented Nov 3, 2023 •

edited

Loading

gspencergoog commented Nov 3, 2023

mossmana commented Nov 6, 2023

gspencergoog commented Nov 6, 2023 •

edited

Loading

gspencergoog Nov 6, 2023

dkwingsmt Nov 6, 2023

gspencergoog Nov 6, 2023

dkwingsmt left a comment

gmackall Jul 11, 2024

gspencergoog Jul 22, 2024

Add KeyEventDeviceType to KeyData #47315

Add KeyEventDeviceType to KeyData #47315

Conversation

gspencergoog commented Oct 25, 2023 • edited Loading

Description

Related Issues

Tests

dkwingsmt Oct 27, 2023

Choose a reason for hiding this comment

gspencergoog Oct 27, 2023

Choose a reason for hiding this comment

mossmana commented Nov 3, 2023 • edited Loading

gspencergoog commented Nov 3, 2023

mossmana commented Nov 6, 2023

gspencergoog commented Nov 6, 2023 • edited Loading

gspencergoog Nov 6, 2023

Choose a reason for hiding this comment

dkwingsmt Nov 6, 2023

Choose a reason for hiding this comment

gspencergoog Nov 6, 2023

Choose a reason for hiding this comment

dkwingsmt left a comment

Choose a reason for hiding this comment

gmackall Jul 11, 2024

Choose a reason for hiding this comment

gspencergoog Jul 22, 2024

Choose a reason for hiding this comment

Add `KeyEventDeviceType` to `KeyData` #47315

Add `KeyEventDeviceType` to `KeyData` #47315

gspencergoog commented Oct 25, 2023 •

edited

Loading

mossmana commented Nov 3, 2023 •

edited

Loading

gspencergoog commented Nov 6, 2023 •

edited

Loading