Fix monodrag gestures for #112403 and b/249091367 #121615
Conversation
dnfield
requested review from
dkwingsmt,
goderbauer,
moffatman and
xu-baolin
flutter-dashboard
bot
added
f: gestures
| final DragGestureRecognizer recognizer = VerticalDragGestureRecognizer(); | ||
| const PointerDownEvent event = PointerDownEvent(timeStamp: Duration(days: 10)); | ||
|
|
||
| expect(recognizer.debugLastPendingEventTimestamp, null); |
There was a problem hiding this comment.
In theory, this test would work without exposing (and checking) debugLastPendingEventTimestamp. If the bug were to be re-introduced, it would still crash on line 34, no?
I think I would prefer that over exposing the debug field.
There was a problem hiding this comment.
It would crash, but it wouldn't actually be asserting anything besides that methods can be called.
My main concern over not exposing the field is that someone would be able to easily refactor the internals of the class and the test would still pass but they'd re-introduce the failure case. This actually asserts that acceptGesture can tolerate that field being null.
| // Not entirely clear how this can happen, but the bugs mentioned above show | ||
| // we can end up in this state empircally. |
There was a problem hiding this comment.
This would really be interesting to figure out... Wondering if that's a bug somewhere in the framework in how we use the gesture detector, or in user code, or do the embedders send bogus events??
(Not actionable for this PR)
There was a problem hiding this comment.
Yes, I suspect there's something off here but I don't know that subsystem well enough to know. I'm half hoping one of the people I tagged on this PR will know :)
dnfield
added
the
autosubmit
2 participants
Re-attempts #121553, which itself was another attempt at #117551
Fixes #112403 and b/249091367
I am not super familiar with our gesture recognizer API, so I tried to tag some people here who are probably more familiar and can help me out if my unit test is doing something super weird. My main goal here was just to have a test that covers the bad behavior. Unfortunately, reporters of this bug don't have clear repros, they just have bug reports from the field about null checks going wrong.
@reidbaker FYI - this is a slightly different take on the PR you opened yesterday.
@jmagman FYI as well since youc ommented on the PR yesterday.