Make DraggableScrollableSheet Reflect Parameter Updates #90354
Conversation
flutter-dashboard
bot
added
the
framework
caseycrogers
force-pushed
the
sheet_update_fix
branch
from
d415cb5 to
90b9c0a
Compare
| testWidgets('Setting snapSizes to $snapSizes resolves to min and max', (WidgetTester tester) async { | ||
| const Key stackKey = ValueKey<String>('stack'); | ||
| const Key containerKey = ValueKey<String>('container'); | ||
| await tester.pumpWidget(_boilerplate(null, |
There was a problem hiding this comment.
Not particularly related to this PR, but this code was improperly formatted.
| /// Rebuilding the sheet with a new [initialChildSize] will only move the | ||
| /// the sheet to the new value if the sheet has not yet been dragged since it | ||
| /// was first built or since the last call to [DraggableScrollableActuator.reset]. | ||
| /// |
There was a problem hiding this comment.
This seemed like the most intuitive/desirable behavior, but I'd love some input here as this was just a judgement call.
Also see documentation changes for snap and snapSizes immediately below.
caseycrogers
changed the title
|
cc @dnfield could you review this for me? It adds |
dnfield
left a comment
dnfield
left a comment
There was a problem hiding this comment.
LGTM with a couple nitpicks
| context: context, | ||
| oldPosition: oldPosition, | ||
| extent: extent, | ||
| controller: this, |
There was a problem hiding this comment.
This is all private so it's probably not a big deal, but it seems wrong to give a position knowledge of the controller.
It would probably be better to have an abstract class like
abstract class _DraggableSheetExtentHolder {
_DraggableSheetExtent extent;
}and make the controller implement that, to make sure that no one comes along later and starts tinkering with controller API in the position and messes something up.
There was a problem hiding this comment.
What about providing an anonymous function that returns extent? This feels a little lighter weight and I think makes the intent more obvious (to provide mutable state). However, the extent holder seems fine too.
I just pushed the function version, let me know if you prefer the holder version and I'll switch to it.
| } | ||
|
|
||
| @override | ||
| _DraggableScrollableSheetScrollPosition get position => |
There was a problem hiding this comment.
Needed so we can access .goBallistic from _replaceExtent.
| super.dispose(); | ||
| } | ||
|
|
||
| void _replaceExtent() { |
There was a problem hiding this comment.
I named this _replaceExtent instead of _updateExtent so that it wouldn't get confused with extent.updateExtent.
packages/flutter/lib/src/widgets/primary_scroll_controller.dart
Outdated
Show resolved
Hide resolved
|
@dnfield It appears this one got stuck in CI limbo too? Are we able to get it through anyway? (sorry to ping you so much, let me know if you want me to cut back on the mentions). |
|
Looks like just google testing is waiting, which can take a bit. I added the label, if google testing doesn't complete in the next day or so we can revisit. No worries about the pings :) |
|
(Triage) Confirmed the G testing shard passed, will manually merge. |
4 participants
Currently,
DraggableScrollableSheetsaves copies of widget parameters (snap,minChildSize, etc.) to a private instance variable in the state object and then does not update that object when the widget is rebuilt with new parameters. This PR fixes this issue.Additional logic is included to update the current position of the sheet if the new parameters necessitate it. eg if the sheet is at
.9and is rebuilt with a new max size of.8, the sheet will be rebuilt with a current size of.8.This PR takes some logic from #69724
Fixes #41599
Here is a gist I made for playing around with and visualizing the new behavior.
Pre-launch Checklist
///).