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

fix: support using non brew IINA as a player on macOS. #1386

Closed
wants to merge 4 commits into from

Conversation

asamahy
Copy link
Contributor

@asamahy asamahy commented Jul 28, 2024

If IINA was built from source or downloaded directly from iina.io then ani-cli wont find it unless added manually to PATH, this works aroung it without much change to rest of script.

If IINA was built from source or downloaded directly from iina.io then ani-cli wont find it unless added manually to PATH, this works aroung it without much change to rest of script.
@asamahy asamahy requested a review from Derisis13 as a code owner July 28, 2024 23:30
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.

This is way too wordy of a change.
Please decomment and make this smaller

@port19x port19x changed the title Fix: Support using non brew IINA as a player on macOS. fix: support using non brew IINA as a player on macOS. Jul 29, 2024
@port19x
Copy link
Collaborator

port19x commented Jul 29, 2024

Does which iina-cli detect your manually installed iina btw?

@port19x
Copy link
Collaborator

port19x commented Jul 29, 2024

Because if it does, your fix should comfortably fit into L325

@asamahy
Copy link
Contributor Author

asamahy commented Jul 29, 2024

This is way too wordy of a change. Please decomment and make this smaller

which part is wordy? the description or the title?

Does which iina-cli detect your manually installed iina btw?

yeah it works for both. which_iina() only works if iina wasnt in PATH. even though it doesnt make a difference because brew already installs in /Applications but i didnt want to assume in case someone changed it.

Because if it does, your fix should comfortably fit into L325

it cant fit here because the rest of the script assumes the value to be "iina" in "player_function".
L325 is required to reach play_episode() L292;

do you want to remove the reliance on the variable ANI_CLI_PLAYER?

@port19x
Copy link
Collaborator

port19x commented Jul 29, 2024

The change is wordy, altough I see now that's a bad adjective to describe it.
I'd like to have some fallback to iina-cli if iina is not present.
Is that not possible within L325?

@asamahy
Copy link
Contributor Author

asamahy commented Jul 29, 2024

... I'd like to have some fallback to iina-cli if iina is not present. Is that not possible within L325?

iina-cli itself is not exposed to PATH but is symlinked to /usr/local/bin/iina, which is in PATH. Therefore, the fallback for brew iina is manually installing it in /Applications, which my change covers.

i hope i understood your point and answered adequately.

@port19x
Copy link
Collaborator

port19x commented Jul 29, 2024

I think I'll just need to experiment myself with the iina on my machine at home

asamahy and others added 3 commits August 6, 2024 00:52
the spell checker didn’t like leaving the which unquoted command to prevent splitting “which” was irrelevant since I’m only looking for the exit status not the output.
@port19x
Copy link
Collaborator

port19x commented Aug 10, 2024

Alright, I can confirm that currently, out-of-band iina doesn't work and that your patch fixes that

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.

let me be more specific with the changes I want.

You know the section where player_function currently gets set to iina?
There you can call a modified version of you where_iina function that should return the appropriate iina as a string.
dep_ch can probably remain unchanged, or have a very small iina catch instead of the current thing

@port19x
Copy link
Collaborator

port19x commented Aug 16, 2024

image

@port19x
Copy link
Collaborator

port19x commented Aug 16, 2024

If you can do a little testing in my follow up PR, that would be great.
I will be at home for testing in 2-3 hours at the earliest

@asamahy
Copy link
Contributor Author

asamahy commented Aug 16, 2024

srry i honestly didnt have the time to revisit this. you cant change player_function to anything other than iina without breaking playing the next episode

integrating Line 113 and 116 would remove the need to reset the player_function as i did there

@port19x
Copy link
Collaborator

port19x commented Aug 16, 2024

Good to know. Then I'll explicitly test playing the next episode

@port19x
Copy link
Collaborator

port19x commented Aug 16, 2024

I still included you as co-author, so you'll appear in the next release notes.
Good work for a first PR and good catch with the iina in the first place

@asamahy
Copy link
Contributor Author

asamahy commented Aug 16, 2024

thanks. new account and trying to contribute instead of just keeping them private.

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

Successfully merging this pull request may close these issues.

2 participants