serenader2014 / flutter_carousel_slider

A flutter carousel widget, support infinite scroll, and custom child widget.
https://pub.dev/packages/carousel_slider
MIT License
1.59k stars 579 forks source link

A couple of problems with dynamic slider #44

Open davbaron opened 5 years ago

davbaron commented 5 years ago

I am using the slider in multiple functions. For pure 'read only' pages, the slider works great. However I have 'read-write' pages where a user can add or remove photos to a 'product'. I use the slider to allow the user to see all the photos they have attached. Nothing ground-breaking. The slider mostly works, but seems to run into a couple of [I believe related] issues.

First off, when adding photos, I 'add to' an underlying list of 'photoObjects' and upon setState, the page rebuilds, the slider gets re-build, etc. I then set a timer for 200 ms to 'AnimateToPage(lastPageIndex)' and boom, the slider slides to the new photo. Very nice. However, an issue arises whereby I can scroll infinitely to the 'next' (swipe to left) photo but... if I try to scroll 'back' (swipe to right), I can only go as far as the first photo added. Again, on read-only pages and sliders with multiple photos, I can swipe in either direction forever. Note that the definition of the slider includes setting the 'enableInfiniteScroll' to true when there are more than 1 photos, or false when only 1. If I leave 'enableInfiniteScroll' to its default of True, then when there is only one slide, the user can still 'see' the other edges of it, and can swipe back and forth to itself - a pretty poor user experience, I think. Someone else asked about 'preventing the swipe', but the solution did not seem favorable. Again, for read-only scenarios, this approach (turning if False for one slide) works great, but in a dynamic scenario strange stuff happens.

The second issue is much harder to explain: When a photo is removed, depending on 'where' in the list it is, the slider gets 'messed up'. I have tried two approaches to removing items from the slider: 1) Remove the 'data' from the underlying list (inside a setState block), and then setting a timer for 200 ms to then 'animateToPage()' so the user can see the appropriate remaining slide. 2) AnimateToPage() first, going to the 'appropriate' slide, and THEN deleting the underlying list's data (inside a setState block).

The first approach only works in some circumstances. Depending on how many items were in the list before the delete, the 'page order' seems to get messed up, meaning for example, the second page becomes the first page, and vice-a-versa. It is very strange. I also do not like this approach, because after doing the data delete inside of setState, the slider 'blinks' to a seemingly inconsistent slide - sometimes it 'goes to' slide 0, other times the last of the remaining slides.

The second approach looks much nicer, however, like the first approach, the slider/pager gets messed up. It is hard to explain all of the various combinations of moving to pages, deleting pages, adding pages and the results. In short, things are messed up.

It seems that the slider 'wants to go' to certain pages after certain operations. I cannot see a pattern.

The bottom line is that I cannot seem to implement 'removing a slide' in any reliable and accurate way, not to mention the issue with 'enableInfiniteScroll'.

I'm sure you will ask for code, and I will have to build a sample page for that... but, until then, does any of this make sense or look familiar?

Thank you...

joelbrostrom commented 5 years ago

Let's see if I got it right.

  1. If enableInfinityScroll = false you can not scroll past the original image.
  2. If enableInfinityScroll = true you can scroll the items in an infinite loop.
  3. At some state you can scroll infinitely forward, but only to the first item backwards.

Setting enableInfiniteScroll to false actually set the realPage equal to the initialPage. This is how we prevent the slider to circle backwards and by specifying the number of items in the recycler we limit it to go past the last item in the forward direction. When creating a new CarouselSlider with enableInfinityScroll = false you are implicitly setting the realPage (or the offset) to the initialPage (defaults to 0). If you add items to the CarouselSlider at this point you must make sure to create a new CarouselSlider with enableInfinityScroll = true to reset realPage (defaults to 10000, which allows for 10000 backwards scrolls). It sounds like your issue could be that you are setting enableInfinityScroll = false and then add items to the carousel without giving it a realPage value, resulting in no pages before the initial one.

Please make sure the carousel is rebuilt with enableInfinityScroll = true after you set it to false.

joelbrostrom commented 5 years ago

Defining the problem:

  1. Removing an item causes item order to change/shift.

The first approach only works in some circumstances. Depending on how many items were in the list before the delete, the 'page order' seems to get messed up, meaning for example, the second page becomes the first page.

Consider items [a,b,c]. If you first remove an item, say a, and then animate to item with index 1 you might expect to go to b, but since a was deleted the array is now [b, c] and thus you animate to c. If you instead first animate to index 1 and then delete a (your second example) you will end up at bas expected.

If you have an infinite loop of items things get a little more tricky.

The item to display is calculated as position%length. If your at pos 10 with 3 items [a,b,c] the item to display will be 1%3 -> 1, and b will be displayed.

Now if you remove a you have [b,c] and the item to display will be 1%2 ->1, and c will be displayed. Notice how the item displayed changed from b->c.

Now if you remove a you have [b,c] and the item to display will be 10%2 ->0, and b will be displayed. Notice how the item index changed from 1 -> 0.

This is probably the cause to the random behaviour. Mathematically what we would have to do is lowering the pageView position by

final offset = initialPage - currentPosition; //how many pages have been swiped
final loopCount = ceil(offset / oldItemLength); //how many times have we looped
final newPageIndex = currentPageIndex - loopCount; //remove one item per loop
animateTo(newPageIndex);

I've not tested this so take it with a grain of salt.

To sum it up: when adding or removing an item you have to adjust the pageIndex with ±1 for each loop you've made, since the item will be added/removed from all previous loops.

davbaron commented 5 years ago

This is a lot to digest.

The reason I use enableInfiniteScroll is to prevent scrolling when there is only one page. @the-javatar also wanted to disable scrolling (so that he could use buttons), and found that using physics:new NeverScrollableScrollPhysics() was the solution. He made a pull request a couple of weeks ago. I would suggest another property: disableSwipe and when set to true, you use the physics thing. That way we can leave the enableInfiniteScroll out of the discussion.

First issue: adding slides, but not being able to 'go backwards': I have gone through your code a few times now but will admit that I am not following the logic very clearly. I see you are using a Flutter PageView and PageController, both of which I have very little experience with. After just a couple of Google searches, I guess the PageView does not 'officially' go 'backwards' infinitely, only forward. However this items seems to show how it can be done: https://stackoverflow.com/questions/50321213/how-to-extend-pageview-to-both-sides-with-builder It is hard for me to see if you are already using this technique or not - if you are, I apologize. My code is essentially:

setState(() {
      _marketItemObj.photos.add(newSliderPhoto);
});

//set a short timer to 'go to' the new photo
//BUT only if there are more than one photos...
//we want to wait a moment, since the rest of the page must build first
if (_marketItemObj.photos.length > 1) {
  Timer(Duration(milliseconds: 200), () {
    setState(() {
    _imgSlider.animateToPage(_marketItemObj.photos.length - 1,
        duration: Duration(milliseconds: 300), curve: Curves.linear)
        .then((result){
          setState(() {
            //maybe trigger another rebuild of carousel?
          });
    });
    });
  });
}

So, in the above code, the underlying list (of objects) is increased, and setState should trigger the rebuild of the page and carousel. During rebuild, the enableInfiniteScroll would be set to True. So, as the user is 'viewing' the screen (and assuming they are on the 'last' page), they 'see' the new page get rendered to the right of the centered page. Then, 200 ms later, they are 'animatedToPage' to the new page. That part all works great. However, if they try to slide 'backwards', they cannot go past the original 1st page. Why would that be? If the carousel is 'rebuilt', and has multiple pages, and enableInfiniteScroll is true, then why would it not be able to scroll backwards?

On the second issue, removing pages, I am unclear as to the the nature of the problem, or the solution. I also do not understand how there can be any issue if the entire page/carousel is being rebuilt every time the underlying list is modified. I think the main problem for me is that after deleting a page I want the user to view a specific page, and, for the 'order' of pages to be the same as it was before the delete. Using your examples: if I have [a,b,c] and I am 'on' b (_currentSliderIndex == 1), and the user deletes b, then the list would be [a,c]. I would want the 'current page' to remain at 1, i.e., the 2nd page. Upon deleting the item from the underlying list, and then rebuilding the page/carousel, wouldn't the user 'see' page 0? If so, wouldn't my solution of setting a short timer (200 ms) after the delete work? Meaning, wouldn't the timer happen after the rebuild of the page, and via AnimateToPage take the user to page 1 (versus staying on page 0)? Same thing if I have [a,b,c] and I am 'on' c (_currentSliderIndex == 2) and they delete that page. In this case, the page/carousel should get rebuild with [a,b], and viewing page 0. Then the timer would happen and take the user to page 1. Your code examples above were a bit confusing... you used the value '10' sometimes, but then also the value '1', and I'm not sure if all/some of the '1's should be '10's. Here is my code for the delete approach that deletes first, then invokes the timer/animation:

int postDeleteIndex = _currentSliderIndex;
int animateToIndex = _currentSliderIndex;

//now delete from photos...
setState(() {
  _marketItemObj.photos.removeAt(_currentSliderIndex);

  //now set the index that we will 'goto' after deleting, re-building, and such...
  //if we WERE on the last photo, then need to decrease _currentSliderIndex by 1
  if (_currentSliderIndex > (_marketItemObj.photos.length - 1)) {
    animateToIndex = _currentSliderIndex - 1;
    _currentSliderIndex --;
  } else {
    animateToIndex = _currentSliderIndex;
  }
  postDeleteIndex = _currentSliderIndex;
});

//finally, set a short timer to 'go to' the appropriate [remaining] photo
//BUT only if there are more than one photos...
//we want to wait a moment, since the rest of the page must build first
if (_marketItemObj.photos.length > 1) {
  Timer(Duration(milliseconds: 200), () {
    setState(() {
      _imgSlider.animateToPage(animateToIndex,
          duration: Duration(milliseconds: 300), curve: Curves.linear);
    });
  });
}

Let me know how I can help solve these issues.