Add key to mouse annotations by dkwingsmt · Pull Request #46034 · flutter/flutter

dkwingsmt · 2019-12-03T23:47:33Z

Description

This PR adds keys to MouseTrackerAnnotation. When determining mouse enter and exit, annotations with equal keys will be considered the same.

This is a prerequisite to mouse cursors, because render objects want to change properties of MouseTrackerAnnotations, which is a const class. Without a key to track the inherited presence, there is no way to switch properties without triggering onExit and onEnter. The first design of mouse cursor had to use a function to resolve this, which seems odd, as pointed out by https://github.com/flutter/flutter/pull/42797/files#r342219907.

This PR also

Moves the definitions of UniqueKey and ObjectKey to the foundation package so that it's available at lower-layer packages.
Changes MouseTracker.{attach,detach}Annotation to accepting a key instead of annotations.
Removes _MouseRegionElement since they're no longer needed.
Improves the performance of the annotation dispatching algorithm.
Fixes an issue where changing MouseRegion.opaque doesn't work.

Related Issues

N/A

Tests

I added the following tests:

annotations with the same key should inherit presence
Changing MouseRegion's callbacks is effective and doesn't repaint
Changing MouseRegion.opaque is effective and repaints

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.

Yes, this is a breaking change.
- I wrote a design doc: https://flutter.dev/go/key-based-mouse-tracker-annotation
- I got input from the developer relations team, specifically from: Andrew Brogdon
- I wrote a migration guide: Breaking change: MouseTrackerAnnotation requires parameter "key" website#3468

dkwingsmt · 2019-12-05T01:40:37Z

Close this to split it into multiple PRs, with potentially different approaches.

dkwingsmt · 2019-12-05T02:00:11Z

Never mind, the "different approach" can not be done without this.

gspencergoog · 2019-12-05T18:30:43Z

Might want to update your PR description to mention the tests and any issues.

gspencergoog · 2019-12-05T18:32:41Z

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

+  // Don't return `identityHashCode(value)` directly since the hash of this key
+  // needs to be different from that of its value.
+  @override
+  int get hashCode => identityHashCode(T) ^ identityHashCode(value);


Suggested change

int get hashCode => identityHashCode(T) ^ identityHashCode(value);

int get hashCode => hashValues(identityHashCode(T), identityHashCode(value));

I really want to reduce the overhead of hashCode since it will be used a lot every frame. And since it's a private class we don't need a very secure hashing. The only feature we need is that theKey.hashCode != theKey.value.hashCode.

I changed it to a XOR with a const value. What do you think?

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

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

packages/flutter/test/gestures/mouse_tracking_test.dart

packages/flutter/test/widgets/mouse_region_test.dart

gspencergoog · 2019-12-05T19:09:10Z

packages/flutter/test/widgets/mouse_region_test.dart

  });
 }

+// Render `foreground` at the top-left corner, on top of a full-screen `background`.


No need for markdown here.

The original doc was outdated. I changed it to

// Render widget `topLeft` at the top-left corner, stacking on top of the widget // `background`.

Hixie · 2019-12-06T21:34:30Z

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

+  // class.
+  @override
+  int get hashCode => _hashCodeMask ^ identityHashCode(value);
+  static const int _hashCodeMask = 0xc366d8df10ee8;


why does it need to be different? would we ever put instances of Key and instances of MouseTrackerAnnotation into the same map?

No. If it's not required that hashCode must be different, I'm glad to remove the mask.

If it's not required, why do we have the mask?

Hixie · 2019-12-06T21:35:11Z

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

+  bool operator ==(Object other) {
+    if (identical(this, other))
+      return true;
+    return other is _IdenticalAnnotationKey && identical(other.value, value);


Please use the common boilerplates:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#common-boilerplates-for-operator-and-hashcode

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

goderbauer · 2019-12-13T17:45:18Z

packages/flutter/lib/src/foundation/key.dart

 import 'dart:ui' show hashValues;

 import 'package:meta/meta.dart';
+import './diagnostics.dart';


should this be import 'diagnostics.dart';? Also should be in its own section.

goderbauer · 2019-12-13T17:51:13Z

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

    assert(unhandledEvent != null);
-    assert(trackedAnnotations != null);
+    assert(trackedAnnotationKeys != null);
    // Order is important for mouse event callbacks. The `findAnnotations`


How do we keep the order now? Below the code is iterating over non-order preserving iterables, as far as I can tell.

The lastAnnotations and nextAnnotations are still Lists, which is where order is kept. The lastKeys and nextKeys below are HashSets because they are only used to check if an annotation is hovered during the last frame / this frame.

There are tests that ensure the order is kept, such as the several tests that start at

flutter/packages/flutter/test/gestures/mouse_tracking_test.dart

Line 782 in 7f4a05d

test('should trigger callbacks between parents and children in correct order', () {

goderbauer · 2019-12-13T17:52:25Z

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

  /// {@endtemplate}
  ///  * Attaching an annotation that has been attached will assert.
-  void attachAnnotation(MouseTrackerAnnotation annotation) {
+  void attachAnnotation(Key annotationKey) {


SHould this continue to tale an annotation and just internally grab the key from there?

I've tried to keep it accepting MouseTrackerAnnotation for a long time during the development of this PR, but eventually removed it because it makes the implementation of RenderMouseRegion noticeably more complicated.

One of the problems is that, when the render object is detached, it needs to detach the current annotation. If detachAnnotation only accepts MouseTrackerAnnotations, then either the render object must keep its own hover annotation, or it builds a fake annotation when disposed. Keeping the hover annotation adds noticeably more bookkeeping overhead, since otherwise it could've built the annotation anew during every paint (which is how the PR is implemented currently, see here).

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

goderbauer · 2019-12-13T17:55:04Z

packages/flutter/test/widgets/listener_deprecated_test.dart

      ));
      expect(exit, isNull);
-      expect(tester.binding.mouseTracker.isAnnotationAttached(renderListener.hoverAnnotation), isFalse);
+      expect(tester.binding.mouseTracker.isAnnotationAttached(renderListener.annotationKey), isFalse);


I believe the new breaking change policy would consider these changes as breaking.

The isAnnotationAttached is a visibleForTest method, and is not used in g3. If this is still a concern I can use a 2-phase deprecation instead.

dkwingsmt · 2019-12-14T01:36:50Z

@Hixie I've changed so that MouseTrackerAnnotation.key is required.

gspencergoog

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

…nto mouse-annotation-key

dkwingsmt · 2020-01-09T20:18:08Z

Closing in favor of #48453.
Most changes in this PR was moved to #48453 except for the keys.

dkwingsmt added 5 commits December 2, 2019 16:44

Implement key

f653e0c

Remove MouseRegionElement

bc4f60d

Add test

69379ea

Use List instead of LinkedHashSet

1f38155

Asserts

70e561b

fluttergithubbot added the framework flutter/packages/flutter repository. See also f: labels. label Dec 3, 2019

googlebot added the cla: yes label Dec 3, 2019

dkwingsmt added 4 commits December 3, 2019 16:57

Use _IdenticalKey

4b6d9f4

Optimize algorithm

23941e5

Better test

cf656e2

Merge remote-tracking branch 'upstream/master' into mouse-annotation-key

87d7900

dkwingsmt added a: desktop Running on desktop f: gestures flutter/packages/flutter/gestures repository. labels Dec 4, 2019

dkwingsmt requested a review from gspencergoog December 4, 2019 22:24

dkwingsmt closed this Dec 5, 2019

WIP: Update

80b354d

dkwingsmt reopened this Dec 5, 2019

dkwingsmt added 3 commits December 4, 2019 18:41

Finish property update

62d5241

The missing test

c4e2f55

Split tests

75f51fd

gspencergoog reviewed Dec 5, 2019

View reviewed changes

dkwingsmt added 2 commits December 5, 2019 13:10

Doc fixes based on review

4379021

_IdenticalAnnotationKey

ba8b296

Hixie reviewed Dec 6, 2019

View reviewed changes

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

Hixie reviewed Dec 6, 2019

View reviewed changes

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

Comments

7f4a05d

goderbauer reviewed Dec 13, 2019

View reviewed changes

dkwingsmt and others added 2 commits December 13, 2019 14:15

Lint fix

2df6467

Requiring key

0c067e2

gspencergoog approved these changes Dec 14, 2019

View reviewed changes

Hixie reviewed Dec 16, 2019

View reviewed changes

packages/flutter/lib/src/gestures/mouse_tracking.dart Outdated Show resolved Hide resolved

dkwingsmt added 4 commits December 16, 2019 16:17

Merge remote-tracking branch 'upstream/master' into mouse-annotation-key

aca38b5

Use LocalKey.

4fbeb51

Use local key and consistent key.

ac5eb81

Merge remote-tracking branch 'upstream/master' into mouse-annotation-key

790d4bc

dkwingsmt mentioned this pull request Dec 20, 2019

Breaking change: MouseTrackerAnnotation requires parameter "key" flutter/website#3468

Closed

dkwingsmt and others added 6 commits December 20, 2019 14:53

Remove outdated assert

e74b5fe

Merge remote-tracking branch 'upstream/master' into mouse-annotation-key

99eb1d1

Merge remote-tracking branch 'upstream/master' into mouse-annotation-key

b1f1fb2

Merge remote-tracking branch 'upstream/master' into mouse-annotation-key

c86e18f

Merge remote-tracking branch 'upstream/master' into mouse-annotation-key

3f45bdb

Merge branch 'master' into mouse-annotation-key

6bae727

dkwingsmt added waiting for tree to go green and removed waiting for tree to go green labels Jan 8, 2020

dkwingsmt added 4 commits January 8, 2020 12:05

Handle exit in MouseRegion, remove anno attachment

da4ebfa

Merge branch 'mouse-annotation-key' of github.com:dkwingsmt/flutter i…

7caa751

…nto mouse-annotation-key

Merge remote-tracking branch 'upstream/master' into mouse-annotation-key

13b8ff4

Fix platform view test

ba21d60

dkwingsmt added a commit to dkwingsmt/flutter that referenced this pull request Jan 8, 2020

Cherrypick non-key changes from flutter#46034

e66c75c

dkwingsmt closed this Jan 9, 2020

dkwingsmt deleted the mouse-annotation-key branch January 9, 2020 20:18

dkwingsmt mentioned this pull request May 8, 2020

System mouse cursors #54171

Merged

13 tasks

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

	int get hashCode => identityHashCode(T) ^ identityHashCode(value);
	int get hashCode => hashValues(identityHashCode(T), identityHashCode(value));

Conversation

dkwingsmt commented Dec 3, 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!

dkwingsmt commented Dec 5, 2019

Uh oh!

dkwingsmt commented Dec 5, 2019

Uh oh!

gspencergoog commented Dec 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkwingsmt Dec 13, 2019 • 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!

dkwingsmt Dec 13, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

dkwingsmt Dec 13, 2019 • 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!

dkwingsmt commented Dec 14, 2019

Uh oh!

gspencergoog left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dkwingsmt commented Jan 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dkwingsmt commented Dec 3, 2019 •

edited

Loading

dkwingsmt Dec 13, 2019 •

edited

Loading

dkwingsmt Dec 13, 2019 •

edited

Loading

dkwingsmt Dec 13, 2019 •

edited

Loading