Skip to content

Allow Scaffold to decline SnackBars from ScaffoldMessenger#69907

Closed
Piinks wants to merge 1 commit into
flutter:masterfrom
Piinks:scaffoldFlag
Closed

Allow Scaffold to decline SnackBars from ScaffoldMessenger#69907
Piinks wants to merge 1 commit into
flutter:masterfrom
Piinks:scaffoldFlag

Conversation

@Piinks

@Piinks Piinks commented Nov 5, 2020

Copy link
Copy Markdown
Contributor

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.

  • Update ScaffoldMessenger migration guide to speak more to nested Scaffolds
  • Update Scaffold API docs to add to the Troubleshooting section on nested Scaffolds

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.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

  • No, no existing tests failed, so this is not a breaking change.

@Piinks Piinks requested a review from HansMuller November 5, 2020 18:55
@flutter-dashboard flutter-dashboard Bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Nov 5, 2020
@google-cla google-cla Bot added the cla: yes label Nov 5, 2020
@Piinks Piinks changed the title Allow Scaffold to decline SnackBars Allow Scaffold to decline SnackBars from ScaffoldMessenger Nov 5, 2020
@Piinks

Piinks commented Nov 5, 2020

Copy link
Copy Markdown
Contributor Author

WDYT @mehmetf ?

this.drawerEdgeDragWidth,
this.drawerEnableOpenDragGesture = true,
this.endDrawerEnableOpenDragGesture = true,
this.registerWithMessenger =true,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

= true

_scaffoldMessenger = _currentScaffoldMessenger;
_scaffoldMessenger?._register(this);

if (widget.registerWithMessenger) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would useful to test the registration logic. Rebuilding with registerWithMessenger: true, or with a new Scaffold (change the Scaffold's key).

@mehmetf

mehmetf commented Nov 5, 2020

Copy link
Copy Markdown
Contributor

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.

@Piinks

Piinks commented Nov 6, 2020

Copy link
Copy Markdown
Contributor Author

Sure! We can move discussion here. :)
The intention for handling nested Scaffolds with the ScaffoldMessenger is to place a ScaffoldMessenger in between them, Scaffolds will only receive SnackBars from the closest ancestor ScaffoldMessenger, so there would only be one SnackBar shown.

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.
This is why the ScaffoldMessenger defines a scope for showing SnackBars.

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.

@mehmetf

mehmetf commented Nov 6, 2020

Copy link
Copy Markdown
Contributor

I absolutely agree with everything you said 👍

If you place a ScaffoldMessenger between the two Scaffolds then each messenger will see exactly one Scaffold registered, correct?

What I meant was something like this. In ScaffoldMessengerState:

void _register(ScaffoldState scaffold) {
    assert(_scaffolds does not contain any item that is a parent of the scaffold, 'You should really scope your Scaffolds....');
    _scaffolds.add(scaffold);
    if (_snackBars.isNotEmpty) {
      scaffold._updateSnackBar();
    }
  }

@Piinks

Piinks commented Nov 6, 2020

Copy link
Copy Markdown
Contributor Author

If you place a ScaffoldMessenger between the two Scaffolds then each messenger will see exactly one Scaffold registered, correct?

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.

What I meant was something like this. In ScaffoldMessengerState:

void _register(ScaffoldState scaffold) {
    assert(_scaffolds does not contain any item that is a parent of the scaffold, 'You should really scope your Scaffolds....');
    _scaffolds.add(scaffold);
    if (_snackBars.isNotEmpty) {
      scaffold._updateSnackBar();
    }
  }

I see what you mean now, thank you! 🎉
I think maybe we can do this by checking if the Scaffold that is being registered has a Scaffold ancestor that is already registered with the ScaffoldMessenger.

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!

@Piinks Piinks closed this Nov 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants