[Android] Fix IndexOutOfBoundsException on media delete fail

[Romain Vimont]

On Tue, Jan 09, 2018 at 03:31:42PM +0000, Geoffrey Métais wrote:
> Yes exactly,
> 
> And reinserting it at a wrong position could mess up the current sorting.
> Or if item is added at the end of the list, user is likely to not see it
> showing back at all.
> 
> I don't have any good solution for this particular case.

Currently, the add/remove is applied to the displayed list, and the
filters are not persistent (they depend on the sequence of change
events, so it's inherently racy, that's why when the list is updated
after you filtered it, the filter is lost).

Instead, I propose that the adapter should store:
 - the full list;
 - the current sorting;
 - the current filter.

Every time the list is modified for any reason (e.g. when an item is
added or removed), the action would be applied *to the full list*
(always on the same thread), then the sorting and filter would be
re-applied (some optimizations may later avoid unnecessary steps). As a
consequence, all add/remove requests must not be applied to a specific
position (which would be meaningless at the time of the call).

Of course, filtering and sorting should still be executed outside of the
main thread.

I started to implement this last month, as a requirement to solve
<https://code.videolan.org/videolan/vlc-android/issues/404>, but I
stopped when I found that several asynchronous changes depended on a
particular (outdated) position: this proposal required more preliminary
work.

Do you think this may work? Do you see any "blocker" (something which is
obviously incompatible with this)?

Thank you

> At least, user is now warned by a snack that deletion failed. So user won't
> be surprised to see it appear later.
> 
> For successful reinsertion, we may use diffutil for safety.
> 
> Le mar. 9 janv. 2018 à 16:06, Romain Vimont <rom at rom1v.com> a écrit :
> 
> > On Tue, Jan 09, 2018 at 03:36:54PM +0100, Geoffrey Métais wrote:
> > > vlc-android | branch: master | Geoffrey Métais <
> > geoffrey.metais at gmail.com> | Tue Jan  9 09:51:49 2018 +0100|
> > [5cc334ce561866ee91cab70f533732c845b78829] | committer: Geoffrey Métais
> > >
> > > Fix IndexOutOfBoundsException on media delete fail
> > >
> > > >
> > https://code.videolan.org/videolan/vlc-android/commit/5cc334ce561866ee91cab70f533732c845b78829
> > > ---
> > >
> > >  vlc-android/src/org/videolan/vlc/gui/audio/AudioBrowserAdapter.java | 1
> > +
> > >  vlc-android/src/org/videolan/vlc/gui/video/VideoListAdapter.java    | 1
> > +
> > >  2 files changed, 2 insertions(+)
> > >
> > > diff --git
> > a/vlc-android/src/org/videolan/vlc/gui/audio/AudioBrowserAdapter.java
> > b/vlc-android/src/org/videolan/vlc/gui/audio/AudioBrowserAdapter.java
> > > index c0df3a9a1..a2930c820 100644
> > > --- a/vlc-android/src/org/videolan/vlc/gui/audio/AudioBrowserAdapter.java
> > > +++ b/vlc-android/src/org/videolan/vlc/gui/audio/AudioBrowserAdapter.java
> > > @@ -333,6 +333,7 @@ public class AudioBrowserAdapter extends
> > SortableAdapter<MediaLibraryItem, Audio
> > >
> > >      public void addItem(final int position, final MediaLibraryItem
> > item) {
> > >          final List<MediaLibraryItem> referenceList = peekLast();
> > > +        if (position < 0 || position >= referenceList.size()) return;
> >
> > IMO, providing a valid position is the caller responsability. Here, the
> > error is silently ignored.
> >
> > This method is always called from a Runnable passed to
> > UiTools.snackerWithCancel(), relying on the position of the item when it
> > was deleted. But the list may have changed in the meantime, so adding
> > the item back should not rely on the position.
> >
> > In the worst case, the position is out-of-range (with this patch it does
> > not crash anymore, but the item is not added back); but even when it is
> > in-range, there is no guarantee that the position is correct.
> >
> > >          final List<MediaLibraryItem> dataList = new
> > ArrayList<>(referenceList);
> > >          dataList.add(position,item);
> > >          update(dataList);
> > > diff --git
> > a/vlc-android/src/org/videolan/vlc/gui/video/VideoListAdapter.java
> > b/vlc-android/src/org/videolan/vlc/gui/video/VideoListAdapter.java
> > > index 2f5863fcf..4ad8f091b 100644
> > > --- a/vlc-android/src/org/videolan/vlc/gui/video/VideoListAdapter.java
> > > +++ b/vlc-android/src/org/videolan/vlc/gui/video/VideoListAdapter.java
> > > @@ -148,6 +148,7 @@ public class VideoListAdapter extends
> > SortableAdapter<MediaWrapper, VideoListAda
> > >      @MainThread
> > >      public void add(MediaWrapper item, int position) {
> > >          final List<MediaWrapper> list = new ArrayList<>(peekLast());
> > > +        if (position < 0 || position >= list.size()) return;
> > >          list.add(position, item);
> > >          update(list);
> > >      }
> > >
> > > _______________________________________________
> > > Android mailing list
> > > Android at videolan.org
> > > https://mailman.videolan.org/listinfo/android
> > _______________________________________________
> > Android mailing list
> > Android at videolan.org
> > https://mailman.videolan.org/listinfo/android
> >

> _______________________________________________
> Android mailing list
> Android at videolan.org
> https://mailman.videolan.org/listinfo/android