-
Notifications
You must be signed in to change notification settings - Fork 306
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
Refactors AutoDisposeMixin
to have separate cancel methods for listeners, stream subscriptions, and focus nodes
#3540
Conversation
@@ -158,8 +158,6 @@ class SnapshotFilterState extends State<SnapshotFilterDialog> | |||
for (var key in oldFiltered.keys) { | |||
oldFilteredLibraries[key] = oldFiltered[key].first.hide; | |||
} | |||
|
|||
cancel(); |
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.
The listeners this was canceling were removed in #2275, so this should be safe to remove.
@@ -160,7 +160,7 @@ class _ConsoleOutputState extends State<_ConsoleOutput> | |||
@override | |||
void didUpdateWidget(_ConsoleOutput oldWidget) { | |||
if (oldWidget.lines != widget.lines) { | |||
cancel(); | |||
cancelListeners(); |
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.
not your fault, but let's move this to _initHelper to keep it grouped with where the listeners are added
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.
Done!
@@ -180,11 +180,12 @@ class ConsoleService extends Disposer { | |||
} | |||
|
|||
void vmServiceOpened(VmServiceWrapper service) { | |||
cancel(); | |||
cancelStreamSubscriptions(); |
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.
we need to cancel listeners here too since we are also adding an auto dispose listener
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.
Ah good catch! Done
@@ -207,7 +207,7 @@ class MemoryEventsPaneState extends State<MemoryEventsPane> | |||
final themeData = Theme.of(context); | |||
colorScheme = themeData.colorScheme; | |||
|
|||
cancel(); | |||
cancelListeners(); |
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.
again not your fault, but this looks like a bug below that we have a call to addAutoDisposeListener
nested inside a listener for another addAutoDisposeListener
call. Let's move the second call outside of the listener block
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.
SG, removed the nesting
Fixes #3538
Follow up to #3532, this way we can use
autoDisposeFocusNode
in widgets we weren't able to use it in before.Also renames the
autoDispose
method toautoDisposeStreamSubscription
for clarity.