pystardust / ani-cli

A cli tool to browse and play anime
GNU General Public License v3.0
7.44k stars 530 forks source link

Episode timestamp tracking script which also picks where left off & removes episodes from tracker file when completed #1213

Closed c2700 closed 10 months ago

c2700 commented 10 months ago

Pull Request Template

Type of change

Description

justchokingaround commented 10 months ago

although you put a lot of effort, i am against the integration of an additional lua script

justchokingaround commented 10 months ago
mpv "$link" 2>&1 | tee /tmp/ani-cli-position
stopped_at=$(sed -nE "s@.*AV: ([0-9:]*) / ([0-9:]*) \(([0-9]*)%\).*@\1@p" "/tmp/ani-cli-position"  | tail -1)
percentage_progress=$(sed -nE "s@.*AV: ([0-9:]*) / ([0-9:]*) \(([0-9]*)%\).*@\3@p" "/tmp/ani-cli-position" | tail -1)

this would be a preferrable implementation (requires additional history tinkering ofc) ^^^

c2700 commented 10 months ago

although you put a lot of effort, i am against the integration of an additional lua script

would you have preferred an ELF binary that would communicate over a socket? I mean, I DID try a socket communication, only thing is I'm not so sure of how it would behave in windows. looked around & it requires the winsock library which isn't there for linux (as far as I have looked around i.e.)

c2700 commented 10 months ago
mpv "$link" 2>&1 | tee /tmp/ani-cli-position
stopped_at=$(sed -nE "s@.*AV: ([0-9:]*) / ([0-9:]*) \(([0-9]*)%\).*@\1@p" "/tmp/ani-cli-position"  | tail -1)
percentage_progress=$(sed -nE "s@.*AV: ([0-9:]*) / ([0-9:]*) \(([0-9]*)%\).*@\3@p" "/tmp/ani-cli-position" | tail -1)

this would be a preferrable implementation (requires additional history tinkering ofc) ^^^

the history tinkering is also present in the lua script....just that it's commented cuz I would've liked to know how you wold want it to be implemented

justchokingaround commented 10 months ago

the core philosophy of ani-cli is minimalism so an external script/binary it relies on is something we are against

justchokingaround commented 10 months ago

i do however like the idea of a separate file that would track timestamps history

c2700 commented 10 months ago

the core philosophy of ani-cli is minimalism so an external script/binary it relies on is something we are against

oh, okay, so that means that I should've made changes in the ani-cli script itself right?

justchokingaround commented 10 months ago

yes, and no lua please (iirc it's not available by default on some linux distros)

c2700 commented 10 months ago

yes, and no lua please (iirc it's not available by default on some linux distros)

okay, lua not being available on some distros is news to me. but okay, I got you. make changes on the shell script it is

justchokingaround commented 10 months ago

both my windows 11 and debian VMs tell me the lua command isn't recognized

c2700 commented 10 months ago

both my windows 11 and debian VMs tell me the lua command isn't recognized

got it sir. working on the script itself then

port19x commented 10 months ago

yes, and no lua please (iirc it's not available by default on some linux distros)

*most

port19x commented 10 months ago

I can appreciate the effort put forth, but +200 loc is unacceptable. Also lua isn't going into the codebase, stick to shell or standalone dependencies please.

I don't think I 100% understand the semantics as you put them in the description, but I'm generally also in favour of additional time tracking in a state file. Let's see where we can get with shell. I'd expect somwehere around 30-60loc addition with maybe a commonly packaged helper dependency

port19x commented 10 months ago

Best case, if this is some straight forward 10-20 loc addition, we can merge. I'll have to see some of the details regarding the tracking semantics before judging a 20-50 loc addition.

50 loc becomes a hard sell.

port19x commented 10 months ago

missclick, sry

justchokingaround commented 10 months ago

apology rejected

c2700 commented 10 months ago

Best case, if this is some straight forward 10-20 loc addition, we can merge. I'll have to see some of the details regarding the tracking semantics before judging a 20-50 loc addition.

50 loc becomes a hard sell.

got it, any other conditions I might need to know? asking as I'm unaware

c2700 commented 10 months ago

I like the approach of adding this functionality as an optional script for mpv, what I don't like is hosting it in our repository. We're focusing here on shell development, having to maintain Lua code would be outside of our scope.

I recommend you upload your script to your own repo (that way you can take proper credit and responsibility for your work) and we'll merge only the most necessary changes to ani-cli to allow for loading custom mpv scripts in one way or another.

We can even have a link to your repo in our readme to promote your work that way - in case it works well enough.

all inputs taken

justchokingaround commented 10 months ago

are we closing this?

c2700 commented 10 months ago

are we closing this?

Well since my changes didn't follow your guidelines I thought "why not close this PR, make the changes in the main script & open another one?". Or would you have this one left open?

justchokingaround commented 10 months ago

ah ok. i prefer a new PR if u dont mind pls

c2700 commented 10 months ago

ah ok. i prefer a new PR if u dont mind pls

Sure. That was the idea anyway