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

Support for more players and removal of ampersands #256

Merged
merged 13 commits into from
Jan 19, 2022

Conversation

tennyson-mccalla
Copy link

This allowed iina to work for me

@tennyson-mccalla
Copy link
Author

Addresses #254

Copy link
Contributor

@RaynardGerraldo RaynardGerraldo left a comment

Choose a reason for hiding this comment

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

change ORIGINAL_DIR="/Users/Tennyson/ani-cli" this part back to 0

@RaynardGerraldo
Copy link
Contributor

RaynardGerraldo commented Jan 1, 2022

oh and also maybe we are going to support more video players but we are not going to match it by making a case for every video player, try removing "iina") and change it to *) to do a wildcard match and see if it still works

@tennyson-mccalla
Copy link
Author

change ORIGINAL_DIR="/Users/Tennyson/ani-cli" this part back to 0

I caught that on diff and the second commit fixed it.

Will try the wildcard thing right now.

@tennyson-mccalla
Copy link
Author

Wildcard worked well. Making a new PR.

@RaynardGerraldo
Copy link
Contributor

@tennyson-mccalla since this pr now supports all video players that could play streams,you can change the title to something along those lines

@tennyson-mccalla tennyson-mccalla changed the title Support for iina and removal of ampersands Support for more players and removal of ampersands Jan 1, 2022
@tennyson-mccalla
Copy link
Author

Ok, better name. Thanks for the help here @RaynardGerraldo.

@RaynardGerraldo
Copy link
Contributor

RaynardGerraldo commented Jan 2, 2022

Ok, better name. Thanks for the help here @RaynardGerraldo.

to make this even better, you can put this -o use media player of your own choice : ani-cli -o <media-player> in help_text()
and add "knuhdHDqo:p:-:v" to

ani-cli/ani-cli

Line 386 in 4a94745

while getopts 'knuhdHDq:p:-:v' OPT; do
, then add o) player_fn=$OPTARG ;; (follow previous indentations) in

ani-cli/ani-cli

Line 387 in 4a94745

case $OPT in

This way users can specify what media player they want using -o and not change it directly from the code, and make sure that this change works on your end

@port19x port19x linked an issue Jan 2, 2022 that may be closed by this pull request
@port19x port19x self-requested a review January 2, 2022 11:50
@RaynardGerraldo
Copy link
Contributor

@ura43 i noticed that you assigned me to this pr, I can implement the changes now but can I wait for @tennyson-mccalla to do it so that he can test it with IINA?

@Dink4n
Copy link
Contributor

Dink4n commented Jan 2, 2022

Yes, you can

@tennyson-mccalla
Copy link
Author

@ura43 i noticed that you assigned me to this pr, I can implement the changes now but can I wait for @tennyson-mccalla to do it so that he can test it with IINA?

Testing and reporting back later today.

@port19x
Copy link
Collaborator

port19x commented Jan 2, 2022

I assigned you to do the bulk of the reviewing on this PR @RaynardGerraldo

Just curious, could we just have no case statement at all?
Like have it always use the *) case and just pass mpv as the default for that?

@RaynardGerraldo
Copy link
Contributor

Just curious, could we just have no case statement at all? Like have it always use the *) case and just pass mpv as the default for that?

I have been thinking about this ,but with that we're removing the -v(vlc option) ,good?

@tennyson-mccalla
Copy link
Author

The o option with user choice for player works well with mpv (default) and iina. I'm going to push this commit to my fork and add to the PR.

Also corrected a few minor whitespace issues.

@tennyson-mccalla
Copy link
Author

I’ll also do a rewrite with the case statement removed just to test and see how it feels.

@port19x
Copy link
Collaborator

port19x commented Jan 4, 2022

But lets not loose hope, this is a highly desireable feature and we're moving fast

@port19x port19x linked an issue Jan 9, 2022 that may be closed by this pull request
Copy link
Collaborator

@port19x port19x left a comment

Choose a reason for hiding this comment

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

Please bump to latest changes, we may be able to merge this before the end of the month

@tennyson-mccalla
Copy link
Author

Got busy once holiday break ended and couldn’t get back to this. Will try.

@port19x port19x marked this pull request as draft January 14, 2022 18:34
@tennyson-mccalla tennyson-mccalla marked this pull request as ready for review January 16, 2022 17:08
@tennyson-mccalla
Copy link
Author

Should be better now but I can't help but feel that this is still fragile as I don't know many other players will use the "referrer" format that mpv uses. But I can confirm that mpv, vlc, and iina work with this. Please test it out yourself.

@port19x port19x mentioned this pull request Jan 17, 2022
3 tasks
@port19x
Copy link
Collaborator

port19x commented Jan 18, 2022

Tested incompatible with celluloid

@port19x
Copy link
Collaborator

port19x commented Jan 18, 2022

Could you maybe list some players that were tested successful? Because if it's just IINA, I propose we call it -i and actually make it specificly iina

@tennyson-mccalla
Copy link
Author

Could you maybe list some players that were tested successful? Because if it's just IINA, I propose we call it -i and actually make it specificly iina

Right now all I'm testing are MPV, VLC, and IINA. All MPV derivatives should probably work with that default case. I don't have any other players to test with.

This piece was definitely simpler before all of the referrer stuff altered things. I feel like coming up with an alphabet soup to support every player is going to be as bad as a huge case statement that does the same.

@tennyson-mccalla
Copy link
Author

Tested incompatible with celluloid

When did it last have celluloid compatibility?

@port19x
Copy link
Collaborator

port19x commented Jan 18, 2022

Tested incompatible with celluloid

When did it last have celluloid compatibility?

Celluloid is a gtk mpv frontend. Our assumption that caused us supporting iina with a default case was that all, or even most, mpv frontends would share the flags

@port19x
Copy link
Collaborator

port19x commented Jan 18, 2022

I still welcome the change, but in this case I would find a -i that calls iina move appealing

@tennyson-mccalla
Copy link
Author

I still welcome the change, but in this case I would find a -i that calls iina move appealing

Alright, I’ll make the change. If it gets merged in see if you can figure out what it might take to get celluloid working. I don’t have easy access to a Linux machine right now.

@port19x
Copy link
Collaborator

port19x commented Jan 19, 2022

Tested successfully, so at least it didn't break linux+mpv

@port19x port19x merged commit 725f3bd into pystardust:master Jan 19, 2022
@DaBigBlob
Copy link
Contributor

DaBigBlob commented Jun 6, 2022

hello hello hello
#741
👆this should fix everything
i went through the iina docs and have fixed everything related to this issue

@tennyson-mccalla
Copy link
Author

I'll check this out later today. Thanks for the work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
os: mac mac compatibility priority 3: low Default for enhancements type: feature request feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suggestion: Detect player based on start of string Compatibility with iina?
5 participants