Closed AzureOrange404 closed 9 months ago
You did a nice job implementing the feature in a way that's consistent to the other parts of the script. There's just one thing missing: ani-skip install instructions in the readme. There should be a reference to their instructions with some description on what it does
Hi, I am opening a pull request to include the ani-skip directly into ani-cli
. There's no need to include ani-skip as a dependency.
It would be nice, If this PR is merged first. I'll only have to replace the function.
I've also wrote a lua script for vlc player. VLC is not as flexible as MPV, We'll need to find a suitable way to pass in options or hard-code them.
Before you accept this pr, it would be better if you add ani-skip onto the features and the dependency requirements. Additionally, warn windows users as I get ani-skip not installed
and there's no official app for it.
Before you accept this pr, it would be better if you add ani-skip onto the features and the dependency requirements. Additionally, warn windows users as I get
ani-skip not installed
and there's no official app for it.
I'll include the code directly into ani-cli script, you won't have to install ani-skip explicitly
Hi, I am opening a pull request to include the ani-skip directly into
ani-cli
. There's no need to include ani-skip as a dependency.It would be nice, If this PR is merged first. I'll only have to replace the function.
I'm against including lines of code instead of an available solution: just as we outsourced our ui to fzf and rofi, I see no point in re-implementing ani-skip. The reason is we have to maintain our code, but dependencies take care of themselves.
This PR looks fine as is.
I've also wrote a lua script for vlc player. VLC is not as flexible as MPV, We'll need to find a suitable way to pass in options or hard-code them.
This is welcome once the PR is merged
Sorry about the thousend commits on the readme, it's definitely to late to work now ... haha. Is there a script or something to do all the checks before commiting them?
You did a nice job implementing the feature in a way that's consistent to the other parts of the script. There's just one thing missing: ani-skip install instructions in the readme. There should be a reference to their instructions with some description on what it does
I did update the README as well, I've put everything under the dependencies section (should be fine I guess).
Before you accept this pr, it would be better if you add ani-skip onto the features and the dependency requirements. Additionally, warn windows users as I get
ani-skip not installed
and there's no official app for it.
I added a warning to the ani-skip description. I should work under Android and MacOS right? (Not shure about iOS though.)
I can test mac compatibility tomorrow, code and readme looks good. I'm also against inclusion of ani-skip code in mainline, unless the expansion over the currently proposed solution would be negligible. Either way I'm in favour of merging early and making concessions to windows users later.
I'm also against inclusion of ani-skip code in mainline, unless the expansion over the currently proposed solution would be negligible
it's 27 lines of shell scripting and another 27 lines of lua. Since we can't include the lua scrip anyway I'd leave ani-skip in the hands of its current maintainer.
I'm also against inclusion of ani-skip code in mainline, unless the expansion over the currently proposed solution would be negligible
it's 27 lines of shell scripting and another 27 lines of lua. Since we can't include the lua scrip anyway I'd leave ani-skip in the hands of its current maintainer.
Yes, lua scripts are another issue.
Pls add the copying of ani-skip
shell script to $PATH
in the readme install instructions
Pls add the copying of
ani-skip
shell script to$PATH
in the readme install instructions
I have updated it, do tell me If there are any more issues with ani-skip
.
Update the ani-cli's get_skip_times
, as I've updated ani-skip script. So now you'll have to just call ani-skip, it will handle the errors itself.
@synacktraa pls make the script in the repo an executable to reduce the install steps
@synacktraa pls make the script in the repo an executable to reduce the install steps
Done. Interesting, I had no idea it could be done like that
ani-cli
should not exit on a failed retrieval ofskip_argument
.get_skip_times
may be omitted as per review of @synacktraa
it is only exiting if the dependency is missing, otherwise it prints a warning, but still runs. I'll look in to the rest of the suggestions though.
Edit: Ah I see, the error is now printed by ani-skip directly, I'll include the changes.
I have updated it, do tell me If there are any more issues with
ani-skip
.Update the ani-cli's
get_skip_times
, as I've updated ani-skip script. So now you'll have to just call ani-skip, it will handle the errors itself.
I don't know if it's just me, but now the player jumps to 0:00 after skipping. I get this mpv error:
[skip] Error: Can't convert '301.499 --script=/home/azure/.config/mpv/scripts/skip.lua' to number!
[skip] script-opts: error converting value '301.499 --script=/home/azure/.config/mpv/scripts/skip.lua' for key 'end_time'
I have updated it, do tell me If there are any more issues with
ani-skip
.Update the ani-cli's
get_skip_times
, as I've updated ani-skip script. So now you'll have to just call ani-skip, it will handle the errors itself.I don't know if it's just me, but now the player jumps to 0:00 after skipping.
I get this mpv error:
[skip] Error: Can't convert '301.499 --script=/home/azure/.config/mpv/scripts/skip.lua' to number! [skip] script-opts: error converting value '301.499 --script=/home/azure/.config/mpv/scripts/skip.lua' for key 'end_time'
Wait, I'll look into it
Wait, I'll look into it
If mpv is run manually, it works. It's just the implementation via ani-cli. I guess somehow due to variable handling.
Try:
mpv "$(ani-skip "Ansatsu Kyoushitsu (TV) (22 episodes)" 1)" https://video.wixstatic.com/video/f5c58e_f62ec058c47d41798e86987023d35063/1080p/mp4/file.mp4
If mpv is run manually, it works. It's just the implementation via ani-cli. I guess somehow due to variable handling.
Try:
mpv "$(ani-skip "Ansatsu Kyoushitsu (TV) (22 episodes)" 1)" https://video.wixstatic.com/video/f5c58e_f62ec058c47d41798e86987023d35063/1080p/mp4/file.mp4
Can you once show how is it being in your ani-cli?
Can you once show how is it being in your ani-cli?
I implemeted your changes:
mpv*) nohup "$player_function" "$skip_flag" --force-media-title="${allanime_title}Episode ${ep_no}" "$episode" >/dev/null 2>&1 & ;;
Can you once show how is it being in your ani-cli?
I implemeted your changes:
mpv*) nohup "$player_function" "$skip_flag" --force-media-title="${allanime_title}Episode ${ep_no}" "$episode" >/dev/null 2>&1 & ;;
remove quotes from $skip_flag
, as it has two flags
remove quotes from
$skip_flag
, as it has two flags
That's the problem, shellcheck will complain if the quotes are removed. There is no way (I know of) in POSIX to pass two flags to a command using one variable. (That's why I had two variables before, I guess.)
remove quotes from
$skip_flag
, as it has two flagsThat's the problem, shellcheck will complain if the quotes are removed. There is no way (I know of) in POSIX to pass two flags to a command using one variable. (That's why I had two variables before, I guess.)
Yeah, I get it. My previous script was not even including the lua script but I thought ani-skip
script should be the one to return the script and script-opts flag together. let's see what maintainers say and then I'll update ani-skip accordingly.
Yeah, I get it. My previous script was not even including the lua script but I thought
ani-skip
script should be the one to return the script and script-opts flag together. let's see what maintainers say and then I'll update ani-skip accordingly.
I could just split your output to two variables, would be no problem.
at all I just saw that, if there is no output (e. g. anime missing) and the flag is empty '', mpv throws the following error:
[file] Cannot open file '': No such file or directory
Failed to open .
But if everyone's fine with this, it can just be ignored.
I could just split your output to two variables, would be no problem.
at all I just saw that, if there is no output (e. g. anime missing) and the flag is empty '', mpv throws the following error:
[file] Cannot open file '': No such file or directory Failed to open .
But if everyone's fine with this, it can just be ignored.
Or we can just call it like this:
mpv*) nohup "$player_function" --force-media-title="${allanime_title}Episode ${ep_no}" "$episode" "$skip_flag" >/dev/null 2>&1 & ;;
Or we can just call it like this:
mpv*) nohup "$player_function" --force-media-title="${allanime_title}Episode ${ep_no}" "$episode" "$skip_flag" >/dev/null 2>&1 & ;;
Yeah, then the errors just appear after the video finished, but it doesn't matter as they are suppressed by nohup
anyways.
Or we can just call it like this:
mpv*) nohup "$player_function" --force-media-title="${allanime_title}Episode ${ep_no}" "$episode" "$skip_flag" >/dev/null 2>&1 & ;;
Yeah, then the errors just appear after the video finished, but it doesn't matter as they are suppressed by
nohup
anyways.
You already pushed the changes :), I was planning to do something about it.
You already pushed the changes :), I was planning to do something about it.
Oh haha, yeah you still can, I guess. I'll just annoy everyone with another commit. :)
remove quotes from
$skip_flag
, as it has two flagsThat's the problem, shellcheck will complain if the quotes are removed. There is no way (I know of) in POSIX to pass two flags to a command using one variable. (That's why I had two variables before, I guess.)
That's because this is one of the few instances where you actually want shell word splitting. You can add a shellcheck ignore directive as found in a few other places in the script
That's because this is one of the few instances where you actually want shell word splitting.
You can add a shellcheck ignore directive as found in a few other places in the script
If quotes can be omitted, it will also get rid of the empty $skip_flag
issue
@port19x Is this the correct shellcheck?
# shellcheck disable=SC2086
if your shellcheck complains, you can run shellcheck outside of script like this --
shellcheck ani-cli
it will spit out warnings/errors and also those SCXXXX error codes, just copy and paste them just above the line of code where you want to disable it.
@port19x Is this the correct shellcheck?
# shellcheck disable=SC2086
yes
if your shellcheck complains, you can run shellcheck outside of script like this --
shellcheck ani-cli
it will spit out warnings/errors and also those SCXXXX error codes, just copy and paste them just above the line of code where you want to disable it.
@AzureOrange404 Update it accordingly, you should have waited for the solution before committing :>
That's because this is one of the few instances where you actually want shell word splitting. You can add a shellcheck ignore directive as found in a few other places in the script
Didn't know, didn't look it up. But now it's implemented and it works, thanks.
@AzureOrange404 Update it accordingly, you should have waited for the solution before committing :>
Yeah, maybe I should have, haha.
dick, and additionally, balls
@synacktraa Could you change printf
in ani-skip line 37 to die
? Otherwise it'll still output. ;)
also why tf are we using an external script with ani-skip
. i get why the lua script is needed, but why call another script inside of ani-cli ?
(this can probably be answered if i backread the whole thread, but i won't be doing that)
@synacktraa Could you change
printf
in ani-skip line 37 todie
? Otherwise it'll still output. ;)
Yeah, what a stupid mistake
also why tf are we using an external script with
ani-skip
. i get why the lua script is needed, but why call another script inside of ani-cli ?(this can probably be answered if i backread the whole thread, but i won't be doing that)
Because it's a preexisting solution from what I've gathered and we delegate responsibility and locs that way
Perhaps tomorrow I'll finally get around to the mac testing I promised
Since we are using the unquoted variable approach, it would be much better to implement a proper player argument option and integrate ani-skip
in it. however that can be left to 4.7.x
@synacktraa can u state a testcase anime and episode for ani-skip
@synacktraa also pls implement skipping for outro as well
modify the script to redirect stderr if the script cannot find the anime dont rely on a edge case
did none of u check if this runs, modify ani-cli or ani-skip to discard the (n episodes)
MAL is already extremely picky and does not have the best search engine, it will never give the right results if we add unnecessary metadata to the query
modify the script to redirect stderr if the script cannot find the anime
dont rely on a edge case
Isn't die function already redirecting it to stderr?
did none of u check if this runs, modify ani-cli or ani-skip to discard the
(n episodes)
MAL is already extremely picky and does not have the best search engine, it will never give the right results if we add unnecessary metadata to the query
I haven't tested ani-skip with ani-cli yet, been just resolving ani-skip issues
my bad, it looks like it does discard the (n episodes) however it seems that u try to match the exact title name in the mal results. this however will more than likely not work for anime with >1 seasons and those which are named differently in allanime
did none of u check if this runs, modify ani-cli or ani-skip to discard the
(n episodes)
MAL is already extremely picky and does not have the best search engine, it will never give the right results if we add unnecessary metadata to the query
I haven't tested ani-skip with ani-cli yet, been just resolving ani-skip issues
Tell me if you're modifying ani-cli, else I'll do something with ani-skip
Pull Request
Type of change
Description
I saw the issue #1223 and sympathized with the idea of skipping intros. So implemented the skipping function into ani-cli (it still needs ani-skip to be installed though.)
Skipping does not work for all anime I've tested, but does for some. If ani-skip fails, ani-cli just prints a message stating, that it failed and proceeds as normal.
Also it only works with mpv (due to how ani-skip works) and this is noted in the help function as well as the man page.
Checklist
[x] bumped version
-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--dub
and regular (sub) mode both work[x] all providers return links (not necessarily on a single anime, use debug mode to confirm)
-h
help info is up to dateAdditional Testcases