-
Notifications
You must be signed in to change notification settings - Fork 6k
Hardware keyboard: Web, embedder, and dart:ui #23466
Conversation
stuartmorgan-g
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect to see tests for the new embedder APIs (i.e., changes in embedder/test/) as part of this.
shell/platform/embedder/embedder.h
Outdated
| typedef struct { | ||
| /// The size of this struct. Must be sizeof(FlutterKeyEvent). | ||
| size_t struct_size; | ||
| // Timestamp in microseconds. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment should be very clear about the requirements here. E.g., is using a monotonic clock a requirement, or is it okay for later events to have earlier timestamps when there's a time change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the time stamp of key events are defined as FlutterEngineGetCurrentTime. Currently I'm using as much time from native as possible in my PRs, wondering if it might provide extra precision or information to applications with certain requirement. Do you recommend me to switch to FlutterEngineGetCurrentTime?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not if we don't need it; I just think we should be explicit about what is (or isn't required). If we're okay with native event time, we should say that.
Is it the case that Flutter itself will be fine with non-monotonic event times?
shell/platform/embedder/embedder.h
Outdated
| uint64_t physical; | ||
| // The logical key of the event, usually the effect of the key without | ||
| // modifier, but might include modifier if the information is not available. | ||
| // Can be 0 for empty. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not obvious what this means exactly; what is the definitive source of numbers for logical keys?
Since this API surface is intended for use by third-party developers as well, it's important not to assume too much context in the comments.
| bool _onKeyData(ui.KeyData data) { | ||
| bool? result; | ||
| EnginePlatformDispatcher.instance.invokeOnKeyData(data, | ||
| (bool handled) { result = handled; }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this callback called synchronously? I'm worried that _onKeyData may return before the callback gets a chance to update result.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is supposed to. If the code is returned before the handler is called, result will be null and the code will throw on the ! of the return statement. Let me add a comment to explain it.
stuartmorgan-g
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes all LGTM!
Description
This PR is part of Hardware Keyboard project, specifically the embedder API, dart:ui API, and Web engine parts. The embedder and dart:ui parts are the foundation to other engine changes, while Web is required to change simultaneously for the API mirroring check.
The hardware keyboard API requires each platform to convert native events to "key data" that meets Flutter's specification and send them via the embedder API. The framework is then able to receive
ui.KeyDatas in a regular, unified form, and perform the simplest 1-to-1 conversion intoKeyEvent(to be added) and assertions.The key event model is described at here. Simply put,
Public API changes
This PR adds the following APIs to dart:ui:
KeyChangeKeyDataKeyDataCallbackPlatformDispatcher.onKeyDataSingletonFlutterWindow.onKeyDataThis PR adds the following APIs to embedder.h:
FlutterKeyEventKindFlutterKeyEventFlutterEngineSendKeyEventFlutterEngineSendKeyEventFnPtrFlutterEngineProcTable.SendKeyEventAlthough the key mapping file claims to be auto-generated, the codegen script is not checked in until flutter/flutter#73440.
Related Issues
Tests
I added the following tests:
For web engine:
For embedder:
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.Reviewer Checklist
Breaking Change
Did any tests fail when you ran them? Please read handling breaking changes.