Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: added menu for downloading subtitles from opensubtitles.com #756

Merged
merged 2 commits into from
Oct 31, 2023

Conversation

tomasklaen
Copy link
Owner

@tomasklaen tomasklaen commented Oct 29, 2023

subs

New menu command download-subtitles which can also be opened by clicking on the Download option in the subtitles menu.

It uses a new ziggy binary which needs to be build with tools/build ziggy command (haven't tested outside windows btw). If you don't want to set up go, you can download binaries from here, and extract them into src/uosc/bin (you'll probably have to mark linux/mac binaries as executable).

ziggy will also hash the file and send the hash to Open Subtitles, so you can search even with empty query and if your file is known, you'll get results exactly for it.

The subtitle file is downloaded into the same directory as currently opened file, or ~~/subtitles if URL is being played.

The uosc's Open Subtitles API key is currently set to development mode, so we have 100 global downloads per day. Try not to blow through it when testing :) But it seems like downloading same file over and over doesn't decrement the remaining downloads by more than one, so do that.

closes #490

@Zabooby
Copy link

Zabooby commented Oct 29, 2023

Not sure which method is better, but would it be possible to add this script into uosc?

If not it would be nice to see an option to automatically download subtitles if none are detected.

@tomasklaen
Copy link
Owner Author

Not sure which method is better, but would it be possible to add this script into uosc?

Why does it need to be added to uosc? If you want mpv-autosub functionality, than just install and use mpv-autosub... uosc is a UI, and this PR extends the UI with searching, browsing, and downloading subtitles on demand.

@Zabooby
Copy link

Zabooby commented Oct 29, 2023

Why does it need to be added to uosc? If you want mpv-autosub functionality, than just install and use mpv-autosub... uosc is a UI, and this PR extends the UI with searching, browsing, and downloading subtitles on demand.

I was asking if integrating mpv-autosub would be easier than to start from scratch.

@po5
Copy link
Contributor

po5 commented Oct 30, 2023

Why is "Subtitles loaded & applied" and other info in the menu? Would make more sense as a toast or osd message.

@tomasklaen
Copy link
Owner Author

Why is "Subtitles loaded & applied" and other info in the menu? Would make more sense as a toast or osd message.

Since the open subtitles limits are quite low (5 downloads per day by default) I wanted to give people a chance to read them and close at their leisure. But I guess it makes sense to cancel the input field on the last window.

@dyphire
Copy link
Contributor

dyphire commented Oct 31, 2023

I think adding some new script APIs and using them as third-party scripts is much better than directly integrating subtitle downloads, but this may be very challenging.

I really don't like the practice of placing binary files in the script directory. I suggest adding a new option of ziggy_path that allows users to specify paths. The ziggy_path=ziggy will finding executable files next to the mpv.exe program or in environment variables on Windows, search it in the system PATH at Linux or Mac.

@tomasklaen
Copy link
Owner Author

I didn't make it a 3rd party script because I realized I'd have to write a standalone UI so it works outside of uosc, and I really didn't feel like doing that. And since it'll only work with uosc, it might as well be part of it.

I also don't want to make ziggy optional. I intend it to become a multitool that'll allow us to do stuff we can't do from within mpv's lua environment.

I really don't like the practice of placing binary files in the script directory. I suggest...

So make everything more complicated, and add additional installation steps? And what would be the benefit of that? It just adds unnecessary installation steps, undoing my primary goals for making ziggy in go:

  • no dependencies on external runtimes
  • no additional installation steps or setups
  • everything just works

People already put python files in their mpv scrips. This is pretty much the same thing it just doesn't depend on external runtime.

@dyphire
Copy link
Contributor

dyphire commented Oct 31, 2023

I really don't like the practice of placing binary files in the script directory. I suggest...

So make everything more complicated, and add additional installation steps? And what would be the benefit of that? It just adds unnecessary installation steps, undoing my primary goals for making ziggy in go:

  • no dependencies on external runtimes
  • no additional installation steps or setups
  • everything just works

The default value of ziggy_path=~~/scripts/uosc/bin will do the same thing, but do not hardcode the path. This way, users who need it can manually change the options to avoid binary files being placed in the script folder.
This is just an improvement without adding complex or additional operations, as there are no changes by default.

@tomasklaen
Copy link
Owner Author

This way, users who need it can manually change the options to avoid binary files being placed in the script folder.

Are you going to change ziggy_path and copy & paste the bin folder to a different place after every uosc update? 😬 ... why?

New menu command `download-subtitles` which can also be opened by clicking on the **Download** option in the `subtitles` menu.

It uses a new `ziggy` binary which needs to be build with `tools/build ziggy` command.

ziggy will also hash the file and send the hash to Open Subtitles, so you can search even with empty query and if your file is known, you'll get results exactly for it.

The subtitle file is downloaded into the same directory as currently opened file, or `~~/subtitles` if URL is being played.
@dyphire
Copy link
Contributor

dyphire commented Oct 31, 2023

I didn't make it a 3rd party script because I realized I'd have to write a standalone UI so it works outside of uosc, and I really didn't feel like doing that. And since it'll only work with uosc, it might as well be part of it.

There is already a third-party script that is only applicable to uosc: recent-menu, and I don't think it's bad.
The reason for raising this point is that I believe subtitle downloading is an optional feature, and the necessity of integrating it is not enough. Especially I noticed that there are API restrictions here.

@dyphire
Copy link
Contributor

dyphire commented Oct 31, 2023

This way, users who need it can manually change the options to avoid binary files being placed in the script folder.

Are you going to change ziggy_path and copy & paste the bin folder to a different place after every uosc update? 😬 ... why?

As I said, the behavior of placing binary files in the script folder troubles me. You can understand it as a form of obsessive-compulsive disorder.

Edit: If you don't want to add this option, it's okay. I can modify it myself, it's not complicated.

@tomasklaen tomasklaen merged commit 542f0db into main Oct 31, 2023
@gunir
Copy link

gunir commented Oct 31, 2023

Great feature, honestly not being able to get subtitle easily was always the downside of MPV vs VLC which has VLCSub.lua plugin which download subtitles from OpenSubtitles, thanks for intergrating it into uosc!

This is so great and if it's easy to extend, maybe we should allow people to add new subs sources.

@AndydeCleyre
Copy link

I'm excited for this!

The error messages could be more informative, at least if reading terminal output. My naive testing of the new feature always reports "process exited with code -3 error" in the mpv window when I submit a search, but I don't know what that signifies.

Will it be possible to use our own opensubtitles login to increase query limits?

@po5
Copy link
Contributor

po5 commented Nov 1, 2023

Will it be possible to use our own opensubtitles login to increase query limits?

Already possible with the open_subtitles_api_key script-opt, which isn't documented for whatever reason.
For tomas: If it's trying to obscure the builtin key from the user, make an empty string default to that key. That way it's not in the config file and can be silently updated from your side if the need arises.

@tomasklaen
Copy link
Owner Author

tomasklaen commented Nov 1, 2023

Will it be possible to use our own opensubtitles login to increase query limits?

As far as I understand from reading the open subtitles docs, authenticating users bumps the limit from 5 to a WHOLE 10, so I didn't feel like it was worth spending the time implementing it. You can increase it further by having some VIP accounts or something, but how many people will do that...

And 5 downloads per day isn't that bad, since if you need more than 5, you should probably just deal with it in the browser beforehand so you don't have to bother every time a new episode starts. But I do agree that it's insanely small for what this is. Even the search result payloads are often bigger than the downloaded subtitles, and those are unlimited and need database queries to put together.

Already possible with the open_subtitles_api_key script-opt, which isn't documented for whatever reason.
For tomas: If it's trying to obscure the builtin key from the user, make an empty string default to that key. That way it's not in the config file and can be silently updated from your side if the need arises.

The open_subtitles_api_key variable is internal config that is not exposed to user options. The way open subtitles work is the API keys are per app, and users should be authenticated with their username:password pairs. This is annoying to implement (especially since iirc the session lasts only 24 hours), and bumps the limit only to 10, so I didn't bother.

The user could theoretically make their own API key on open subtitles, set it to development mode (which gives you 100 downloads per day global, shared with all IPs using it), and use it only for themselves, but that feels like being a bad consumer, and I don't want open subtitles to start blocking our user agent, so the API key will remain non-configurable. But of course I can't stop someone from modifying the source code :)

In other words, I don't want to do stuff that doesn't adhere to open subtitles rules and guidelines.

maybe we should allow people to add new subs sources.

Afaik there are no other subtitle services with an API endpoints, and I'm not maintaining html serializers and trying to work around rate limiting, captchas, and stuff like that.

@tomasklaen
Copy link
Owner Author

tomasklaen commented Nov 1, 2023

The error messages could be more informative, at least if reading terminal output. My naive testing of the new feature always reports "process exited with code -3 error" in the mpv window when I submit a search, but I don't know what that signifies.

When error happens, try looking in console (backtick key), it should have more info (whole stdout + stderr), since I didn't want to put full error messages into the one line menu item title. I'll add some "see console for details" appendix to the UI message.

And I assume the message means you didn't have binaries built?

If you have go installed, you can just run:

tools/build ziggy

If not, here they are: bin.zip

They need to be placed in uosc/bin directory. And you need to mark binaries as executable if not on windows.

@AndydeCleyre
Copy link

Thanks! When the error occurs, a red [uosc] line is added, but nothing else on it. Oh yes, I didn't actively build any go code, so I'll do it now: no, I couldn't use the build script because the elif on line 17 has no corresponding if.

@tomasklaen
Copy link
Owner Author

because the elif on line 17 has no corresponding if

This was fixed yesterday. Pull latest commit from main.

@AndydeCleyre
Copy link

Sorry, thanks it works great!

@tomasklaen tomasklaen deleted the subs branch November 4, 2023 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Online subtitle search
6 participants