<div dir="ltr"><div><div><div><div><div>No real blocker.<br></div>VLC adapters already support filtering.<br><br></div>Problem would be corner cases, potentially leading to bloated code:<br></div>If a user deletes 5 files consecutively, how de we do?<br><br></div>This is the only issue I see.<br><br></div>(Thus, when we'll have a ViewModel layer, it will be cleaner to manage all of this.)<br><div><br><div class="gmail_quote"><div dir="ltr">Le mar. 9 janv. 2018 à 17:39, Romain Vimont <<a href="mailto:rom@rom1v.com" target="_blank">rom@rom1v.com</a>> a écrit :<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Tue, Jan 09, 2018 at 03:31:42PM +0000, Geoffrey Métais wrote:<br>
> Yes exactly,<br>
><br>
> And reinserting it at a wrong position could mess up the current sorting.<br>
> Or if item is added at the end of the list, user is likely to not see it<br>
> showing back at all.<br>
><br>
> I don't have any good solution for this particular case.<br>
<br>
Currently, the add/remove is applied to the displayed list, and the<br>
filters are not persistent (they depend on the sequence of change<br>
events, so it's inherently racy, that's why when the list is updated<br>
after you filtered it, the filter is lost).<br>
<br>
Instead, I propose that the adapter should store:<br>
- the full list;<br>
- the current sorting;<br>
- the current filter.<br>
<br>
Every time the list is modified for any reason (e.g. when an item is<br>
added or removed), the action would be applied *to the full list*<br>
(always on the same thread), then the sorting and filter would be<br>
re-applied (some optimizations may later avoid unnecessary steps). As a<br>
consequence, all add/remove requests must not be applied to a specific<br>
position (which would be meaningless at the time of the call).<br>
<br>
Of course, filtering and sorting should still be executed outside of the<br>
main thread.<br>
<br>
I started to implement this last month, as a requirement to solve<br>
<<a href="https://code.videolan.org/videolan/vlc-android/issues/404" rel="noreferrer" target="_blank">https://code.videolan.org/videolan/vlc-android/issues/404</a>>, but I<br>
stopped when I found that several asynchronous changes depended on a<br>
particular (outdated) position: this proposal required more preliminary<br>
work.<br>
<br>
Do you think this may work? Do you see any "blocker" (something which is<br>
obviously incompatible with this)?<br>
<br>
Thank you<br>
<br>
> At least, user is now warned by a snack that deletion failed. So user won't<br>
> be surprised to see it appear later.<br>
><br>
> For successful reinsertion, we may use diffutil for safety.<br>
><br>
> Le mar. 9 janv. 2018 à 16:06, Romain Vimont <<a href="mailto:rom@rom1v.com" target="_blank">rom@rom1v.com</a>> a écrit :<br>
><br>
> > On Tue, Jan 09, 2018 at 03:36:54PM +0100, Geoffrey Métais wrote:<br>
> > > vlc-android | branch: master | Geoffrey Métais <<br>
> > <a href="mailto:geoffrey.metais@gmail.com" target="_blank">geoffrey.metais@gmail.com</a>> | Tue Jan 9 09:51:49 2018 +0100|<br>
> > [5cc334ce561866ee91cab70f533732c845b78829] | committer: Geoffrey Métais<br>
> > ><br>
> > > Fix IndexOutOfBoundsException on media delete fail<br>
> > ><br>
> > > ><br>
> > <a href="https://code.videolan.org/videolan/vlc-android/commit/5cc334ce561866ee91cab70f533732c845b78829" rel="noreferrer" target="_blank">https://code.videolan.org/videolan/vlc-android/commit/5cc334ce561866ee91cab70f533732c845b78829</a><br>
> > > ---<br>
> > ><br>
> > > vlc-android/src/org/videolan/vlc/gui/audio/AudioBrowserAdapter.java | 1<br>
> > +<br>
> > > vlc-android/src/org/videolan/vlc/gui/video/VideoListAdapter.java | 1<br>
> > +<br>
> > > 2 files changed, 2 insertions(+)<br>
> > ><br>
> > > diff --git<br>
> > a/vlc-android/src/org/videolan/vlc/gui/audio/AudioBrowserAdapter.java<br>
> > b/vlc-android/src/org/videolan/vlc/gui/audio/AudioBrowserAdapter.java<br>
> > > index c0df3a9a1..a2930c820 100644<br>
> > > --- a/vlc-android/src/org/videolan/vlc/gui/audio/AudioBrowserAdapter.java<br>
> > > +++ b/vlc-android/src/org/videolan/vlc/gui/audio/AudioBrowserAdapter.java<br>
> > > @@ -333,6 +333,7 @@ public class AudioBrowserAdapter extends<br>
> > SortableAdapter<MediaLibraryItem, Audio<br>
> > ><br>
> > > public void addItem(final int position, final MediaLibraryItem<br>
> > item) {<br>
> > > final List<MediaLibraryItem> referenceList = peekLast();<br>
> > > + if (position < 0 || position >= referenceList.size()) return;<br>
> ><br>
> > IMO, providing a valid position is the caller responsability. Here, the<br>
> > error is silently ignored.<br>
> ><br>
> > This method is always called from a Runnable passed to<br>
> > UiTools.snackerWithCancel(), relying on the position of the item when it<br>
> > was deleted. But the list may have changed in the meantime, so adding<br>
> > the item back should not rely on the position.<br>
> ><br>
> > In the worst case, the position is out-of-range (with this patch it does<br>
> > not crash anymore, but the item is not added back); but even when it is<br>
> > in-range, there is no guarantee that the position is correct.<br>
> ><br>
> > > final List<MediaLibraryItem> dataList = new<br>
> > ArrayList<>(referenceList);<br>
> > > dataList.add(position,item);<br>
> > > update(dataList);<br>
> > > diff --git<br>
> > a/vlc-android/src/org/videolan/vlc/gui/video/VideoListAdapter.java<br>
> > b/vlc-android/src/org/videolan/vlc/gui/video/VideoListAdapter.java<br>
> > > index 2f5863fcf..4ad8f091b 100644<br>
> > > --- a/vlc-android/src/org/videolan/vlc/gui/video/VideoListAdapter.java<br>
> > > +++ b/vlc-android/src/org/videolan/vlc/gui/video/VideoListAdapter.java<br>
> > > @@ -148,6 +148,7 @@ public class VideoListAdapter extends<br>
> > SortableAdapter<MediaWrapper, VideoListAda<br>
> > > @MainThread<br>
> > > public void add(MediaWrapper item, int position) {<br>
> > > final List<MediaWrapper> list = new ArrayList<>(peekLast());<br>
> > > + if (position < 0 || position >= list.size()) return;<br>
> > > list.add(position, item);<br>
> > > update(list);<br>
> > > }<br>
> > ><br>
> > > _______________________________________________<br>
> > > Android mailing list<br>
> > > <a href="mailto:Android@videolan.org" target="_blank">Android@videolan.org</a><br>
> > > <a href="https://mailman.videolan.org/listinfo/android" rel="noreferrer" target="_blank">https://mailman.videolan.org/listinfo/android</a><br>
> > _______________________________________________<br>
> > Android mailing list<br>
> > <a href="mailto:Android@videolan.org" target="_blank">Android@videolan.org</a><br>
> > <a href="https://mailman.videolan.org/listinfo/android" rel="noreferrer" target="_blank">https://mailman.videolan.org/listinfo/android</a><br>
> ><br>
<br>
> _______________________________________________<br>
> Android mailing list<br>
> <a href="mailto:Android@videolan.org" target="_blank">Android@videolan.org</a><br>
> <a href="https://mailman.videolan.org/listinfo/android" rel="noreferrer" target="_blank">https://mailman.videolan.org/listinfo/android</a><br>
<br>
_______________________________________________<br>
Android mailing list<br>
<a href="mailto:Android@videolan.org" target="_blank">Android@videolan.org</a><br>
<a href="https://mailman.videolan.org/listinfo/android" rel="noreferrer" target="_blank">https://mailman.videolan.org/listinfo/android</a><br>
</blockquote></div></div></div>