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

feat: added flag for mpv as flatpak #1411

Merged
merged 8 commits into from
Sep 18, 2024
Merged

feat: added flag for mpv as flatpak #1411

merged 8 commits into from
Sep 18, 2024

Conversation

Derisis13
Copy link
Collaborator

Pull Request Template

Type of change

  • Bug fix
  • Feature
  • Documentation update

Description

Comply with request #1408

Checklist

  • any anime playing
  • bumped version

  • next, prev and replay work
  • -c history and continue work
  • -d downloads work
  • -s syncplay works
  • -q quality works
  • -v vlc works
  • -e select episode works
  • -S select index works
  • -r range selection works
  • --skip ani-skip works
  • --skip-title ani-skip title argument works
  • --no-detach no detach works
  • --dub and regular (sub) mode both work
  • all providers return links (not necessarily on a single anime, use debug mode to confirm)

  • -h help info is up to date
  • Readme is up to date
  • Man page is up to date

Additional Testcases

  • The safe bet: One Piece
  • Episode 0: Saenai Heroine no Sodatekata ♭
  • Unicode: Saenai Heroine no Sodatekata ♭
  • Non-whole episodes: Tensei shitara slime datta ken (ep. 24.5, ep. 24.9)

@port19x
Copy link
Collaborator

port19x commented Sep 5, 2024

Didn't we already have that at some point?

@Derisis13
Copy link
Collaborator Author

Didn't we already have that at some point?

We may have. But it seems like there's a need for this. I see no reason why we shouldn't have it.

@port19x
Copy link
Collaborator

port19x commented Sep 5, 2024

Because we already have a flatpak_mpv player for steamdeck implemented.
I'd rather have player detection fall back to flatpack mpv when a regular mpv can't be detected.
A similar pattern to what I want is already used for iina, where manually installed iina needs different handling than iina installed via homebrew

@port19x
Copy link
Collaborator

port19x commented Sep 5, 2024

This would have the additional upside of consolidating case branches with steamdeck

@Derisis13
Copy link
Collaborator Author

Ok, can do. It makes sense not to not trust it onto our users to know which package format they use

fix: auto-detect mpv installation similar to iina
ani-cli Outdated
*) player_function="${ANI_CLI_PLAYER:-mpv}" ;; # Linux OS
*)
player_function="${ANI_CLI_PLAYER:-$(where_mpv)}" # Linux OS
[ $? -eq 0 ] || exit $?
Copy link
Collaborator

Choose a reason for hiding this comment

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

This part is actually not necessary, since dep_ch will already exit

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tested it, it doesn't.

@Derisis13
Copy link
Collaborator Author

I've tested what I could on linux, but @ykhan21 I'd like you to test it on windows if it correctly detects installed and uninstalled mpv, and @Nannk to check it on the steamdeck if it still finds the flatpak version.
Also @port19x I think there's a little issue with where_iina() which I couldn't test: when the subshell exits, the main program continues uninterrupted. I suggest to use a similar structure like I did with where_mpv()

@port19x
Copy link
Collaborator

port19x commented Sep 16, 2024

iirc it worked fine in my testing on mac os. return exits subshells, exit always aborts the shell script.

SO Post

Where is this not the case?

@port19x
Copy link
Collaborator

port19x commented Sep 16, 2024

Could it be because you use a default assignment? ${ANI_CLI_PLAYER:-$(where_mpv)}

In where iina, that is checked within the function, but I'd be surprised if that changes things

@Derisis13
Copy link
Collaborator Author

iirc it worked fine in my testing on mac os. return exits subshells, exit always aborts the shell script.

SO Post

Where is this not the case?

Linux and dash. Same if the default assignment is not present, like with iina.

@Derisis13
Copy link
Collaborator Author

Derisis13 commented Sep 16, 2024

And checking the shell specification's related section reveals that exit only returns from the current subshell and continues from the environment it was called from.

@port19x
Copy link
Collaborator

port19x commented Sep 16, 2024

Alright, I'm taking the L

@Derisis13
Copy link
Collaborator Author

Now then why haven't the two remaining checks ran?

@port19x
Copy link
Collaborator

port19x commented Sep 16, 2024

Now then why haven't the two remaining checks ran?

I have no idea. I added the completion files to path ignore, but you haven't changed those in this PR, so I'm surprised they aren't running. Github Actions is a bit fragile at times.

Wanna patch my where_iina to be more in line with your where_mpv?

Let's see if github wakes from its slumber after another commit

@Derisis13
Copy link
Collaborator Author

Wanna patch my where_iina to be more in line with your where_mpv?

Sure, let's see

@port19x
Copy link
Collaborator

port19x commented Sep 16, 2024

How odd. Could it be that I have misunderstood how the filtering works in github actions?

@port19x
Copy link
Collaborator

port19x commented Sep 16, 2024

Now that's a surprise. Perhaps a brown out on githubs part to bully people into migrating away from a deprecated nodejs version (used by checkout v3)

@port19x port19x merged commit 62e872e into master Sep 18, 2024
7 checks passed
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