search

jellyfin / jellyfin-plugin-opensubtitles

https://jellyfin.org
GNU General Public License v3.0
121 stars 26 forks source link

Switch to new API #54

Closed MBR-0001 closed 3 years ago

MBR-0001 commented 3 years ago

This PR replaces the old XML API handler with a new one that uses the new OpenSubtitles REST API and also adds login info validation

Instructions for those who decide to use/test this:

Fixes #59, fixes #58, fixes #53, fixes #46, fixes #41, fixes #28, fixes #25, fixes #22, closes #24, closes #49

h1dden-da3m0n commented 3 years ago

I know this is still a draft, but mind me asking if you have any particular need to rename the OpenSubtitlesHandler instead of only updating the code within it to use the new OpenSubtitles REST API?

MBR-0001 commented 3 years ago

well I kinda just made the new REST handler and then deleted the old one, I didn't modify it, I could rename it if needed though

also, I've been working on login info validation

https://user-images.githubusercontent.com/55142207/123542615-d8378100-d74a-11eb-87dc-6e0fd39ae8c0.mp4

It works by doing a request to the API from inside the browser, is this a problem? (should it maybe be done on the server side?)

h1dden-da3m0n commented 3 years ago

Hey @MBR-0001,

Thank you for your response! the validation looks slick, however if you could move it to the server instead of the web client that would be ideal. After all, its the server that needs to connect to opensubtitles.com not the client in the end, so it should be validated that it can actually connect.

Regarding the naming, personally I am in favour of renaming it back to not include REST.

Other than that, thank you for your contribution so far and feel free to give me or the rest of the plugin team a shout if there is anything you are uncertain about or once you are done :wink:

MBR-0001 commented 3 years ago

@h1dden-da3m0n it looks like it is not possible to do config validation on the server, it can only respond with http response 204, I could make it just not save the config if the info is bad but then user wouldn't know about it

crobibero commented 3 years ago

You could always add an endpoint to do the testing.

martijnmelchers commented 3 years ago

I've also made a pull request on your fork with some improvements in search and I removed some helped functions which weren't needed. I've tested it on my server with about ~200 movies and ~400 episodes and it works great, however the initial version you provided had some issues. See https://github.com/MBR-0001/jellyfin-plugin-opensubtitles/pull/3

MBR-0001 commented 3 years ago

wow I don't know how I didn't notice the PR, I'll check it out, thanks

crobibero commented 3 years ago

In case you didn't know, marking a PR from Draft -> Ready for review doesn't notify us. So this doesn't get forgotten, please just ping me when it's ready for review!

MBR-0001 commented 3 years ago

@crobibero ready for review

gabri94 commented 3 years ago

I've tested it on my jellyfin instance and I get this error (and no subtitle):

[23:10:15] [ERR] Error downloading subtitles
System.Net.Http.HttpRequestException: Subtitle with Id 3711500 could not be downloaded: InternalServerError
   at Jellyfin.Plugin.OpenSubtitles.OpenSubtitleDownloader.GetSubtitlesInternal(String id, CancellationToken cancellationToken) in /home/gabriel/git/jellyfin-plugin-opensubtitles/Jellyfin.Plugin.OpenSubtitles/OpenSubtitleDownloader.cs:line 264
   at MediaBrowser.Providers.Subtitles.SubtitleManager.DownloadSubtitles(Video video, LibraryOptions libraryOptions, String subtitleId, CancellationToken cancellationToken)
   at Jellyfin.Api.Controllers.SubtitleController.DownloadRemoteSubtitles(Guid itemId, String subtitleId)
crobibero commented 3 years ago

InternalServerError

This means that OpenSubtitles returned a 500 - InternalServerError, nothing we can do to fix it.

gabri94 commented 3 years ago

Weird, i can download the same subtitle from the website without any problem