[widgets/raw_menu_anchor.dart] Remove listener from isScrollingNotifier on dispose.#187881
Conversation
github-actions
Bot
added
the
framework
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request ensures that the ancestor scroll listener is removed and the scroll position is cleared when the root menu anchor is disposed, and adds a corresponding regression test. The reviewer suggests simplifying the test by directly checking hasListeners on the default isScrollingNotifier using // ignore: invalid_use_of_protected_member, which allows removing the custom TrackingNotifier, SpyScrollPosition, and SpyScrollController helper classes.
| class TrackingNotifier extends ValueNotifier<bool> { | ||
| TrackingNotifier(super.value); | ||
| int listenerCount = 0; | ||
|
|
||
| @override | ||
| void addListener(VoidCallback listener) { | ||
| listenerCount++; | ||
| super.addListener(listener); | ||
| } | ||
|
|
||
| @override | ||
| void removeListener(VoidCallback listener) { | ||
| listenerCount--; | ||
| super.removeListener(listener); | ||
| } | ||
| } | ||
|
|
||
| class SpyScrollPosition extends ScrollPositionWithSingleContext { | ||
| SpyScrollPosition({required super.physics, required super.context, super.oldPosition}); | ||
|
|
||
| final TrackingNotifier _isScrollingNotifier = TrackingNotifier(false); | ||
|
|
||
| @override | ||
| ValueNotifier<bool> get isScrollingNotifier => _isScrollingNotifier; | ||
| } | ||
|
|
||
| class SpyScrollController extends ScrollController { | ||
| @override | ||
| ScrollPosition createScrollPosition( | ||
| ScrollPhysics physics, | ||
| ScrollContext context, | ||
| ScrollPosition? oldPosition, | ||
| ) { | ||
| return SpyScrollPosition(physics: physics, context: context, oldPosition: oldPosition); | ||
| } | ||
| } |
There was a problem hiding this comment.
These helper classes (TrackingNotifier, SpyScrollPosition, and SpyScrollController) are no longer needed if we use // ignore: invalid_use_of_protected_member to check hasListeners directly in the test. We can safely remove them.
References
- Suggest simplification and refactoring to enhance readability and maintainability. (link)
navaronbracke
left a comment
navaronbracke
left a comment
There was a problem hiding this comment.
LGTM with a nit about the test
|
Looks like Linux analyze is unhappy, though |
|
@navaronbracke My bad! I forgot to remove the dead code. Should be ready. |
dkwingsmt
left a comment
dkwingsmt
left a comment
There was a problem hiding this comment.
LGTM. Thanks for the quick investigation!
dkwingsmt
added
the
autosubmit
auto-submit
Bot
removed
the
autosubmit
|
autosubmit label was removed for flutter/flutter/187881, because The base commit of the PR is older than 7 days and can not be merged. Please merge the latest changes from the main into this branch and resubmit the PR. |
4 participants
This removes the isScrollingNotifier listener from RawMenuAnchor in the dispose block. This led to a memory leak, as reported by one user. This bug appears to have been present since the original MenuAnchor PR, so I'm surprised it hasn't caused more issues.
Fixes #187853
The test required a bit of gymnastics since hasListeners is protected. Let me know if it would be better to add a comment ignoring the protected status.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.