Adds force press gesture detector and recognizer#24554
Adds force press gesture detector and recognizer#24554jslavitz merged 20 commits intoflutter:masterfrom
Conversation
| peaked, | ||
| } | ||
|
|
||
| /// Details object for callbacks that use [GestureForcePressStartCallback], |
There was a problem hiding this comment.
If you reached start, you should already have a pressure value no?
There was a problem hiding this comment.
Yes? But the pressure value will be sent in onUpdate. In this way, onStart and the first onUpdate will sent in the same frames. It's the way it's done for monodrag.dart so I thought it would make sense to replicate the behavior on this recognizer. I guess it would still make sense to include the pressure value for the onStart callback even though it will be the same as the one sent for onUpdate?
There was a problem hiding this comment.
Ah I see, didn't realize it was on the same frame. Sounds ok then.
There was a problem hiding this comment.
I decided to just add the pressure into all the calls – so you don't have to have an onUpdate to get the pressure if you really only want the onStart call.
| possible, | ||
|
|
||
| // A pointer is down and a force press gesture has been detected. However, if | ||
| // ForcePressGestureRecognizer the only recognizer in the arena, the gesture |
There was a problem hiding this comment.
Not sure I understand this comment. What makes the gesture get accepted if it hasn't started yet?
There was a problem hiding this comment.
So apparently gestures will be accepted by default if they're the only gestures in the arena.
There was a problem hiding this comment.
I would write:
However, if the ForcePressGestureRecognizer is the only recognizer in the arena, thus accepted as soon as the gesture state is possible, the gesture will not yet have started.
to connect the relationship
zoechi
added
framework
| resolve(GestureDisposition.rejected); | ||
| } | ||
| // In case this is the only gesture detector we still don't want to start | ||
| // the gesture until the pressure is greater than the startPressure. |
There was a problem hiding this comment.
Not sure what this means. If line 181 ran, wouldn't this always always run? Maybe I'm missing something.
There was a problem hiding this comment.
It wouldn't because I set the state to started before line 181 runs. I'm navigating the case where the gesture has been accepted, but the pressure isn't greater than the start pressure. That's the scenario in which this line will run.
|
To be clear, the range of pressures is:
If the OS has a different concept of "normal" then the data should be scaled to fit that range by the engine. The meanings of values between 0 and 1 and above 1 are never going to be very consistent, even with the same hardware, user to user, since different hands have different strength muscles. I think we could have the gesture recognizer normalize the range further, e.g. 0.0-1.0 for values less than 1.0 and 1.0-2.0 for values above 1.0, and base it on the normalized range (for most devices, the value will always be 1.0). It won't be perfect, but it's not clear to me what a developer would do that could be better even if we provide raw values to a predicate. |
xster
left a comment
xster
left a comment
There was a problem hiding this comment.
The rest looks good but we need to solve the range issue.
| int buttons = 0, | ||
| bool obscured = false, | ||
| double pressure = 1.0, | ||
| double pressure = 0.0, |
There was a problem hiding this comment.
If we make (though minor) breaking changes here and for pointer added, we should announce it in flutter-dev
| possible, | ||
|
|
||
| // A pointer is down and a force press gesture has been detected. However, if | ||
| // ForcePressGestureRecognizer the only recognizer in the arena, the gesture |
There was a problem hiding this comment.
I would write:
However, if the ForcePressGestureRecognizer is the only recognizer in the arena, thus accepted as soon as the gesture state is possible, the gesture will not yet have started.
to connect the relationship
| /// The pressure of the pointer on the screen. | ||
| final double pressure; | ||
| } | ||
|
|
There was a problem hiding this comment.
It's fine to do it in a different PR but I was saying above, there will be cases where users will use the pressure double value before start is triggered. See on the home screen, when you barely force press an icon, it'll visually respond before you trigger the start.
There was a problem hiding this comment.
I thought start was supposed to be the pressure at which it begins to visually respond?
| /// Creates a force press gesture recognizer. | ||
| /// | ||
| /// The [startPressure] defaults to 0.4, and [peakPressure] defaults to 0.85 | ||
| /// where a value of 0.0 is no pressure and a value of 1.0 is maximum pressure. |
There was a problem hiding this comment.
We definitely need to address this before merging.
To @Hixie's comment. We don't really need to worry about subjectively interpreting the 0-1 range. The Android and iOS APIs already contractually assign meanings to those bounds. We can just pass them directly.
There are no higher bound API definitions. I would prefer letting start and peak be predicates in those cases. The user may want to curve differently if the device can go to 3 vs if it can go to 10. We don't have that future information and shouldn't lock our users to our present interpretation.
|
\o/ |
| // A static pointer with changes in pressure creates PointerMoveEvent events. | ||
| if (event is PointerMoveEvent || event is PointerDownEvent) { | ||
| final double pressure = interpolation(event.pressureMin, event.pressureMax, event.pressure); | ||
| assert(pressure.isNaN ? true : (pressure <= 1.0 && pressure >= 0.0)); |
There was a problem hiding this comment.
Generally speaking I would recommend using asserts that only use || and &&, and ranges should be checked low-then-high for clarity, so this would be better written as:
assert(pressure.isNaN || (pressure >= 0.0 && pressure <= 1.0));cc @xster who reviewed
| /// interpolation equation, the pressure reported will correspond with the | ||
| /// custom curve. (ie. if the interpolation maps t(0.5) -> 0.1, a value of 0.1 | ||
| /// will be reported at a device pressure value of 0.5). | ||
| /// |
There was a problem hiding this comment.
cc @xster: when reviewing, please look out for trailing blank lines on comments such as here
| /// The [startPressure] defaults to 0.4, and [peakPressure] defaults to 0.85 | ||
| /// where a value of 0.0 is no pressure and a value of 1.0 is maximum pressure. | ||
| /// | ||
| /// [startPressure], [peakPressure] and [interpolation] must not be null. |
There was a problem hiding this comment.
@xster while i'm at it: when reviewing, please look out for sentences that don't start with a capital letter. This would be better written as: The arguments [...], [...], and [...] must not be null.
|
@Hixie understood. Will be addressed in #26563 (comment) |
5 participants
Adds for force touch support for force touch enabled devices through the creation of a force touch gesture recognizer. Fixes #4838.