[Android] Fix IndexOutOfBoundsException on media delete fail

Geoffrey Métais geoffrey.metais at gmail.com
Tue Jan 9 18:56:26 CET 2018 

Yeah, not a big deal. I don't see any potential blocker then

Le mar. 9 janv. 2018 à 18:44, Romain Vimont <rom at rom1v.com> a écrit :

>
>
> Le 9 janvier 2018 17:51:47 GMT+01:00, "Geoffrey Métais" <
> geoffrey.metais at gmail.com> a écrit :
> >No real blocker.
> >VLC adapters already support filtering.
> >
> >Problem would be corner cases, potentially leading to bloated code:
> >If a user deletes 5 files consecutively, how de we do?
>
> What's the problem, then?
>
> If it's in one action, we can delete these items from the full list, and
> re-apply filtering/sorting.
>
> If it's consecutively in several actions, then several deletions +
> filter/sorting.
>
> Note that the optimisations I talked about would avoid to apply sorting in
> this case (deleting an item does not break sorting), and filtering is
> simplified (no need to retest all items). When you say "bloated code", do
> you refer to these optimizations?
>
> >This is the only issue I see.
> >
> >(Thus, when we'll have a ViewModel layer, it will be cleaner to manage
> >all
> >of this.)
>
> Yes, using ViewModel is probably better than not, but I think these issues
> are quite independant of the problems that ViewModel aims to solve (IIUC,
> the consequence of the fact that a single _Android activity_ may be managed
> successively by several java _Activity_ instances, e.g. after a rotation)
> (although it would probably be easier to workaround issue #404).
>
> >Le mar. 9 janv. 2018 à 17:39, Romain Vimont <rom at rom1v.com> a écrit :
> >
> >> 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
> >>
> >> _______________________________________________
> >> Android mailing list
> >> Android at videolan.org
> >> https://mailman.videolan.org/listinfo/android
> >>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/android/attachments/20180109/7957f585/attachment-0001.html>

More information about the Android mailing list