Keep alive support for 2D scrolling #131641

Piinks · 2023-08-01T00:05:49Z

This adds support for keep alive to the 2D scrolling foundation. The TwoDimensionalChildBuilderDelegate and TwoDimensionalChildListDelegate will both add automatic keep alives to their children, matching the convention from SliverChildDelegates. The TwoDimensionalViewportParentData now incorporates keep alive and which is managed by the RenderTwoDimensionalViewport.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
All existing and new tests are passing.

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

chunhtai

I have a more general question. I assume this class will be extended by a first party package?

In that case why isn't the two_dimentional_viewport.dart in that package instead of here?

chunhtai · 2023-08-01T17:02:18Z

packages/flutter/lib/src/widgets/two_dimensional_viewport.dart

      visitor(child);
      child = parentDataOf(child)._nextSibling;
    }
+    _keepAliveBucket.values.forEach(visitor);


I am slightly surprised keptalive children are visited as well in this method. I did see the slivermultiboxadapter also does this, but I am not entirely sure why.

I also am not aware of why. I traced it back to #11010 when keep alive as first added. I think of it being kind of similar to the Offstage widget. The child is kept alive without painting anything, without making the child available for hit testing, and without taking any room in the parent. However, Offstage children are still active: they can receive focus and have keyboard input directed to them.

chunhtai · 2023-08-01T17:31:57Z

packages/flutter/lib/src/widgets/two_dimensional_viewport.dart

+    _keepAliveBucket.values.forEach(visitor);
  }

  @override


it looks like visitChildrenForSemantics only visits children that are visible, how does it work if user trying to visit offscreen item (thus will cause scroll)?

Trying to remember... I think this was because ensureVisible/getOffsetToReveal is not yet implemented for RenderTwoDimensionalViewport. That is in #128812. Otherwise right now it would crash, so I think I remember putting the visible part in place to prevent a crash until it was properly implemented. I almost forgot about it, I added a note to the PR for when I get back to it.

Can you add a todo to here just to make sure we don't forget it once we implemented these methods?

chunhtai · 2023-08-01T18:11:17Z

packages/flutter/lib/src/widgets/two_dimensional_viewport.dart

+    }
+    // If the child in the bucket is not current child, that means someone has
+    // already moved and replaced current child, and we cannot remove this
+    // child.


I think this may only possible if the earlier movechild move another child into this place. In this case this child will be in _debugDanglingKeepAlives. Maybe add an assert check here?

For this and the comment below, this matches almost exactly to the implementation in RenderSliverMultiBoxAdaptor, should the same changes be made there? I traced this back to #31978 for context.

chunhtai · 2023-08-01T18:15:32Z

packages/flutter/lib/src/widgets/two_dimensional_viewport.dart

+      return;
    }
-    assert(_debugTrackOrphans(noLongerOrphan: child));
+    assert(_keepAliveBucket[childParentData.vicinity] == child);


if they are in _debugDanglingKeepAlives they won't be in here, no?

They should be. Every child in _debugDanglingKeepAlives should be in the _keepAliveBucket, but not necessarily the other way around.

you are right, I misread the code

chunhtai · 2023-08-01T18:15:55Z

packages/flutter/lib/src/widgets/two_dimensional_viewport.dart


  void _insertChild(RenderBox child, ChildVicinity slot) {
    assert(_debugTrackOrphans(newOrphan: _children[slot]));
+    assert(!_keepAliveBucket.containsValue(child));


also check _debugDanglingKeepAlives ?

Should it be empty, or not contain the child? We do not have a similar check in RenderSliverMultiBoxAdaptor, should I add one there too? RenderTwoDimensionalViewport is like a cross between RenderSliverMultiBoxAdaptor and Viewport with two offsets, so I have been trying to match behavior as much as possible for consistency. :)

I think they should not contain the child because it should be removed from the _debugDanglingKeepAlives.

Given this is mirroring the 1D case, I am fine either you add both or not.

Can also think about extracting a mixin with generic slot type if things start to get repeated.

chunhtai · 2023-08-01T18:17:39Z

packages/flutter/lib/src/widgets/two_dimensional_viewport.dart

  /// Returns the child for a given [ChildVicinity].
  ///
  /// This method will build the child if it has not been already, or will reuse
  /// it if it already exists.


This method can have more detail doc string. Maybe add some comment around when this method should be call (in layoutChildSequence I think?) and the childvicinity must be what is visible on the screen because this method will insert the child into active children list

The children aren't required to be visible since they could be in the cache extent and are ok as active children there. I will add more docs though.

The comment seems not updated yet?

Ah thank you!

Piinks

Thanks for review! Good questions!

I assume this class will be extended by a first party package?

Yup! The first version is in flutter/packages#4536

In that case why isn't the two_dimensional_viewport.dart in that package instead of here?

A couple of reasons. The main one being that the 2D foundation must be in the framework due to some overrides of private API in Scrollable.
Additionally, there is a lot of scrolling functionality built-in to other widgets in the framework, and we want them all to work the same whether it is 1D or 2D.

ensureVisible is a good example, which supports default focus traversal. We would not want developers to have to call ensureVisible for 1D and then some other method for 2D, on top of the fact that several places in the framework call this and we want it to just work rather than having two different code paths.

Implementing the core functionality of 2D in the framework and providing base classes for developers to build on top of was the general idea. The 1P packages we are building on top of it in flutter/packages may move into the framework later, but we wanted to be able to iterate on them more rapidly from the package repo first.

Piinks · 2023-08-02T19:53:12Z

packages/flutter/lib/src/widgets/two_dimensional_viewport.dart

      visitor(child);
      child = parentDataOf(child)._nextSibling;
    }
+    _keepAliveBucket.values.forEach(visitor);


I also am not aware of why. I traced it back to #11010 when keep alive as first added. I think of it being kind of similar to the Offstage widget. The child is kept alive without painting anything, without making the child available for hit testing, and without taking any room in the parent. However, Offstage children are still active: they can receive focus and have keyboard input directed to them.

Piinks · 2023-08-02T20:20:47Z

packages/flutter/lib/src/widgets/two_dimensional_viewport.dart

+    _keepAliveBucket.values.forEach(visitor);
  }

  @override


Trying to remember... I think this was because ensureVisible/getOffsetToReveal is not yet implemented for RenderTwoDimensionalViewport. That is in #128812. Otherwise right now it would crash, so I think I remember putting the visible part in place to prevent a crash until it was properly implemented. I almost forgot about it, I added a note to the PR for when I get back to it.

Piinks · 2023-08-02T20:22:14Z

packages/flutter/lib/src/widgets/two_dimensional_viewport.dart

  /// Returns the child for a given [ChildVicinity].
  ///
  /// This method will build the child if it has not been already, or will reuse
  /// it if it already exists.


The children aren't required to be visible since they could be in the cache extent and are ok as active children there. I will add more docs though.

Piinks · 2023-08-02T20:25:17Z

packages/flutter/lib/src/widgets/two_dimensional_viewport.dart


  void _insertChild(RenderBox child, ChildVicinity slot) {
    assert(_debugTrackOrphans(newOrphan: _children[slot]));
+    assert(!_keepAliveBucket.containsValue(child));


Should it be empty, or not contain the child? We do not have a similar check in RenderSliverMultiBoxAdaptor, should I add one there too? RenderTwoDimensionalViewport is like a cross between RenderSliverMultiBoxAdaptor and Viewport with two offsets, so I have been trying to match behavior as much as possible for consistency. :)

Piinks · 2023-08-02T20:28:52Z

packages/flutter/lib/src/widgets/two_dimensional_viewport.dart

+    }
+    // If the child in the bucket is not current child, that means someone has
+    // already moved and replaced current child, and we cannot remove this
+    // child.


For this and the comment below, this matches almost exactly to the implementation in RenderSliverMultiBoxAdaptor, should the same changes be made there? I traced this back to #31978 for context.

Piinks · 2023-08-02T20:33:02Z

packages/flutter/lib/src/widgets/two_dimensional_viewport.dart

+      return;
    }
-    assert(_debugTrackOrphans(noLongerOrphan: child));
+    assert(_keepAliveBucket[childParentData.vicinity] == child);


They should be. Every child in _debugDanglingKeepAlives should be in the _keepAliveBucket, but not necessarily the other way around.

chunhtai

LGTM, just left some minor comments

chunhtai · 2023-08-09T16:56:50Z

packages/flutter/lib/src/widgets/two_dimensional_viewport.dart

+    _keepAliveBucket.values.forEach(visitor);
  }

  @override


Can you add a todo to here just to make sure we don't forget it once we implemented these methods?

chunhtai · 2023-08-09T16:59:12Z

packages/flutter/lib/src/widgets/two_dimensional_viewport.dart

  /// Returns the child for a given [ChildVicinity].
  ///
  /// This method will build the child if it has not been already, or will reuse
  /// it if it already exists.


The comment seems not updated yet?

chunhtai · 2023-08-09T17:00:11Z

packages/flutter/lib/src/widgets/two_dimensional_viewport.dart

        _childManager._buildChild(vicinity);
      });
    } else {
+      if (_keepAliveBucket.containsKey(vicinity)) {


is this check needed? I think you can just remove even if it is not in the map?

Oh right. Good call.

chunhtai · 2023-08-09T17:04:37Z

packages/flutter/lib/src/widgets/two_dimensional_viewport.dart


  void _insertChild(RenderBox child, ChildVicinity slot) {
    assert(_debugTrackOrphans(newOrphan: _children[slot]));
+    assert(!_keepAliveBucket.containsValue(child));


I think they should not contain the child because it should be removed from the _debugDanglingKeepAlives.

Given this is mirroring the 1D case, I am fine either you add both or not.

Can also think about extracting a mixin with generic slot type if things start to get repeated.

chunhtai · 2023-08-09T17:08:27Z

packages/flutter/lib/src/widgets/two_dimensional_viewport.dart

+      return;
    }
-    assert(_debugTrackOrphans(noLongerOrphan: child));
+    assert(_keepAliveBucket[childParentData.vicinity] == child);


you are right, I misread the code

auto-submit · 2023-08-09T22:38:37Z

auto label is removed for flutter/flutter/131641, due to - The status or check suite Linux web_tests_7_last has failed. Please fix the issues identified (or deflake) before re-applying this label.

Piinks · 2023-08-09T22:41:26Z

Oh whoops I broke something

Piinks · 2023-08-09T22:45:31Z

Yay for tests. 😍

Kee alive support for 2D scrolling

763b6ac

Piinks added c: new feature Nothing broken; request for a new capability framework flutter/packages/flutter repository. See also f: labels. f: scrolling Viewports, list views, slivers, etc. labels Aug 1, 2023

Piinks requested a review from chunhtai August 1, 2023 00:05

chunhtai reviewed Aug 1, 2023

View reviewed changes

Piinks commented Aug 2, 2023

View reviewed changes

Piinks requested a review from chunhtai August 8, 2023 19:02

chunhtai approved these changes Aug 9, 2023

View reviewed changes

Piinks added 2 commits August 9, 2023 17:18

Merge branch 'master' of github.com:flutter/flutter into keepAlives2D

103d9aa

Review feedbakc

3693129

Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 9, 2023

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 9, 2023

Fix null check

5484c68

Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 9, 2023

auto-submit bot merged commit 0c80ed6 into flutter:master Aug 10, 2023

engine-flutter-autoroll mentioned this pull request Aug 10, 2023

Roll Flutter from f4c25bbb351c to 7c59dfebb957 (22 revisions) flutter/packages#4669

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 10, 2023

0c80ed6 Keep alive support for 2D scrolling (flutter/flutter#131641)

0eb930d

engine-flutter-autoroll mentioned this pull request Aug 10, 2023

Roll Flutter from f4c25bbb351c to c428d9154859 (23 revisions) flutter/packages#4670

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 10, 2023

0c80ed6 Keep alive support for 2D scrolling (flutter/flutter#131641)

67778db

engine-flutter-autoroll mentioned this pull request Aug 10, 2023

Roll Flutter from f4c25bbb351c to 96e02c61dcc3 (24 revisions) flutter/packages#4671

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 10, 2023

0c80ed6 Keep alive support for 2D scrolling (flutter/flutter#131641)

64aad7a

engine-flutter-autoroll mentioned this pull request Aug 10, 2023

Roll Flutter from f4c25bbb351c to 52a6de255871 (26 revisions) flutter/packages#4672

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 10, 2023

0c80ed6 Keep alive support for 2D scrolling (flutter/flutter#131641)

09cf9e4

engine-flutter-autoroll mentioned this pull request Aug 10, 2023

Roll Flutter from f4c25bbb351c to e11cc3507608 (27 revisions) flutter/packages#4673

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 10, 2023

0c80ed6 Keep alive support for 2D scrolling (flutter/flutter#131641)

2d485a1

engine-flutter-autoroll mentioned this pull request Aug 10, 2023

Roll Flutter from f4c25bbb351c to 64a0683b4197 (28 revisions) flutter/packages#4674

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 10, 2023

0c80ed6 Keep alive support for 2D scrolling (flutter/flutter#131641)

7da9e58

engine-flutter-autoroll mentioned this pull request Aug 10, 2023

Roll Flutter from f4c25bbb351c to a6118612ac4d (29 revisions) flutter/packages#4675

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 10, 2023

0c80ed6 Keep alive support for 2D scrolling (flutter/flutter#131641)

333e1b7

engine-flutter-autoroll mentioned this pull request Aug 10, 2023

Roll Flutter from f4c25bbb351c to 685141bf3b5f (30 revisions) flutter/packages#4676

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 10, 2023

0c80ed6 Keep alive support for 2D scrolling (flutter/flutter#131641)

271a464

HansMuller mentioned this pull request Aug 11, 2023

Flutter Framework Team: Notable Commits #121415

Closed

This was referenced Aug 28, 2023

KeepAlive.debugTypicalAncestorWidgetClass needs update plus docs #133497

Closed

Update KeepAlive.debugTypicalAncestorWidgetClass #133498

Merged

Expose addAutomaticKeepAlives in TableView child delegates #133583

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 15, 2023

0c80ed6 Keep alive support for 2D scrolling (flutter/flutter#131641)

2dd076c

Piinks deleted the keepAlives2D branch March 14, 2025 17:13

Keep alive support for 2D scrolling #131641

Keep alive support for 2D scrolling #131641

Uh oh!

Conversation

Piinks commented Aug 1, 2023

Pre-launch Checklist

Uh oh!

chunhtai left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

auto-submit bot commented Aug 9, 2023

Uh oh!

Piinks commented Aug 9, 2023

Uh oh!

Piinks commented Aug 9, 2023

chunhtai left a comment •

edited

Loading