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

Insertion order with getUpdatesWithData #14

Open
narcodico opened this issue Apr 10, 2022 · 11 comments
Open

Insertion order with getUpdatesWithData #14

narcodico opened this issue Apr 10, 2022 · 11 comments

Comments

@narcodico
Copy link

I'm using you library with an animated list and I'm also aiming to animate the initial items rendering.

I noticed that when using getUpdatesWithData and comparing an initial empty list with some incoming data, e.g.: [] with [Model1(), Model2()], the insertion order will be: Model2 at 0, Model1 at 0.
This is obviously bad in terms of UI, since the logic way of seeing items appearing on the screen would start with first and end with the last one.

What's your take on this and have you planned on addressing this @knaeckeKami ?

@knaeckeKami
Copy link
Owner

I understand your problem, unfortunately, I don't have a solution as of yet.

You could maybe do a post-processing stuff on the Iterable<DataDiffUpdate> to reorder the result while maintaining the correct positions (adjusted for the reordering)?

@narcodico
Copy link
Author

That's exactly what I currently do, but I feel this package would be even more valuable than already is if it would handle this internally.

@knaeckeKami
Copy link
Owner

Yes. I don't know how to define the "best" update order for any list diff, though.

@narcodico
Copy link
Author

I feel there's no such thing as best but rather useful.

@knaeckeKami
Copy link
Owner

Yes, I also don't know how to define the usefulness of the diff result.
Can you show me the algorithm to reorder the result to make it work for you?

@narcodico
Copy link
Author

narcodico commented Apr 12, 2022

final currentItemsCount = oldItems.length;
        _changes = List<diffutil.DataInsert<Item>>.from(changes)
            .reversed
            .mapIndexed((index, change) => diffutil.DataInsert<Item>(
                  position: index + currentItemsCount,
                  data: change.data,
                ));

In my particular case I do this only when I have multiple inserts that I need to handle, for obvious reasons(order of UI inserts).
changes are the ones computed by your diffutil. oldItems are used with calculateDiff.
So with this approach I get initial inserts with order of 0 1 2...
Say we already have 10 items in the list, then the insertion would look like 10 11 12...

I feel this works well when there are multiple consecutive inserts. This would get a bit more complex when dealing with deletes too.

@knaeckeKami
Copy link
Owner

Thanks. It might be possible to get your expected insertion order by tweaking the iteration order of the algorithm, especially reversing the iteration order of these loop:
https://github.com/knaeckeKami/diffutil.dart/blob/master/lib/src/diffutil_impl.dart#L683
https://github.com/knaeckeKami/diffutil.dart/blob/master/lib/src/diffutil_impl.dart#L720

However, when reversing the iteration order, the body of the loops needs to be tweaked as well.

@knaeckeKami
Copy link
Owner

Another way to go would be to implement some sort of Batching (like when you call getUpdates (batch:true)) on diffs with data.
So you would not have a [DataInsert(position: 0, data: "b"), DataInsert(position: 0, data : "a")] but rather something like
[BatchedDataInsert(position: 0, data: ["a", "b"])] .

This would be easier to implement.
Would this help?

@narcodico
Copy link
Author

narcodico commented Apr 12, 2022

That's not gonna work well because you can't obtained the right order by doing inserts at the same position, no matter where that position is. You basically need [DataInsert(position: 0, data: "a"), DataInsert(position: 1, data : "b")], so the batch approach doesn't play well with this unless you're just grouping together the aforementioned models into a batch model which would have to look different, because like I mentioned earlier, you won't obtain the needed result by using the same position.

@knaeckeKami
Copy link
Owner

Something like this?

import 'package:diffutil_dart/src/model/diffupdate_with_data.dart';

extension Sort<T> on Iterable<DataDiffUpdate<T>> {
  Iterable<DataDiffUpdate<T>> reorder() sync* {
    DataDiffUpdate<T>? lastUpdate;
    final List<DataDiffUpdate<T>> _batchedUpdates = [];
    for (final update in this) {
      if (lastUpdate.runtimeType != update.runtimeType) {
        if (_batchedUpdates.isNotEmpty) {
          // end of consecutive updates of the same type
          yield* _yieldResult(_batchedUpdates);
          _batchedUpdates.clear();
          yield update;
        } else {
          // first new update
          _batchedUpdates.add(update);
        }
        lastUpdate = update;
      } else {
        // moves and changes cannot be batched
        if (lastUpdate is DataMove<T> || lastUpdate is DataChange<T>) {
          yield lastUpdate!;
          lastUpdate = update;
        } else if (update is DataInsert<T>) {
          // handle consecutive inserts
          final lastInsert = lastUpdate as DataInsert<T>;
          if (update.position == lastInsert.position) {
            _batchedUpdates.add(update);
          } else {
            yield* _yieldResult(_batchedUpdates);
            _batchedUpdates.clear();
            yield update;
            lastUpdate = update;
          }
        } else {
          // handle consecutive deletes
          final remove = update as DataRemove<T>;
          final lastRemove = lastUpdate as DataRemove<T>;
          if (remove.position == lastRemove.position) {
            _batchedUpdates.add(update);
          } else {
            yield* _yieldResult(_batchedUpdates);
            _batchedUpdates.clear();
            yield update;
            lastUpdate = update;
          }
        }
      }
    }
    // yield last items
    yield* _yieldResult(_batchedUpdates);
  }

  Iterable<DataDiffUpdate<T>> _yieldResult(List<DataDiffUpdate<T>> updates) sync* {
    if (updates.isEmpty) {
      return;
    }
    int correction = updates.first is DataInsert<T> ? 0 : updates.length - 1;
    for (final correctedUpdate in updates.reversed) {
      yield correctedUpdate.when(
          insert: (pos, data) => DataDiffUpdate.insert(position: pos + correction++, data: data),
          remove: (pos, data) => DataDiffUpdate.remove(position: pos + correction--, data: data),
          change: (_, __, ___) => throw AssertionError("unreachable"),
          move: (_, __, ___) => throw AssertionError("unreachable"));
    }
  }
}

(note: proof of concept, this is untested and may be buggy)

@narcodico
Copy link
Author

Looks like a promising starting point. And yeah, writing some tests would help solidify it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants