angular-ui / ui-scroll

Unlimited bidirectional scrolling over a limited element buffer for AngularJS applications
http://angular-ui.github.io/ui-scroll/demo/
MIT License
327 stars 107 forks source link

Issue when batch applyUpdates() #231

Closed priandsf closed 8 months ago

priandsf commented 5 years ago

This issue is hard to reproduce with the samples, although I can reproduce it systematically within our app. I can put trace if needed.

In our app, a user can select multiple rows that can expand beyond the buffer capacity and delete them. To execute the action, we loop through the rows to delete and call applyUpdates() on each one. This is done in a synchronous loop. Each call to applyUpdates()calls doAdjust(). Such calls to doAdjust() can trigger a fetch() if the deleted row was part of the displayed buffer. Fetch is obviously getting its result asynchronously. Now subsequent calls from the loop to applyUdates() and thus doAjust(), will invalidate the rid used by the fetch call. So when the fetch gets the result back, it discards it. Unfortunately, by doing so, the adapter isLoading flag is not updated and thus the scroll panel become frozen.

There might be multiple ways to fix the issue. In our app, to understand what was going on, we added a new noAdjust option to applyUpdate(). We then make sure that his flag is set for all the updates in the loop, but the last one:

  applyUpdates(arg1, arg2, arg3) {
    if (typeof arg1 === 'function') {
      this.applyUpdatesFunc(arg1, arg2);
      if(arg2 && arg2.noAdjust) return;
    } else {
      this.applyUpdatesIndex(arg1, arg2, arg3);
      if(arg3 && arg3.noAdjust) return;
    }
    this.doAdjust();
  }

That works, but I bet we can come with a more complete solution. Maybe we can also have applyUpdate() with no param just callingdoAdjust()? Or a kind of transaction like api with beginUpdates()/endUpdates()?

dhilt commented 5 years ago

@priandsf noAdjust option makes sense for me. As you know and thanks to you, we already have arg3 in v1.7.6 which is an object containing immutableTop option, so we can proceed with

{
  immutableTop: false,
  noAdjust: false
}

The challenge I see is to provide proper tests spec for this option, that goes hand-in-hand with the demo app which should show how it works in real life. Do you have an idea hot to reproduce the issue with minimal but illustrative demo? If you could make a repro app, say, from this stackblitz, then I would use it both for tests and demo.

priandsf commented 5 years ago

@dhilt yep, this is what I'm using as so far it works for us (see code above). I'll think about a test that can showcase the issue, although this might take a little while. But I'll work on it :-)