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

Refresh calls ignored when other refresh in progress #338

Open
prokyhb opened this issue Aug 3, 2024 · 13 comments
Open

Refresh calls ignored when other refresh in progress #338

prokyhb opened this issue Aug 3, 2024 · 13 comments

Comments

@prokyhb
Copy link

prokyhb commented Aug 3, 2024

Hello,

I need to be able to refresh page (after filtering) quite frequently (using barcode scanner), but once one "refresh" is in progress and I execute another one, the 2nd gets ignored.

I've tried several approaches like using buffer listener (triggering another refresh after complete status, but refresh is not triggered in listener at all) and another techniques, but with no luck.

Is there any option how to cancel current refresh operation and trigger new one?

Thank you.

@Bertus-W
Copy link

was looking for this as well. I saw a thing on the paging controller called controller.value.status which is of the type PagingStatus but that status never seems to change either.

@ShankarKakumani
Copy link

I also have this issue now. Not sure how to solve it.
had to move away from library because of this issue.

@r-i-c-o
Copy link

r-i-c-o commented Oct 18, 2024

I have found a pretty minimalistic way to fix this issue. The root of this problem is that PagingController can't tell the difference between subsequent refreshed states, because they are all equal to each other and are filtered out by ValueNotifier superclass. So we can simply assign some tag (int in my case) in PagingState subclass to guarantee distinction between states after refresh(). Now we can use VersionedPagingController everywhere without additional refactor.

///Implementation of [PagingController] that supports distinction between refreshes
class VersionedPagingController<PageKeyType, ItemType> extends PagingController<PageKeyType, ItemType> {
  VersionedPagingController({required PageKeyType firstPageKey})
      : super.fromValue(
          VersionedPagingState<PageKeyType, ItemType>(version: 0, nextPageKey: firstPageKey),
          firstPageKey: firstPageKey,
        );

  int _currentStateVersion = 0;

  int get currentStateVersion => _currentStateVersion;

  @override
  set value(PagingState<PageKeyType, ItemType> newValue) {
    final versionedState = VersionedPagingState(
      version: _currentStateVersion,
      nextPageKey: newValue.nextPageKey,
      itemList: newValue.itemList,
      error: newValue.error,
    );
    super.value = versionedState;
  }


  @override
  void refresh() {
    _currentStateVersion += 1;
    super.refresh();
  }
}

class VersionedPagingState<PageKeyType, ItemType> extends PagingState<PageKeyType, ItemType> {
  const VersionedPagingState({
    super.nextPageKey,
    super.itemList,
    super.error,
    required this.version,
  });

  final int version;

  @override
  String toString() => 'VersionedPagingState (itemList: $itemList, error: $error, nextPageKey: $nextPageKey, version: $version)';

  @override
  bool operator ==(Object other) {
    if (identical(this, other)) {
      return true;
    }
    return other is VersionedPagingState &&
        other.itemList == itemList &&
        other.error == error &&
        other.nextPageKey == nextPageKey &&
        other.version == version;
  }

  @override
  int get hashCode => Object.hash(
        itemList.hashCode,
        error.hashCode,
        nextPageKey.hashCode,
        version.hashCode,
      );
}

@shubham-shukla1
Copy link

using this version causing duplicates.

@martipello
Copy link

martipello commented Dec 31, 2024

Is there any option how to cancel current refresh operation and trigger new one?

No, is the short answer, but I do have a solution you can try see my last sentence.

What makes it more frustrating is that addPageRequestListener triggers the fetch immediately consider this scenario:
For our fetchPage method to be called we must register a listener with addPageRequestListener.
For our fetchPage method we need some initial value from a Stream for example a filter object.
The addPageRequestListener() fires immediately and calls our fetchPage method which uses the filter object but this filter object isnt available yet as it is built by an asynchronous Stream so we must set the filters first and then call addPageRequestListener().
This would mean calling addPageRequestListener() from a place where the filter model will change, the Stream listeners callback.
A side effect of this is addPageRequestListener() would be called many times without some mitigation.
An example of mitigation could be to check if the listener is registered and not registering more if it is but there isn't currently a way of knowing if any listeners have been added or not.

I think maybe having a hasListeners method for both status and page request listeners would be a nice thing to add. But i would prefer that addPageRequestListener just didnt fire until i told it to or if i could configure the way it behaves in regards to which operator is used I'd prefer a debounce to what feels like a throttle.

My solution you can try is I think in your original scenario outlined above you are changing filters I would expect the list not to be refreshed but to be completely new in which case you can just call fetchPage(0);

@martipello
Copy link

Is there any option how to cancel current refresh operation and trigger new one?

No, is the short answer, but I do have a solution you can try see my last sentence.

What makes it more frustrating is that addPageRequestListener triggers the fetch immediately consider this scenario: For our fetchPage method to be called we must register a listener with addPageRequestListener. For our fetchPage method we need some initial value from a Stream for example a filter object. The addPageRequestListener() fires immediately and calls our fetchPage method which uses the filter object but this filter object isnt available yet as it is built by an asynchronous Stream so we must set the filters first and then call addPageRequestListener(). This would mean calling addPageRequestListener() from a place where the filter model will change, the Stream listeners callback. A side effect of this is addPageRequestListener() would be called many times without some mitigation. An example of mitigation could be to check if the listener is registered and not registering more if it is but there isn't currently a way of knowing if any listeners have been added or not.

I think maybe having a hasListeners method for both status and page request listeners would be a nice thing to add. But i would prefer that addPageRequestListener just didnt fire until i told it to or if i could configure the way it behaves in regards to which operator is used I'd prefer a debounce to what feels like a throttle.

My solution you can try is I think in your original scenario outlined above you are changing filters I would expect the list not to be refreshed but to be completely new in which case you can just call fetchPage(0);

looks like this also causes duplicates

@clragon
Copy link
Collaborator

clragon commented Jan 11, 2025

The next version of the package, v5, fixes this issue, as it allows to interrupt previous operations of the controller when a refresh is called. You can see the code for that on the new-gen branch.

https://github.com/EdsonBueno/infinite_scroll_pagination/blob/new-gen/lib/src/core/paging_controller.dart#L113

As soon as I have written a migration guide, it will be released.

@andynewman10
Copy link

@clragon I updated to the latest available new-gen branch code for infinite_scroll_pagination in my pubspec.yaml.

I successfully migrated the calling code to follow your instructions here:

https://github.com/EdsonBueno/infinite_scroll_pagination/blob/new-gen/example/example.md

I know you're still working on this, but I wanted to test anyway.

My calling code is 100% aligned with your example code.

My page provides a search field that needs to refresh the paging controller (and cancel any pending refresh before if necessary):

return TextField(
      controller: itemController,
      decoration: InputDecoration(
          border: const UnderlineInputBorder(),
          labelText: 'Enter a name here',
          prefixIcon: const Icon(Icons.search)),
      onChanged: (text) {
          _pagingController.refresh();
         notifyListeners();
      }
    );

For state synchronization, I don't use setState, but instead use a ChangeNotifierProvider and a Consumer that wraps my whole page, so notifyListeners() forces a refresh of all page widgets.

So, the migration is smooth (it builds and provides the same functionality as before), but _pagingController.refresh() still has weird behavior and creates, eg. doublons.

Is there any other function I need to call besides _pagingController.refresh() to cancel any pending refresh and force a new one?

@clragon
Copy link
Collaborator

clragon commented Jan 20, 2025

Hi @andynewman10

The new version is conceptualized to allow customizing your state managment to 100%,
so if you want to use your own ChangeNotifier, then you dont need PagingController at all!

However, to benefit from the interruptable requests, you either need to implement a mechanism like in PagingController or use PagingController after all. PagingController is already a ChangeNotifier internally, so you can use it directly with a ValueNotifierProvider instead of creating another extra controller. You can also extend PagingController however you like, as we have written the class with that in mind. This is to solve the whole "having two controllers" problem that the package runs into currently.

I will take a bit of extra time to look into whether the interruption mechanism works correctly.

@andynewman10
Copy link

andynewman10 commented Jan 20, 2025

What I just meant is that instead of doing this:

setState() { _searchTerms = searchTerms; }
pagingController.refresh();

I am just doing:

_searchTerms = searchTerms;
model.notifyListeners(); // instead of setState()
pagingController.refresh();

My whole page is enclosed in a ChangeNotifierProvider, that the whole widget tree consumes with a Consumer widet located just underneath (that's roughly equivalent to the setState mechanism):

  ...
  @override
  Widget build(BuildContext context) {
    return ChangeNotifierProvider( // wrap everything in a ChangeNotifierProvider
        create: (context) => model,
        child: Consumer<MyModel>(builder: (context, cart, child) { // and let the whole widget tree consume it
          return PagingListener(
            controller: pagingController,
            builder: (context, state, fetchNextPage) => PagedSliverList<int, Photo>(
              state: state,
              fetchNextPage: fetchNextPage,
              builderDelegate: PagedChildBuilderDelegate(
                animateTransitions: true,
                itemBuilder: (context, item, index) => ImageListTile(item: item),
              ),
          ),
     );
}

When I execute this code, infinite scroll does work visually speaking when I scroll down, but I get plenty of exceptions in the console:

══╡ EXCEPTION CAUGHT BY FOUNDATION LIBRARY ╞════════════════════════════════════════════════════════
The following assertion was thrown while dispatching notifications for PagingController<int,
MyData>:
setState() or markNeedsBuild() called during build.
...
The PagingController<int, Photo> sending notification was:
  PagingController<int, Photo>#2178a(PagingStateBase<int, Photo>(pages: null, keys: null,
  error: null, hasNextPage: true, isLoading: true))

Additionally, refresh() creates doublons and weird things.

@andynewman10
Copy link

I am also getting the same exceptions with the sample as-is provided here (new-gen branch):

https://github.com/EdsonBueno/infinite_scroll_pagination/tree/new-gen/example

I am using a Flutter web target. Testing with Chrome.

@martipello
Copy link

martipello commented Jan 26, 2025

I get the same issue

Its when it tries to get the value

/// If called while a page is fetching or no more pages are available, this method does nothing.
  void fetchNextPage() async {
    // We are already loading a new page.`
    if (this.operation != null) return;

    final operation = this.operation = Object();

    value = value.copyWith(
      isLoading: true,
      error: null,
    );

here where it calls notifyListeners

 @override
  T get value => _value;
  T _value;
  set value(T newValue) {
    if (_value == newValue) {
      return;
    }
    _value = newValue;
    notifyListeners();
  }

@clragon
Copy link
Collaborator

clragon commented Jan 27, 2025

Hi @andynewman10

Thank you for your report. We have committed a fix for these two issues.
Please let us know if it works for you now.

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

8 participants