[WIP] Mouse cursor (engine) by dkwingsmt · Pull Request #13152 · flutter/engine

dkwingsmt · 2019-10-15T21:38:45Z

Description

This is a WIP about adding mouse cursor control.

The engine section of the mouse cursor system is basically a bridge between the framework section, which it talks to via a method channel, and the system calls. Although all platforms use the same channel, each platform has their own interfaces depending on how features are provided by the platform. That being said, all platform interfaces are designed with the following principles:

The argument is a Map<String, dynamic>.
States are maintained by the framework if possible.
For example, macOS requires hide and unhide calls to be balanced. This is counted and enforced by the framework, while the engine blindly follows commands.
Exception: When we implement custom cursors in the future, the cursor objects have to be cached and maintained by the engine.
Argument values should be system constants whenever possible.
For example, Flutter has MouseCursorSystemShape which are platform-independent constants that represent system cursors. The mapping between them to Android/GLFW platform constants are done by the framework, so that the engine can directly use the platform constants it receives.
Exception: On macOS the system mouse cursors are not represented by integer constants but by objects, therefore we have to define our own system constants.

Related Issues

Framework: [WIP] Mouse cursor (framework) flutter#42797

mklim

I gave this PR a high-level review and didn't closely vet for readability/style nits since this is still a WIP.

One thing I noticed is that in the engine there's a v1 and a v2 embedding. These changes need to be added to both places, because we're currently supporting both.

Depending on whether context needs to be tied to an activity, and assuming it doesn't, I think it makes more sense for the channel to be initialized and live in FlutterEngine in v2 of the embedding instead of FlutterView. If it really needs a context then it should probably be in the v2 FlutterView, shell/platform/android/io/flutter/embedding/android/FlutterView.java.

Currently this PR is just adding it to the v1 "FlutterView," shell/platform/android/io/flutter/view/FlutterView.java, which I think does make sense as the right place for the v1 embedding.

You can look up LifecycleChannel for an example of how something is used in both places.

Other than that the basic approach doesn't seem to have any issues to me.

/cc @matthew-carroll, I think it would be good for him to verify this too.

mklim · 2019-10-16T00:19:41Z

shell/platform/android/io/flutter/embedding/engine/systemchannels/MouseCursorChannel.java

+  private MouseCursorMethodHandler mouseCursorMethodHandler;
+
+  public MouseCursorChannel(@NonNull DartExecutor dartExecutor) {
+    channel = new MethodChannel(dartExecutor, CHANNEL_NAME, JSONMethodCodec.INSTANCE);


Any reason why JSON instead of the Standard method codec here?

Edit: I looked at the other channels and they seem to vary. I'm not sure which is preferred but plugins always uses Standard. /cc @matthew-carroll in case he knows.

Unfortunately, I don't know. I added structure around the channels, but didn't change any of them.

mklim · 2019-10-16T00:21:09Z

shell/platform/android/io/flutter/embedding/engine/systemchannels/MouseCursorChannel.java

+        // Argument list is [device, cursor]. Since Android only supports one pointer device,
+        // we can ignore argument 0.
+        cursor = argumentList.getInt(1);
+      } catch (Exception e) {


readability nit: It's generally not great to catch Exception as a top level type. This ends up potentially masking completely unrelated issues to actually parsing the message. I would change this into the specific parsing exceptions that are a concern.

mklim · 2019-10-16T00:25:58Z

shell/platform/android/io/flutter/embedding/engine/systemchannels/MouseCursorChannel.java

+  }
+
+  @NonNull
+  @VisibleForTesting


readability nit: I wouldn't make this @VisibleForTesting protected unless you really need to expose this for a test and just for a test for some reason. In the state this PR is now it would be better to make this private.

mklim · 2019-10-16T00:30:03Z

shell/platform/android/io/flutter/plugin/mouse/MouseCursorController.java

+   * The given {@code dartExecutor} is used to construct the method channel with which the
+   * platform communicates with the framework.
+   *
+   * The given {@code context} is used to create system cursor object. Usually it's the current


Usually it's the current activity.

Does it need to be linked to an activity, or is it okay to be just a system context? I'm asking because with the v2 add-to-app embedding changes, it's possible for engine code like this to be executing even when there is no visible UI or activity to speak of.

mklim · 2019-10-16T00:32:06Z

shell/platform/android/io/flutter/plugin/mouse/MouseCursorController.java

+  @NonNull
+  private View view;
+
+  // The target application context.


"application context" would mean that this is never an activity, so this is a little confusing. (Android is confusing in general about this. See the #getApplicationContext docs.)

mklim · 2019-10-16T00:58:43Z

shell/platform/android/io/flutter/plugin/mouse/MouseCursors.java

+
+  /* Displays no cursor at the pointer.
+   */
+  public final static int none = 0x334c4a4c;


This is hard to read and understand. Would Strings make sense here? What's the significance of these specific int values?

mklim · 2019-10-16T01:02:02Z

shell/platform/android/io/flutter/plugin/mouse/MouseCursors.java

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package io.flutter.plugin.mouse;


Is this expected to be used in plugins? I think this would maybe make more sense in io.flutter.embedding.engine.systemchannels.

matthew-carroll · 2019-10-16T21:23:40Z

shell/platform/android/io/flutter/embedding/engine/systemchannels/MouseCursorChannel.java

+  private MouseCursorMethodHandler mouseCursorMethodHandler;
+
+  public MouseCursorChannel(@NonNull DartExecutor dartExecutor) {
+    channel = new MethodChannel(dartExecutor, CHANNEL_NAME, JSONMethodCodec.INSTANCE);


Unfortunately, I don't know. I added structure around the channels, but didn't change any of them.

matthew-carroll · 2019-10-16T21:24:17Z

shell/platform/android/io/flutter/plugin/mouse/MouseCursorController.java

+import io.flutter.plugin.mouse.MouseCursors;
+
+/**
+ * Manages mouse cursor of the mouse device.


Please include <p> tags in all javadoc comments, as shown in other files.

matthew-carroll · 2019-10-16T21:25:10Z

shell/platform/android/io/flutter/view/FlutterView.java

        mImm = (InputMethodManager) getContext().getSystemService(Context.INPUT_METHOD_SERVICE);
        PlatformViewsController platformViewsController = mNativeView.getPluginRegistry().getPlatformViewsController();
        mTextInputPlugin = new TextInputPlugin(this, dartExecutor, platformViewsController);
+        mMouseCursorController = new MouseCursorController(this, dartExecutor, context);


Are changes needed in the new FlutterView, too?

matthew-carroll · 2019-10-16T21:26:57Z

shell/platform/android/io/flutter/plugin/mouse/MouseCursorController.java

+  ) {
+    channel = new MouseCursorChannel(dartExecutor);
+    channel.setMethodHandler(channelHandler);
+    this.view = view;


Rather than manipulate a View within this class, I would recommend defining a delegate interface and then implement that interface within the old and new FlutterViews. That way, there is no magic or mystery when reading FlutterView as to what it's doing. When we move control of something like a View to other classes, sooner or later we get surprised by conflicts or unexpected behaviors.

dkwingsmt · 2019-11-07T23:26:44Z

Close it in favor of separate PRs for each platform. Also I have some major changes in progress.

dkwingsmt added 5 commits October 11, 2019 18:09

First workable version

c748162

Merge remote-tracking branch 'upstream/master' into mouse-cursor

a33e246

Change to single cursor

d9098e6

Remove redundant code

641322b

Remove redundant code

5332208

dkwingsmt added platform-android Work in progress (WIP) Not ready (yet) for review! affects: desktop labels Oct 15, 2019

googlebot added the cla: yes label Oct 15, 2019

Remove redundant import

a528f57

dkwingsmt mentioned this pull request Oct 15, 2019

[WIP] Mouse cursor (framework) flutter/flutter#42797

Closed

9 tasks

mklim self-requested a review October 15, 2019 22:01

mklim self-assigned this Oct 15, 2019

mklim reviewed Oct 16, 2019

View reviewed changes

mklim requested a review from matthew-carroll October 16, 2019 21:09

matthew-carroll reviewed Oct 16, 2019

View reviewed changes

dkwingsmt added 9 commits October 18, 2019 10:41

Merge remote-tracking branch 'upstream/master' into mouse-cursor

65a4fb8

Merge remote-tracking branch 'upstream/master' into mouse-cursor

4cba5c6

Merge remote-tracking branch 'upstream/master' into mouse-cursor

6fa78f2

First impl of GLFW (not tested)

a24cfd5

Merge remote-tracking branch 'upstream/master' into mouse-cursor

b1fde68

Adapt to latest API

1accba6

First implementation of mac (untested)

a26702f

Working macOS impl

830d862

Adapt to latest API

f54d161

dkwingsmt closed this Nov 7, 2019

dkwingsmt mentioned this pull request May 26, 2020

System mouse cursor: Android #18569

Merged

dkwingsmt deleted the mouse-cursor branch January 6, 2022 01:46

Conversation

dkwingsmt commented Oct 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Uh oh!

mklim left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkwingsmt commented Nov 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dkwingsmt commented Oct 15, 2019 •

edited

Loading