EdsonBueno / infinite_scroll_pagination

Flutter package to help you lazily load and display pages of items as the user scrolls down your screen.
https://pub.dev/packages/infinite_scroll_pagination
MIT License
626 stars 213 forks source link

Pull to refresh while fetching the next page bug #274

Closed avishail closed 1 year ago

avishail commented 1 year ago

I'm facing a bug where when I'm pulling to refresh (_controller.reset()) while the next page N is being fetched, the final result is messy and composed of a mix of the the new page after the reset and the result of the fetched page N.

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

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

  @override
  State<ActivityTab> createState() => _ActivityTabController();
}

class _ActivityTabController extends State<ActivityTab> {
  final PagingController<int, String> _pagingController =
      PagingController(firstPageKey: 0);

  @override
  void initState() {
    _pagingController.addPageRequestListener((pageKey) {
      _fetchPage(pageKey);
    });
    super.initState();
  }

  @override
  void dispose() {
    _pagingController.dispose();
    super.dispose();
  }

  Future<void> _fetchPage(int pageKey) async {
    await Future<void>.delayed(const Duration(seconds: 3));
    try {
      final List<String> newItems = [];

      for (int i = pageKey; i < pageKey + 10; i++) {
        newItems.add("Activity " + i.toString());
      }

      if (pageKey > 30) {
        _pagingController.appendLastPage(newItems);
      } else {
        final nextPageKey = pageKey + newItems.length;
        _pagingController.appendPage(newItems, nextPageKey);
      }
    } catch (error) {
      _pagingController.error = error;
    }
  }

  @override
  Widget build(BuildContext context) => _ActivityTabView(this);
}

class _ActivityTabView extends StatelessWidget {
  final _ActivityTabController state;

  ActivityTab get widget => state.widget;

  _ActivityTabView(this.state, {Key? key}) : super(key: key);

  @override
  Widget build(BuildContext context) {
    return RefreshIndicator(
        key: GlobalKey<RefreshIndicatorState>(),
        color: Colors.white,
        backgroundColor: Colors.deepPurple,
        onRefresh: () async {
          state._pagingController.refresh();
          return Future<void>.delayed(const Duration(seconds: 3));
        },
        // Pull from top to show refresh indicator.
        child: PagedListView<int, String>(
            pagingController: state._pagingController,
            builderDelegate: PagedChildBuilderDelegate<String>(
              itemBuilder: (context, item, index) => ListTile(
                title: Text(item),
                subtitle: Text("subtitle"),
              ),
            ),
          );
        ));
  }
}
avishail commented 1 year ago

In the end I managed to solve this like this:

I have a source of truth List A

when starting to refresh I'm turning on a boolean and when finish I'm turning it off.

in case I need to update the data as a result of my refresh, I'm calling the pagingController.refresh() and then making sure when _fetchPage is being called then I'm returning my source of truth.

On my _fetchPage method, I'm checking if the boolean is turned on and if so, I'm simply returning without adding any page to the pagingController.

Future<void> _fetchPage(int pageKey) async {
    if (refreshIsRunning && pageKey != 0) {
      return Future.value();
    }
    try {
      final activities = pageKey == 0
          ? await _dataProvider.fetchInitialActivities() // this will return my source of truth list if it is not empty, or will trigger fetchMoreActivities if it is.
          : await _dataProvider.fetchMoreActivities();

      if (refreshIsRunning && pageKey != 0) {
        return Future.value();
      }

      if (activities.length < 20) {
        _pagingController.appendLastPage(activities);
      } else {}
      _pagingController.appendPage(activities, pageKey + 1);
    } catch (error) {
      _pagingController.error = error;
    }
  }
csondi10 commented 1 year ago

Same issue here, it would be nice to solve it without a hacky workaround

EdsonBueno commented 1 year ago

@avishail and @csondi10, this isn't in the package's scope. The problem here is how you're handling your concurrent requests – reason why you easily solved it on your end. There's no way for the package to cancel your ongoing request if it's not in control of that code, right? Depending on how you set up your state management and API calls, that bug doesn't happen.

avishail commented 1 year ago

@EdsonBueno , I do not expect the package to cancel the outgoing requests but I do expect it do disregard the content fetched.

I expect results started prior to reset to be dropped by the infra when they come back. I did it manually and I think you should do it in the package.

EdsonBueno commented 1 year ago

The way you add canceled and valid content is the same. How's the package supposed to know? The package does two things: display items and request them. You're in charge of the rest.

avishail commented 1 year ago

I do not 100% understand your question, but you can maintain a counter and each refreah will increase it. On every page fetch you attach the counter's value and disregard the reault in case it doesn't match the current counter.

dickermoshe commented 10 months ago

@EdsonBueno This should be stated clearly in the documentation for refresh. Would be open for a PR that would document:

Resets [value] to its initial state. Page requests that are currently running won't be canceled by calling refresh.
Jiraiya8 commented 8 months ago

The fact proves that the Android Paging library can solve this problem very well.

Jiraiya8 commented 8 months ago

PageRequestListener should return a Future type and should operate on data within the library, not allowing the controller to set data.

Jiraiya8 commented 8 months ago

Kotlin coroutines can be easily cancelled, but how can Dart's Future be cancelled? This is a problem.Developers should not have to deal with such tedious matters.