Allow remove listener on disposed change notifier #97988
Conversation
flutter-dashboard
bot
added
the
framework
| class ChangeNotifier implements Listenable { | ||
| int _count = 0; | ||
| List<VoidCallback?> _listeners = List<VoidCallback?>.filled(0, null); | ||
| late List<VoidCallback?> _listeners; |
There was a problem hiding this comment.
Why move this to late?
For some background: This was carefully designed with performance in mind and I suspect that late will make this slightly slower. (Unless we have a benchmark to prove otherwise...)
| @override | ||
| void removeListener(VoidCallback listener) { | ||
| assert(_debugAssertNotDisposed()); | ||
| if (_debugDisposed) |
There was a problem hiding this comment.
_debug variables should only be sued in debug contexts (i.e. other debug methods or asserts).
There was a problem hiding this comment.
good catch i totally forgot this
| _debugDisposed = true; | ||
| return true; | ||
| }()); | ||
| _listeners = const <VoidCallback?>[]; |
There was a problem hiding this comment.
Let's follow the performance-optimized pattern suggested in https://github.com/flutter/flutter/pull/71947/files#r545722476 here.
There was a problem hiding this comment.
sounds good, I will also leave a comment as the original pr suggested..
| _debugDisposed = true; | ||
| return true; | ||
| }()); | ||
| _listeners = const <VoidCallback?>[]; |
There was a problem hiding this comment.
Set _count to 0 as well and then the for loop in removeListener would just immediately terminate without the need for the _debugDisposed guard?
|
a friendly bump |
| List<VoidCallback?> _listeners = List<VoidCallback?>.filled(0, null); | ||
| // The _listeners is intentionally set to a fixed-length _GrowableList instead | ||
| // of const [] for performance reasons. | ||
| // See https://github.com/flutter/flutter/pull/71947/files#r545722476 for |
There was a problem hiding this comment.
can we inline the explanation here rather than linking to github? the code will probably outlive that link.
4 participants
The current logic causes problem when there are overlay involve. The owner of an overlay entry is usually a State of a statefulwidget. The problem occur when the overlay entry usually out-lived the owner for one frame. so it is very hard to figure out the clean up order if there are change notifiers involved. This PR allows removeListen calls to a disposed change notifier.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.