woodruffw / ff2mpv

A Firefox/Chrome add-on for playing URLs in mpv.
https://addons.mozilla.org/en-US/firefox/addon/ff2mpv/
Other
521 stars 52 forks source link

Small PR for Error Feedback using Alert #68

Closed benyaminl closed 2 years ago

benyaminl commented 2 years ago

Hello @woodruffw, I hope this help a little, I tried to implement the error feedback, so people can see the terminal output in alert() on respected page, I hope it simplify the option as I don't think we need notification permission (?) because as far as the user on the page, it will see the alert, just as far as the user doesn't dismiss it.

--no-terminal flag as no effect on windows, so I dismiss it, as https://github.com/yt-dlp/yt-dlp return error doesn't shown on pOut (stdout), when this flags on, so I hope this doesn't affect Linux or *nix/Darwin/macOS box (hopefully)

This method employ https://docs.python.org/3/library/subprocess.html#subprocess.Popen.communicate

Related to #40

Note : I only test this on Windows 10 21H2 for now, with Python 3.8 (as Python 3.6, 3.7 failed to run, so best to assume 3.8 is the minimum requirement). I will tried to test on my Linux box next week on Office.

Example : image

woodruffw commented 2 years ago

Thanks for the PR. However, I'm inclined to pass on this, sorry. My reasoning:

TL;DR: This feature is too invasive and brittle for the functionality that it introduces. If you want notifications, I suggest modifying your local copy of the native client (ff2mpv or ff2mpv.py) to include code that produces a native notification. That's what I have locally, and it looks like this (in Ruby):

require "json"

len = STDIN.read(4).unpack1("L")
data = JSON.parse(STDIN.read(len))
url = data["url"]

args = %w[--no-terminal]

system "notify-send", "ff2mpv", url
pid = spawn "mpv", *args, "--", url, in: :close, out: "/dev/null", err: "/dev/null"

Process.detach pid
benyaminl commented 2 years ago

@woodruffw Will you receive it if I change it to Firefox Notification? I think it's not hard, just I'm experimenting with this. Or you intend to implement error on Python or Ruby side code, like you have done before on your own script, as Python has notify2/py-notifier, well we can also use msgbox from vbs or windows 10 notification.

woodruffw commented 2 years ago

Will you receive it if I change it to Firefox Notification?

Not at this time, no, sorry. I'd still prefer to keep the extension's permissions to a bare minimum.

At this time, I'd prefer it if individual users made changes to their local copies of the native client to accommodate their own notification needs. My justification there is that cross-platform notifications are hard to do without introducing dependencies, which in turn makes it harder for novice users to correctly install the extension and client correctly. And harder installation means more issues, maintenance burden, etc.