Feature: Add AnimatedList with separators #144899

Peetee06 · 2024-03-10T14:30:49Z

This PR adds AnimatedList.separated. A widget like an AnimatedList with animated separators. animated_list_separated.0.dart extends animated_list.0.dart to work with AnimatedList.separated

Related issue: #48226

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.
I followed the breaking change policy and added Data Driven Fixes where supported.
All existing and new tests are passing.

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

Peetee06 · 2024-03-10T14:36:30Z

I'm unsure about how to best handle removeItem und removeAllItems. If we want to reuse _AnimatedScrollViewState, we need to prevent users from using those, because they don't come with a parameter for building removed separators. Throwing an exception was the best I can could come up with.

Is there a better way to do this? Would it be preferred to re-implement _AnimatedScrollViewState with separators?

Hixie · 2024-03-24T18:37:20Z

examples/api/lib/widgets/animated_list/animated_list_separated.0.dart

+  // Build a separator for items that have been removed from the list.
+  /// The widget will be used by the [AnimatedListSeparatedState.removeSeparatedItem] method's
+  /// `separatorBuilder` parameter.
+  Widget _buildRemovedSeparator(int item , BuildContext context, Animation<double> animation) => SizeTransition(


trivial nit: remove the extra space before the comma

Hixie · 2024-03-24T18:38:33Z

examples/api/lib/widgets/animated_list/animated_list_separated.0.dart

+  // Insert the "next item" into the list model.
+  void _insert() {
+    final int index = _selectedItem == null ? _list.length : _list.indexOf(_selectedItem!);
+    _list.insert(index, _nextItem++);


to make this code clearer to new programmers, i recommend increasing _nextItem separate from inserting it into the list, so it's 100% clear whether the inserted value is the pre-increment or post-increment value.

Hixie · 2024-03-24T18:39:07Z

examples/api/lib/widgets/animated_list/animated_list_separated.0.dart

+            ),
+          ),


nit: indentation

Hixie · 2024-03-24T18:42:34Z

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

+          ErrorSummary('AnimatedListSeparated.of() called with a context that does not contain an AnimatedListSeparated.'),
+          ErrorDescription(
+            'No AnimatedListSeparated ancestor could be found starting'
+            ' from the context that was passed to AnimatedListSeparated.of().',


nit: we prefer having the space on the end of the line before the cut

(if this is copy/pasted from some other code, bonus points for fixing that code too)

Hixie · 2024-03-24T18:44:53Z

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

+  /// This method is not supported by [AnimatedListSeparatedState]. Use [removeSeparatedItem] instead.
+  @override
+  void removeItem(int index, AnimatedRemovedItemBuilder builder, { Duration duration = _kDuration }) {
+    throw Exception('Separated list items can not be removed with this method. Use removeSeparatedItem instead.');
+  }
+
+  /// This method is not supported by [AnimatedListSeparatedState]. Use [removeAllSeparatedItems] instead.
+  @override
+  void removeAllItems(AnimatedRemovedItemBuilder builder, { Duration duration = _kDuration }) {
+    throw Exception('Separated list items can not be removed with this method. Use removeAllSeparatedItems instead.');
+  }


I think this would lead to confusion. Can we instead find some way to let removeItem be how you remove items, for consistency across the various APIs?

e.g. have the superclass have an @protected internalRemoveItem or some such, and then have the subclasses other than this one introduce a removeItem that just forwards everything to internalRemoveItem?

Hixie · 2024-03-24T18:47:02Z

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

+  int _computeItemIndex(int index) {
+    if (index == 0) {
+      return index;
+    } else {


avoid else after return

Hixie · 2024-03-24T18:53:25Z

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

+///
+/// [AnimatedListSeparated] item input handlers can also refer to their [AnimatedListSeparatedState]
+/// with the static [AnimatedListSeparated.of] method.
+class AnimatedListSeparatedState extends _AnimatedScrollViewState<AnimatedListSeparated> {


@goderbauer / @Piinks
We probably shouldn't do it in this PR but the fact that we couldn't implement AnimatedListSeparatedState outside of the framework because _AnimatedScrollViewState is private probably means we have some magic here and should consider making _AnimatedScrollViewState public.

Piinks

Hi @Peetee06 welcome! Thanks very much for contributing.

Rather than a new class, we would typically approach this with a special constructor in the existing AnimatedList class. The ListView widget is a good example of this, with its ListView.separated constructor. There is a fair bit of duplicated code between the AnimatedListSeparated here and the AnimatedList that already exists in the framework, which would mean we need to maintain both, ensuring when we fix a bug in one class, we remember to go fix it as well in the other class, and so on.

Would you be interested in implementing this feature in the existing class?

Peetee06 · 2024-03-31T06:38:13Z

Hi @Piinks,

that was actually the first thing I tried, but I wasn't able to find a solution for the removeItem method. It requires a single removal builder for the AnimatedList but two removal builders for the AnimatedList with separators, because we need to remove the separators as well. I could implement a second method like I did already but I agree with @Hixie here, that having two removal methods, where one is unused for each AnimatedList might lead to confusion. Do you have an idea on how to implement it as AnimatedList.separated while not having methods that are only used in either AnimatedList xor AnimatedList.separated?

I like @Hixie's suggestion about replacing the superclass's removeItem with a protected member to be able to implement both a removeItem with a single removal builder for the default AnimatedList and one with two removal builders for the AnimatedList.separated if we would choose to stay with a separated AnimatedListSeparated class. What do you think about this refactoring?

I would be happy to improve the code so that it has a clean, developer-friendly API.

Piinks · 2024-04-03T22:19:53Z

that was actually the first thing I tried, but I wasn't able to find a solution for the removeItem method. It requires a single removal builder for the AnimatedList but two removal builders for the AnimatedList with separators, because we need to remove the separators as well. I could implement a second method like I did already but I agree with @Hixie #144899 (comment), that having two removal methods, where one is unused for each AnimatedList might lead to confusion. Do you have an idea on how to implement it as AnimatedList.separated while not having methods that are only used in either AnimatedList xor AnimatedList.separated?

Would the removal of the separators just be expected by the user without having to call it themselves? I feel like that's what I would expect. In that case, we could just internally remove the leftover separators when items are removed. Maybe the constructor can have the user provide the add and remove builders just for the separators then?

Piinks · 2024-04-03T22:20:47Z

What do you think about this refactoring?

I think these are great avenues to explore, thanks for working on this! :)

Piinks · 2024-05-01T22:53:50Z

@Peetee06 is this ready for another review?

Peetee06 · 2024-05-02T03:22:04Z

@Piinks yes! I wrote a few infos about it and two different branches with other possible solutions in the "hackers-new" channel on discord. Here is a summary:

All in all, we now have 3 possible solutions:

Use @Protected internal methods for _AnimatedScrollViewState and pass a removedSeparatorBuilder to removeItem and removeAllItems: #144899
Pass removedSeparatorBuilder to the constructor of AnimatedListSeparated. No need to make _AnimatedScrollViewState's methods protected, because they can just be overriden now: https://github.com/Peetee06/flutter/tree/feat/add_AnimatedListSeparated_with_separator_removal_builder
Same as 2, but adding the removedSeparatorBuilder to _AnimatedScrollView and adding conditional logic to _AnimatedScrollViewState's methods to handle the case of having separators or not: https://github.com/Peetee06/flutter/tree/feature/add_AnimatedList_separated

From a user's perspective, I like solution 3 the most, because the API seems the cleanest and is familiar in regards to ListView.separated.

Looking forward to hearing your opinion, or ideas on how to improve on these approaches.

Piinks

Wow thank you so much for digging into this so thoroughly! Sorry it took me a while to review all of the options you presented, with sample code too, so grateful you included that. :)

I really like option 3 too, consistency across APIs is really wonderful when we can achieve it. Shall we go with 3 then?

This is a really fantastic contribution for a first PR, thank you again.

Peetee06 · 2024-05-08T06:33:28Z

Sounds good! I will update this PR to use option 3 tomorrow. :)

Peetee06 · 2024-05-09T08:47:07Z

@Hixie @Piinks
The PR is now ready for review.

There is a limitation regarding removedSeparatorBuilder that is observable in the example in animated_list_separated.0.dart. As far as I can tell, there is no way to use the removed item for building the removed separator. The ListModel.removeAt method removes the item from the list, while storing it in a variable. It then uses that item in the removedItemBuilder. Because we are passing the removedSeparatorBuilder to the AnimatedList.separated constructor instead of removeItem, we don't have access to the item when the removedItemBuilder is called.

Let me know if we should consider more options, or should just go with this solution for now.

Looking forward to your reviews! :)

Piinks

Thank you for the updates, and for your patience!

examples/api/lib/widgets/animated_list/animated_list_separated.0.dart

Piinks · 2024-05-21T21:02:13Z

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

+  /// [MediaQuery]'s padding. To avoid this behavior, override with a
+  /// zero [padding] property.


Should 'zero' here be [EdgeInsets.zero]?

Not sure, I copy pasted this from [AnimatedList]'s docstring :)
Maybe @HansMuller or @TahaTesser can give more insights?

Hmm. We should probably know what these docs mean before we commit this.

Depending on the use case one might want to remove only the padding for a specific side. So IMO "override with a zero [padding] property." is more fitting than "override with EdgeInsets.zero [padding] property."

What do you think?

SGTM! Thanks for verifying!

Piinks · 2024-05-21T21:02:49Z

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

+  ///
+  /// {@tool snippet}
+  /// The following example demonstrates how to override the default top and
+  /// bottom padding using [MediaQuery.removePadding].


Is this another way, other than zero padding?

see answer regarding 'zero'

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

Piinks

Looks like it's just a few docs nits remaining here, very nearly ready to land! :)

Piinks · 2024-05-31T15:36:18Z

examples/api/lib/widgets/animated_list/animated_list_separated.0.dart

+    return SizeTransition(
+                sizeFactor: animation,
+                child: ItemSeparator(
+                  animation: animation,
+                  item: null,
+                )
+              );


The indentation here is off, can you fix it?

Piinks · 2024-05-31T15:37:10Z

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

+  /// [MediaQuery]'s padding. To avoid this behavior, override with a
+  /// zero [padding] property.


Hmm. We should probably know what these docs mean before we commit this.

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

Piinks

Thanks for all of your hard work on this! 🎉

…mBuilder docstring

…ItemBuilder

Co-authored-by: Kate Lovett <katelovett@google.com>

Co-authored-by: Nate <nathan.wilson1232@gmail.com>

auto-submit · 2024-06-05T05:23:24Z

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

Peetee06 · 2024-06-05T06:42:41Z

A build check fails with

Execution failed for task ':app:packageDebug'.
> A failure occurred while executing com.android.build.gradle.tasks.PackageAndroidArtifact$IncrementalSplitterRunnable
> java.lang.OutOfMemoryError (no error message)

Is that related to the broken tree? Can I do anything to fix this? OutOfMemoryError to me looks unrelated to the changes.

…4899)

nate-thegrate · 2024-06-05T15:52:57Z

A build check fails with
Execution failed for task ':app:packageDebug'.
> A failure occurred while executing com.android.build.gradle.tasks.PackageAndroidArtifact$IncrementalSplitterRunnable
> java.lang.OutOfMemoryError (no error message)
Is that related to the broken tree? Can I do anything to fix this? OutOfMemoryError to me looks unrelated to the changes.

I believe that error was a flake—we just needed to re-run the test :)

When that happens in the future, feel free to click the "Rebase branch" button, or you can wait for somebody with commit access to manually reboot the failing tests.

…4899)

flutter/flutter@c246ecd...27e0656 2024-06-05 98614782+auto-submit[bot]@users.noreply.github.com Reverts "TreeSliver & associated classes (#147171)" (flutter/flutter#149754) 2024-06-05 p.trost93@gmail.com Feature: Add AnimatedList with separators (flutter/flutter#144899) 2024-06-05 yjbanov@google.com make output of flutter run web tests verbose (flutter/flutter#149694) 2024-06-05 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Request focus if `SemanticsAction.focus` is sent to a focusable widget (#142942)" (flutter/flutter#149741) 2024-06-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from e88469090fed to 11a32d43e3f6 (2 revisions) (flutter/flutter#149699) 2024-06-05 gspencergoog@users.noreply.github.com Request focus if `SemanticsAction.focus` is sent to a focusable widget (flutter/flutter#142942) 2024-06-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from 3dd40156afb6 to e88469090fed (2 revisions) (flutter/flutter#149695) 2024-06-04 katelovett@google.com TreeSliver & associated classes (flutter/flutter#147171) 2024-06-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from fe00f023666c to 3dd40156afb6 (3 revisions) (flutter/flutter#149692) 2024-06-04 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.25.7 to 3.25.8 (flutter/flutter#149691) 2024-06-04 47866232+chunhtai@users.noreply.github.com Prepares semantics_update_test for upcoming heading level changes (flutter/flutter#149671) 2024-06-04 34871572+gmackall@users.noreply.github.com Identify and re-throw our dependency checking errors in `flutter.groovy` (flutter/flutter#149609) 2024-06-04 hans.muller@gmail.com Scrollbar thumb drag gestures now produce one start and one end scroll notification (flutter/flutter#146654) 2024-06-04 15619084+vashworth@users.noreply.github.com Disable sandboxing for macOS apps and tests in CI (flutter/flutter#149618) 2024-06-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from e211c43f3dc1 to fe00f023666c (3 revisions) (flutter/flutter#149680) 2024-06-04 gspencergoog@users.noreply.github.com Allow `find.byTooltip` to use a RegEx (flutter/flutter#149348) 2024-06-04 engine-flutter-autoroll@skia.org Roll Flutter Engine from a6aa5d826649 to e211c43f3dc1 (8 revisions) (flutter/flutter#149658) 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 dit@google.com,rmistry@google.com,stuartmorgan@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

…4899)

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: scrolling Viewports, list views, slivers, etc. d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos labels Mar 10, 2024

Piinks self-requested a review March 15, 2024 19:20

Peetee06 marked this pull request as ready for review March 24, 2024 12:58

Hixie reviewed Mar 24, 2024

View reviewed changes

Piinks reviewed Mar 27, 2024

View reviewed changes

Piinks reviewed May 7, 2024

View reviewed changes

goderbauer requested a review from Piinks May 14, 2024 22:04

Piinks reviewed May 21, 2024

View reviewed changes

Peetee06 force-pushed the feature/add_AnimatedListSeparated branch 2 times, most recently from ae398f4 to be85aee Compare May 26, 2024 04:37

Piinks reviewed May 31, 2024

View reviewed changes

Peetee06 force-pushed the feature/add_AnimatedListSeparated branch from 62d9f7a to a040676 Compare May 31, 2024 17:10

Piinks approved these changes Jun 4, 2024

View reviewed changes

Peetee06 and others added 14 commits June 5, 2024 06:55

ref: Improve readability and update doc strings

1a19896

fix: AnimatedList.separated sample

d76a192

fix: Remove taboo word from docstring

365f2d1

fix: Syntax error in snippet

cd7f4b3

test: Update test for AnimatedListSeparatedSample

bcf214a

ref: Use block body

1c5b3c2

ref: Add descriptive comment

4a6f920

ref: Add descriptive comment to removeItem

504b361

ref: Remove AnimatedSeparatorBuilder signature and update AnimatedIte…

72451c0

…mBuilder docstring

ref: Remove and replace AnimatedRemovedSeparatorBuilder with Animated…

32ada08

…ItemBuilder

fix: docstring references

e16b618

ref: Adjust indentation

2d5f16c

Add new line

dd243b2

Co-authored-by: Kate Lovett <katelovett@google.com>

Update packages/flutter/lib/src/widgets/animated_scroll_view.dart

03d0fcc

Co-authored-by: Nate <nathan.wilson1232@gmail.com>

Peetee06 force-pushed the feature/add_AnimatedListSeparated branch from 41226da to 03d0fcc Compare June 5, 2024 04:55

nate-thegrate added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 5, 2024

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jun 5, 2024

nate-thegrate added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 5, 2024

auto-submit bot merged commit 39472d9 into flutter:master Jun 5, 2024
73 checks passed

engine-flutter-autoroll mentioned this pull request Jun 5, 2024

Roll Flutter from c246ecdf8e8d to 39472d9b6193 (16 revisions) flutter/packages#6873

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 5, 2024

39472d9 Feature: Add AnimatedList with separators (flutter/flutter#14…

f776ff6

…4899)

engine-flutter-autoroll mentioned this pull request Jun 5, 2024

Roll Flutter from c246ecdf8e8d to 27e06569a157 (17 revisions) flutter/packages#6875

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 5, 2024

39472d9 Feature: Add AnimatedList with separators (flutter/flutter#14…

84c9362

…4899)

Piinks mentioned this pull request Jun 7, 2024

Flutter Framework Team: Notable Commits #121415

Open

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

39472d9 Feature: Add AnimatedList with separators (flutter/flutter#14…

270a9ab

…4899)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: Add AnimatedList with separators #144899

Feature: Add AnimatedList with separators #144899

Peetee06 commented Mar 10, 2024 •

edited

Loading

Peetee06 commented Mar 10, 2024

Hixie Mar 24, 2024

Hixie Mar 24, 2024

Hixie Mar 24, 2024

Hixie Mar 24, 2024

Hixie Mar 24, 2024

Hixie Mar 24, 2024

Hixie Mar 24, 2024

Hixie Mar 24, 2024

Piinks left a comment

Peetee06 commented Mar 31, 2024

Piinks commented Apr 3, 2024

Piinks commented Apr 3, 2024

Piinks commented May 1, 2024

Peetee06 commented May 2, 2024

Piinks left a comment

Peetee06 commented May 8, 2024

Peetee06 commented May 9, 2024 •

edited

Loading

Piinks left a comment

Piinks May 21, 2024

Peetee06 May 25, 2024 •

edited

Loading

Piinks May 31, 2024

Peetee06 May 31, 2024

Piinks Jun 4, 2024

Piinks May 21, 2024

Peetee06 May 25, 2024

Piinks left a comment

Piinks May 31, 2024

Peetee06 May 31, 2024

Piinks May 31, 2024

Piinks left a comment

auto-submit bot commented Jun 5, 2024

Peetee06 commented Jun 5, 2024

nate-thegrate commented Jun 5, 2024

		/// [MediaQuery]'s padding. To avoid this behavior, override with a
		/// zero [padding] property.

Feature: Add AnimatedList with separators #144899

Feature: Add AnimatedList with separators #144899

Conversation

Peetee06 commented Mar 10, 2024 • edited Loading

Pre-launch Checklist

Peetee06 commented Mar 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Piinks left a comment

Choose a reason for hiding this comment

Peetee06 commented Mar 31, 2024

Piinks commented Apr 3, 2024

Piinks commented Apr 3, 2024

Piinks commented May 1, 2024

Peetee06 commented May 2, 2024

Piinks left a comment

Choose a reason for hiding this comment

Peetee06 commented May 8, 2024

Peetee06 commented May 9, 2024 • edited Loading

Piinks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Peetee06 May 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Piinks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Piinks left a comment

Choose a reason for hiding this comment

auto-submit bot commented Jun 5, 2024

Peetee06 commented Jun 5, 2024

nate-thegrate commented Jun 5, 2024

Peetee06 commented Mar 10, 2024 •

edited

Loading

Peetee06 commented May 9, 2024 •

edited

Loading

Peetee06 May 25, 2024 •

edited

Loading