Fix scale delta docs and calculation #85718

justinmc · 2021-07-01T18:39:40Z

The delta given in ScaleUpdateDetails is incorrect mislabeled as implemented in #85009. This fixes it and gets the docs right.

Related to #43833

LongCatIsLooong · 2021-07-01T19:01:08Z

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

@@ -516,7 +532,7 @@ class ScaleGestureRecognizer extends OneSequenceGestureRecognizer {
          localFocalPoint: PointerEvent.transformPosition(_lastTransform, _currentFocalPoint),


nit: this is already calculated.

LongCatIsLooong · 2021-07-01T19:02:09Z

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

@@ -330,6 +331,7 @@ class ScaleGestureRecognizer extends OneSequenceGestureRecognizer {
  late Map<int, Offset> _pointerLocations;
  late List<int> _pointerQueue; // A queue to sort pointers in order of entrance
  final Map<int, VelocityTracker> _velocityTrackers = <int, VelocityTracker>{};
+  late Offset _delta;


Do we need an instance variable? In _update we always need to recompute the value so maybe we can use a local variable instead?

Can you take another look at this? Sorry, I was in the middle of changing the PR, I should have used a WIP label. As it is now, I need the previousFocalPoint to calculate delta (since now it's just the delta since the last call, not since the start). So I think I either need _delta or _previousFocalPoint as an instance variable.

Ah sorry I thought ScaleUpdateDetails was constructed in _update nvm.

LongCatIsLooong · 2021-07-01T19:07:51Z

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

+    final Offset localPreviousFocalPoint = _state != _ScaleState.started
+        ? localFocalPoint
+        : PointerEvent.transformPosition(
+            _lastTransform,


if the receiving render object moved then _lastTransform is also changed right (I'm assuming _lastTransform is the transform from the global coordinate space to the local coordinate space). Should we use a local variable to track localFocalPoint?

Done, good idea.

You don't have to compute localPreviousFocalPoint I think? It's just _localFocalPoint before you updated it in line 425.

Ah, I was working around the fact that _localFocalPoint is late and won't be initialized on the first run here, so I can't grab the value out of it here.

I've updated it to an if/else that avoids the recalculation, let me know if you like the old way better.

LongCatIsLooong · 2021-07-01T19:11:04Z

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

-  /// The amount the pointer has moved in the coordinate space of the event
-  /// receiver since the previous update.
+  /// The amount the gesture's focal point has moved in the coordinate space of
+  /// the event receiver since the previous update.
  ///
  /// Defaults to zero if not specified in the constructor.
  final Offset delta;


Maybe we should use a more self-explanatory name instead of delta?

I'm trying to mirror the delta parameter in DragUpdateDetails, but it's true it's not exactly the same thing. Maybe focalPointDelta?

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

Lines 156 to 164 in 529a599

/// The amount the pointer has moved in the coordinate space of the event

/// receiver since the previous update.

///

/// If the [GestureDragUpdateCallback] is for a one-dimensional drag (e.g.,

/// a horizontal or vertical drag), then this offset contains only the delta

/// in that direction (i.e., the coordinate in the other direction is zero).

///

/// Defaults to zero if not specified in the constructor.

final Offset delta;

Yeah that sounds good! I was a bit confused about what delta was since there are at least 2 pointers involved in a scale gesture.

LongCatIsLooong · 2021-07-02T01:41:49Z

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

@@ -410,12 +414,27 @@ class ScaleGestureRecognizer extends OneSequenceGestureRecognizer {
  void _update() {
    final int count = _pointerLocations.keys.length;

+    final Offset? previousFocalPoint = _updated ? _currentFocalPoint : null;


nit: instead of using an extra boolean to store _update, is it possible to make _currentFocalPoint an Offset? ?

Yes if I add a few exclamation points where it's used, good call.

LongCatIsLooong · 2021-07-02T01:47:41Z

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

+    final Offset localPreviousFocalPoint = _state != _ScaleState.started
+        ? localFocalPoint
+        : PointerEvent.transformPosition(
+            _lastTransform,


You don't have to compute localPreviousFocalPoint I think? It's just _localFocalPoint before you updated it in line 425.

goderbauer · 2021-07-28T21:51:17Z

(PR triage): @justinmc Do you still have plans for this one?

justinmc · 2021-08-12T00:17:41Z

@LongCatIsLooong Ready for re-review. Sorry I lost track of some PRs so it's been awhile here.

LongCatIsLooong

LGTM

justinmc · 2021-08-13T01:00:51Z

I had to squash the commits to fix a failure in the SVG package for some reason.

Corrects the exact definition in code and docs of ScaleUpDetails delta.

justinmc requested a review from LongCatIsLooong July 1, 2021 18:39

justinmc self-assigned this Jul 1, 2021

flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Jul 1, 2021

google-cla bot added the cla: yes label Jul 1, 2021

LongCatIsLooong reviewed Jul 1, 2021

View reviewed changes

justinmc mentioned this pull request Jul 1, 2021

Add delta param to scaleupdate so it matches dragupdate #85009

Merged

justinmc marked this pull request as ready for review July 1, 2021 21:02

LongCatIsLooong reviewed Jul 2, 2021

View reviewed changes

LongCatIsLooong approved these changes Aug 12, 2021

View reviewed changes

ScaleUpDetails focalPointDelta

45a7372

justinmc force-pushed the scale-delta-fix branch from fd71aad to 45a7372 Compare August 12, 2021 22:43

justinmc merged commit 4bd8b28 into flutter:master Aug 13, 2021

justinmc deleted the scale-delta-fix branch August 13, 2021 01:02

blasten pushed a commit to blasten/flutter that referenced this pull request Aug 19, 2021

ScaleUpDetails focalPointDelta (flutter#85718)

82ff209

Corrects the exact definition in code and docs of ScaleUpDetails delta.

cpboyd mentioned this pull request Sep 23, 2021

beta branch issue: delta renamed focalPointDelta fluttercandies/extended_image#400

Closed

danagbemava-nc mentioned this pull request Dec 9, 2021

Delta cannot be found by ScaleUpdateDetails after update 2.8 #94907

Closed

		@@ -516,7 +532,7 @@ class ScaleGestureRecognizer extends OneSequenceGestureRecognizer {
		localFocalPoint: PointerEvent.transformPosition(_lastTransform, _currentFocalPoint),

	/// The amount the pointer has moved in the coordinate space of the event
	/// receiver since the previous update.
	///
	/// If the [GestureDragUpdateCallback] is for a one-dimensional drag (e.g.,
	/// a horizontal or vertical drag), then this offset contains only the delta
	/// in that direction (i.e., the coordinate in the other direction is zero).
	///
	/// Defaults to zero if not specified in the constructor.
	final Offset delta;

Fix scale delta docs and calculation #85718

Fix scale delta docs and calculation #85718

Uh oh!

Conversation

justinmc commented Jul 1, 2021

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goderbauer commented Jul 28, 2021

Uh oh!

justinmc commented Aug 12, 2021

Uh oh!

LongCatIsLooong left a comment

Choose a reason for hiding this comment

Uh oh!

justinmc commented Aug 13, 2021

Uh oh!

Uh oh!