[Android] Fix IndexOutOfBoundsException on media delete fail
Geoffrey Métais
geoffrey.metais at gmail.com
Tue Jan 9 17:51:47 CET 2018
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?
This is the only issue I see.
(Thus, when we'll have a ViewModel layer, it will be cleaner to manage all
of this.)
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/740b0405/attachment-0001.html>
More information about the Android
mailing list