-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Fix scale delta docs and calculation #85718
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, good idea.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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? ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
(PR triage): @justinmc Do you still have plans for this one? |
|
@LongCatIsLooong Ready for re-review. Sorry I lost track of some PRs so it's been awhile here. |
LongCatIsLooong
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
fd71aad to
45a7372
Compare
|
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.
The
deltagiven in ScaleUpdateDetails is incorrect mislabeled as implemented in #85009. This fixes it and gets the docs right.Related to #43833