System mouse cursors by dkwingsmt · Pull Request #54171 · flutter/flutter

dkwingsmt · 2020-04-07T05:56:57Z

Description

This PR adds the basic framework for the mouse cursor system. While only system cursors are supported for now, the APIs are designed with the consideration of asynchronously loaded cursors (image cursors).

With this PR:

Cursors are assigned to regions in a declarative way using data classes.
MouseRegion, RenderMouseRegion and MouseTrackerAnnotation have a new property MouseCursor cursor that can be used to customize mouse cursors of pointers that are hovering over the region.
Inkwell and a few other widgets also have a new parameter MouseCursor mouseCursor
Hidden cursor, uncontrolled cursor, and several system cursors are supported.

This PR is only the framework part of the system. Without the upcoming engine change, these changes won't take effect, but will not break existing apps either.

~~For more information about the design, see https://flutter.dev/go/system-mouse-cursor~~ Edit: this design doc is out of date

Related Issues

Part of Add the ability to programmatically change mouse cursors. #31952

Tests

I added the following tests:

Many tests to the cursor aspect of MouseTracker
Test the cursor aspect of MouseRegion
Test MouseCursorController

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 read the Tree Hygiene wiki page, which explains my responsibilities.
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

Did any tests fail when you ran them? Please read Handling breaking changes.

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: Replace with a link to your migration guide

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

gspencergoog

Mostly just doc comments. It looks pretty good overall.

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

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

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

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

goderbauer · 2020-04-15T21:16:10Z

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

+/// defines a kind of mouse cursor, such as an arrow, a pointing hand, or an
+/// I-beam.
+///
+/// Internally, when the mouse pointer moves, it finds the front-most region


region as in MouseRegion? Maybe say [MouseRegion] then? Or is this a different region?

In some way this is intentionally put in a vague way, because the answer depends on the cases. It can be an AnnotatedRegionLayer<MouseTrackerAnnotation>, RenderMouseRegion, or MouseRegion. There are also other render objects or widgets that do not directly use (Render)MouseRegion but directly push annotations, such as PlatformView, among a few.

The only thing we're certain of is that cursors have to be assigned to logical regions of the screen, because mouse tracking is based on positions.

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

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

goderbauer · 2020-04-15T23:16:02Z

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

+  // Define the actions when a pointer should change to a prepared cursor by
+  // calling system functions.
+  Future<void> _handleActivateCursor(int device, PreparedMouseCursor cursor) async {
+    if (cursor is NoopMouseCursor)


Maybe I missunderstod what NoopMouseCursor does, but don't you have to tell the system that flutter is no longer controlling the cursor?

"Not controlling the cursor" is only effective when the pointer moves across the "cursor boundary". When the pointer is moving within the "cursor boundary" is entirely covered by NoopMouseCursor, the cursor won't change at all.

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

goderbauer · 2020-04-15T23:21:09Z

The analyzer is unhappy.

dkwingsmt · 2020-04-22T04:00:01Z

@Hixie @goderbauer I made some major changes:

PreparedMouseCursor now has a protected method performActivate, which is overridden by subclasses. It replaces the is type-checks in MouseTracker.
The method channel calls are split into a MouseCursorController class, which only has static methods, such as activateSystemCursor, and is to be used by PreparedMouseCursor.performActivate.
The cursor part of MouseTracker is split into a mixin MouseTrackerCursorMixin. The original MouseTracker is renamed to BaseMouseTracker, whereas the one with all mixins is the new MouseTracker.

Can you take a look?

goderbauer · 2020-04-23T17:31:06Z

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

+    MouseTrackerAnnotation result;
+    for (final MouseTrackerAnnotation annotation in annotations) {
+      if (annotation.cursor != null) {
+        result = annotation;
+        break;
+      }
+    }
+    return result ?? const _FallbackAnnotation();


Feel free to ignore, but I find the following structure slightly clearer:

Suggested change

MouseTrackerAnnotation result;

for (final MouseTrackerAnnotation annotation in annotations) {

if (annotation.cursor != null) {

result = annotation;

break;

}

}

return result ?? const _FallbackAnnotation();

for (final MouseTrackerAnnotation annotation in annotations) {

if (annotation.cursor != null) {

return annotation;

}

}

return const _FallbackAnnotation();

goderbauer · 2020-04-23T17:36:08Z

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

+    final bool hadState = _mouseCursorStates.containsKey(device);
+    _mouseCursorStates.putIfAbsent(device, () {
+      final _MouseCursorState state = _MouseCursorState();
+      state.onCursorChange = () {


It seems as if all _MouseCursorState always have the same onCursorChange method. Why not make it an instance method (and pass the device in in the constructor)?

goderbauer · 2020-04-23T17:37:27Z

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

+      return state;
+    });
+
+    final _MouseCursorState state = _mouseCursorStates[device];


putIfAbsent above returns exactly this. Just assign it to state instead of looking it up again here?

goderbauer · 2020-04-23T17:39:05Z

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

+      return;
+    }
+
+    final bool hadState = _mouseCursorStates.containsKey(device);


looks like this value is only used for asserts. Maybe rename it to debugHadState and wrap its assignment in an assert so it only happens when asserts are enabled?

goderbauer · 2020-04-23T17:44:35Z

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

+  // prevents instantiation and extension.
+  MouseCursorController._();
+
+  static MethodChannel get _channel => SystemChannels.mouseCursor;


nit: the indirection seems unnecessary, just use SystemChannels.mouseCursor below?

goderbauer · 2020-04-23T17:54:09Z

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

+  @override
+  @protected
+  Future<void> performActivate(int device) async {
+  }


nit: add a comment to the body indicating why it's empty.

goderbauer · 2020-04-23T17:55:13Z

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

+  String get debugDescription => '';
+}
+
+/// A mouse cursor that is standard on the platform that the application is


nit: "standard" seems strange. Isn't it just one that ships with the platform or is available there?

How about "native"?

/// A mouse cursor that is natively supported on the platform that the /// application is running on.

goderbauer · 2020-04-23T17:55:47Z

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

+  }
+
+  @override
+  String get debugDescription => '';


I am surprised that the empty string is used here.

I added a comment to explain:

// The [debugDescription] is '' so that its toString() returns 'NoopMouseCursor()'.

goderbauer · 2020-04-23T18:41:06Z

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

+  /// one on the screen in hit-test order, or [SystemMouseCursors.basic] if no
+  /// others can be found.
+  ///
+  /// The [MouseTrackerAnnotation] is immutable and does not support varying


I still have some questions about the "immutable" and the change notification part. Maybe we can do a quick video sync?

goderbauer · 2020-04-23T18:54:52Z

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

  // It uses [LinkedHashSet] to keep the insertion order.
  LinkedHashSet<MouseTrackerAnnotation> get annotations => _annotations;
-  LinkedHashSet<MouseTrackerAnnotation> _annotations = LinkedHashSet<MouseTrackerAnnotation>();
+  LinkedHashSet<MouseTrackerAnnotation> _annotations = <MouseTrackerAnnotation>{} as LinkedHashSet<MouseTrackerAnnotation>;


Why this change? The old version (without the cast) seemed cleaner...

goderbauer · 2020-04-23T21:28:33Z

Avoiding the repaint for when cursors change adds a lot of complexibility. I am not sure if that's worth it, TBH. The repaint shouldn't be that expensive and I would image that when you need to change the cursor, you'd also be changing other aspects of the UI to "justify" the new cursor. For that, you'd have to do a repaint anyways...

dkwingsmt · 2020-05-04T08:08:25Z

@goderbauer Can you take a look at the new design, which I call "session-based":

Overview of design:

MouseCursor is an immutable class and stores data. Subclasses overrides the method createSession to create a MouseCursorSession object.
Every MouseCursor has a corresponding MouseCursorSession class, which represents the duration when a device displays a cursor, i.e. the tuple (device, cursor).
MouseCursorSession is a mutable class that stores states and provides methods activate and dispose to override. The activate will handle the method calls as well as resource fetching, caching, fallback cursors, etc.
MouseTrackerAnnotation stays const, and doesn't have listeners at all.
No more MouseTrackerRenderObjectMixin.
- RenderMouseRegion implements variable cursor by implementing MouseTrackerAnnotation, and pushing itself as the annotation.

A demo of implementing image cursor can be found at dkwingsmt#1.

winwisely99 · 2020-05-07T09:29:12Z

Just adding myself to this so I can see when it's ready as we need it for our flutter project.

https://github.com/getcouragenow

ja2375 · 2020-05-07T18:57:10Z

I think the default behavior for this on web should be the cursor autochanges when it detects you are hovering a button, text, etc. Will this be the behavior for web or will it be necessary to add an extra widget on every clickable, text widget we want to show a different cursor on?

Many thanks for implementing this!

goderbauer

I like this design!

(haven't looked closely at the tests yet)

packages/flutter/lib/src/widgets/basic.dart

packages/flutter/lib/src/rendering/platform_view.dart

packages/flutter/lib/src/rendering/proxy_box.dart

goderbauer · 2020-05-08T18:11:26Z

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

+  /// The last event that the device observed before the update.
+  ///
+  /// If the update is triggered by a frame, it is not null, since the pointer
+  /// must have been added before. If the update is triggered by an event,


Is this only null for specific events (e.g. pointer added)? Maybe document that?

I changed it to

/// The last event that the device observed before the update. /// /// If the update is triggered by a frame, the [previousEvent] is never null, /// since the pointer must have been added before. /// /// If the update is triggered by a pointer event, the [previousEvent] is not /// null except for cases where the event is the first event observed by the /// pointer (which is not necessarily a [PointerAddedEvent]).

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

goderbauer · 2020-05-08T18:24:27Z

packages/flutter/test/widgets/mouse_region_test.dart

+    logEnters.clear();
+  });
+
+  testWidgets('Changing whether MouseRegion.cursor is null is effective and repaints', (WidgetTester tester) async {


Do you have a test to verify that the cursor of the MouseRegion underneath the one that changed it cursor to null becomes active?

Good idea. I changed this test by adding a background MouseRegion

dkwingsmt · 2020-05-08T23:22:02Z

@goderbauer I think I've addressed all comments. Can you take a look again?

dkwingsmt · 2020-05-09T00:23:26Z

Besides the comments, I also changed SystemMouseCursor, instead of int shapeCode, it now uses String kind, suggested by Stuart for clarity. Let me know if it's considered inappropreate.

goderbauer

LGTM after comment is resolved

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

…or-2

fluttergithubbot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Apr 7, 2020

googlebot added the cla: yes label Apr 7, 2020

dkwingsmt force-pushed the system-mouse-cursor-2 branch from b1e644f to e64ca1b Compare April 7, 2020 06:06

orestesgaolin mentioned this pull request Apr 13, 2020

Custom Cursor for [mac] and [web] flutter/plugins#2462

Closed

dkwingsmt force-pushed the system-mouse-cursor-2 branch from 48f59a7 to 277adb0 Compare April 14, 2020 20:59

dkwingsmt requested a review from gspencergoog April 14, 2020 21:07

gspencergoog reviewed Apr 14, 2020

View reviewed changes

packages/flutter/lib/src/rendering/mouse_cursor.dart Outdated Show resolved Hide resolved

gspencergoog reviewed Apr 14, 2020

View reviewed changes

dkwingsmt mentioned this pull request Apr 14, 2020

System mouse cursor: Web flutter/engine#17718

Merged

goderbauer reviewed Apr 15, 2020

View reviewed changes

fluttergithubbot added the a: tests "flutter test", flutter_test, or one of our tests label Apr 22, 2020

goderbauer reviewed Apr 23, 2020

View reviewed changes

Rebase: session-based desing

0201c21

dkwingsmt force-pushed the system-mouse-cursor-2 branch from cb9a0fe to 0201c21 Compare May 4, 2020 21:02

dkwingsmt mentioned this pull request May 4, 2020

System mouse cursor: macOS flutter/engine#18131

Merged

goderbauer reviewed May 8, 2020

View reviewed changes

dkwingsmt added 4 commits May 8, 2020 14:05

Address comments

2df2659

Remove some structures and add a test upon resetting

9906c62

Fix docs per comment

a307630

Better test. Doc

2005729

Rename shapeCode to kind

826544e

Revert unintended change

7b4f2f2

dkwingsmt and others added 8 commits May 8, 2020 17:26

Update mouse_tracking_cursor_test.dart

b091e1f

Update binding.dart

e60d199

Update platform_view.dart

244677b

Update mouse_cursor.dart

aca9eb8

Fix dependency

07b7b50

Fix import

4a6b1cf

Fix import

86f7a1d

Fix doc

3b34458

goderbauer approved these changes May 11, 2020

View reviewed changes

packages/flutter/lib/src/rendering/mouse_cursor.dart Outdated Show resolved Hide resolved

dkwingsmt and others added 3 commits May 11, 2020 10:25

Merge remote-tracking branch 'upstream/master' into system-mouse-curs…

f819ff8

…or-2

Use kind for debugDesc

dcfe2a6

Fix InkWell.mouseCursor

695c855

dkwingsmt merged commit 7f8cb7f into flutter:master May 11, 2020

gspencergoog mentioned this pull request May 21, 2020

Add the ability to programmatically change mouse cursors. #31952

Open

17 tasks

github-actions bot locked as resolved and limited conversation to collaborators Jul 31, 2021

Conversation

dkwingsmt commented Apr 7, 2020 • 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!

Uh oh!

gspencergoog left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkwingsmt Apr 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

goderbauer commented Apr 15, 2020

Uh oh!

dkwingsmt commented Apr 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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 May 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

goderbauer commented Apr 23, 2020

Uh oh!

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

Uh oh!

Uh oh!

winwisely99 commented May 7, 2020

Uh oh!

ja2375 commented May 7, 2020

dkwingsmt commented Apr 7, 2020 •

edited

Loading

dkwingsmt Apr 16, 2020 •

edited

Loading

dkwingsmt commented Apr 22, 2020 •

edited

Loading

dkwingsmt May 4, 2020 •

edited

Loading

dkwingsmt commented May 4, 2020 •

edited

Loading