player/command: add prepend and insert-prev flags to loadfile/loadlist - #17250
player/command: add prepend and insert-prev flags to loadfile/loadlist#17250Dudemanguy wants to merge 2 commits into
Conversation
|
There is no reason to add these when they can already be achieved with the index argument set to 0 ( |
|
Shorthands. Of course you can already achieve these (and others) if you use insert-at. |
8b7d71c to
4c83aa3
Compare
|
These "shorthands" only add interface complexity that led to #16837. I think it is better to document the |
|
|
|
I still think mention in documentation is enough. We should not add less generic interfaces when generic ones are available. |
|
I agree that making interface more complex is not good idea in general, but here we are only adding an alias which doesn't make it more complex in practice. Though the usefulness of this is limited too, as anyone already would need to handle it differently. That said, I don't think why this shouldn't be merged. Bikesheding this is waste of time, shrug. |
|
We have aliases for appending things so I figured why not have them for prepending too. ¯_(ツ)_/¯ I'm sure most users would append 99% of the time but still. |
| case LOAD_TYPE_INSERT_NEXT: | ||
| return playlist_get_next(mpctx->playlist, +1); | ||
| case LOAD_TYPE_INSERT_PREV: | ||
| return mpctx->playing ? mpctx->playing : playlist_entry_from_index(mpctx->playlist, 0); |
There was a problem hiding this comment.
As you are manipulating the playlist, shouldn't this be
mpctx->playlist->current ? mpctx->playlist->current : playlist_entry_from_index(mpctx->playlist, 0)
instead?
There was a problem hiding this comment.
mpctx->playing is the same as mpctx->playlist->current
There was a problem hiding this comment.
mpctx->playingis the same asmpctx->playlist->current
The version that uses mpctx->playing and the version that uses mpctx->playlist->current can produce different runtime behavior.
Consider the attached program (the attached program requires setting the MPV_BIN environment variable). Running the attached program with the line
return mpctx->playing ? mpctx->playing : playlist_entry_from_index(mpctx->playlist, 0);
prints
initial: ['a.wav:CP', 'b.wav', 'c.wav']
['x.wav', 'a.wav:P', 'b.wav:C', 'c.wav']
whereas with the line
return mpctx->playlist->current ? mpctx->playlist->current : playlist_entry_from_index(mpctx->playlist, 0);
it prints
initial: ['a.wav:CP', 'b.wav', 'c.wav']
['a.wav:P', 'x.wav', 'b.wav:C', 'c.wav']
So they are different in the sense that they can exhibit different runtime behavior.
There was a problem hiding this comment.
I see. Good observation. In this case, it appears that mpctx->playing is the one with the correct behavior. I took a look at the logs here. For both cases you have this sequence of events (expected):
[ 0.068][v][cplayer] Opening done: /tmp/mpv-different-position-1c0i2bqu/a.wav
....
[ 0.077][v][cplayer] Starting playback...
....
[ 0.093][d][cplayer] Run command: loadfile, flags=64, args=[url="/tmp/mpv-different-position-1c0i2bqu/x.wav", flags="insert-prev", index="-1", options=""]
If we're doing an insert-prev here, then the x.wav should come before a.wav in the playlist. I'm not sure why but mpctx->playlist->current is pointing to b.wav at this point in time which is wrong.
No need to have a switch here. It's not like the enum order should be changed willy-nilly (although the next commit is going to do that).
4c83aa3 to
14b90dd
Compare
4 participants
No description provided.