Skip to content

Conversation

@justinmc
Copy link
Contributor

@justinmc justinmc commented Jul 1, 2021

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

Related to #43833

@justinmc justinmc requested a review from LongCatIsLooong July 1, 2021 18:39
@justinmc justinmc self-assigned this Jul 1, 2021
@flutter-dashboard flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Jul 1, 2021
@google-cla google-cla bot added the cla: yes label Jul 1, 2021
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this is already calculated.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah sorry I thought ScaleUpdateDetails was constructed in _update nvm.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, good idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

/// 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;

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@goderbauer
Copy link
Member

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

@justinmc
Copy link
Contributor Author

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

Copy link
Contributor

@LongCatIsLooong LongCatIsLooong left a comment

Choose a reason for hiding this comment

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

LGTM

@justinmc
Copy link
Contributor Author

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

@justinmc justinmc merged commit 4bd8b28 into flutter:master Aug 13, 2021
@justinmc 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
Corrects the exact definition in code and docs of ScaleUpDetails delta.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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