trackma icon indicating copy to clipboard operation
trackma copied to clipboard

CLI: Added optional range param to 'play' command for easier continuous play of show

Open joaqim opened this issue 4 years ago • 22 comments

I have added @FichteFoll's suggestion of passing ranges of episodes to play function. His examples:

play 12 1- play show 12 from episode 1 onwards. play 12 - play show 12 from the next unseen episode onwards. play 12 3-5 play episodes 3 to 5.

Additionally:

play 12 1 10 is equal to play 12 1-10 play 12 -5 play show 12 from next unseen to 5

Now also works with curses but no with secondary optional parameter (eg: play 12 1 10) only play 12 1-10 and so on works.

Added some verbosity to curses dialog

[Play] Episodes to play (eg: 1, 1-3 or - to play all unseen):

Added clearer error message on wrong input to play_episode

Episode[s] must be numeric or with optional range (eg: 1-3, -3, or - to play all unseen episodes)

Might be a bit to verbose, I'm open to suggestions.

joaqim avatar Apr 21 '20 12:04 joaqim

Noticed a problem with mpv where it goes recursively into the folder and adds any media file (.jpg/.flac/.mp3) to playlist. Could be improved but so far those files are put last in the playlist so it's just an annoyance.

joaqim avatar Apr 21 '20 12:04 joaqim

After more use, trackma-cli doesn't properly update series watched count. Is there a way to skip confirming update each time you watch an episode?

joaqim avatar Apr 21 '20 16:04 joaqim

I believe a better solution would be to allow inputting an episode range to the play argument that may be open-ended on both sides and just appends all episodes from the library as arguments to the player call. I think most players support this. The defaults for the lower end would be the next episode and the higher and would be the last (continuous?) episode in the library.

Examples: play 12 1- play show 12 from episode 1 onwards. play 12 - play show 12 from the next unseen episode onwards. play 12 3-5 play episodes 3-5.

FichteFoll avatar Apr 22 '20 01:04 FichteFoll

That sounds like a better idea. I'll look into that tomorrow (12-16 hours from now).

Removed mpv --playlist-index=[episode number] for now since playlist-index won't always correspond to the correct episode.

joaqim avatar Apr 22 '20 01:04 joaqim

I have implemented a play range function, I will test further. As @FichteFoll suggested:

Examples: play 12 1- play show 12 from episode 1 onwards. play 12 - play show 12 from the next unseen episode onwards. play 12 3-5 play episodes 3-5.

joaqim avatar Apr 22 '20 11:04 joaqim

In my local setup, some months ago i changed Engine.play_episode to always play the given episode and all others after it. For me it's very handy, i just select an episode (or the next unseen one) and watch until i close the player. Since i only changed inside the function, i can see episodes sequentially in any interface.

There's really a use case for selecting a specific range of episodes? It can be easy on CLI, but creating an option for this in the other interfaces is not trivial.

Also, probably some trackers have problems when you enqueue multiple episodes at once (i use MPRIS), so i didn't make a PR before, but with a config, this behavior could be enabled only when supported.

What do y'all think about it? Should I also make a PR?

imsamuka avatar Aug 08 '22 00:08 imsamuka

For me it's very handy, i just select an episode (or the next unseen one) and watch until i close the player.

I can get behind that. Personally, I don't use the feature at all (as I have different means of selecting the next episode(s) to watch), but use case seems like a reasonable default to me and it's also easier to configure as a simple flag compared to a more complex episode range specification. Adjusting the various popups where you can insert an episode number in an input field to state that all following episodes are also queued should make this easy to understand as well.

Also, probably some trackers have problems when you enqueue multiple episodes at once (i use MPRIS)

What does the tracker have to do with this? Generally, the tracker should work regardless of whether the player is playing from a playlist or not (and I'd argue the player shouldn't hold file handles for files it isn't playing in the first place for trackers like inotify or fs handle polling).

There's really a use case for selecting a specific range of episodes?

Hardly, but I can see a use case for CLI specifically when invoked by other processes or on the shell for example. Not so much in a GUI.

FichteFoll avatar Aug 08 '22 19:08 FichteFoll

What does the tracker have to do with this? Generally, the tracker should work regardless of whether the player is playing from a playlist or not (and I'd argue the player shouldn't hold file handles for files it isn't playing in the first place for trackers like inotify or fs handle polling).

I'm sorry, i just assumed that, since only MPRIS can handle ani-cli #600. I did a test here and pyinotify also can identify videos in playlists.

Hardly, but I can see a use case for CLI specifically when invoked by other processes or on the shell for example. Not so much in a GUI.

I thought the same. So it would be better to implement both ideas then. I disagree with a number of code decisions here, so after #634 is closed, i will open my own PR and the maintainers can decide the best impl.

Also, regarding the range notation: -N should be absolute or relative? play X -3 could mean:

  • "play (next unseen) until episode 3" or
  • "play the next 3 episodes"

Imho the relative behavior is more useful (both manually and in scripts), also the absolute behavior can be emulated with 0-3, but the relative can't.

imsamuka avatar Aug 08 '22 21:08 imsamuka

Imho the relative behavior is more useful (both manually and in scripts), also the absolute behavior can be emulated with 0-3, but the relative can't.

Agreed. In summary, I believe we have the following syntax options:

  • 3 - play episode 3+ (until the next missing episode or the end)
  • ~~-3 - play episode (#episodes-3) (until …)~~ (this is ridiculous)
  • 3-3 play episode 3 only
  • 1-5 play episodes 1 to 5, excluding missing episodes (or only until the next missing ep?)
  • optional: 1,10,12 play episodes 1, 10 and 12
  • optional: 1-2,10-12 play episodes 1, 2, 10, 11, 12 (a mix of the behavior above)

FichteFoll avatar Aug 09 '22 22:08 FichteFoll

excluding missing episodes (or only until the next missing ep?)

I think that we should always stop at the first missing episode, otherwise one could miss an episode without noticing, which is very frustrating. Especially if one have episodes from different sources.

Or maybe we could make a new parameter, to change the behavior on CLI.

optional: 1,10,12 play episodes 1, 10 and 12

We could always convert 1,10,12 to 1-1,10-10,12-12, since 1-,10-,12- wouldn't make much sense! If someone want 1-,10-,12- they could do it explicitly.

But even if i love the chaining with ,, if we want this, we need to parse the string inside Engine.play_episode, or create another method: we can't call it repeatedly, as it would create multiple player instances.

I have possible a solution: changing Engine.play_episode to receive either an int or an iterator of int. This would also remove the need for a second parameter too. I need to test the viability of this.

play episode (#episodes-3) (until …)

I'm not sure if i understood you mean (total - 3) onwards?

My summary:

Argument Result
0 or None "Next Unseen" (behavior changeable)
3 Episode 3 (behavior changeable)
3-3 Episode 3
3- Episode 3 and onwards
-1 "Next Unseen"
-3 "Next Unseen" plus 2 more
0- or - "Next Unseen" and onwards
0-3 "Next Unseen" until/and Episode 3
1-3 Episode 1 until/and Episode 3

Then, a play-sequentially config, that when activated would make: X become X-. All other possibilities are stable and would work independent of the config being activated or not.

imsamuka avatar Aug 10 '22 02:08 imsamuka

I'm not sure if i understood you mean (total - 3) onwards?

That is indeed what I meant and understood from your proposal, but that was definitely a misunderstanding. An open range is much more useful and consitent with the right-open-range version (and what I proposed initially), so I'd go with that.

However, that still leaves us with the question about whether the numbers should be relative to the "last seen" episode or to 0. I propose to make them relative we can use a + sign:

Argument Result
None or +1 Next Unseen and onwards (behavior changeable)
3 Episode 3 and onwards (behavior changeable)
3-3 Episode 3
3- Episode 3 and onwards
+1-+1 Next Unseen
+1-+3 Next Unseen plus 2 more
1- or - All episodes
1-3 Episodes 1 until Episode 3 (inclusive)
0-3 Invalid (there is no 0th episode)
+0 (optional) last seen episode and onwards (behavior changeable)
+-1 (optional and rather not) second to last seen episode and onwards (behavior changeable)
+-1-+-1 (optional and rather not) second to last seen episode (this is why I'd rather not do this)

+1 for stopping at the first unseen episode as well.

We could always convert 1,10,12 to 1-1,10-10,12-12, since 1-,10-,12- wouldn't make much sense!

It's easier to just treat the expression differently when a comma occurs.

But even if i love the chaining with ,, if we want this, we need to parse the string inside Engine.play_episode

SGTM, though it should be renamed to play_episodes.

FichteFoll avatar Aug 11 '22 14:08 FichteFoll

SGTM, though it should be renamed to play_episodes.

Agreed.

That is indeed what I meant and understood from your proposal, but that was definitely a misunderstanding.

It seems like it should be better to change - into ~ to avoid confusion with the subtraction operator. It probably would make them treat the symbol more like until/onwards other than minus.

I'm also not a fan of + as a relative "modifier", it seems to bring a lot of complexity, especially with +-1 and +-1-+-1 being possible uses. Maybe it would be better if we used another character like ! * ? % it could be better (i prefer !). Maybe # for "absolute" modifier too.

Since in scripts one should prefer stable arguments (where the outcome shouldn't change based on config.json), it would drive me nuts to see an bash script calling this:

trackma play 10 +1-   # continue playing
trackma play 10 +1-+3 # continue playing and 2 more
trackma play 10 +-2-+-2,+-1-+-1,+0-+0 # see past 3 episodes
trackma play 10 +-2,+-1,+0         # see past 3 episodes

I think 0 should still be equivalent as the "next unseen" (and also +0), as a shortcut to reduce complexity, maybe.

In your examples you didn't show -X. Do you disagree with what i proposed? Is that what you mean with "right-open-range version"?

imsamuka avatar Aug 11 '22 20:08 imsamuka

It seems like it should be better to change - into ~ to avoid confusion with the subtraction operator. It probably would make them treat the symbol more like until/onwards other than minus.

Disagree regarding introducing ~ as a "range" punctuation char. That's not intuitive or expected in my opinion and I don't remember seeing it used like that elsewhere.

I'm also not a fan of + as a relative "modifier", it seems to bring a lot of complexity, especially with +-1 and +-1-+-1 being possible uses. Maybe it would be better if we used another character like ! * ? % it could be better (i prefer !). Maybe # for "absolute" modifier too.

None of your proposed characters can be re-purposed for marking a number as relative, though we could expore # as a marker for absolute numbers and make bare numbers relative by default. However, that would be a breaking change (at least for the CLI usage) and the usage of play 10 2 to play the next next unseen episode seems weird for such a simple call, so I'd rather interpret that the same as play 10 -2, i.e. play the next two unseen episodes and not support negative indices at all (because that becomes confusing quickly). 0 can still refer to the last seen episode but any episode before that must be specified by its absolute index.

Argument Result (where n is the last seen episode)
None n+1 - end or n+1 (behavior changeable)
3 or 1-3 or -3 n+1 - n+3
3-3 n+3
1- or - n+1 - end
1-1 n+1
0-3 n - n+3
0 n - end or n (behavior changeable)
#1-#3 or -#3 1 - 3
#1 1 - end or 1 (behavior changeable)
#1- 1 - end
#0 invalid

Since in scripts one should prefer stable arguments (where the outcome shouldn't change based on config.json), it would drive me nuts to see an bash script calling this:

If the script wants to explicit about it, it should indeed always specify a range. With the # proposal, ranges would be much simpler, however, and I agree that +-2-+-2 is not feasible, which is why I rather wouldn't want to implement negative numbers at all in the earlier proposal.

I think 0 should still be equivalent as the "next unseen" (and also +0), as a shortcut to reduce complexity, maybe.

Making 0 equal 1 is too abstract imo. If we make relative numbers the default, it may refer to the last seen episode just fine, though.

In your examples you didn't show -X. Do you disagree with what i proposed? Is that what you mean with "right-open-range version"?

I was undecided whether a left-open range should begin at episode 1 or at the next unseen episode, so I didn't include it. One attempt would be to base this decision on whether the right end is relative or not, as I did above in the # proposal.

FichteFoll avatar Aug 24 '22 21:08 FichteFoll

One attempt would be to base this decision on whether the right end is relative or not, as I did above in the # proposal.

I think the left-open-range should always expand the same way, it's more intuitive and easier to implement correctly.

Except for that, after thinking for a while, i agree completely with your last proposal, it's the most simple and reasonable solution. I'm working in the PR.

imsamuka avatar Jan 13 '23 23:01 imsamuka

Argument Result (where n is the last seen episode)
3 or 1-3 or -3 n+1 - n+3
0 n - end or n (behavior changeable)
#1 1 - end or 1 (behavior changeable)

These examples contradict each other. 3 could either become 3- like the second/third row, and can become -3, like in the first row.

I highly favor the conversion of the second/third row, it's way more useful and simpler to setup; so i will leave that way for now.

So as it is, the syntax parsing and opening of episodes is done, but i still need to add the new config option, along with documentation.

I'm thinking, should i leave the option true by default? It's what you should expect from every other "media-center" and streaming services; the next episode always start after one finishes. Also, anyone writing scripts will immediately reconsider using #3 instead of the #3-#3 version, since they know first hand that the behavior is unstable.

imsamuka avatar Jan 14 '23 02:01 imsamuka

These examples contradict each other. 3 could either become 3- like the second/third row, and can become -3, like in the first row.

You're right, this doesn't make sense in the last proposal. 3 must expand to either -3 or 3- (or 3-3) and 0 should follow for consistency.

I find play 3 to play the next 3 episodes to be the more natural and useful approach, so a left-open range, whereas for #3 I would favor a right-open range (if configured). The former would also not be affected by the configuration since that only affects the right-side limit. This creates a discrepancy between the open ranges of absolute and relative numbers, but I believe this makes more sense usability-wise and I also expect play n to be the most-used version of this command, so I would weight it higher.

FichteFoll avatar Jan 14 '23 11:01 FichteFoll

I see, i think this inconsistency will favor a more usable interface. In my opinion, these will be the more common uses (ignoring the show index):

  • play play next episode onwards
  • play - play next episode onwards
  • play 1 play next episode
  • play #3 play episode 3 (onwards or not)
  • play 3 play the next 3 episodes

imsamuka avatar Jan 14 '23 13:01 imsamuka

I disagree with any breaking changes (changing the current default behavior). Even if the new behavior might be more natural, people who use the command might (and will) be confused if we suddenly change the default behavior to be relative rather than absolute. If + is a weird behavior we could use an alphabetic character, like for example n from next or r from range. That, or simply creating a new command, rplay, playn or similar.

z411 avatar Jan 17 '23 12:01 z411

I see, i personally would prefer to prepend a character like n to signify relative numbers then, and leave bare numbers to be absolute; without #. It wouldn't change THAT much.

  • play or play - play next episode onwards
  • play n1 play next episode
  • play n3 play the next 3 episodes
  • play 3 play episode 3 (onwards or not)

Would you guys prefer any other characters?

imsamuka avatar Jan 18 '23 21:01 imsamuka

I disagree with any breaking changes (changing the current default behavior)

I forgot to ask. Then you are also against leaving watch_continuously config true as default? Leaving it false, will maintain the current behavior, but when it's true, it will convert play 3 to play 3- (using absolute numbers).

You are against leaving the blank range the same as - too? It also changes the current behavior; but i can make this configurable by using the watch_continuously config too.

imsamuka avatar Jan 18 '23 21:01 imsamuka

In a vacuum, I believe what we agreed on earlier is a pretty good solution, but it does indeed break compatibility. Out of the alternatives presented so far:

  • play n1 (play next 1 episodes) looks pretty weird and definitely inferior to a separate command, playn 1.
  • playn 1 (same as above) is decent. It is sufficiently distinguishable from play 1 and can independently support ranges with the same syntax as normal play.
  • play +1 (play next 1) works fine in this simple form, but should play +3 play just the third next episode or the three next episodes? If we aim for symmetry with the standard play 1 version, this quickly becomes confusing and hard to understand and use - and we already came to the conclusion that this syntax isn't feasible for ranges.
  • rplay 1 (???) not clear what this should do.

As such, I favor the separate playn command while additionally implementing range support via - for both commands and also having separate open range handling from normal play, should we still be interested in it. playn without an episode number would then adhere to the watch_continuously setting and either play only the next or all next episodes.

FichteFoll avatar Jan 19 '23 16:01 FichteFoll

But if we use the playn and play commands, assuming they have different behaviors (one absolute and the other relative), then which version should be used in the curses ui? Should we create another command in curses too?

We also lose the ability to mix absolute and relative numbers: play n0-6 (replay the last watched and stop at episode 6) wouldn't be possible, for example.

I still prefer to remain with a single command, even if the relative prefix is a little odd. There's not that much of semantic difference between playn 3 and play n3, both are quite crypt if the user doesn't know what n stands for, anyway.

imsamuka avatar Jan 22 '23 04:01 imsamuka