-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
plugins.crunchyroll: update/fix #4510
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you're changing the _access_type
from the Android app to the Windows desktop app, it's probably not a bad idea to change the _user_agent
(or alternatively, import from the existing user agents)
Initially, I thought what you wrote about updating the user agent was a good idea, but there are also a lot of other Android related headers sent at the moment too. The thing is, I can't test this plugin properly myself as I don't have an account with Crunchyroll to do so. #4453 (comment) suggests that these changes alone result in a working plugin, so I'm reluctant to make changes without being able to test fully. I could look at the Windows app traffic and copy the UA and headers from it so that the plugin looks and acts like the app, although I'll need to get folks in #4453 to help test it fully. |
Some things to note:
The HTTP client in the Windows app does not send many headers. It doesn't send It seems like there has been a minor bug since the refactor in #1820 which caused the I've tested this on a free stream, but cannot test the login and therefore access to premium content. I will ask if folks can test it fully in #4453. Hopefully that will all still work correctly, as little has changed and the request sending the login data is the same as that used by the Windows app. |
I've added the I've added some code (not yet pushed to GH) to also try to close #3674, which will need testing with a selection of beta URLs, as I only have access to those in the issue. I've also updated the tests to use Hopefully not too much more to do. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While your changes will fix the plugin issue according to the user responses, there's still a ton of things wrong with the current plugin implementation, because it hasn't been properly maintained and updated in years. The plugin should get rewritten from scratch at some point in the future, to fix all the other issues. Fixing the current issues should be good for now though.
fa8a935
to
ca115a4
Compare
* update params for Windows app * remove Android headers * fix device_id cached value * add id to metadata * minor spelling corrections * add support for beta URLs * update tests * update cache.set() calls to use expires kw * update expires_at=0 to expires=0
@mkbloke Do you want to wait for some responses to your question in regards to the site's beta version, or do you think that the current changes are good enough and can get merged into master? |
A couple of people have reported feedback on the beta URL support in issue #4453 so far to say that it's working for them, so, I think the code is probably OK to be merged as is, but I'm also happy to wait a bit longer for more testing and possible feedback on it. |
closes #3674
closes #4453