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

Add BuildContext.mounted #111619

Merged
merged 3 commits into from Sep 15, 2022
Merged

Add BuildContext.mounted #111619

merged 3 commits into from Sep 15, 2022

Conversation

goderbauer
Copy link
Member

@goderbauer goderbauer commented Sep 14, 2022

This makes it a little easier to use BuildContext error-free across asynchronous gaps in StatelessWidgets and other instances where you can't check State.mounted.

Fixes #111488

DO NOT MERGE until use_build_context_synchronously lint has been updated to recognize BuildContext.mounted: dart-lang/linter#3688

@flutter-dashboard flutter-dashboard bot added a: text input Entering text in a text field or keyboard related problems. framework flutter/packages/flutter repository. See also f: labels. labels Sep 14, 2022
/// an asynchronous operation), consider checking [mounted] to determine whether
/// the context is still valid before interacting with it:
///
/// ```dart
Copy link
Contributor

@gspencergoog gspencergoog Sep 14, 2022

Choose a reason for hiding this comment

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

This is almost a functional example app. Would it make sense to make it into a full sample?

Copy link
Member Author

@goderbauer goderbauer Sep 14, 2022

Choose a reason for hiding this comment

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

Hm, it doesn't demonstrate anything visually though. In this case I think a full sample would actually distract from the point this is trying to make...

Copy link
Contributor

@gspencergoog gspencergoog Sep 14, 2022

Choose a reason for hiding this comment

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

Ok, makes sense. I mean it could show a dialog going away after a second, but I agree it's not all that visual.

Copy link
Contributor

@chunhtai chunhtai left a comment

LGTM

Copy link
Contributor

@gspencergoog gspencergoog left a comment

32384589-a60f0e74-c078-11e7-9bc1-e5b5287aea9d

Copy link
Contributor

@fzyzcjy fzyzcjy left a comment

Not an expert here, but I see State.mounted is sometimes not enough for a State.context to be used.

What I have seen: error, Looking up a deactivated widget's ancestor is unsafe., when using SomeInheritedWidget.of(context) - you know, the very common practice. The error is like

Looking up a deactivated widget's ancestor is unsafe.
At this point the state of the widget's element tree is no longer stable.
To safely refer to a widget's ancestor in its dispose() method, save a reference to the ancestor by calling dependOnInheritedWidgetOfExactType() in the widget's didChangeDependencies() method.

When the exception was thrown, this was the stack: 
#0      Element._debugCheckStateIsActiveForAncestorLookup.<anonymous closure> (package:flutter/src/widgets/framework.dart:3990:9)
#1      Element._debugCheckStateIsActiveForAncestorLookup (package:flutter/src/widgets/framework.dart:4004:6)
#2      Element.dependOnInheritedWidgetOfExactType (package:flutter/src/widgets/framework.dart:4019:12)
#3      SomeOfMyInheritedWidget.of (...)

That seems to come from when the widget is deactivated and not yet (re) activated.

My personal workaround is that, has an extra variable tracking activate/deactivate, and never call them when it is deactivated and not yet activated. But that does not sound elegant.

Notice it uses _debugCheckStateIsActiveForAncestorLookup, and your example code, Navigator.of(context), calls findRootAncestorStateOfType which also has the _debugCheckStateIsActiveForAncestorLookup assertion.
Therefore, I suspect this PR will suffer from the same error.

/// onPressed: () async {
/// await Future<void>.delayed(const Duration(seconds: 1));
/// if (context.mounted) {
/// Navigator.of(context).pop();
Copy link
Contributor

@fzyzcjy fzyzcjy Sep 14, 2022

Choose a reason for hiding this comment

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

(here)

@goderbauer
Copy link
Member Author

goderbauer commented Sep 15, 2022

@fzyzcjy Can you file an issue for that? Ideally with some code that reproduces the error you're seeing? I am curious to see how you can do a dependency lookup while a global key move or reparenting is in progress.

@goderbauer goderbauer added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 15, 2022
@auto-submit auto-submit bot merged commit b8df71f into flutter:master Sep 15, 2022
64 checks passed
@fzyzcjy
Copy link
Contributor

fzyzcjy commented Sep 15, 2022

@goderbauer Sure I will do that when see the bug again

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 16, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Sep 16, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Sep 16, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 16, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 16, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: text input Entering text in a text field or keyboard related problems. autosubmit Merge PR when tree becomes green via auto submit App framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants