Re-land ScaffoldMessenger by Piinks · Pull Request #66504 · flutter/flutter

Piinks · 2020-09-23T23:14:04Z

Description

This will re-introduce the ScaffoldMessenger, this time leaving in place the old API for SnackBars via the Scaffold. This will allow for an easier migration for our customers.
As such, this adds an assertion to ensure we are only using one SnackBar API, since they are not meant to be used interchangeably.

The documentation and samples have been updated to recommend ScaffoldMessenger in order to mitigate confusion during the interim.

Original PR: #64101
Migration Guide: flutter/website#4527

Follow-up work:

Migrate the rest of the framework (see [WIP] ScaffoldMessenger Migration #64170)
There is a SnackBar demo in the cookbook that will need updating
Check the rest of website for snackbar use
Notify & migrate customers, deprecate old methods after migration

Related Issues

Fixes #62921

Tests

Oodles.
All existing SnackBar tests have been migrated and passed. Added a few more for new persistence across routes.
Several control tests for the original API have been preserved to ensure we are not breaking the old API while bringing in the new.

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.

…enger3

HansMuller

I haven't looked at the tests yet.

Is there some way to avoid temporarily adding SnackBarAction,SnackBar.useScaffoldMessenger?

HansMuller · 2020-09-24T01:04:38Z

+  ///       _counter++;
+  ///     });
+  ///     if (_counter % 10 == 0) {
+  ///       _scaffoldMessengerKey.currentState.showSnackBar(const SnackBar(


In this case you do have access to the context, because State.context. If the incrementCounter method and its state were in a separate class, this "no context" scenario would be clearer.

Piinks · 2020-09-24T16:13:43Z

Is there some way to avoid temporarily adding SnackBarAction,SnackBar.useScaffoldMessenger?

I haven't been able to come up with another way yet. 🤔 Since SnackBarAction and SnackBar call on the SnackBar methods, and we are going to support both APIs temporarily, they have to know which one to call, Scaffold or ScaffoldMessenger.

…enger4

Piinks · 2020-09-25T16:50:50Z

Marking as WIP, @HansMuller and I discussed a re-work that will simplify the way SnackBars can handle themselves in this 2-way API interim.

Piinks · 2020-09-25T22:13:52Z

@HansMuller applied the changes from our brainstorming session, PTAL. :)

gspencergoog · 2020-10-02T19:07:11Z

 ///           tooltip: 'Show Snackbar',
 ///           onPressed: () {
-///             scaffoldKey.currentState.showSnackBar(snackBar);
+///             ScaffoldMessenger.of(context).showSnackBar(snackBar);


Can't the of call return null if there's no messenger in the tree?

It can, but in this case it there is a MaterialApp above in the sample code, so it has a ScaffoldMessenger by default.

gspencergoog · 2020-10-02T19:12:46Z

+  ///
+  ///  * [debugCheckHasScaffoldMessenger], which asserts that the given context
+  ///    has a [ScaffoldMessenger] ancestor.
+  static ScaffoldMessengerState of(BuildContext context) {


Should this include a call to debugCheckHasScaffoldMessenger to assert with a nice message when it fails to find one it expected to find?

Also, a common pattern with these of calls is to include a nullOk optional bool that allows it to not assert and just return null.

I realize we do also have this pattern in some places of just returning null, but I guess I'm asking which is more appropriate for the use case: defaulting to throwing if it isn't found, or defaulting to not throwing and making the developer check for it? I think I'd lean toward the former, because it'll make them think about their choice.

At one point I had it the way you describe, but it was massively breaking at the time. It's been re-worked a fair bit since then, so I think I should be able to add this back now. Thank you!

I've applied your suggestions, PTAL. :)

gspencergoog

fluttergithubbot · 2020-10-05T19:37:03Z

This pull request is not suitable for automatic merging in its current state.

The status or check suite Linux framework_tests has failed. Please fix the issues identified (or deflake) before re-applying this label.

fluttergithubbot · 2020-10-05T20:02:03Z

This pull request is not suitable for automatic merging in its current state.

The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

…enger4

goderbauer

Fly-by comment

Co-authored-by: Michael Goderbauer <[email protected]>

Piinks added 14 commits August 26, 2020 12:34

ScaffoldMessenger

d608e4a

Remove unintended changes

8f7593c

Fix sample

6e784ab

++

20d8afd

Apply suggestions from code review

d9f57e3

Update docs

063e159

Review feedback

c4632bd

Sample app

843e287

Merge branch 'master' of github.com:flutter/flutter into scaffoldMess…

2042863

…enger3

Fix sample code

e3cd0d7

Review feedback

72469cd

Merge branch 'master' of github.com:flutter/flutter into scaffoldMess…

3626dc3

…enger3

Update to re-land

c04ffa1

More test

a475e3f

Piinks added c: new feature Nothing broken; request for a new capability framework flutter/packages/flutter repository. See also f: labels. p: material_ui material_ui package in flutter/packages a: quality A truly polished experience customer: money (g3) labels Sep 23, 2020

googlebot added the cla: yes label Sep 23, 2020

Fix analyzer for deprecation.

0c1373b

Piinks requested a review from HansMuller September 23, 2020 23:31

HansMuller reviewed Sep 24, 2020

View reviewed changes

Piinks added 6 commits September 24, 2020 11:51

Review feedback

30981fc

Analyzer

a667729

Re-trigger build

739b0e2

++

ac6a955

Merge branch 'master' of github.com:flutter/flutter into scaffoldMess…

3117436

…enger4

Analyzer again

587dc5b

Piinks added the work in progress; do not review label Sep 25, 2020

Piinks added 2 commits September 25, 2020 16:58

Re-work for untouched SnackBar classes

43bd2ea

Tidying

aa1d646

Piinks mentioned this pull request Sep 25, 2020

Better rationale should be documented for using GlobalKeys #66690

Open

Piinks removed the work in progress; do not review label Sep 25, 2020

gspencergoog reviewed Oct 2, 2020

View reviewed changes

Piinks added 2 commits October 5, 2020 13:29

Add nullOk for .of, + tests

c552f47

Analyzer

df692db

gspencergoog approved these changes Oct 5, 2020

View reviewed changes

Piinks added the waiting for tree to go green label Oct 5, 2020

fluttergithubbot removed the waiting for tree to go green label Oct 5, 2020

Piinks added the waiting for tree to go green label Oct 5, 2020

fluttergithubbot removed the waiting for tree to go green label Oct 5, 2020

Piinks added 2 commits October 5, 2020 15:12

Merge branch 'master' of github.com:flutter/flutter into scaffoldMess…

5216711

…enger4

Merge branch 'master' of github.com:flutter/flutter into scaffoldMess…

ef5d262

…enger4

goderbauer reviewed Oct 5, 2020

View reviewed changes

Comment thread packages/flutter/lib/src/material/scaffold.dart Outdated

Update packages/flutter/lib/src/material/scaffold.dart

6ce0413

Co-authored-by: Michael Goderbauer <[email protected]>

Piinks added the waiting for tree to go green label Oct 5, 2020

Piinks merged commit 64c845c into flutter:master Oct 5, 2020

This was referenced Oct 12, 2020

Migrate SnackBar methods to ScaffoldMessenger flutter-team-archive/gallery#343

Merged

Migrate SnackBar methods to ScaffoldMessenger flutter/packages#223

Merged

Migrate SnackBar methods to ScaffoldMessenger jocosocial/rainbowmonkey#330

Merged

Conversation

Piinks commented Sep 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

HansMuller left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

HansMuller Sep 24, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Piinks commented Sep 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Piinks commented Sep 25, 2020

Uh oh!

Piinks commented Sep 25, 2020

Uh oh!

gspencergoog Oct 2, 2020

Choose a reason for hiding this comment

Uh oh!

Piinks Oct 2, 2020

Choose a reason for hiding this comment

Uh oh!

gspencergoog Oct 2, 2020

Choose a reason for hiding this comment

Uh oh!

Piinks Oct 2, 2020

Choose a reason for hiding this comment

Uh oh!

Piinks Oct 5, 2020

Choose a reason for hiding this comment

Uh oh!

gspencergoog left a comment

Choose a reason for hiding this comment

Uh oh!

fluttergithubbot commented Oct 5, 2020

Uh oh!

fluttergithubbot commented Oct 5, 2020

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Piinks commented Sep 23, 2020 •

edited

Loading

Piinks commented Sep 24, 2020 •

edited

Loading