[Android] Fix IndexOutOfBoundsException on media delete fail
Romain Vimont
rom at rom1v.com
Tue Jan 9 18:44:07 CET 2018
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
>>
More information about the Android
mailing list