Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update the "setState called during build" in common-errors.md #11353

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

554richard
Copy link

I ran into the error "setState called during build" while writing an app which uses the pluto_grid package. I had the need to validate a cell in the onChanged callback of the grid, but got the error when I tried to do a showDialog to alert the user to an error.
To solve it I went to the common-errors page in the flutter docs, but found a) the code describing the problem was a snippet with no obvious functionality and b) the solution code is for a different situation, not a direct solution to either my problem or the one in the given snippet.

I can see that there are different solutions in different scenarios (Navigator, FutureBuilder, addPostFrameCallback) but I feel the addPostFrameCallback is the most intuitive and direct, particularly in the showDialog scenario.

The example code I give is a bit arbitrary and useless, but I can see that including code with the pluto_grid package would be inappropriate, and I wasn't sure what else I could use for an illustration.

This is my first attempt at contributing to an Open Source project of any sort. Please let me know if I've messed up., so I can improve at the procedure.

1. Change the "problem code" from a snippet to code that triggers the error.
2. Change the "fix code" to working code that directly solves the working code.
@554richard 554richard requested review from sfshaza2, parlough and a team as code owners November 1, 2024 20:26
Copy link

google-cla bot commented Nov 1, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Copy link
Member

@nate-thegrate nate-thegrate left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

I don't normally review flutter/website PRs, but I did want to share some thoughts regarding the Dart code here :)

src/content/testing/common-errors.md Outdated Show resolved Hide resolved
Comment on lines +527 to +537
//You can do this:
WidgetsBinding.instance.addPostFrameCallback((_) {
showDialog<void>(
context: context,
builder: (BuildContext context) {
return const AlertDialog(
title: Text('Alert Dialog'),
);
},
);
});
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding the post-frame callback suggestion!

In its current state, though, I'm not sure if this is something we want to encourage in the documentation: scheduling a dialog in the build method means a new dialog is created each time the widget is rebuilt. If there's something like Theme.of(context) or MediaQuery.sizeOf(context) in the build method, resizing the window or switching to dark mode might make a new dialog pop up every frame!

The ideal setup would be to use a stateful widget (so the dialog could be scheduled inside initState) or make it part of an onPressed callback (similar what the original example did). Perhaps this page could show both!

Comment on lines 448 to 449
<?code-excerpt "lib/set_state_build.dart (problem)"?>
```dart
Copy link
Member

Choose a reason for hiding this comment

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

If all the code required for running an app is being added here, maybe it could be structured as an interactive DartPad sample! I'm not sure exactly how it works but I did see another page that was formatted this way:

## Interactive example
<?code-excerpt "lib/main.dart"?>
```dartpad title="Flutter TabBar DartPad hands-on example" run="true"
import 'package:flutter/material.dart';
void main() {
runApp(const TabBarDemo());
}
class TabBarDemo extends StatelessWidget {
const TabBarDemo({super.key});
@override
Widget build(BuildContext context) {
return MaterialApp(
home: DefaultTabController(
length: 3,
child: Scaffold(
appBar: AppBar(
bottom: const TabBar(
tabs: [
Tab(icon: Icon(Icons.directions_car)),
Tab(icon: Icon(Icons.directions_transit)),
Tab(icon: Icon(Icons.directions_bike)),
],
),
title: const Text('Tabs Demo'),
),
body: const TabBarView(
children: [
Icon(Icons.directions_car),
Icon(Icons.directions_transit),
Icon(Icons.directions_bike),
],
),
),
),
);
}
}

Copy link
Author

Choose a reason for hiding this comment

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

That's a great idea, but I don't know how I'd be able to test the functionality before submitting it for review. I can get common-errors.md onto my PC, but how would I serve it to myself for testing, especially with the an interactive DartPad in place?

Copy link
Member

Choose a reason for hiding this comment

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

If the goal is to test out your sample code, feel free to copy-paste it into the existing TabBar demo :)

src/content/testing/common-errors.md Outdated Show resolved Hide resolved
for the second page and another for the dialog.
One way to avoid this error is instruct flutter to finish rendering the page
before implementing the showDialog. This can be done by using
addPostFrameCallback() method. The following code illustrates this on the broken example just given:
Copy link
Member

Choose a reason for hiding this comment

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

Generally we want files to have lines of <= 80 characters. (more info here)

Copy link
Author

Choose a reason for hiding this comment

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

I split line 501 into two in my fork of the code, but I'm not sure if this change makes its way automatically into the PR, or whether I need to do something to update it.

Copy link
Member

Choose a reason for hiding this comment

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

It looks like this PR is using the 554richard:patch-1 branch. If you push changes to that branch, they'll show up in this pull request!

Comment on lines -501 to -513
onPressed: () {
// Navigate to the second screen using a named route.
Navigator.pushNamed(context, '/second');
// Immediately show a dialog upon loading the second screen.
Navigator.push(
context,
PageRouteBuilder(
barrierDismissible: true,
opaque: false,
pageBuilder: (_, anim1, anim2) => const MyDialog(),
),
);
},
Copy link
Member

@nate-thegrate nate-thegrate Nov 5, 2024

Choose a reason for hiding this comment

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

Here's a possible solution that doesn't involve stateful widgets (using the Navigator's context for the dialog, to ensure it isn't disposed of by the end of the frame):

onPressed: () async {
  final NavigatorState navigator = Navigator.of(context);
  navigator.pushNamed('/second');

  // The default Material page transition duration is 300 ms.
  await Future.delayed(Durations.medium2);
  showDialog(
    context: navigator.context,
    builder: (context) => const MyDialog(),
  );
},

Copy link
Author

Choose a reason for hiding this comment

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

The difficulty I'm having with implementing this is that my example code doesn't have an onPressed callback anywhere in it, and if I create a button in the Scaffold and put the showDialog in its onPressed:,, the "setState error during build" error disappears without the need of a addPostFrameCallback.

I'm afraid that the broken code I give as an example is crazy - nobody in their right mind would write it. The actual code that caused me the problem was with pluto_grid, and I don't know how to create a sensible example of the error without it - and I can see that pluto_grid should not be included in a page like this. I guess the solution snippet in the current version is a good solution to a problem which was also too specific - which is why I didn't find it helpful.

I suppose I've got into trouble because I'm trying to post a solution to a specific problem in a "common errors" page whose purpose has to be much more general.

If I can't find a sensible example of broken code - which uses a callback like onPressed or onChanged and causes this particular error without using pluto_grid, I don't think I can make a sensible contribution. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for sharing your thoughts.

I think you made a good point about the post-frame callback not being necessary here 🙂
I edited the comment above accordingly, thanks!

@sfshaza2
Copy link
Contributor

sfshaza2 commented Nov 7, 2024

/gcbrun

@sfshaza2
Copy link
Contributor

sfshaza2 commented Nov 7, 2024

@nate-thegrate, THANK YOU for jumping in here!!! I've seen the discussion and the updates. I've just kicked off the CI testing to see if it's all happy.

@flutter-website-bot
Copy link
Collaborator

Visit the preview URL for this PR (updated for commit f347472):

https://flutter-docs-prod--pr11353-patch-1-hpez88pc.web.app

@sfshaza2
Copy link
Contributor

sfshaza2 commented Nov 7, 2024

Hi, @554richard! According to the CI results, you need to refresh the code excerpts for the site. If you click Details on the "test" line, it tells you how to do this. Just run the script locally and commit the changes. Also, it's not 100% clear to me if you and @nate-thegrate have a solution that you are both happy with.

@sfshaza2 sfshaza2 added the review.await-update Awaiting Updates after Edits label Nov 7, 2024
@554richard
Copy link
Author

I'm afraid my naivety in this business is a problem. I see the test failure and found the line to I need to run and then recommit, but I made all the original changes by forking, editing and PR within the GitHub website, and I don't know how or where to run the script and recommit.

Also after discussion I am not happy with the solution I have presented. I feel that my pull request has weaknesses which make it maybe no better than the original document, and I don't know how to correct them (see my last comment above). I am perfectly happy to continue but I need expert advice. Should I maybe drop this request and file some kind of request/bug report?

@nate-thegrate
Copy link
Member

Error: Some code excerpts needed to be updated!
  Run `./dash_site refresh-excerpts` to update.

My guess would be that you'd need to get your branch downloaded to a local machine in order to run that script, but I'm not really familiar with it either 😅

Overall, I think this is a nice contribution—at the very least, it'd be nice to update this file so it uses the showDialog() function rather than a page route builder. And in the case of showing a popup on launch, I think it does make sense to do it via post-frame callback, but inside initState rather than build.

Hopefully we can get this figured out!

@554richard
Copy link
Author

I think the pop-up has to come from an onChanged or onPressed callback to make any sense. This could set a flag to tell the initState to execute the showDialog. I think this may be a working solution even without the the Postframe callback. I can experiment with that in my pluto_grid code. The problem I have is how to make a "before" example which triggers the "setState called during build" without using pluto_grid, but with an onPressed callback or the like. I guess we could leave the "before" code snippet which is on the page as it is - somebody else can improve it if they get any bright ideas.

As for the test failure, I'm figuring out how to get my branch onto my local machine - I failed with VSCode, I'm trying now with GitHub Desktop. I guess I'll learn a bit more about how GitHub works ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review.await-update Awaiting Updates after Edits
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants