Allow Scaffold to decline SnackBars from ScaffoldMessenger#69907
Conversation
|
WDYT @mehmetf ? |
| this.drawerEdgeDragWidth, | ||
| this.drawerEnableOpenDragGesture = true, | ||
| this.endDrawerEnableOpenDragGesture = true, | ||
| this.registerWithMessenger =true, |
| _scaffoldMessenger = _currentScaffoldMessenger; | ||
| _scaffoldMessenger?._register(this); | ||
|
|
||
| if (widget.registerWithMessenger) { |
There was a problem hiding this comment.
This runs every time any of the inherited widgets Scaffold depends on changes, so it seems we could end up registering with the same scaffold messenger more than once. Unless _register is safe to call more than once and ensures that the old registration is cleared before a new one is created?
| /// [ScaffoldMessenger] to receive [SnackBar]s. | ||
| /// | ||
| /// Defaults to true. | ||
| final bool registerWithMessenger; |
There was a problem hiding this comment.
Might be better to use the whole "ScaffoldMessenger" name and to come up with a name that reflects what's important from the developer's perspective: that we're disabling this Scaffold's ScaffoldMessenger (or that it's "declining snackbars" or something).
| expect(error.message, contains('Only one API should be used to manage SnackBars.')); | ||
| }); | ||
|
|
||
| testWidgets('Scaffolds can decline SnackBars', (WidgetTester tester) async { |
There was a problem hiding this comment.
It would useful to test the registration logic. Rebuilding with registerWithMessenger: true, or with a new Scaffold (change the Scaffold's key).
|
I made some comments about this in the internal bug that Kate and I are discussion. Should we move the discussion here? I am particularly miffed about not being able to detect nested Scaffolds. It would be great to be able to throw an assertion error if we can detect a snackbar will appear multiple times on the same screen. |
|
Sure! We can move discussion here. :) If you want to access a ScaffoldMessenger higher in the tree (above the closest ancestor), you can use a key. This follows the same pattern of accessing Scaffolds with keys to show SnackBars. It might be possible to detect nested Scaffolds, I haven't thought of a way yet, but I'm not sure how we would then decide to handle the SnackBar. I don't know that we can assume which Scaffold the user would want to have their SnackBar in at that point. The API docs do have a troubleshooting section on nested Scaffolds here: https://api.flutter.dev/flutter/material/Scaffold-class.html I think we can definitely add to this. |
|
I absolutely agree with everything you said 👍 If you place a What I meant was something like this. In ScaffoldMessengerState: |
Not necessarily, since there could be multiple Scaffolds registered with one ScaffoldMessenger from a stack of routes, or there could be more than one registered during a page transition, or across Tabs in a TabView, etc.
I see what you mean now, thank you! 🎉 There is also the case of side-by-side Scaffolds, which would result in multiple SnackBars as well. I don't know how we could detect that in the ScaffoldMessenger. I am going to close this, I've filed #69929 for tracking these improvements. Thank you! |
Description
This PR adds a flag to Scaffold that will allow users to not register that Scaffold with the ScaffoldMessenger.
The ScaffoldMessenger is intended to be used to set a scope around the Scaffolds it manages SnackBars for.
When Scaffolds are nested inside one another, you can put a ScaffoldMessenger in between to control these SnackBar scopes, but recent customer feedback has shown this is might be improved by introducing a simple flag. I am not sure that this is the best approach though since adding a ScaffoldMessenger effectively closes the scope between Scaffolds.
TODO
ScaffoldMessenger has not shipped to stable yet, so this is a good opportunity to improve the docs in anticipation of similar experiences.
Related Issues
b/171649782
Tests
Scaffolds can decline SnackBars
Checklist
Before you create this PR, confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change
Did any tests fail when you ran them? Please read Handling breaking changes.