add sourceTimeStamp to ScaleUpdateDetails #135936

yakagami · 2023-10-03T23:01:26Z

This PR adds the ability to get the sourceTimeStamp from ScaleUpdateDetails in a GestureScaleUpdateCallback like so:

onScaleUpdate: (ScaleUpdateDetails details){
  print(details.sourceTimeStamp);
}

sourceTimeStamp is necessary when tracking velocity eg.

VelocityTracker tracker = VelocityTracker.withKind(PointerDeviceKind.touch);
///...
onScaleUpdate: (ScaleUpdateDetails details){
  tracker.addPosition(details.sourceTimeStamp!, details.focalPoint);
}

The docs say:

Having both a pan gesture recognizer and a scale gesture recognizer is redundant; scale is a superset of pan. Just use the scale gesture recognizer.

Currently this is not entirely accurate, and should be fixed, as noted in #43833 (comment). This PR does not add sourceTimeStamp to ScaleStartDetails because it is more involved. Specifically, ScaleStartDetails can be created in acceptGesture which does not have access to the PointerEvent to get the event.timeStamp (https://github.com/flutter/flutter/blob/54fa25543243e3bf31af6af0c1fef6adabc1d5c1/packages/flutter/lib/src/gestures/scale.dart#L730C5-L730C5).

fixes #135873. See also #43833 which added delta and #49025 which added numPointers to ScaleUpdateDetails for the reason given above. sourceTimeStamp should probably be added to ScaleStartDetails as well because it exists in DragStartDetails and therefore in onPanStart.

I am not sure how to add tests for this, any input about this PR would be appreciated.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
[] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

flutter-dashboard · 2023-10-03T23:01:28Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie or stuartmorgan on the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

moffatman · 2023-10-03T23:37:31Z

Start of testing...

diff --git a/packages/flutter/test/gestures/scale_test.dart b/packages/flutter/test/gestures/scale_test.dart
index fd4bcfaf00..1b5429b38a 100644
--- a/packages/flutter/test/gestures/scale_test.dart
+++ b/packages/flutter/test/gestures/scale_test.dart
@@ -27,12 +27,14 @@ void main() {
     double? updatedHorizontalScale;
     double? updatedVerticalScale;
     Offset? updatedDelta;
+    Duration? updatedSourceTimestamp;
     scale.onUpdate = (ScaleUpdateDetails details) {
       updatedScale = details.scale;
       updatedHorizontalScale = details.horizontalScale;
       updatedVerticalScale = details.verticalScale;
       updatedFocalPoint = details.focalPoint;
       updatedDelta = details.focalPointDelta;
+      updatedSourceTimestamp = details.sourceTimeStamp;
     };
 
     bool didEndScale = false;
@@ -56,6 +58,7 @@ void main() {
     expect(updatedScale, isNull);
     expect(updatedFocalPoint, isNull);
     expect(updatedDelta, isNull);
+    expect(updatedSourceTimestamp, isNull);
     expect(didEndScale, isFalse);
     expect(didTap, isFalse);
 
@@ -65,10 +68,12 @@ void main() {
     expect(updatedScale, isNull);
     expect(updatedFocalPoint, isNull);
     expect(updatedDelta, isNull);
+    expect(updatedSourceTimestamp, isNull);
     expect(didEndScale, isFalse);
     expect(didTap, isFalse);
 
-    tester.route(pointer1.move(const Offset(20.0, 30.0)));
+    const ts1 = Duration(milliseconds: 10);
+    tester.route(pointer1.move(const Offset(20.0, 30.0), timeStamp: ts1));
     expect(didStartScale, isTrue);
     didStartScale = false;
     expect(updatedFocalPoint, const Offset(20.0, 30.0));
@@ -77,6 +82,8 @@ void main() {
     updatedScale = null;
     expect(updatedDelta, const Offset(20.0, 30.0));
     updatedDelta = null;
+    expect(updatedSourceTimestamp, ts1);
+    updatedSourceTimestamp = null;
     expect(didEndScale, isFalse);
     expect(didTap, isFalse);
     expect(scale.pointerCount, 1);

yakagami · 2023-10-04T02:45:06Z

I've added some tests. In line with https://github.com/flutter/flutter/blob/master/packages/flutter/test/gestures/drag_test.dart I've started the first event at const Duration(milliseconds: 100) and increased each event by 100 milliseconds.

edit: now changed to intervals of 10.

goderbauer · 2023-10-10T22:08:55Z

(triage): Looks like a lot of checks are failing. Can you take a look and fix those issues?

yakagami · 2023-10-11T03:23:39Z

@goderbauer thanks. All checks should be passing now.

Renzo-Olivares · 2023-10-11T20:49:55Z

Hi @yakagami, thank you for your contribution! This seems like a reasonable addition and I agree with the points made in your PR description. I do think we should add this to ScaleStartDetails as well and am happy to provide an explanation on how to do that.

It looks like there are two potential starts to a scale gesture. One is addAllowedPointerPanZoom which is called when a scale start without a PointerDownEvent (the user first touching down on the screen). The second is addAllowedPointer which is called when a scale starts when the user first touches down on the screen and then pinches to scale. Given these two cases I think we should track the initial event time stamp in Duration? _initialEventTimestamp; and set this variable in addAllowedPointer and addAllowedPointerPanZoom. We can then pass _initialEventTimestamp to ScaleStartDetails which is instantiated in _dispatchOnStartCallbackIfNeeded. We should also make sure to null the _intialEventTimestamp after it is used directly in _dispatchOnStartCallbackIfNeeded. Let me know if this explanation makes sense or if it needs some clarifying!

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

yakagami · 2023-10-11T22:14:35Z

@Renzo-Olivares How should I test for this? Should I create an initailSoruceTimeStamp and test that the ScaleUpadateDetails is the same as the given Duration everywhere that
expect(didStartScale, isTrue);
appears?

yakagami · 2023-10-11T22:51:14Z

hmm testing this shows that _initialEventTimestamp is always null when called in _dispatchOnStartCallbackIfNeeded... perhaps there are more than two potential starts to a scale gesture?

packages/flutter/test/gestures/scale_test.dart

Renzo-Olivares · 2023-11-28T22:16:53Z

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

  /// recognizer.
  final int pointerCount;

+  /// Recorded timestamp of the source pointer event that triggered the drag


nit: consider changing drag to scale.

Renzo-Olivares · 2023-11-28T22:17:11Z

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

  /// recognizer.
  final int pointerCount;

+  /// Recorded timestamp of the source pointer event that triggered the drag


nit: consider changing drag to scale.

Renzo-Olivares

Hi @yakagami, just had a few last comments. Also try rebasing your pr to master and see if that fixes the failing customer tests. They seem unrelated to this pr.

Renzo-Olivares

LGTM

gspencergoog

…5936)

flutter/flutter@5e5b529...918e336 2023-11-30 [email protected] Move Impeller tests on Pixel 7 Pro from staging to prod (flutter/flutter#139280) 2023-11-30 [email protected] Introduce multi-touch drag strategies for `DragGestureRecognizer` (flutter/flutter#136708) 2023-11-30 [email protected] Use the correct recipe on fuchsia_precache. (flutter/flutter#139279) 2023-11-30 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Migration for the `sendTiming` events for `package:unified_analytics`" (flutter/flutter#139278) 2023-11-30 [email protected] Migrate fuchsia_precache to shard tests. (flutter/flutter#139202) 2023-11-29 [email protected] Fix chips `onDeleted` callback don't show the delete button when disabled (flutter/flutter#137685) 2023-11-29 [email protected] Roll Flutter Engine from 9a7e49d75411 to 35939ca8534f (5 revisions) (flutter/flutter#139259) 2023-11-29 [email protected] Refactor to use Apple system fonts (flutter/flutter#137275) 2023-11-29 [email protected] Dynamic view sizing (flutter/flutter#138648) 2023-11-29 [email protected] Roll dependencies (flutter/flutter#139203) 2023-11-29 [email protected] add sourceTimeStamp to ScaleUpdateDetails (flutter/flutter#135936) 2023-11-29 [email protected] Roll Flutter Engine from 222beb28a8eb to 9a7e49d75411 (1 revision) (flutter/flutter#139250) 2023-11-29 [email protected] Remove deprecated `PlatformMenuBar.body` (flutter/flutter#138509) 2023-11-29 [email protected] Migration for the `sendTiming` events for `package:unified_analytics` (flutter/flutter#138896) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

This PR adds the ability to get the `sourceTimeStamp` from `ScaleUpdateDetails` in a `GestureScaleUpdateCallback` like so: ```dart onScaleUpdate: (ScaleUpdateDetails details){ print(details.sourceTimeStamp); } ``` `sourceTimeStamp` is necessary when tracking velocity eg. ```dart VelocityTracker tracker = VelocityTracker.withKind(PointerDeviceKind.touch); ///... onScaleUpdate: (ScaleUpdateDetails details){ tracker.addPosition(details.sourceTimeStamp!, details.focalPoint); } ``` The docs say: >Having both a pan gesture recognizer and a scale gesture recognizer is redundant; scale is a superset of pan. Just use the scale gesture recognizer. Currently this is not entirely accurate, and should be fixed, as noted in flutter#43833 (comment). This PR does not add `sourceTimeStamp` to `ScaleStartDetails` because it is more involved. Specifically, `ScaleStartDetails` can be created in `acceptGesture` which does not have access to the `PointerEvent` to get the `event.timeStamp` (https://github.com/flutter/flutter/blob/54fa25543243e3bf31af6af0c1fef6adabc1d5c1/packages/flutter/lib/src/gestures/scale.dart#L730C5-L730C5). fixes flutter#135873. See also flutter#43833 which added delta and flutter#49025 which added `numPointers` to `ScaleUpdateDetails` for the reason given above. `sourceTimeStamp` should probably be added to `ScaleStartDetails` as well because it exists in `DragStartDetails` and therefore in `onPanStart`. I am not sure how to add tests for this, any input about this PR would be appreciated. - [] All existing and new tests are passing.

…5936)

Adds the `PointerDeviceKind` of the scale start event to `ScaleStartDetails`, which is currently missing. This is a bug according to #115061. Additionally, according to the docs: >Having both a pan gesture recognizer and a scale gesture recognizer is redundant; scale is a superset of pan. Just use the scale gesture recognizer. See also #135936 for similar issues and the PRs fixing them. If multiple pointers are contacting the screen at scale start, then the `PointerDeviceKind` of the first pointer is used. In practice this should be good enough as I don't know of any UI that expects you to use two different pointer kinds at the same time. However, if this is an issue, we could either give a list of pointers with their kinds or a set of all active `PointerDeviceKind`s. I don't think this is necessary though. Closes #115061. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord].  [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md

Adds the `PointerDeviceKind` of the scale start event to `ScaleStartDetails`, which is currently missing. This is a bug according to flutter#115061. Additionally, according to the docs: >Having both a pan gesture recognizer and a scale gesture recognizer is redundant; scale is a superset of pan. Just use the scale gesture recognizer. See also flutter#135936 for similar issues and the PRs fixing them. If multiple pointers are contacting the screen at scale start, then the `PointerDeviceKind` of the first pointer is used. In practice this should be good enough as I don't know of any UI that expects you to use two different pointer kinds at the same time. However, if this is an issue, we could either give a list of pointers with their kinds or a set of all active `PointerDeviceKind`s. I don't think this is necessary though. Closes flutter#115061. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord].  [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md

add sourceTimeStamp to ScaleUpdateDetails

f383d51

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: gestures flutter/packages/flutter/gestures repository. labels Oct 3, 2023

yakagami mentioned this pull request Oct 3, 2023

ScaleUpdateDetails should give sourceTimeStamp #135873

Closed

2 tasks

add tests

57071ce

yakagami added 2 commits October 9, 2023 10:15

add sourceTimeStamp to ScaleUpdateDetails

e214158

add tests

d091caf

yakagami added 4 commits October 10, 2023 20:42

fix tests

b05bf64

Merge branch 'master' of https://github.com/yakagami/flutter

a0b284d

fix tests

c8fd13b

fix tests

393e531

goderbauer requested a review from Renzo-Olivares October 11, 2023 16:44

yakagami and others added 2 commits October 11, 2023 14:24

removed extra declaration casued by merge conflict

1544417

Merge branch 'master' into master

f686cd8

yakagami added 3 commits October 11, 2023 17:53

add sourceTimeStamp to ScaleStartDetails

f13c8ea

Merge branch 'master' of https://github.com/yakagami/flutter

8646a83

make _initialEventTimestamp null after it's used

d53cc80

Renzo-Olivares reviewed Oct 11, 2023

View reviewed changes

packages/flutter/lib/src/gestures/scale.dart Show resolved Hide resolved

yakagami added 4 commits October 11, 2023 18:56

always make _initialEventTimestamp null after use.

a3bf60b

make sure _initialEventTimestamp is set on first interaction

037c05a

remove print statements

059901a

test that initialSourceTimestamp isNull after every non-starting move.

7687692

Renzo-Olivares reviewed Nov 22, 2023

View reviewed changes

packages/flutter/test/gestures/scale_test.dart Outdated Show resolved Hide resolved

Renzo-Olivares reviewed Nov 22, 2023

View reviewed changes

packages/flutter/test/gestures/scale_test.dart Outdated Show resolved Hide resolved

yakagami and others added 6 commits November 22, 2023 18:00

fix spelling of "their"

8aaac6a

Merge branch 'master' of https://github.com/yakagami/flutter

e856b8b

Merge branch 'master' into master

f52ee4c

Merge branch 'master' into master

72ee2b7

add periods to comments

c90e552

Merge branch 'master' of https://github.com/yakagami/flutter

afa0f39

Renzo-Olivares reviewed Nov 28, 2023

View reviewed changes

yakagami and others added 2 commits November 28, 2023 17:46

fix comment to reflect gesture type (drag -> scale)

5d9a1ce

Merge branch 'master' into master

60bd40f

Renzo-Olivares approved these changes Nov 28, 2023

View reviewed changes

yakagami added 2 commits November 28, 2023 19:07

Merge branch 'master' into master

866b173

Merge branch 'master' into master

95463d1

gspencergoog approved these changes Nov 29, 2023

View reviewed changes

Renzo-Olivares added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 29, 2023

auto-submit bot merged commit 443d861 into flutter:master Nov 29, 2023

engine-flutter-autoroll mentioned this pull request Nov 30, 2023

Roll Flutter from 5e5b5292f739 to 918e336014e1 (14 revisions) flutter/packages#5526

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 30, 2023

443d861 add sourceTimeStamp to ScaleUpdateDetails (flutter/flutter#13…

c0013a0

…5936)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 16, 2024

443d861 add sourceTimeStamp to ScaleUpdateDetails (flutter/flutter#13…

1f051e6

…5936)

yakagami mentioned this pull request Mar 13, 2025

add PointerDeviceKind to ScaleStartDetails #165096

Merged

9 tasks

add sourceTimeStamp to ScaleUpdateDetails #135936

add sourceTimeStamp to ScaleUpdateDetails #135936

Uh oh!

Conversation

yakagami commented Oct 3, 2023

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Oct 3, 2023

Uh oh!

moffatman commented Oct 3, 2023

Uh oh!

yakagami commented Oct 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

goderbauer commented Oct 10, 2023

Uh oh!

yakagami commented Oct 11, 2023

Uh oh!

Renzo-Olivares commented Oct 11, 2023

Uh oh!

Uh oh!

yakagami commented Oct 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yakagami commented Oct 11, 2023

Uh oh!

Uh oh!

Uh oh!

Renzo-Olivares Nov 28, 2023

Choose a reason for hiding this comment

Uh oh!

Renzo-Olivares Nov 28, 2023

Choose a reason for hiding this comment

Uh oh!

Renzo-Olivares left a comment

Choose a reason for hiding this comment

Uh oh!

Renzo-Olivares left a comment

Choose a reason for hiding this comment

Uh oh!

gspencergoog left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

yakagami commented Oct 4, 2023 •

edited

Loading

yakagami commented Oct 11, 2023 •

edited

Loading