fix a _DraggableScrollableSheetScrollPosition update bug #103328
Conversation
flutter-dashboard
bot
added
f: scrolling
There was a problem hiding this comment.
Do we know that this will always be true? Since this is private implementation?
There was a problem hiding this comment.
Actually, this is not always true.
This may be another type of position when the widget type changes.
Make a type check here also cause some tests failed.
There was a problem hiding this comment.
Hmm, is there another way we could indicate this? Perhaps a flag?
Using is here is not congruent with the style guide: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#avoid-interleaving-multiple-concepts-together
There was a problem hiding this comment.
oops, seems a bit difficult.
I took a look and here is the only chance to catch old position information.
I agree with the above guidance, but here is just to distinguish whether the old type is consistent with myself, and does not interleave other concepts, I think it is reasonable, what do you think? ： ）
There was a problem hiding this comment.
I dunno, tbh I am not very familiar with DraggableScrollableSheet, @dnfield may know more.
All of this is private implementation, so it might be ok to distinguish this case.
What tests failed if the type is assumed to be _DraggableScrollableSheetScrollPosition?
I'll reach out to @Hixie on Discord in case this is a style guide exception.
There was a problem hiding this comment.
I checked to see what tests failed - mostly 'ScrollPositionWithSingleContext' is not a subtype of type '_DraggableScrollableSheetScrollPosition?' in type cast
(I was adding a type cast in the createPosition function of the controller)
What about adding a copyWith to _DraggableScrollableSheetScrollPosition, that way when it is created, you can copy over any ScrollPosition related info (including subclasses like ScrollPositionWithSingleContext)? Just throwing out ideas.. IIRC the copyWith in ScrollBehavior was due to a similar situation. I saw you landed a fix in that recently by the way - thanks!
There was a problem hiding this comment.
@Piinks Thank you so much for catching this bad smell 😄
I think with your ideas I found the perfect solution.
| _DraggableSheetExtent get extent => getExtent(); | ||
|
|
||
| @override | ||
| void absorb(ScrollPosition other) { |
There was a problem hiding this comment.
I agree this approach looks much cleaner and easier to follow.
| super.absorb(other); | ||
| assert(_dragCancelCallback == null); | ||
|
|
||
| if (other is! _DraggableScrollableSheetScrollPosition) { |
There was a problem hiding this comment.
This still uses is, but I checked for more clarity on the style guide.
I think this use case is ok since it is specific to this private class in order to maintain functionality.
I found a number of places in the framework where is is used, and i feel like this is one of those cases where it is appropriate.
Brief chat on discord for reference: https://discord.com/channels/608014603317936148/608018585025118217/979083546989838366
4 participants
Fixes #89681
If update the
_DraggableScrollableSheetScrollPositionduring drag will lose_dragCancelCallback, and will trigger an assertion.