[WIP] Mouse cursor (framework) by dkwingsmt · Pull Request #42797 · flutter/flutter

dkwingsmt · 2019-10-15T21:33:23Z

Description

This PR is the framework side of adding mouse cursor support. The engine side (see Related Issues) is required for mouse cursor to work, but isn't required to run.

The framework side of the system consists of 3 parts:

MouseCursor, which are stateless, platform-independent definitions of a kind of cursor. They are extended by Flutter and app developers, and used by widgets. "A kind of cursor" here stands for an arrow, a pointing hand, etc.
MouseCursorPlatformDelegate, which are interfaces for operations related to mouse cursor on different platforms. They are extended by Flutter, one for each platform that supports mouse cursor, and their implementations are tightly related to their shells.
MouseCursorManager, which is a class for the Flutter framework (more specifically, MouseTracker) to dispatch mouse cursor changes.

Feature-wise, this PR includes:

System cursor, as well as a limited collection of constants SystemCursors.
Platform delegate on Android, GLFW, and macOS.
Adds mouseCursor to InkWell, InkResponse, RawMaterialButton and FloatingActionButton. (It's there for the convenience of testing, up to discussion if we want to keep them)

Future work includes:

Add more system cursors
Add custom cursors (from assets, from external links, and from fallbacks)
Add mouseCursor to more widgets (up to discussion)

Related Issues

A first step to fixing Add the ability to programmatically change mouse cursors. #31952
Engine [WIP] Mouse cursor (engine) engine#13152

Tests

I added the following tests:

Replace this with a list of the tests that you added as part of this PR. A change in behaviour with no test covering it
will likely get reverted accidentally sooner or later. PRs must include tests for all changed/updated/fixed behaviors. See Test Coverage.

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.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
No, this is not a breaking change.

goderbauer · 2019-11-04T18:44:12Z

packages/flutter/lib/src/gestures/mouse_cursor.dart

+///  * [MouseCursorPlatformDelegate], which uses this type to define how
+///    system cursors are implemented on platforms.
+enum MouseCursorSystemShape {
+  /// The shape that corresponds to [SystemCursors.none].


I couldn't find the class SystemCursors in this PR.

goderbauer

Left some comments from a first pass-through to get familiar with the change.

My main high-level question is: What's the advantage of having the different platform-specific delegates on the framework side? Could the framework just send to the embedder: I want curser xyz and then the embedder translates that into the platform-specific ID for its platform?

goderbauer · 2019-11-04T18:48:11Z

packages/flutter/lib/src/gestures/mouse_cursor.dart

+}
+
+/// Details for [MouseCursorPlatformDelegate.activateSystemCursor], such as the
+/// target device and the system cursor shape.


From reading the implementation it sounds as if this class represents a request to change the cursor of device to systemShape? Maybe add that to the documentation?

goderbauer · 2019-11-04T18:49:57Z

packages/flutter/lib/src/gestures/mouse_cursor.dart

+
+  @override
+  bool operator ==(dynamic other) {
+    if (other.runtimeType != MouseCursorPlatformActivateSystemCursorDetails)


if (other.runtimeType != runtimeType)

goderbauer · 2019-11-04T18:51:19Z

packages/flutter/lib/src/gestures/mouse_cursor.dart

+  /// Asks the platform to change the cursor of `device` to the system cursor
+  /// specified by `systemShape`.
+  ///
+  /// It resolves to `true` if the operation is successful, `false` if the


true and false don't need to be in `

goderbauer · 2019-11-04T18:51:40Z

packages/flutter/lib/src/gestures/mouse_cursor.dart

+  /// Create a [MouseCursorPlatformDelegate].
+  const MouseCursorPlatformDelegate();
+
+  /// Asks the platform to change the cursor of `device` to the system cursor


Maybe add that device and systemShape are provided within the details object?

goderbauer · 2019-11-04T18:52:27Z

packages/flutter/lib/src/gestures/mouse_cursor.dart

+/// Details for [MouseCursorPlatformDelegate.activateSystemCursor], such as the
+/// target device and the system cursor shape.
+@immutable
+class MouseCursorPlatformActivateSystemCursorDetails {


This name is a mouthful. Can we find something a little shorter?

goderbauer · 2019-11-04T19:21:02Z

packages/flutter/lib/src/services/mouse_cursor_android.dart

+
+// The channel interface with the platform.
+//
+// It is separated to a class for the conventience of reference by the shell


What do you mean by this?

goderbauer · 2019-11-04T19:22:10Z

packages/flutter/lib/src/services/mouse_cursor_android.dart

+
+  /// The same constant as Android's `PointerIcon.TYPE_DEFAULT`,
+  /// used internally to set system cursor.
+  static const int kPlatformConstantDefault = 1000;


Do these have to be public?

goderbauer · 2019-11-04T19:23:18Z

packages/flutter/lib/src/services/mouse_cursor_android.dart

+  final _AndroidPlatformActions _platform;
+
+  Future<bool> _activatePlatformConstant(int platformConstant) async {
+    await _platform.setAsSystemCursor(platformConstant: platformConstant);


Why the indirection via _AndroidPlatformActions? Couldn't this method just implement that method call directly?

goderbauer · 2019-11-04T19:25:56Z

packages/flutter/lib/src/services/system_channels.dart

+  ///
+  ///  * `MouseCursor.setCursors`: Request to set the cursors of multiple pointer
+  ///    devices. The argument is a `Map<int, int>` that maps from device to
+  ///    cursor. See [MouseCursors] for a list of system cursors.


MouseCursors doesn't exist?

What does this map to? device id to what?

goderbauer · 2019-11-04T19:29:57Z

packages/flutter/lib/src/services/mouse_cursor_platform.dart

+    // throw errors.
+    if (kIsWeb)
+      return const MouseCursorUnsupportedPlatformDelegate();
+    if (Platform.isLinux) {


What's the advantage of having these different platform-specific delegates on the framework side? Could the framework just send to the embedder: I want curser xyz and then the embedder translates that into the platform-specific ID?

Yeah I'm not a fan of having this platform-specific code in the framework.

dkwingsmt · 2019-11-04T21:04:05Z

What's the advantage of having these different platform-specific delegates on the framework side? Could the framework just send to the embedder: I want curser xyz and then the embedder translates that into the platform-specific ID?

The most significant reason is that, Dart code is easier to maintain and test. We have to write the same code either in the framework or in the engine. By writing most code in Dart, we leave only a one-to-one API translator in the engine.

dkwingsmt

Answered 2 concerns. I'll work on the new design in the mean time.

dkwingsmt · 2019-11-04T22:51:27Z

packages/flutter/lib/src/gestures/mouse_tracking.dart

+    // Change mouse cursor.
+    final MouseCursor cursor = _findDeviceCursor(nextAnnotations);
+    if (cursor != mouseState.cursor) {
+      _cursorManager.setDeviceCursor(device, cursor);


If the cursor is not available, the action is considered successful, therefore older shells will not throw exceptions for not implementing a latest feature introduced in the framework. Only actual runtime errors are failures.

dkwingsmt · 2019-11-04T22:54:35Z

packages/flutter/lib/src/material/ink_well.dart

  /// material.
  final ValueChanged<bool> onHover;

+  /// The cursor for a mouse pointer when it enters or is hovering the region.


Mouse cursors are changed based on Enter and Exit (instead of Hover), therefore it does not rely on whether the pointer is down. The short answer is nothing will be affected.

If you want to change the cursor when the mouse is down, you can assign different cursors to MouseRegion.cursor depending on the state.

Hixie · 2019-11-04T23:18:47Z

packages/flutter/lib/src/gestures/mouse_cursor.dart

+
+import 'package:flutter/foundation.dart';
+
+/// Internal identifiers for the system cursors supported by Flutter.


should it be private then?

I don't really understand the value of this enum.

Hixie · 2019-11-04T23:20:24Z

packages/flutter/lib/src/gestures/mouse_cursor.dart

+abstract class MouseCursor {
+  /// Create a mouse cursor.
+  ///
+  /// The base constructor does nothing.


for no-argument const constructors, please use the boilerplate that we use for all no-argument const constructors.

(this applies to other classes in this PR as well)

Hixie · 2019-11-04T23:20:50Z

packages/flutter/lib/src/gestures/mouse_cursor.dart

+  /// A platform-independent short description for this cursor.
+  ///
+  /// It is usually one or several readable words.
+  String describeCursor();


this could be a getter

Hixie · 2019-11-04T23:21:33Z

packages/flutter/lib/src/gestures/mouse_cursor.dart

+  /// Create a [NoopMouseCursor].
+  const NoopMouseCursor();
+
+  /// Does nothing and immediately returns true.


we generally don't want to override the docs of virtual methods, it hides the documentation of the superclass. better to put that information in the class docs.

Hixie · 2019-11-04T23:21:44Z

packages/flutter/lib/src/gestures/mouse_cursor.dart

+  }
+
+  @override
+  String describeCursor() => 'noop';


Hixie · 2019-11-04T23:22:37Z

packages/flutter/lib/src/gestures/mouse_cursor.dart

+///  * [MouseCursorPlatformDelegate], which uses this type to define how
+///    system cursors are implemented on platforms.
+enum MouseCursorSystemShape {
+  /// The shape that corresponds to [SystemCursors.none].


is "SystemCursors" supposed to be "SystemMouseCursors"? I couldn't find a definition of SystemCursors.

Hixie · 2019-11-04T23:25:00Z

packages/flutter/lib/src/services/mouse_cursor_glfw.dart

+
+// The channel interface with the platform.
+//
+// It is separated to a class for the conventience of reference by the shell


typo: convenience

Hixie · 2019-11-04T23:28:18Z

packages/flutter/lib/src/gestures/mouse_tracking.dart

      ? '<none>'
      : callbacks.join(' ');
    return '${describeIdentity(this)}(callbacks: $describeCallbacks)';
+    // TODO(dkwingsmt): Add property for cursor


please do this before landing

Hixie · 2019-11-04T23:30:32Z

I had trouble following this PR. I kept looking for a class with detailed dartdocs explaining how everything fit together. I recommend taking whatever class you feel is the center of this API, and adding significant detail to that class's dartdocs, along the lines of the docs for RenderBox, explaining how all the parts fit together, how to create a custom cursor, how to port the API to a new platform, how to add a new system cursor, etc.

…e-cursor

dkwingsmt · 2019-11-07T23:25:35Z

I'll close it for now until after some major changes I'm working on.

dkwingsmt added 15 commits September 5, 2019 15:53

First implementation

2477e7e

Add MouseCursors.fallthrough. Remove designator

ed8079f

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

a095a44

Use delegate for easier dependency injection

6eb0ff0

Split file and adjust doc

ebe5835

Logic

61844dd

Remove fallthrough

a037bcc

Add test to default delegate

0938c21

Change arg to argList

e0c535d

First workable version

a4e2eb8

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

07c950d

Merge the state of MCManager to MouseTracker

ef28280

Doc fix

da68502

Better warning string

f2598f8

Change to single cursor request

fd43621

dkwingsmt added work in progress; do not review a: desktop Running on desktop labels Oct 15, 2019

googlebot added the cla: yes label Oct 15, 2019

Remove redundant files

478a544

dkwingsmt changed the title ~~[WIP] Mouse cursor~~ [WIP] Mouse cursor (framework) Oct 15, 2019

dkwingsmt mentioned this pull request Oct 15, 2019

[WIP] Mouse cursor (engine) flutter/engine#13152

Closed

dkwingsmt removed d: examples Sample code and demos c: contributor-productivity Team-specific productivity, code health, technical debt. customer: gallery Relating to flutter/gallery repository. Please transfer non-framework issues there. labels Oct 17, 2019

dkwingsmt added 2 commits October 18, 2019 10:39

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

82b5e9e

Add cursor to FAB, button, and inkwell

825fe0a

Run kIsWeb first

afb86c7

dkwingsmt changed the title ~~[WIP] Mouse cursor (framework)~~ Mouse cursor (framework) Oct 25, 2019

fluttergithubbot added the f: material design flutter/packages/flutter/material repository. label Oct 25, 2019

dkwingsmt changed the title ~~Mouse cursor (framework)~~ [WIP] Mouse cursor (framework) Oct 25, 2019

dkwingsmt removed the f: material design flutter/packages/flutter/material repository. label Oct 25, 2019

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

6665dd9

goderbauer reviewed Nov 4, 2019

View reviewed changes

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

2eb84ad

dkwingsmt commented Nov 4, 2019

View reviewed changes

Hixie reviewed Nov 4, 2019

View reviewed changes

packages/flutter/lib/src/gestures/mouse_cursor.dart

}

@override

String describeCursor() => 'noop';

Copy link

Contributor

Hixie Nov 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"no-op"

Hixie reviewed Nov 4, 2019

View reviewed changes

First implementation of new design

f49c35a

dkwingsmt added 5 commits November 5, 2019 11:03

Better API

424c4d6

Fix type

fa240f8

Doc

5b47c5f

More doc

7f96c0d

Merge branch 'mouse-cursor' of github.com:dkwingsmt/flutter into mous…

5b298fe

…e-cursor

dkwingsmt closed this Nov 7, 2019

dkwingsmt mentioned this pull request Nov 18, 2019

Improve MouseTracker lifecycle: Move checks to post-frame #44631

Merged

8 tasks

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


		import 'package:flutter/foundation.dart';

		/// Internal identifiers for the system cursors supported by Flutter.

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

Tests

Checklist

Breaking Change

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goderbauer 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!

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

Uh oh!

Uh oh!

dkwingsmt 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!

Hixie commented Nov 4, 2019

Uh oh!

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

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dkwingsmt commented Oct 15, 2019 •

edited

Loading

dkwingsmt commented Nov 4, 2019 •

edited

Loading

dkwingsmt commented Nov 7, 2019 •

edited

Loading