Skip to content

MouseTracker no longer requires annotations attached#48453

Merged
fluttergithubbot merged 21 commits intoflutter:masterfrom
dkwingsmt:mouse-no-longer-attach
Jan 28, 2020
Merged

MouseTracker no longer requires annotations attached#48453
fluttergithubbot merged 21 commits intoflutter:masterfrom
dkwingsmt:mouse-no-longer-attach

Conversation

@dkwingsmt
Copy link
Contributor

@dkwingsmt dkwingsmt commented Jan 8, 2020

Description

In this PR, MouseTracker no longer provides API to attach, detach, and query the attachment of annotations. Render objects that use MouseTrackerAnnotation no longer need to attach or detach annotations during their lifecycles, simplifying the logic.

Previously, MouseTrackerAnnotation.onExit, RenderMouseRegion and MouseRegion prevent onExit from being executed when the owner is detached. Now this mechanism is removed from the previous two, and is only kept in MouseRegion. This decision is because this mechanism was aimed to prevent exceptions thrown from State, therefore this mechanism should only been done at Widget/Element layer.

This PR also:

  • Fixes an issue where changing MouseRegion.opaque doesn't work.

Related Issues

Replace this paragraph with a list of issues related to this PR from our issue database. Indicate, which of these issues are resolved or fixed by this PR. There should be at least one issue listed here.

Tests

I added the following tests:

  • 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.

@fluttergithubbot fluttergithubbot added the framework flutter/packages/flutter repository. See also f: labels. label Jan 8, 2020
Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

/cc @gspencergoog Can you review this?

@gspencergoog gspencergoog self-requested a review January 13, 2020 19:06
@goderbauer
Copy link
Member

Looks like Cirrus is unhappy.

@dkwingsmt dkwingsmt force-pushed the mouse-no-longer-attach branch 2 times, most recently from 1ad1943 to 8ba9161 Compare January 24, 2020 21:24
@dkwingsmt dkwingsmt force-pushed the mouse-no-longer-attach branch from 8ba9161 to b12abcc Compare January 24, 2020 21:25
/// * A pointer that is hovering this widget has been removed.
/// * A pointer that is hovering this widget has moved away.
///
/// And is __not__ triggered by the following case,
Copy link
Contributor

Choose a reason for hiding this comment

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

You could just eliminate the bullet (since there's only one now), and say "And is __not__ triggered when this widget disappears while being hovered over."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason for the bullet is to make the 4 cases aligned, so that it's obvious that all cases have been enumerated. I still prefer keeping the bullets.

Copy link
Contributor

@gspencergoog gspencergoog left a comment

Choose a reason for hiding this comment

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

32384589-a60f0e74-c078-11e7-9bc1-e5b5287aea9d

Copy link
Contributor Author

@dkwingsmt dkwingsmt left a comment

Choose a reason for hiding this comment

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

I turned the snippets into workable full application samples.

I would still prefer the bullets for its clarity of full coverage, for reasons explained in the in-line comment.

@fluttergithubbot
Copy link
Contributor

This pull request is not suitable for automatic merging in its current state.

  • The status or check suite Cirrus CI has failed. Please fix the issues identified (or deflake) before re-applying this label.

@fluttergithubbot fluttergithubbot merged commit da0a7d8 into flutter:master Jan 28, 2020
@dkwingsmt dkwingsmt deleted the mouse-no-longer-attach branch January 29, 2020 18:58
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

a: desktop Running on desktop framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants