InteractiveViewer scale interpretation improvement #63543
Conversation
| if (_scaleStart == null) { | ||
| return; | ||
| } | ||
| assert(_scaleStart != null); |
There was a problem hiding this comment.
The change from an if to an assert here and below was just something I noticed while I was working on this. It's not related to the fix.
|
thank you very much for quickly improvement! <3 |
|
|
||
| if (_gestureType == _GestureType.pan) { | ||
| _panAxis ??= _getPanAxis(_referenceFocalPoint, focalPointScene); | ||
| // When a gesture first starts, it sometimes has zero scale and rotation |
There was a problem hiding this comment.
You mean the scale can still be 0 when there're more than 1 pointers? That sounds like a bug?
There was a problem hiding this comment.
Yes, for the first 1 or 2 calls to onScaleUpdate. I interpreted it as meaning that the two pointers are exactly the same distance apart as when the gesture started for a tiny period of time. In that case, maybe it's not a bug, even though it is kind of inconvenient.
Also when I wrote zero I meant zero change in scale. The scale is actually 1.0. I've updated the comment to be clear about that.
| final _GestureType currentGestureType = _getGestureType(details); | ||
| if (currentGestureType != _GestureType.pan) { | ||
| _gestureType = currentGestureType; | ||
| } |
There was a problem hiding this comment.
nit: this seems to be equivalent to
| final _GestureType currentGestureType = _getGestureType(details); | |
| if (currentGestureType != _GestureType.pan) { | |
| _gestureType = currentGestureType; | |
| } | |
| _gestureType = _getGestureType(details); |
There was a problem hiding this comment.
Much simpler, thanks.
| case _GestureType.pan: | ||
| if (_referenceFocalPoint == null || details.scale != 1.0) { | ||
| assert(_referenceFocalPoint != null); | ||
| if (details.scale != 1.0) { |
There was a problem hiding this comment.
nit: if details.scale is not 1.0 this gesture would have been reinterpreted to scale or rotate I think?
There was a problem hiding this comment.
I thought the same thing at first, but I remembered that this is for the case where scale is disabled. I'll add a comment about that actually.
| return; | ||
| } | ||
|
|
||
| if (_gestureType == _GestureType.pan) { |
There was a problem hiding this comment.
nit: can this be moved to around line 819 (inside the switch clause)?
There was a problem hiding this comment.
Good catch, I should have done that originally.
| _GestureType _getGestureType(ScaleUpdateDetails details) { | ||
| final double scale = !widget.scaleEnabled ? 1.0 : details.scale; | ||
| final double rotation = !_rotateEnabled ? 0.0 : details.rotation; | ||
| if ((scale - 1).abs() > rotation.abs()) { |
There was a problem hiding this comment.
Off-topic: I haven't tried it, but the heuristic here feels like a bit of a hit or miss? These values can have subtle changes even if the user tries to keep their fingers still. And once it's recognized as scale or rotate it can't be reinterpreted to something else.
There was a problem hiding this comment.
Yeah there is probably a better way to do this. Currently rotate is hardcoded to disabled, so it won't affect anyone, but it did work alright when I was using it. The fact that it can't be reinterpreted is on purpose (at least at perceptible time scales) to avoid small amounts of scale rotation when actually just trying to do the opposite gesture. Google Maps does the same thing.
|
This was mentioned as a candidate for cherry-picking into stable here: #63006 (comment) |
|
removing CP label in favor of issue (63006) |
Description
Users reported difficulty in activating a scale gesture. This seemed to be due to some scale gestures beginning with a scale of 1.0, which were interpreted as pan gestures. This PR allows pan gesture to become scale gestures if they later have a change in scale, which makes the experience much more smooth.
Related Issues
Closes #63281
Closes #63006
Tests
I added the following tests: