Skip to content

Allow PointerEnterEvent and PointerExitEvents to be created from any PointerEvent#28602

Merged
gspencergoog merged 3 commits intoflutter:masterfrom
ds84182:enter_exit_downcast_bug
Mar 27, 2019
Merged

Allow PointerEnterEvent and PointerExitEvents to be created from any PointerEvent#28602
gspencergoog merged 3 commits intoflutter:masterfrom
ds84182:enter_exit_downcast_bug

Conversation

@ds84182
Copy link
Contributor

@ds84182 ds84182 commented Feb 27, 2019 •
edited by gspencergoog
Loading

Description

When testing mouse hover support in a custom embedder, I noticed crashes in MouseTracker due to an implicit downcast from PointerEvent to PointerHoverEvent. The _lastMouseEvent map can contain PointerMoveEvents and PointerDownEvents, in addition to PointerHoverEvents, so changing the type of event handled by PointerEnter/ExitEvent.fromHoverEvent seems like the best course of action. Renaming the constructor would be a breaking change however.

Should I include tests for converting other pointer events into PointerEnter/ExitEvents?

Related Issues

#29696

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.
  • My PR includes tests for all changed/updated/fixed behaviors (See Test Coverage).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • 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?

@ds84182
Allow PointerEnterEvent and PointerExitEvents to be created from any …
fa4c918 
…PointerEvent

Avoids an implicit downcast + crash in MouseTracker, since the last
pointer event can be a Move or Down event, in addition to Hover.
@goderbauer
Copy link
Member

goderbauer commented Feb 27, 2019

/cc @gspencergoog Can you review this?

@gspencergoog gspencergoog self-requested a review February 28, 2019 00:54
gspencergoog
gspencergoog reviewed Feb 28, 2019
View reviewed changes
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.

Thanks for fixing this! It's unfortunate that I didn't see this when I added multiple pointer support, I would have fixed the name too. I don't think it's worth making a breaking change at this point, although I know that there are likely zero usages of that constructor in the wild.

You might add a test in mouse_tracking_test.dart that checks that mouse enter/exit works with mouse move events too. That will exercise this code, as well as the mouse tracking code.

@zoechi zoechi added framework flutter/packages/flutter repository. See also f: labels. f: gestures flutter/packages/flutter/gestures repository. labels Feb 28, 2019
@gspencergoog
Copy link
Contributor

gspencergoog commented Feb 28, 2019

Actually, in discussing it more, we think it would be best to make the breaking change and call it fromMouseEvent. There are literally no uses of this in open source software, and it's a very new API that is deep in internals, so it's unlikely to be used at all by third parties.

To do this, make a copy of your changed version of the constructor and mark the fromHoverEvent one as @deprecated and add a comment saying to use fromMouseEvent instead.

We'll need to make an announcement to the mailing list before committing, but I can do that.

@ds84182
Copy link
Contributor Author

ds84182 commented Feb 28, 2019

The test is going to be a bit more involved, unfortunately. PointerHoverEventListener only takes PointerHoverEvents. Should I create synthetic hover events like PointerEnterEvent does? Otherwise the test crashes here.

@gspencergoog
Copy link
Contributor

gspencergoog commented Feb 28, 2019

Hmm. I wonder if the line before that should be:

 if (hitAnnotation.annotation?.onHover != null && lastEvent is PointerHoverEvent) {

in any case. Move and Down events shouldn't be sent to onHover anyhow.

goderbauer
goderbauer reviewed Feb 28, 2019
View reviewed changes
@ds84182
Copy link
Contributor Author

ds84182 commented Mar 1, 2019

Added a test

@goderbauer goderbauer added the a: desktop Running on desktop label Mar 12, 2019
gspencergoog
gspencergoog approved these changes Mar 20, 2019
View reviewed changes
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

@gspencergoog
Copy link
Contributor

gspencergoog commented Mar 20, 2019

I'll land this as soon as the tree is green.

@gspencergoog
Copy link
Contributor

gspencergoog commented Mar 20, 2019

Actually, because of the deprecations, we need to follow the "breaking change" protocols, so I'll send an announcement and we'll wait a bit for comments.

@goderbauer goderbauer added the c: API break Backwards-incompatible API changes label Mar 20, 2019
@gspencergoog gspencergoog merged commit 393521d into flutter:master Mar 27, 2019
@gspencergoog gspencergoog mentioned this pull request Apr 9, 2019
Closed
@Piinks Piinks mentioned this pull request Dec 15, 2020
Merged
9 tasks
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@goderbauer goderbauer goderbauer left review comments

@gspencergoog gspencergoog gspencergoog approved these changes

Assignees

No one assigned

Labels

a: desktop Running on desktop c: API break Backwards-incompatible API changes f: gestures flutter/packages/flutter/gestures repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ds84182 @goderbauer @gspencergoog @zoechi @googlebot