search

Roshan-R / termv

A terminal iptv player written in bash
GNU General Public License v3.0
232 stars 21 forks source link

Add swallow support (based on devour) and prevent termv execution from changing PWD #7

Closed Kabouik closed 3 years ago

Kabouik commented 3 years ago

Submitting just in case because I have it ready anyway, but no worries if you think this is out of scope for termv. I think keeping termv execution from changing directory would be welcome though. I didn't test in many situations, but it seemed to work fine and adequately detect if devour is in $PATH.

I couldn't make devour to work when using Fish shell though, but I'm new to Fish and maybe devour is simply not compatible.

Roshan-R commented 3 years ago

Hey @Kabouik thanks for the PR! I really like the idea for adding devour's functionality into termv, but I feel implementing it with a flag like -d would be better.

Not changing directory is a really welcome addition

Kabouik commented 3 years ago

I absolutely agree, it would be better to make it a flag. There are some cases where one would want to be able to disable it even if it's installed, for instance to see what's happening when buffering is slow, or to see the error mpv is encountering if playback doesn't start.

Additionally, I recently started (to try) using Fish instead of Bash, and on the computer where I use Fish as default, devour doesn't seem to work. I haven't yet really looked into whether this is related or not.

Also I found a limitation with the PR I submitted. In master, when mpv is closed, termv and its fzf list are resumed, while I noticed termv is terminated when I quit mpv in my fork.

I'll try to have a look into it over the week-end hopefully.

On 2021-05-20 07:46 Roshan R Chandar @.***> wrote:

Hey @Kabouik thanks for the PR! I really like the idea for adding devour's functionality into termv, but I feel implementing it with a flag like -d would be better.

Not changing directory is a really welcome addition

--
You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/Roshan-R/termv/pull/7#issuecomment-844718010

Kabouik commented 3 years ago

This is not necessarily the final candidate PR, I need to test it on other machines. Also I included some simple devour code directly into termv to avoid the dependency, but I'm not quite sure all functionality is there, as I see the devour author later published a version written in C and not just bash.

Kabouik commented 3 years ago

That's what it looks like on my end, but don't pay too much attention to the end when closing mpv in -s mode: the terminal is immediately restored but it looks like it didn't because I use a tiling WM and was recording in floating mode, and the terminal window reappeared in tiling mode, hence why the fzf text was out of the frame.

https://user-images.githubusercontent.com/7107523/119186903-dd811c00-ba78-11eb-973b-c576a5635daf.mp4

Sorry it's a bit slow, even if I edited to shorten the buffering time because this was recorded on my phone in a Debian LXC container, not exactly the best performance (no hardware acceleration).

The issue I was having in Fish shell is fixed.

Roshan-R commented 3 years ago

Wow, awesome work!

Are we using xdo instead of devour for swallowing the terminal now? Im not really familiar with xdo, but it looks really good

Kabouik commented 3 years ago

As far as I understand the first devour version was just a bash script using xdo, while the newer version is written in C (which I can't read, sorry). Since xdo is probably installed on most systems using X11 (I think), I thought it would be best to include part of the code directly because it's just a few lines and would not need extra deps in most cases. And yes, it's mostly about using base features of xdo. I left the reference to devour (old version) still because I've taken the idea from it.

We might miss some more advanced features of the newer devour, but they are probably related to the more diverses applications and file names it can be used with, while here we only need it to work with mpv and channel URLs never have spaces.

On 2021-05-22 07:16 Roshan R Chandar @.***> wrote:

Wow, awesome work!

Are we using xdo instead of devour for swallowing the terminal now? Im not really familiar with xdo, but it looks really good

--
You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/Roshan-R/termv/pull/7#issuecomment-846352172

Roshan-R commented 3 years ago

Sorry I'm responding late, I got really busy xdo seems to work fine for our usecase and I'm glad you brought that up I'm wondering if we should add a timer or something in case it takes a while for mpv to realize if a stream does not exist.

Anyways, awesome work @Kabouik !

Kabouik commented 3 years ago

I was thinking the same, because unfortunately there's a lot of georestricted channels with IPTV, so failure to stream is common. However I'm not exactly sure how to implement that, I'm not exactly good at scripting. :> I'll do a bit of research.

I think ideally it should be a variable too, because it may depend on connection speed. Like termv -s 10 with a default value of 20 seconds maybe.

Thanks for your kind words! I have verified without xdo and got the error message as expected, and I had someone try on Mac without X11 and it seemed to work without -s too.

On 2021-05-23 11:33 Roshan R Chandar @.***> wrote:

Sorry I'm responding late, I got really busy xdo seems to work fine for our usecase and I'm glad you brought that up I'm wondering if we should add a timer or something in case it takes a
while for mpv to realize if a stream does not exist.

Anyways, awesome work @Kabouik !

--
You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/Roshan-R/termv/pull/7#issuecomment-846533212

Kabouik commented 3 years ago

Found some solutions to add a timeout but they depend on coreutils. I'm currently looking into alternatives in Bash, or even POSIX, to minimize unnecessary dependencies.

Kabouik commented 3 years ago

Actually no, solutions are not that easy because the mpv process is started whether or not a window is opened, so we need a way to discriminate when mpv is fetching the stream and when playback is on. We could look for open windows but this would break if other mpv windows are already open. There is a --network-timeout option in mpv but sadly it is broken due to limitations for the ffmpeg API, according to a dev.

Kabouik commented 3 years ago

This might actually be more convenient, yet more minimal. What do you think? Ideally we could make mpv show an overlay like "Fetching stream, please wait…" and hide it when playback starts, which would require detecting when playback starts, but this could perhaps come in another PR.

On 2021-05-23 11:33 Roshan R Chandar @.***> wrote:

Sorry I'm responding late, I got really busy xdo seems to work fine for our usecase and I'm glad you brought that up I'm wondering if we should add a timer or something in case it takes a
while for mpv to realize if a stream does not exist.

Anyways, awesome work @Kabouik !

--
You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/Roshan-R/termv/pull/7#issuecomment-846533212

Roshan-R commented 3 years ago

wow mpv's --force-window=immediate looks like a really handy feature, i think this would suffice for now, I'll check if I can do the overlay