marcglasberg / indexed_list_view

Flutter package: Similar to a ListView, but lets you programmatically jump to any item, by index.
BSD 2-Clause "Simplified" License
251 stars 21 forks source link

Jump and animate confusion #35

Closed lhengl closed 2 years ago

lhengl commented 2 years ago

First of all, great package. After some research yours is the only package that supports positive and negative ListView, which should really be a default behaviour in flutter but I digress.

I noticed the following issues:

  1. The animateTo() method is not forwarding the duration and curve parameter to the animateToIndexAndOffset() method. I've had to override this controller to be able to use it effectively with duration and curve.
  2. It took me a lot of hours to try and get a snapping behaviour to work, mainly because of the confusion around the animateToIndex() method. Finally, after hours of frustration, and testing, the animatetToIndex does not really do what the name suggest. It only animates back to an anchor point known as the _originIndex, and jumps each time a new index is called. So I abandoned its usage and tried other methods. May I suggest that it is renamed to animateToAnchor(). This would make it more clear. If developer wants to jump to a specific index, they could just use jumpTo() instead. Baking in a use case for jumping to an index in an "animate" method is confusing.
  3. I ended up using the animateToWithSameOriginIndex() method to achieve what I thought animateToIndex would achieve. I feel like this should have been named animateTo() though because it actually does the animation since the origin does not change. The animateTo() should really be renamed to jumpOrAnimateToAnchor() because it really only animates if the anchor hasn't changed from a previous programatic jump.
  4. Overall, I just think the explicit use of the word jump and animate to do what they are suppose to do will make it less confusing.
  5. May I suggest the change in the name of _originIndex to _anchorIndex as anchor seems to be more meaningful to understand the purpose of the index.
  6. Wrapping the IndexedListView with a NotificationListener to listen for scroll events seem to clash with the animation/jump method. To overcome this issue I had to delay the call to the animation code. See below:
    if (scrollInfo is ScrollEndNotification) {
      if (widget.controller.offset != itemStartLocation) {
        // Controller will only register the animation after a short delay
        Future.delayed(
          const Duration(milliseconds: 1),
          () => widget.controller.animateToWithSameOriginIndex(itemStartLocation, duration: widget.snappingSpeed, curve: widget.animationCurve),
        );
      }
    }

Sorry for the long post, but I do think that this is a really great package and I want it to gain more popularity and should really be adopted as part of the flutter SDK.

marcglasberg commented 2 years ago

Thank you for your report.

I have fixed your item (1) above in indexed_list_view: ^2.0.2.

Regarding the name change suggestions, while I agree with most of your suggestions, these would all be breaking changes and would force every single person using this lib to change names. So I decided not to touch those.

I believe negative indexes and jump/animate to index methods should be part of the Flutter SDK scrollables; but as this package itself was created simply as a proof of concept during discussions with the Flutter team and other people about the lack of a jumpToIndex method in the ListView and other scrollables.

tahaaslam1 commented 2 years ago

Hi @henk-lim could you please contact me at tahaaslam045@gmail.com I have some issues regarding this issue : https://github.com/Milad-Akarie/auto_route_library/issues/792