Skip to content
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 TwoDimensionalViewport's keep alive child not always removed (when no longer should be kept alive) #148298

Conversation

gawi151
Copy link
Contributor

@gawi151 gawi151 commented May 14, 2024

  • Fixes a child not removed from _keepAliveBucket when widget is no longer kept alive offscreen. Bug was triggering assert in performLayout.
  • Adds test to cover the case from bug report

Fixes #138977

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: scrolling Viewports, list views, slivers, etc. labels May 14, 2024
@gawi151
Copy link
Contributor Author

gawi151 commented May 14, 2024

Hi, @Piinks Would you be so kind to review the fix?

@gawi151 gawi151 changed the title Fix two dimensional keep alive child not always removed Fix two dimensional's viewport keep alive child not always removed May 14, 2024
@gawi151 gawi151 changed the title Fix two dimensional's viewport keep alive child not always removed Fix TwoDimensionalViewport's keep alive child not always removed (when no longer should be kept alive) May 14, 2024
@goderbauer goderbauer requested a review from Piinks May 14, 2024 22:09
Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with some nits, thank you for sending a PR!

bool get wantKeepAlive => _hovered;

@override
Widget build(BuildContext context) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super clever!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx ;)

gawi151 and others added 7 commits May 21, 2024 16:53
Signed-off-by: Lukasz Gawron <gawronlucas@gmail.com>
Signed-off-by: Lukasz Gawron <gawronlucas@gmail.com>
Signed-off-by: Lukasz Gawron <gawronlucas@gmail.com>
Signed-off-by: Lukasz Gawron <gawronlucas@gmail.com>
@Piinks Piinks force-pushed the fix/#138977-two-dimensional-keep-alive-child-not-always-removed branch from c997e8a to 807923d Compare May 21, 2024 21:53
@gawi151
Copy link
Contributor Author

gawi151 commented May 22, 2024

@Piinks Thx for fixing wording for me ;)

@gawi151
Copy link
Contributor Author

gawi151 commented May 23, 2024

@Piinks, could you please assist with adding the auto-submit label to this PR? We would greatly appreciate your help in getting it merged promptly. It's crucial for us to have this included in the next bugfix release, as we are currently unable to upgrade beyond Flutter 3.13. Thank you for your support!

@Piinks
Copy link
Contributor

Piinks commented May 23, 2024

This PR still needs one more review before it can be merged, I am working on getting that now.

Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

await tester.pump();
expect(find.byKey(checkBoxKey), findsNothing);

// Bring back into view
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Period here at the end.

@bleroux bleroux added the autosubmit Merge PR when tree becomes green via auto submit App label May 24, 2024
@auto-submit auto-submit bot merged commit a53b78d into flutter:master May 24, 2024
73 checks passed
@gawi151 gawi151 deleted the fix/#138977-two-dimensional-keep-alive-child-not-always-removed branch May 25, 2024 14:14
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 25, 2024
auto-submit bot pushed a commit to flutter/packages that referenced this pull request May 25, 2024
flutter/flutter@8dd0831...cb26a01

2024-05-25 engine-flutter-autoroll@skia.org Roll Flutter Engine from e0207131ea13 to 1d154ff93a87 (1 revision) (flutter/flutter#149070)
2024-05-25 engine-flutter-autoroll@skia.org Roll Flutter Engine from f693fdd840e8 to e0207131ea13 (7 revisions) (flutter/flutter#149068)
2024-05-25 engine-flutter-autoroll@skia.org Roll Flutter Engine from b5e98f9b8d0f to f693fdd840e8 (4 revisions) (flutter/flutter#149057)
2024-05-24 wyattliang@gmail.com fix popup menu offset when using `useRootNavigator` in `PopupMenu` (flutter/flutter#144670)
2024-05-24 jhy03261997@gmail.com [GAR][a11y] Update assessment app to unblock tester from testing dark mode (flutter/flutter#149055)
2024-05-24 jhy03261997@gmail.com [a11y] Slider should respect bold text setting (flutter/flutter#149053)
2024-05-24 sokolovskyi.konstantin@gmail.com Add tests for editable_text.on_changed.0.dart API example. (flutter/flutter#148874)
2024-05-24 engine-flutter-autoroll@skia.org Roll Flutter Engine from 979214444aca to b5e98f9b8d0f (1 revision) (flutter/flutter#149051)
2024-05-24 sokolovskyi.konstantin@gmail.com Add test for text_editing_controller.0.dart API example. (flutter/flutter#148872)
2024-05-24 engine-flutter-autoroll@skia.org Roll Flutter Engine from 4ab442475223 to 979214444aca (3 revisions) (flutter/flutter#149045)
2024-05-24 gawronlucas@gmail.com Fix TwoDimensionalViewport's keep alive child not always removed (when no longer should be kept alive) (flutter/flutter#148298)
2024-05-24 engine-flutter-autoroll@skia.org Roll Packages from 1008d9e to 7b423f5 (2 revisions) (flutter/flutter#149043)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC rmistry@google.com,stuartmorgan@google.com,tarrinneal@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TableView: Hovering over an InkWell while scrolling fast causes exceptions.
4 participants