-
Notifications
You must be signed in to change notification settings - Fork 27.2k
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
Fix a problem with disposing of focus nodes in tab scaffold #40100
Conversation
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
); | ||
if (tabFocusNodes.length != widget.tabCount) { | ||
if (tabFocusNodes.length > widget.tabCount) { | ||
discardedNodes.addAll(tabFocusNodes.sublist(widget.tabCount, tabFocusNodes.length)); |
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.
I think this is the same as:
discardedNodes.addAll(tabFocusNodes.sublist(widget.tabCount));
Although, what you've written is probably clearer :-).
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.
Yes, you're right. I'll switch it to yours, since it's more succinct (and probably more canonical).
tabFocusNodes.addAll( | ||
List<FocusScopeNode>.generate( | ||
widget.tabCount - tabFocusNodes.length, | ||
(int index) => FocusScopeNode(debugLabel: '${describeIdentity(widget)} Tab ${index + tabFocusNodes.length}'), |
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.
two space indent
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.
List<FocusScopeNode> tabFocusNodes; | ||
final List<bool> shouldBuildTab = <bool>[]; | ||
final List<FocusScopeNode> tabFocusNodes = <FocusScopeNode>[]; | ||
final List<FocusScopeNode> discardedNodes = <FocusScopeNode>[]; |
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.
It seems odd that we don't eagerly dispose discarded nodes. Maybe explain why we hang on to them until the enclosing widget is disposed, 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.
Done.
ab4b8e7
to
cfb9778
Compare
cfb9778
to
71531b9
Compare
…40100) Previously, focus nodes created in the tab scaffold were not being disposed of properly, causing possible memory leaks. Also, the builder wasn't being passed the right context so that the FocusScope.of operator inside of a builder would find the focus scope for the given tab (it was being passed the context of the overall tab scaffold).
Description
Previously, focus nodes created in the tab scaffold were not being disposed of properly, causing possible memory leaks. Also, the builder wasn't being passed the right context so that the FocusScope.of operator inside of a builder would find the focus scope for the given tab (it was being passed the context of the overall tab scaffold).
Tests
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]
). This will ensure a smooth and quick review process.///
).flutter analyze --flutter-repo
) does not report any problems on my PR.Breaking Change
Does your PR require Flutter developers to manually update their apps to accommodate your change?