Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Joaqim
Copy link

@Joaqim Joaqim commented Apr 21, 2020

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
Copy link
Author

Joaqim commented Apr 21, 2020

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
Copy link
Author

Joaqim commented Apr 21, 2020

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?

@FichteFoll
Copy link
Collaborator

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.

@Joaqim
Copy link
Author

Joaqim commented Apr 22, 2020

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 Joaqim closed this Apr 22, 2020
@Joaqim
Copy link
Author

Joaqim commented Apr 22, 2020

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 Joaqim reopened this Apr 22, 2020
@Joaqim Joaqim changed the title Added 'playfolder' command for easier continuous play of show Added optional range param to 'play' command for easier continuous play of show Apr 22, 2020
@Joaqim Joaqim force-pushed the master branch 4 times, most recently from 651e9d9 to 042f1ee Compare April 22, 2020 11:40
@Joaqim Joaqim changed the title Added optional range param to 'play' command for easier continuous play of show CLI: Added optional range param to 'play' command for easier continuous play of show Apr 22, 2020
Comment on lines +908 to +911
for episode in range(playep, playto+1):
ep = self.get_episode_path(show, episode, error_on_fail=False)
if ep:
arg_list.append(ep)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be safer to stop appending episodes after the first unfound episode because this suggested behavior is equivalent to silent failure. If episode 4 was missing but 5+ were not, that episode would not be added to the player's playlist and a user wanting hands-off playback would be likely to miss that this episode was never there. If instead we stopped adding episodes alltogether, the user is implicitly prompted to start investigating once he reaches the end of the playlist.

An alternative would be to abort on any error and request an episode range that can be found. For that, the default playto value needs to be the last available episode, not the show's total according to metadata.

@imsamuka
Copy link
Contributor

imsamuka commented Aug 8, 2022

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?

@FichteFoll
Copy link
Collaborator

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.

@imsamuka
Copy link
Contributor

imsamuka commented Aug 8, 2022

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.

@FichteFoll
Copy link
Collaborator

FichteFoll commented Aug 9, 2022

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)

@imsamuka
Copy link
Contributor

imsamuka commented Aug 10, 2022

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.

@FichteFoll
Copy link
Collaborator

FichteFoll commented Aug 11, 2022

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.

@imsamuka
Copy link
Contributor

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"?

@FichteFoll
Copy link
Collaborator

FichteFoll commented Aug 24, 2022

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.

@imsamuka
Copy link
Contributor

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
Copy link
Contributor

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.

@FichteFoll
Copy link
Collaborator

FichteFoll commented Jan 14, 2023

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.

@imsamuka
Copy link
Contributor

imsamuka commented Jan 14, 2023

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

@z411
Copy link
Owner

z411 commented Jan 17, 2023

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.

@imsamuka
Copy link
Contributor

imsamuka commented Jan 18, 2023

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
Copy link
Contributor

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.

@FichteFoll
Copy link
Collaborator

FichteFoll commented Jan 19, 2023

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.

@imsamuka
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants