-
Notifications
You must be signed in to change notification settings - Fork 27.4k
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
Do not rebuild when TickerMode changes #93166
Conversation
@@ -1808,12 +1807,12 @@ class EditableTextState extends State<EditableText> with AutomaticKeepAliveClien | |||
_internalScrollController?.dispose(); | |||
_currentAutofillScope?.unregister(autofillId); | |||
widget.controller.removeListener(_didChangeTextEditingValue); | |||
_floatingCursorResetController.dispose(); | |||
_floatingCursorResetController?.dispose(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do we get a dispose without an init state?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also nit: null the variable after disposing it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do we get a dispose without an init state?
These weren't initialized in initState. Instead, the variable was marked as late
, which initializes them on first access. If the floating cursor is never shown throughout the lifetime of this object (which is possible), then the first access was in dispose here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nulling out: Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a test to make sure we don't regress this? Specifically a test that verifies we don't do whatever work these controllers would drive if we just immediately dispose the widget?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I don't see a good way to test this. The test would have to verify that dispose doesn't instantiate any AnimationControllers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh agreed, I misunderstood what's going on here. Filed dart-lang/linter#3058 which could help with this.
/// [TickerMode] ancestor, it is up to the [Widget] to obtain a new | ||
/// [ValueNotifier] from the new ancestor [TickerMode] by calling this method | ||
/// again. [StatefulWidget]s can, for example, do this in [State.activate]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Express this more prescriptively: if the widget is moved, it must unsubscribe from the old notifier and subscribe to any new one that might be available, the correct place to do this is state.deactivate/activate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-wrote this section to incorporate that information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine overall
/// | ||
/// While the [ValueNotifier] is stable for the lifetime of the surrounding | ||
/// [TickerMode], calling this method does not establish a dependency between | ||
/// the `context` and the [TickerMode]. When the [Widget] owning the `context` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Explain the difference between establishing a dependency or not and why you'd want to avoid it (unnecessarily rebuilding to stop an animation controller, building anyway once the animation controller is started)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-wrote this section to incorporate that information.
17fe1ab
to
5c6e19b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PTAL
@@ -1808,12 +1807,12 @@ class EditableTextState extends State<EditableText> with AutomaticKeepAliveClien | |||
_internalScrollController?.dispose(); | |||
_currentAutofillScope?.unregister(autofillId); | |||
widget.controller.removeListener(_didChangeTextEditingValue); | |||
_floatingCursorResetController.dispose(); | |||
_floatingCursorResetController?.dispose(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nulling out: Done.
/// [TickerMode] ancestor, it is up to the [Widget] to obtain a new | ||
/// [ValueNotifier] from the new ancestor [TickerMode] by calling this method | ||
/// again. [StatefulWidget]s can, for example, do this in [State.activate]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-wrote this section to incorporate that information.
/// | ||
/// While the [ValueNotifier] is stable for the lifetime of the surrounding | ||
/// [TickerMode], calling this method does not establish a dependency between | ||
/// the `context` and the [TickerMode]. When the [Widget] owning the `context` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-wrote this section to incorporate that information.
Also, it seems like we should have a test that routes do not rebuild when their ticker mode changes to muted and they are covered. |
Gold has detected about 18 new digest(s) on patchset 2. |
That's what the test "Ticking widgets in old route do not rebuild when new route is pushed" in this PR is covering. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This pull request is not suitable for automatic merging in its current state.
|
Curiously, the test that's failing on the
... is supposed to be skipped: Line 135 in 2a17ce4
|
5c6e19b
to
52a72e9
Compare
Looks like the test has since been excluded from running: #93268 Rebasing and trying again... |
There were some nice improvements to flutter gallery frame build times on this change: https://flutter-flutter-perf.skia.org/e/?begin=1635352385&end=1637009732&keys=X868dfa49d7994e8de2106df6966dcc25&requestType=0&xbaroffset=26521 |
Fixes #93032.
Prior to this change, all widgets using the TickerProviderStateMixin would rebuild unnecessarily when the TickerMode changed. That's because the provider mixin created a dependency on TickerMode. The rebuild is unnecessary, because a changing ticker mode doesn't have any visual effects. All it needs to do is disable/enable the vended Tickers. When the Tickers start ticking again, their listeners may cause rebuilds independently e.g. to advance an animation.
Also fixes an issue where in certain cases EditableTextState would instantiate AnimationControllers in dispose only to dispose them.