Handling/copying the lists provided to DiffUtilSliverList

[GregoryConrad]

The documentation states:

Don't mutate the list passed into DiffUtilSliverList yourself ... either copy the list before mutation: ... Or just copy the list before passing it to DiffUtilSliverList: DiffUtilSliverList<int>(items: List.from(list), ...),

Is there a reason why this library does not automatically copy the list for users so this isn't needed? The list will need to be copied at some point for DiffUtil to work correctly anyways; I don't see why DiffUtilSliverList can't copy the list itself and take some work away from library users. Plus, this addition won't be a breaking change.

If I am missing something here/not understanding this correctly, please do let me know. But, if not, I'd be happy to open a PR for this.

[knaeckeKami]

Thanks for raising this!

I originally implemented it this way in order to achieve a bit better performance in some use cases (in order to avoid copying the list twice, or to avoid copying it when the user does not care that the library mutates the list). However, with the migration to null safety, I had to copy the given list anyway (from a list of type T to a list of type T?, in order to be able to insert nulls as placeholder values when calculating the difference).

So actually, the documentation is outdated. You don't need to do this anymore.

[knaeckeKami]

So, I just looked at the code again (after a long time) and I found that just mutating the list still does not work without copying first..

The relevant code is:

    final tempList = oldWidget.items;
    final newList = widget.items;

    final diff = diffutil
        .calculateListDiff<T>(
          tempList,
          newList,
          detectMoves: false,
          equalityChecker: widget.equalityChecker,
        )
        .getUpdates(batch: true);

and if oldWidget.items is the same also widget.items, this will not find any diff. So I will have to publish a new release where this is not necessary anymore.

[GregoryConrad]

Ahh I see why that happens. You're doing a copy to this.tempList after the diff check. Maybe something as simple as this could work:

@override
  void didUpdateWidget(DiffUtilSliverList<T> oldWidget) {
    super.didUpdateWidget(oldWidget);
    final tempList = oldWidget.items;
    final newList = identical(tempList, widget.items) ? List.of(widget.items) : widget.items; // See this line right here

    final diff = diffutil
        .calculateListDiff<T>(
          tempList,
          newList,
          detectMoves: false,
          equalityChecker: widget.equalityChecker,
        )
        .getUpdates(batch: true);

    this.tempList = List<T?>.of(tempList);
    diff.forEach(_onDiffUpdate);
  }

Edit: that won't actually work, because the list will have the exact same contents. I think the only solution would be to do a copy directly in the constructor so that DiffUtilSliverList will own its own list??

[knaeckeKami]

Yes, the proper way would be to store the old list in the state instance. I think I'll do a new release which takes care of this in the next days.

[knaeckeKami]

I pushed changes to https://github.com/knaeckeKami/diffutil_sliverlist/tree/feature/allow_mutable_list . This adds defensive copies of the list passed in, making any remarks of the mutability of the list and this package mutating the list obsolete.

This adds a slight performance penalty if the user uses an immutable list, but I believe this is small enough to be warranted for easier use. I also refactored the first example to use a single mutable list instead of copying the list on changes.

I would appreciate it if you tried it and let me know of you run into any unexpected issues.

[GregoryConrad]

I'll try it out later today or tomorrow and let you know how it goes.

[GregoryConrad]

I'm getting an exception on removing an item from a list on the new branch. I purposefully created one list of length 3 (my actual application creates the list each build call because the list is only a few elements) and rebuilding after a Future.delayed(const Duration(seconds: 2), list.removeLast); throws the exception:

════════ Exception caught by widgets library ═══════════════════════════════════
The following RangeError was thrown building:
RangeError (index): Invalid value: Only valid value is 0: 1

When the exception was thrown, this was the stack
#0      List.[] (dart:core-patch/growable_array.dart:264:36)
#1      _DiffUtilSliverListState.build.<anonymous closure>
package:diffutil_sliverlist/src/diffutil_sliverlist_widget.dart:130
#2      SliverAnimatedListState._itemBuilder
package:flutter/…/widgets/animated_list.dart:624
#3      SliverChildBuilderDelegate.build
package:flutter/…/widgets/sliver.dart:471
#4      SliverMultiBoxAdaptorElement._build
package:flutter/…/widgets/sliver.dart:1236
#5      SliverMultiBoxAdaptorElement.performRebuild.processElement
package:flutter/…/widgets/sliver.dart:1169
#6      Iterable.forEach (dart:core/iterable.dart:325:35)
#7      SliverMultiBoxAdaptorElement.performRebuild
package:flutter/…/widgets/sliver.dart:1213
#8      SliverMultiBoxAdaptorElement.update
package:flutter/…/widgets/sliver.dart:1146
#9      Element.updateChild
package:flutter/…/widgets/framework.dart:3530
#10     ComponentElement.performRebuild
package:flutter/…/widgets/framework.dart:4832
#11     StatefulElement.performRebuild
package:flutter/…/widgets/framework.dart:4977
#12     Element.rebuild
... (other lines emitted)

Relevant lines in diffutil_sliverlist_widget.dart:

 @override
  Widget build(BuildContext context) {
    return SliverAnimatedList(
      key: listKey,
      initialItemCount: widget.items.length,
      itemBuilder: (context, index, animation) => widget.insertAnimationBuilder(
      //--------------------------------------------------------------- exception thrown from in here   
      context,
        animation,
        widget.builder(context, widget.items[index]),
      ),
    );
  }

It is possible I messed something else up because I threw the code together pretty quickly (I am a bit busy), but if you could reproduce this by calling remove after some set period of time, there there might be a bug.

[knaeckeKami]

can you share a reproducible example? I tried to reproduce the behavior you described in this test:

testWidgets("mutable list", (tester) async {
    final myList = ["1", "2", "3"];
    final  rebuilder = ValueNotifier(0);

    int removeCount = 0;

    final widget = MaterialApp(
      home: CustomScrollView(
        slivers: [
          ValueListenableBuilder(
            valueListenable:  rebuilder,
            builder: (BuildContext context, value, Widget? child) => DiffUtilSliverList<String>(
              items: myList,
              builder: (context, item) => Text(item),
              insertAnimationBuilder: (context, animation, widget) =>
                  SizeTransition(sizeFactor: animation, child: widget),
              removeAnimationBuilder: (context, animation, widget) {
                removeCount++;
                return SizeTransition(sizeFactor: animation, child: widget);

              }
            ),
          ),
        ],
      ),
    );
    await tester.pumpWidget(widget);

    await tester.pumpAndSettle();

    myList.removeLast();
    //trigger a rebuild manually
    rebuilder.value = rebuilder.value +1;

    await tester.pumpAndSettle();

    expect(removeCount, 1);

  });

but that passed without issues.

[GregoryConrad]

I found a bug in my code that was most likely related to that one I had above and forgot to mention it in this thread. Considering that example you gave passes without any exceptions being thrown, my guess is it was in fact a bug with my code, as I did throw the new changes in fairly quickly and probably overlooked something.

[knaeckeKami]

published 0.6.0-beta.1

[GregoryConrad]

@knaeckeKami Revisiting this because I needed to use this package in another part of my app (thanks again for all your work on this package!).

This adds a slight performance penalty if the user uses an immutable list, but I believe this is small enough to be warranted for easier use.

Why not simply do a check to see if the list in question is an UnmodifiableListView before copying? There are obviously other types of immutable lists, especially ones that can be user-defined, but that is by far the most common, and the one freezed uses.

(Actually, freezed uses a subtype of UnmodifiableListView--EqualUnmodifiableListView--that it implements itself, but it still is-a UnmodifiableListView).

This is the best of both worlds: the end user will never have to worry about copying and will always get the best-case performance based on whatever data structure they use.

[knaeckeKami]

This is not relevant any more, anyway. I could only avoid the copy before null safety. Now it's not possible anymore since I cannot insert placeholder null values if the list type is non-nullable, so I need to copy the list in to get a type where I can insert null placeholder values (which are needed for getting the right index for the animation)