ani-cli V4 #955

port19x · 2022-12-25T01:17:09Z

Pull Request Template

Type of change

Rewrite
closes #948

Description

V4 will be a full rewrite, using scraping code written by @CoolnsX and ui/ux code written by @justchokingaround

This will help us pay off the technical debt that has accumulated in ani-cli over the past one and a half year and will let us implement what we learned from experience.
It might even chop our LOC in half.

For users the most significant change will be the introduction of a fully fzf based UI.
We will also make sure that our core features, such as our deep history integration and syncplay support, stay and maybe get even more plentiful in the process.

Checklist

Additional Testcases

The safe bet: One Piece
Episode 0: Saenai Heroine no Sodatekata ♭
Unicode: Saenai Heroine no Sodatekata ♭
Not uploaded: one piece dub episode 590
Unreleased: soredemo ayumu wa yosetekuru
Short id (for decryption): Log Horizon episode 1-2

Derisis13 · 2022-12-27T21:03:58Z

What have I done...

ShiNoNeko47 · 2022-12-29T22:47:23Z

ani-cli

+		printf "Selected link:\n%s\n" "$episode"
+		;;
+	mpv | iina)
+		nohup "$player_function" --force-media-title="${allanime_title}episode-${ep_no}-${mode}" "$episode" > /dev/null &


consider removing the "&" at the end
I don't see a reason the menu should be shown when something is already playing and you can't interact with the player when rofi is open if you're using external menu

Detaching from the main process is useful not only because this is the behaviour everyone got used to but this way the script's process can exit, and the player will stay open. I don't see a reason why we shouldn't.

Then I saw how rofi works and I totally take back what I said. This commit should address the matter

Derisis13 · 2022-12-30T23:24:39Z

Derisis13 · 2023-01-02T18:46:12Z

ani-cli

+	-s | --syncplay) player_function=syncplay && shift ;;
+	-q | --quality) quality="$2" && shift 2 ;;
+	-c | --continue) search=history && shift ;;
+	-d | --download) player_function=download && shift ;;
 	-v | -V | --version) version_info ;;


I'd actually prefer if -v and -V would be used for vlc, as I think that function is used more frequently. Also I'd consider supporting case insensitive arguments for all other options, bc it requires no extra loc (but I'm open on this)

good idea for vlc, will change that in the next commit. added long options in 3b19f3a

Especially -v needs to me vlc.
-V is conventionally a shorthand for --version.
Would also make sense with the heuristic that uppercase makes stuff outside the script.

ani-cli

port19x · 2023-01-03T10:56:22Z

I'll be overhauling the readme at my earliest convenience

Derisis13 · 2023-01-03T11:49:43Z

ani-cli

+        player_function="${ANI_CLI_PLAYER:-flatpak_mpv}"
+    else
+        player_function="${ANI_CLI_PLAYER:-mpv}"
+    fi ;;


since having flatpak mpv support is useful outside of steamdeck users (unlike iina, which is OSX only and unlike am launch which is android only) I'd consider making it an argument rather than dependant on the platform string

We can default to mpv.
If it is not found check for flatpak mpv and use it if found.
In my opinion it would be easier for users. It shouldnt be a problem because both pacage managers have the same version and no exclusive features, so it makes no sense to have both installed at the same time.

Derisis13 · 2023-01-03T12:09:57Z

ani-cli

+		    if flatpak info io.mpv.Mpv 2>&1 | grep -q 'error'; then
+			    die "Program \"$dep\" not found. Please install it."
+            fi
+        fi


I don't understand the logic here, it appears to me that you check for flatpak mpv whenever any dependency is missing. This would be bad because if flatpak mpv is present, it would mask out a missing dependency, eg. grep.
I'm afraid flatpak programs need to be handled separately, if I understand this correctly

oh, you are right. Its a bad code

MrSmoer · 2023-01-04T16:05:55Z

Could also you add line numbers to the search results? V4 is awesome, thanks for your work!

port19x · 2023-01-05T09:04:29Z

Could also you add line numbers to the search results? V4 is awesome, thanks for your work!

Should be pretty easy. We'd just need to pipe through nl before the fzf output

port19x · 2023-01-05T09:48:20Z

I'd like to not squash all the commits into one when merging to master this time.
Instead I'm proposing that I squash the commits to sensible chunks and then add the whole maintainership team as coauthors via the commit message.
The coauthorship is kind of a gimmick, but accurately reflects our collaborative development process imo.
On top of that, it will give all of us Pair Extraordinaire x3.
That would also contribute to the github achievements research done by schweinepriester

Objections?

justchokingaround · 2023-01-05T09:54:01Z

im in favor

port19x · 2023-01-05T09:54:14Z

Seeing as basic functionality works now and most rough edges are smoothed out I'd like to invite testing by the broader userbase. Ready for review

Terraro53 · 2023-01-05T12:16:19Z

Trying to use iina on MacOS results in:"Error Cannot open file or stream"
looking back at previous version, iina had --no-stdin and --keep-running passed.
However I've found only --no-stdin is necessary.
As such
` play_episode() {

...
mpv) nohup "$player_function" --force-media-title="${allanime_title}episode-${ep_no}-${mode}" "$episode" >/dev/null 2>&1 & ;;
iina) nohup "$player_function" --no-stdin --force-media title="${allanime_title}episode-${ep_no}-${mode}" "$episode" >/dev/null 2>&1 & ;;
...`
Fixes this

Derisis13 · 2023-01-05T15:03:54Z

Seeing as basic functionality works now and most rough edges are smoothed out I'd like to invite testing by the broader userbase. Ready for review

No, not ready. Android is broken, history is missing. I'm against releasing incomplete code. This is a volunteer project, there are no deadlines so we should take our time. It'll be worth it for all the issues we won't get because of the incomplete state.

A compromise could be to update the readme with a statement of v4, instructions on how to upgrade and a list of features still missing.

port19x · 2023-01-05T15:05:31Z

history is missing

Uh, nvm then.
Thats a pretty major feature.
I'd be fine with android support being implemented parallel to general review, but history is a pretty significant hole in out featureset

avecpps · 2023-01-05T19:21:12Z

Will V4 support watching the dubbed versions of shows?

CoolnsX · 2023-01-05T19:22:29Z

Will V4 support watching the dubbed versions of shows?

Yus by using dedicated flag or environment variable...

umop3plsdn · 2023-01-07T01:58:16Z

i just wanna say i'm in love with v4 have had no real hiccups to write about! the only thing i wish it had right now was the ability to fetch the dubs. but wanted to chime in and say absolutely love it and can't wait for actual release!

Derisis13 · 2023-01-07T12:12:41Z

i just wanna say i'm in love with v4 have had no real hiccups to write about! the only thing i wish it had right now was the ability to fetch the dubs. but wanted to chime in and say absolutely love it and can't wait for actual release!

You already can by setting the environment variable ANI_CLI_MODE to dub (either with export ANI_CLI_MODE="dub" or with env ANI_CLI_MODE="dub" ani_cli)

ircurry · 2023-01-08T17:06:02Z

It works even better than V3 and I love the fzf interface. The only thing I miss is the ability to resume my place in the series but I know that is being worked on. Thank you for creating such an awesome script!

archeite · 2023-01-09T01:55:19Z

You already can by setting the environment variable ANI_CLI_MODE to dub (either with export ANI_CLI_MODE="dub" or with env ANI_CLI_MODE="dub" ani_cli)

Will there be any documentation on environment variables for ani-cli?

Derisis13 · 2023-01-23T00:20:59Z

Also I forgot from the review that the manpage is not up-to-date, packagers are so gonna screw that one up (I don't even know what to do...)

well that's out of the way

archeite

There's some misspellings (Such as "Controll" having 2 ls), but everything so far looks good to me

ani-cli.1

C0ff33W1thB4c0n · 2023-01-23T09:58:47Z

Could anyone change the vsersion on the PNG
.assets/ani-cli-spec.png & CONTRIBUTING.md#Architecture
It's still v3.4!

justchokingaround · 2023-01-23T10:01:13Z

yup, you're correct, nice catch, i'll fix it in 10 mins

(edit: fixed)

port19x · 2023-01-23T11:40:07Z

Today I'll do:

thorough testing
thorough code review
another rebase
(in that order)

The merge is getting closer

port19x · 2023-01-23T12:38:33Z

I can now confirm that all features documented via -h work and do so as intended.

Only thing I can complain about is that we dropped -D for deleting history.
Unless there is a good rationale for that it should be implemented.

justchokingaround · 2023-01-23T12:40:24Z

-D for dmenu aka external menu (not implemented yet, but for future reference). but then we can just use smtg like --delete or --delete-history, that shouldn't be a problem

port19x · 2023-01-23T12:43:47Z

-D for dmenu aka external menu (not implemented yet, but for future reference). but then we can just use smtg like --delete or --delete-history, that shouldn't be a problem

That is handled with the ANI_CLI_EXTERNAL_MENU env-var, so I see no point in reserving that.

We should think about how we document our advanced features that are accessible via env-vars only.
I'm unsure if they belong in regular -h

justchokingaround · 2023-01-23T12:46:31Z

it's documented in the man page

port19x · 2023-01-23T12:48:04Z

it's documented in the man page

Thats the perfect place for it 👍

README.md

port19x · 2023-01-23T13:09:39Z

README.md

-Note : Vlc Android now works too ;)
+For players you can use the apk (playstore/fdroid) versions of mpv and vlc. Note that these cannot be checked from termux so a warning is generated when checking dependencies.
+
+### Steam Deck


I agree. Having all this in the main readme is a bit much.
<details> could also be a solution, so it's hidden by default but can be folded out.

port19x · 2023-01-23T13:16:42Z

ani-cli

+search="${ANI_CLI_DEFAULT_SOURCE:-scrape}"
+
+while [ $# -gt 0 ]; do
+	case "$1" in


Or maybe that could be a fallback?
Not sure how you would handle that gracefully.
Currently the only option is to set it via envvar.

Co-authored-by: port19 <port19@port19.xyz> Co-authored-by: Derisis13 <email.felhasznalasra@gmail.com> Co-authored-by: CoolnsX <ansari.tan20@gmail.com> Co-authored-by: chokerman <44473782+justchokingaround@users.noreply.github.com> Co-authored-by: zenith71 <71zenith@protonmail.com>

+ docs: remove old deps and add syntax to code blocks + docs: acknowledging history disappearence + docs: missing fzf in brew installation + docs: windows instructions for v4 + docs: termux instructions for v4 Co-authored-by: port19 <port19@port19.xyz> Co-authored-by: Derisis13 <email.felhasznalasra@gmail.com> Co-authored-by: CoolnsX <ansari.tan20@gmail.com> Co-authored-by: chokerman <44473782+justchokingaround@users.noreply.github.com> Co-authored-by: zenith71 <71zenith@protonmail.com>

port19x · 2023-01-23T14:44:19Z

Rebase done.
This time without any merge conflicts, so no regressions.
Remember to pull with --rebase

Derisis13 · 2023-01-23T23:28:06Z

Can we merge now?

archeite

Forgot to create additional comments for other misspellings

ani-cli.1

+ docs: remove v3.4 stuff + docs: dmenu -> rofi dmenu (dmenu doesn't support multi) + docs: remove "new in v4" Co-authored-by: archeite <121004047+archeite@users.noreply.github.com> Co-authored-by: port19 <port19@port19.xyz> Co-authored-by: Derisis13 <email.felhasznalasra@gmail.com> Co-authored-by: CoolnsX <ansari.tan20@gmail.com> Co-authored-by: chokerman <44473782+justchokingaround@users.noreply.github.com> Co-authored-by: zenith71 <71zenith@protonmail.com>

Co-authored-by: port19 <port19@port19.xyz> Co-authored-by: Derisis13 <email.felhasznalasra@gmail.com> Co-authored-by: CoolnsX <ansari.tan20@gmail.com> Co-authored-by: chokerman <44473782+justchokingaround@users.noreply.github.com> Co-authored-by: zenith71 <71zenith@protonmail.com>

The version variable inside the script should always be major.minor.patch major.minor releases are reserved for native packages and tags/releases Co-authored-by: port19 <port19@port19.xyz> Co-authored-by: Derisis13 <email.felhasznalasra@gmail.com> Co-authored-by: CoolnsX <ansari.tan20@gmail.com> Co-authored-by: chokerman <44473782+justchokingaround@users.noreply.github.com> Co-authored-by: zenith71 <71zenith@protonmail.com>

port19x requested a review from Derisis13 December 25, 2022 01:17

port19x assigned justchokingaround, CoolnsX and port19x Dec 25, 2022

ShiNoNeko47 reviewed Dec 29, 2022

View reviewed changes

Derisis13 reviewed Jan 2, 2023

View reviewed changes

ani-cli Outdated Show resolved Hide resolved

Derisis13 reviewed Jan 3, 2023

View reviewed changes

port19x marked this pull request as ready for review January 5, 2023 09:53

mindstormjak mentioned this pull request Jan 5, 2023

Error : Episodes not released yet! While it should have Episodes #966

Closed

port19x marked this pull request as draft January 5, 2023 15:04

Derisis13 mentioned this pull request Jan 7, 2023

Place history file in $XDG_STATE_HOME #923

Closed

archeite suggested changes Jan 23, 2023

View reviewed changes

ani-cli.1 Outdated Show resolved Hide resolved

ani-cli.1 Outdated Show resolved Hide resolved

ani-cli.1 Outdated Show resolved Hide resolved

port19x commented Jan 23, 2023

View reviewed changes

port19x and others added 9 commits January 23, 2023 15:36

port19x force-pushed the v4 branch from c32e470 to a9570f9 Compare January 23, 2023 14:42

archeite suggested changes Jan 24, 2023

View reviewed changes

Derisis13 and others added 3 commits January 24, 2023 08:50

port19x force-pushed the v4 branch from 34af1b6 to a669ec4 Compare January 24, 2023 07:50

port19x merged commit 0d672fd into master Jan 24, 2023

port19x deleted the v4 branch January 24, 2023 08:37

ani-cli V4 #955

ani-cli V4 #955

Conversation

port19x commented Dec 25, 2022 • edited by 71zenith

Pull Request Template

Type of change

Description

Checklist

Additional Testcases

Derisis13 commented Dec 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Derisis13 commented Dec 30, 2022 • edited

Derisis13 Jan 2, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

port19x commented Jan 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrSmoer commented Jan 4, 2023

port19x commented Jan 5, 2023

port19x commented Jan 5, 2023

justchokingaround commented Jan 5, 2023

port19x commented Jan 5, 2023

Terraro53 commented Jan 5, 2023 • edited

Derisis13 commented Jan 5, 2023 • edited

port19x commented Jan 5, 2023 • edited

avecpps commented Jan 5, 2023

CoolnsX commented Jan 5, 2023

umop3plsdn commented Jan 7, 2023

Derisis13 commented Jan 7, 2023 • edited

ircurry commented Jan 8, 2023

archeite commented Jan 9, 2023

Derisis13 commented Jan 23, 2023

archeite left a comment

Choose a reason for hiding this comment

C0ff33W1thB4c0n commented Jan 23, 2023

justchokingaround commented Jan 23, 2023 • edited

port19x commented Jan 23, 2023 • edited

port19x commented Jan 23, 2023

justchokingaround commented Jan 23, 2023

port19x commented Jan 23, 2023

justchokingaround commented Jan 23, 2023

port19x commented Jan 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

port19x commented Jan 23, 2023

Derisis13 commented Jan 23, 2023

archeite left a comment

Choose a reason for hiding this comment

port19x commented Dec 25, 2022 •

edited by 71zenith

Derisis13 commented Dec 30, 2022 •

edited

Derisis13 Jan 2, 2023 •

edited

Terraro53 commented Jan 5, 2023 •

edited

Derisis13 commented Jan 5, 2023 •

edited

port19x commented Jan 5, 2023 •

edited

Derisis13 commented Jan 7, 2023 •

edited

justchokingaround commented Jan 23, 2023 •

edited

port19x commented Jan 23, 2023 •

edited