[two_dimensional_scrollables] Activate leak testing and fix memory leaks#11653
[two_dimensional_scrollables] Activate leak testing and fix memory leaks#11653ValentinVignal wants to merge 7 commits intoflutter:mainfrom
Conversation
|
cc @polina-c |
github-actions
Bot
added
p: two_dimensional_scrollables
| static Widget builder({ | ||
| Key? key, | ||
| bool? primary, |
There was a problem hiding this comment.
I'm not sure whether that is okay to change from a constructor to a static method. Would you have a better design in mind?
| 'merged column that exceeds metrics will assert', | ||
| // The build throws an assertion error which prevents the table from | ||
| // properly disposing the elements. | ||
| experimentalLeakTesting: LeakTesting.settings.withIgnoredAll(), |
There was a problem hiding this comment.
Is it acceptable to have memory leaks when the build asserts?
![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 updates the two_dimensional_scrollables package to version 0.5.2, focusing on resolving memory leaks by refactoring TableView and TreeView to properly manage the lifecycle of their delegates and animations. It introduces leak_tracker_flutter_testing to the test suite and updates existing tests to ensure proper resource disposal. Review feedback identified several critical issues: missing parameter propagation in the new _TableViewBuilder and _TableViewList widgets, a missing didUpdateWidget implementation in _TableViewListState, and concerns regarding the breaking nature of converting constructors to static methods.
| super.dragStartBehavior, | ||
| super.keyboardDismissBehavior, | ||
| super.clipBehavior, | ||
| static Widget builder({ |
There was a problem hiding this comment.
Changing TableView.builder from a constructor to a static method that returns a Widget is a breaking change. This changes the return type from TableView to Widget, which may break existing code that relies on the specific type or uses it in a context where a TableView instance is expected. Since this is a patch version bump (0.5.1 to 0.5.2), breaking changes should generally be avoided. Consider if a minor version bump is more appropriate or if there is a non-breaking way to manage the delegate's lifecycle.
| ), | ||
| ) | ||
| : TableView.builder( | ||
| key: ValueKey(_selectionMode), |
There was a problem hiding this comment.
I'm doing this to force the widget to rebuild (and re-run _buildCell). Because now, when we toggle from "Single-cell" to "Disable" selection, the delegate doesn't change, and _buildCell is not rebuilt. That makes the test Selection SegmentedButton control works in packages/two_dimensional_scrollables/example/test/table_view/simple_table_test.dart:60 fail.
Ideally, I think we'd need some kind of notifier that the widget built in _buildCell would listen to. But since it is only an example, I'd thought this would be enough?
What do you think?
1 participant
The package manipulates disposable objects. This PR activates leak testing to make sure disposable objects are correctly disposed.
It also fixes the memory leak warnings from the tests
See the documentation: https://github.com/dart-lang/leak_tracker/blob/main/doc%2Fleak_tracking%2FDETECT.md
Pre-Review Checklist
[shared_preferences]///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
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.Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2