Skip to content

Conversation

gspencergoog
Copy link
Contributor

Description

This PR solves two problems: currently, the onExit is called for a mouse pointer the moment the removal message is received, except that by the time it actually calls it, there is no _lastEvent for it in the mouse tracker (it's already been removed), resulting in an event being passed to the onExit that contains nulls for the position. Also, removePointer events don't actually get created with a position, although they easily could be, so that even the the _lastEvent in the mouse tracker were still populated, it would still give a null position and delta.

This PR adds support for the position and delta in a PointerRemovedEvent, and populates them. In addition, when a remove event is received, it doesn't actually remove the pointer until the mouse position check that gets scheduled actually happens.

Tests

I added the following tests:

  • A Listener test that will check to make sure that an onExit gets the proper position.
  • Modified the 'detects exit when mouse goes away' test in mouse_tracking_test.dart to verify that a position and delta is included.

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.
  • I checked all the boxes.

Breaking Change

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

  • No, this is not a breaking change.

@goderbauer goderbauer added the framework flutter/packages/flutter repository. See also f: labels. label May 29, 2019
@gspencergoog gspencergoog force-pushed the fix_onexit branch 3 times, most recently from 08ee775 to d02c00b Compare May 30, 2019 00:10
@@ -408,6 +409,8 @@ class PointerEventConverter {
timeStamp: timeStamp,
kind: kind,
device: datum.device,
position: position,
delta: delta,
Copy link
Member

Choose a reason for hiding this comment

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

If we already reported the delta on the hover event above, shouldn’t it be zero for the remove event then? My understanding is that it’s the delta of position since last event? (We should probably have a failing test for this situation...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I'm just going to remove the delta. It's not clear what it's a delta from, since there isn't another event of the same type to diff it from, and nobody's expecting a delta anyhow. And not specifying it will set it to zero in any case.

@gspencergoog gspencergoog force-pushed the fix_onexit branch 2 times, most recently from 27af862 to 99fc11a Compare May 30, 2019 15:28
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.

LGTM

@goderbauer
Copy link
Member

Looks like Cirrus is showing a legit failure though?

@gspencergoog
Copy link
Contributor Author

Yeah, fixed it.

@gspencergoog gspencergoog merged commit 07aede4 into flutter:master May 30, 2019
@gspencergoog gspencergoog deleted the fix_onexit branch June 5, 2019 20:52
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants