Send key events to flutter #73

maun · 2018-06-20T10:06:38Z

Start of implementation for #72

Currently the code pretends to be android, also the scan code and modifiers are not sent. Ideally there would either be a matching platform in flutter, or a generic/desktop platform.
It would also be a good idea to look at the iOS code base how they forward events to multiple plugins.

stuartmorgan-g

Structurally this looks really good! The comments are mostly API and visibility notes.

A general comment: something I've started trying to do with every plugin is to ensure that it's easily testable in the example app; this helps with regression testing, and with bring-up on new platforms. (Text input is the last thing that hasn't been fixed, but adding a text field to the example app is on my to-do list.) Could you add something to the app that would allow trying this out?

Longer term I think the example will need to turn into a gallery-style app since things are getting crowded, but for now maybe a button that pushes a new screen that's a raw key event listener wrapping some kind of feedback UI showing that it's working--perhaps simply displaying the key code and whether it's a key up or down for each incoming event.

stuartmorgan-g · 2018-06-21T21:15:45Z

macos/library/FLEKeyEventPlugin.m

+
+#import "FLEKeyEventPlugin.h"
+
+#import <objc/message.h>


This doesn't appear to be used.

stuartmorgan-g · 2018-06-21T21:19:21Z

macos/library/FLEKeyEventPlugin.m

+
+- (void)handleMethodCall:(FLEMethodCall *)call result:(FLEMethodResult)result {
+    NSString *method = call.methodName;
+    NSLog(@"Unhandled text input method '%@'", method);


Per the documentation for handleMethodCall:result:, implementations must call |result|.

You can replace all of this with just:

result(FLEMethodNotImplemented);

since that will result in an error that the Flutter side would presumably log.

stuartmorgan-g · 2018-06-21T21:26:51Z

macos/library/FLEKeyEventPlugin.m

+}
+
+- (void)dispatchKeyEvent:(NSString *)type event:(NSEvent *)event {
+    // TODO: Do not use 'android'


Given that the only options are android and fuchsia at the moment, it's not entirely clear what course of action this TODO is suggesting; a TODO should have enough information that someone else encountering it can understand what needs to be done. Do you have a path forward in mind for this? E.g., a bug you could reference for adding some option other than those two that would better match desktop?

(Although I would expect that this channel would also fall under #47 and at some point in the future be replaced with a formal embedding API, so that may be the path forward.)

Relatedly, how deeply have you investigated the effects of the keymap type on the implementation? For instance, what differences would result if we passed fuchsia here instead?

@chinmaygarde Any thoughts on the implications of implementing this using the android map (in the short term)?

I added the issue link to the TODO comment.

Using "fuchsia" instead of android would change the class of the data property of the KeyEvent. If we change this handlers in the dart code would need to be adapted. Currently the handler in the example expects RawKeyEventDataAndroid. Also I am not filling all expected properties of the data class.
The Fuchsia implementation has no keyCode field, but instead hidUsage, not sure how to convert this.

Also I am not filling all expected properties of the data class.

Is there a reason you're not passing modifier key state via metaState? That seems like it would be necessary data for many uses.

This wasn't resolved, at it still seems like passing modifier keys will be important, but I'm okay with leaving that for follow-up work.

stuartmorgan-g · 2018-06-21T21:30:44Z

macos/library/FLEViewController.h

+ * Sends a platform message to the Flutter engine on the given channel.
+ * @param message The raw message.
+ */
+- (void)dispatchMessage:(nonnull NSDictionary*)message onChannel:(nonnull NSString *)channel;


Does Flutter use raw messages for any "public" (i.e., supported and stable, unlike these internal framework channels) APIs?

I'd like to keep the public API surface as clean as possible, so unless there's an expectation that stand-alone plugins would need this functionality it would be preferable to put this in FLEViewController+Internal.h as well.

There are flutter plugins defining and using EventChannels which as I understand are just raw messages. It looks like PlattformMessages were changed to FlutterMessageChannel and FlutterMethodChannel. Maybe something similar should be done here.

There are flutter plugins defining and using EventChannels which as I understand are just raw messages.

EventChannels still use a MethodCodec, which means they follow the method call/response structure that the desktop plugins now use as well. The primary difference between the current desktop API and the iOS and Android APIs is that currently desktop hard-codes use of the JSON method codec.

Do you have examples of plugins or API that doesn't use the MethodCodec format, as with the new API you've added?

Maybe something similar should be done here.

That's planned, and covered by Issue #67. The recent plugin API rework was the first phase of aligning with the standard Flutter structure.

stuartmorgan-g · 2018-06-21T21:42:46Z

macos/library/FLEViewController.m

  }
  return self;
 }

+- (void)setup {


Methods called from init are a potential foot-gun (see the style guide discussion) so when it's done it needs to be extremely clear; that's why initPlugins starts (unusually) with init, and why it's clearly documented as being called from init.

Adding a layer of indirection makes that much less clear, so I would much rather the creation of the set (which is, after all, only used by plugins) be folded into initPlugins.

stuartmorgan-g · 2018-06-21T21:43:09Z

macos/library/FLEViewController.m

  }
  return self;
 }

+- (void)setup {
+    self.additionalKeyResponders = [NSMutableSet new];


http://google.github.io/styleguide/objcguide.html#avoid-new

stuartmorgan-g · 2018-06-21T21:45:27Z

macos/library/FLEViewController.h

+/**
+ * A list of additional responders to keyboard events. Keybord events are forwarded to all of them.
+ */
+@property NSMutableSet<NSResponder*> *additionalKeyResponders;


I don't think embedders or stand-alone plugins should be able to interject themselves into the process of handing native keyboard events off to Flutter, so this should be declared in an FLEViewController+Internal.h that's marked as a project header.

Also, even for internal use, the API would be much safer if the variable were private, and there were public methods to add or remove an object. That way it's not possible to accidentally stomp other responders when adding or removing yourself. (As a general rule, public, mutable collections are problematic in the API surface of a class. Wrapped mutators and/or a readonly, copy, immutable collection property, ensure that all changes are mediated by the class.)

stuartmorgan-g · 2018-06-21T21:55:16Z

macos/library/FLEViewController.m

+#pragma mark - NSResponder key events
+
+- (void)keyDown:(NSEvent *)event {
+    for (NSResponder* responder in self.additionalKeyResponders) {


I haven't dug into the Flutter internals here; is there any interaction between the handling on the two different keyboard channels? For instance, does Flutter expect key events to happen in a specific order relative to the higher-level input events? I'd like to be sure this isn't going to regress text input.

This seems to work as is, when using text input and key events the order is as follows:

KeyDown

Text

KeyUp

Because the keyevent plugin is added first it handles the event first. I changed the NSMutableSet to NSOrderedMutableSet to guarantee this.

stuartmorgan-g · 2018-06-21T21:58:59Z

macos/library/FLEViewController.m

 }

+#pragma mark - NSResponder key events


I'd rather use a single group for protocol implementations in general, so just:
#pragma mark - NSResponder
and no second one below.

(There are also some things below this point that aren't NSResponder implementation, but that's a pre-existing ordering problem in this file that I can clean up in a follow-up CL)

stuartmorgan-g · 2018-06-21T22:05:02Z

macos/library/FLEKeyEventPlugin.m

+    NSDictionary* message = @{
+                                @"keymap": @"android",
+                                @"type": type,
+                                @"keyCode": [NSNumber numberWithUnsignedShort: event.keyCode],


That value can just be @(event.keyCode)

… the keyevent plugin is the first entry

maun · 2018-07-02T14:33:18Z

Thanks for your review @stuartmorgan. Can you recheck the PR? The biggest problem is that I misuse the android key event data, not even including all properties, but I do not know how to solve this. Also added some answers to your comments but GitHub hides them because i changed the code.

In the example there seems to be a minor issue: Selecting the text field, then changing to the new page and back, the text field looks like it has focus but you have to select it again to type something. Not sure why requesting the focus does not unfocus the old one.

stuartmorgan-g

A couple a substantive things I didn't catch before, but mostly nits. The changes all look really good. Thanks for the test page in the example app!

stuartmorgan-g · 2018-07-09T18:03:03Z

example_flutter/lib/keyboard_test_page.dart

+import 'package:flutter/scheduler.dart';
+import 'package:flutter/services.dart';
+
+class KeyboardTestPage extends StatefulWidget {


Please document at least the public class (which is an analyzer failure under the current settings).

stuartmorgan-g · 2018-07-09T18:03:52Z

example_flutter/lib/keyboard_test_page.dart

+  @override
+  Widget build(BuildContext context) {
+    return new Scaffold(
+      appBar: AppBar(


It would be nice to give this a title to provide some context.

stuartmorgan-g · 2018-07-09T18:04:27Z

example_flutter/lib/keyboard_test_page.dart

+    );
+  }
+
+  void onKeyEvent(RawKeyEvent event) async {


Why is this async?

stuartmorgan-g · 2018-07-09T18:06:25Z

example_flutter/lib/keyboard_test_page.dart

+
+  void onKeyEvent(RawKeyEvent event) async {
+    int keyCode;
+    bool isKeyDown;


Prefer to declare variables as close to their first use as possible, so keyCode attached to the second switch below, and this attached to the first switch.

stuartmorgan-g · 2018-07-09T18:09:18Z

example_flutter/lib/main.dart

@@ -170,6 +171,10 @@ class _MyHomePage extends StatelessWidget {
            ),
            TextInputTestWidget(),
            FileChooserTestWidget(),
+            new RaisedButton(child: new Text('Test raw keyboard events'), onPressed: () {


Please dartfmt this file; this line is too long.

stuartmorgan-g · 2018-07-09T18:14:48Z

macos/library/FLEKeyEventPlugin.m

+  result(FLEMethodNotImplemented);
+}
+
+- (void)dispatchKeyEvent:(NSString *)type event:(NSEvent *)event {


This needs a declaration comment (per style guide). It should also follow ObjC naming conventions that the word before the : names the param, so dispatchKeyEvent:(...)event ofType:(...)type

stuartmorgan-g · 2018-07-09T18:18:45Z

macos/library/FLEViewController+Internal.h

+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+


This should import FLViewController.h

stuartmorgan-g · 2018-07-09T18:25:05Z

macos/library/FLEViewController.h

- * Sends a platform message to the Flutter engine on the given channel.
+ * Adds a responder for keyboard events. Key up and key down events are forwarded to all added responders.
+ */
+- (void)addKeyResponder:(nonnull NSResponder*)responder;


Per the first part of my comment about this change in the previous round, I don't think arbitrary third-party plugins should be able to be key event handlers, so these should be in the internal header.

stuartmorgan-g · 2018-07-09T18:27:34Z

macos/library/FLEViewController.m

+
+    FlutterResult result = FlutterEngineSendPlatformMessage(_engine, &platformMessage);
+    if (result != kSuccess) {
+        NSLog(@"Flutter engine send unsuccessful response for message %@: (%d).", message, result);


Typo: send->sent

Also, since the dictionary could be a lot to display, I'd put the error response before it:
Flutter engine sent unsuccessful response (%d) for message %@

stuartmorgan-g · 2018-07-09T18:32:34Z

macos/library/FLEKeyEventPlugin.m

+                            @"keymap": @"android",
+                            @"type": type,
+                            @"keyCode": @(event.keyCode),
+                            };


Pulling this into a review comment since GitHub's UI makes it easy to miss replies to comments on outdated changes:

Is there a reason you're not passing modifier key state via metaState? That seems like it would be necessary data for many uses.

No particular reason. I did not need it and was not sure about the encoding.

stuartmorgan-g · 2018-08-06T20:49:31Z

Is this still something you're interested in landing? The last review round is mostly just minor changes, so it's very close.

ghost · 2018-08-10T09:25:26Z

@maun Rearding #73 (comment), you commented that you had trouble with key events. Did you ever get an answer about how to resolve this ?

# Conflicts: # macos/library/FLEViewController.h # macos/library/FLEViewController.m

maun · 2018-08-10T13:02:11Z

@stuartmorgan I was rather busy before, but now got around to address your review, sorry for the delay.

@gedw99 I did not look further into this.

- Add missing license header to new Dart file. - Resolve a Dart analyzer warning. - Revert changes to the now-removed pubspec.lock to avoid conflicts.

stuartmorgan-g · 2018-08-21T03:32:21Z

Thanks for the contribution, and sorry for the long delay!

stuartmorgan-g reviewed Jun 21, 2018

View reviewed changes

Send key events to flutter

a6c03b6

maun force-pushed the keyevent branch from b6bdb28 to a6c03b6 Compare July 2, 2018 11:55

maun added 4 commits July 2, 2018 14:42

Add example for raw keyboard events

c46b0ab

Handle keyevent PR#73 comments

21e600a

Use NSMutableOrderedSet for additionalKeyResponders to guarantee that…

485af71

… the keyevent plugin is the first entry

Move dispatchMethod into internal header file for FLEViewController

70160cc

maun changed the title ~~[WIP] Send key events to flutter~~ Send key events to flutter Jul 2, 2018

stuartmorgan-g reviewed Jul 9, 2018

View reviewed changes

maun added 2 commits August 10, 2018 11:58

Merge branch 'master' into keyevent

7a0b17c

# Conflicts: # macos/library/FLEViewController.h # macos/library/FLEViewController.m

Address review comments and run clang-format.

3556782

Minor cleanup

cb9fe6d

- Add missing license header to new Dart file. - Resolve a Dart analyzer warning. - Revert changes to the now-removed pubspec.lock to avoid conflicts.

stuartmorgan-g approved these changes Aug 21, 2018

View reviewed changes

stuartmorgan-g merged commit 899af73 into google:master Aug 21, 2018

github-actions bot locked as resolved and limited conversation to collaborators Aug 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Send key events to flutter #73

Send key events to flutter #73

maun commented Jun 20, 2018

stuartmorgan-g left a comment

stuartmorgan-g Jun 21, 2018

stuartmorgan-g Jun 21, 2018

stuartmorgan-g Jun 21, 2018

maun Jul 2, 2018

stuartmorgan-g Jul 9, 2018

stuartmorgan-g Aug 21, 2018

stuartmorgan-g Jun 21, 2018

maun Jul 2, 2018

stuartmorgan-g Jul 9, 2018

stuartmorgan-g Jun 21, 2018

stuartmorgan-g Jun 21, 2018

stuartmorgan-g Jun 21, 2018

stuartmorgan-g Jun 21, 2018

maun Jul 2, 2018

stuartmorgan-g Jun 21, 2018

stuartmorgan-g Jun 21, 2018

maun commented Jul 2, 2018

stuartmorgan-g left a comment

stuartmorgan-g Jul 9, 2018

stuartmorgan-g Jul 9, 2018

stuartmorgan-g Jul 9, 2018

stuartmorgan-g Jul 9, 2018

stuartmorgan-g Jul 9, 2018

stuartmorgan-g Jul 9, 2018

stuartmorgan-g Jul 9, 2018

stuartmorgan-g Jul 9, 2018

stuartmorgan-g Jul 9, 2018

stuartmorgan-g Jul 9, 2018

maun Aug 10, 2018

stuartmorgan-g commented Aug 6, 2018

ghost commented Aug 10, 2018

maun commented Aug 10, 2018 •

edited

Loading

stuartmorgan-g commented Aug 21, 2018


		#import "FLEKeyEventPlugin.h"

		#import <objc/message.h>

Send key events to flutter #73

Send key events to flutter #73

Conversation

maun commented Jun 20, 2018

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maun commented Jul 2, 2018

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartmorgan-g commented Aug 6, 2018

ghost commented Aug 10, 2018

maun commented Aug 10, 2018 • edited Loading

stuartmorgan-g commented Aug 21, 2018

maun commented Aug 10, 2018 •

edited

Loading