[vlc-devel] [PATCH 4/8] playlist/item: add_subitem_tree: fix next on flat playlist and empty node

[Filip Roséen]

Hi Rémi,

On 2017-05-21 22:15, Rémi Denis-Courmont wrote:

> Le sunnuntaina 21. toukokuuta 2017, 20.48.14 EEST Filip Roséen a écrit :
> > ---
> >  src/playlist/item.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> > 
> > diff --git a/src/playlist/item.c b/src/playlist/item.c
> > index e912b0bcbc..5be9bba118 100644
> > --- a/src/playlist/item.c
> > +++ b/src/playlist/item.c
> > @@ -190,6 +190,26 @@ static void input_item_add_subitem_tree ( const
> > vlc_event_t * p_event,
> > 
> >              playlist_ViewPlay( p_playlist, NULL, p_play_item );
> >          }
> > +        else if( b_flat && p_playlist->current.i_size > 0 )
> > +        {
> > +            /* If the playlist is flat, empty nodes are automatically
> > deleted; +             * meaning that moving from the current index (the
> > index of the now +             * removed node) to the next would result in
> > a skip of one entry +             * (as the empty node is deleted, the
> > logical next item would be +             * the one that now resides in its
> > place).
> > +             *
> > +             * By resetting the currently playing to the item immediately
> > +             * before the insertion point of the non-existing children, or
> > the +             * start of the playlist if we are at offset 0, this
> > problem is +             * fixed.
> > +             **/
> > +            if( last_pos )
> > +                ResetCurrentlyPlaying( p_playlist,
> > +                    ARRAY_VAL( p_playlist->current, last_pos - 1 ) );
> > +            else
> > +                playlist_ViewPlay( p_playlist, NULL,
> > +                    ARRAY_VAL( p_playlist->current, 0 ) );
> > +        }
> >      }
> > 
> >      PL_UNLOCK;
> 
> Doesn´t this cause the previous item of the parent to be played again? At 
> least, that´s how I interpret the comments.

Later on, after this function is invoked, the playlist will advance to
the next item, so "rewinding" the position to the one before the index
where the deleted node was removed will correctly advance to the
logical next one.

Imagine `{ A, B, C, D }` where `B` is currently played and removed, the
playlist will be at index `1` (0-based index), though after removal of
`B` the playlist is `{ A, C, D }`. When we later go to the "next"
item, we would end up at `D`, even though `C` is supposed to be
played.

By pretending that it is `A` that is currently being played, `C` will
correctly play when advancing the playback.

I wasn't sure how to put the above in shorter or clearer comments than
what I wrote, and I spend quite some time coming up with a good way of
putting it, but maybe I should rethink the comments; or what do you
think?

Best Regards,\
Filip
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20170521/dfa98a3e/attachment.html>