Assertion errors when animating to an item that is removed during the animation

[jellynoone]

Consider the following example:

import 'package:flutter/material.dart';
import 'package:super_sliver_list/super_sliver_list.dart';

void main() {
  runApp(const MainApp());
}

class MainApp extends StatelessWidget {
  const MainApp({super.key});

  @override
  Widget build(BuildContext context) {
    return const MaterialApp(
      home: SuperListViewPage(),
    );
  }
}

class SuperListViewPage extends StatefulWidget {
  const SuperListViewPage({super.key});

  @override
  State<SuperListViewPage> createState() => _SuperListViewPageState();
}

class _SuperListViewPageState extends State<SuperListViewPage> {
  final _controller = ListController();
  final _scrollController = ScrollController();
  var _items = 100;

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: SuperListView.builder(
        controller: _scrollController,
        listController: _controller,
        itemCount: _items,
        itemBuilder: _buildItem,
      ),
      floatingActionButton: FloatingActionButton(onPressed: _onTap),
    );
  }

  void _onTap() {
    if (_items == 0) {
      _reset();
    } else {
      _scroll();
    }
  }

  void _scroll() {
    _controller.animateToItem(
      index: _items - 1,
      scrollController: _scrollController,
      alignment: .5,
      duration: (_) => const Duration(milliseconds: 300),
      curve: (_) => Curves.ease,
    );

    setState(() {
      _items = 0;
    });
  }

  void _reset() {
    setState(() {
      _items = 100;
    });
  }

  static Widget _buildItem(BuildContext context, int index) {
    return Padding(
      padding: const EdgeInsets.symmetric(vertical: 30),
      child: Center(
        child: Text('$index'),
      ),
    );
  }
}

When pressing on the floating button, an assertion error will be issued that the index does not exist, even though at the start of the animation it does.

It seems when performing animations, the simulation expects the final index to be present during the entire animation. While this example is extreme in that removing of the item happens immediately after the animation start.

In addition, looking at: https://github.com/superlistapp/super_sliver_list/blob/a4a4326d4cc89207e9e5079d6e7b39c9aab4993c/lib/src/animate_to_item.dart#L45-L52

It seems index is always used to determine the next position to use. Couldn't this also lead to cases where the items change in such a way that the index would be pointing to a different item not the one originally intended?

[knopp]

I could clamp the index to total number of items in case that changes to get around the assertion. Other than I'm not sure what else to do given that SliverChildDelegate which ultimately backs the children does not have a concept of identity for items and as such can't really detect item removal.

[jellynoone]

I think clamping would be a great start that would cover most code paths.

As for item synchronisation, personally, I think it would rarely happen.

However, a possible solution that comes to mind, (although I haven't checked the internals in detail so I might be wrong here) would be to get the render object of the item being scrolled to and reuse it on further calls? Something, like:

class ExtentManager {
  double? getOffsetToReveal(RenderObject item);
  (double, RenderObject) getInitialItemToRevealData(int index);
}

[knopp]

The RenderObject is not available initially. It is created on demand when it reaches the cache area during scrolling.

[jellynoone]

Thank you for the explanation.

Could another potential solution be to ask the caller about the current index on each animation tick? For instance, instead of

abstract interface class ListController {
  void animateToItem({
    required int index,
    required ScrollController scrollController,
    required double alignment,
    required Duration Function(double estimatedDistance) duration,
    required Curve Function(double estimatedDistance) curve,
    Rect? rect,
  });
}

we'd have:

abstract interface class ListController {
  void animateToItem({
    required int? Function() getItemIndex,
    required ScrollController scrollController,
    required double alignment,
    required Duration Function(double estimatedDistance) duration,
    required Curve Function(double estimatedDistance) curve,
    Rect? rect,
  });
}

Similar to what we have with duration and curve?

If the getItemIndex call returned null, it would indicate the scroll should stop perhaps reusing the last target offset and finishing animating to it?

[knopp]

I like this! I'm not sure if this should be a breaking change or a separate method (if so it needs different name). I think when returning null I'd just stop the animation.

[jellynoone]

I can see a use case for both of these methods, for instance, if you're building a chat application and want to redirect the user to a specific message, you'd use the index getter variant. Whereas if you are building a news application and want to navigate the user to the 10th most popular article, you'd always use the index directly.

A potential solution I have in mind would be something like the following (additional parameters skipped for brevity):

abstract interface class ListController {
  /// Directly jumps to the selected index.
  /// The caller is responsible for providing a valid index.
  void jumpToItem(int index);

  /// Animates to the selected index.
  /// The animation manager validates the index continues to
  /// be valid during the entire duration of the animation. If it
  /// stops being valid, the animation ends.
  void animateToItem({
    required int index,
  });

  /// Animates to the selected live index.
  /// The caller is responsible for keeping the index valid for
  /// the duration of the animation.
  void animateToLiveItem({
    required int? Function() index,
  });
}

At first, I wanted to suggest breaking animateToItem and adding animateToIndex that could potentially just forward the call to animateToItem. However, since we have jumpToItem already and it doesn't make sense for it to come in two variants (because the caller will always have to provide a valid index and only has to do it once). I think its best to keep the current naming and signature for these methods.

Additionally, if animateToIndex simply forwarded a call to the animateToItem variant, the developer could still hit the errors we are trying to prevent here. Which made me think the proposed solution would be best.

@knopp What do you think?

[jellynoone]

@knopp Were you able to give this proposal a thought?

[knopp]

I'd simply go with changing the animateToItem signature to replace item index with a ValueGetter.

void animateToItem({
    required ValueGetter<int> index,
    required ScrollController scrollController,
    required double alignment,
    required Duration Function(double estimatedDistance) duration,
    required Curve Function(double estimatedDistance) curve,
    Rect? rect,
  })

This would be a breaking change but it's rather minor and I think it's better than cluttering API.

[jellynoone]

@knopp Thank you for the fix! But how about the case when the item is fully removed and there is no backing index to return?

[knopp]

Right. I think the return value should be nullable (null cancelling the animation).