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

Fix Hidive - Subtitles and Age-restriction. #5828

Merged
merged 5 commits into from
Feb 12, 2023

Conversation

chexxor
Copy link
Contributor

@chexxor chexxor commented Dec 19, 2022

IMPORTANT: PRs without the template will be CLOSED

Description of your pull request and other information

Currently the Hidive extractor is broken:

  • It doesn't extract subtitles.
  • It doesn't select a Hidive profile, if the user has multiple profiles defined. This applies age-restriction to the content.

After login, Hidive redirects your browser to a profile-selection page. On that page, you need to click the button/icon you want to use, which sends a JSON request to the server which sets the profile id as a cookie.

Also, the subtitle information is no longer available (from what I've seen) in a "data-captions" element on the page, but they _are_available in the "rendition" data structure that Hidive gives us.

Fixes:

Before submitting a pull request make sure you have:

In order to be accepted and merged into yt-dlp each piece of code must be in public domain or released under Unlicense. Check one of the following options:

  • I am the original author of this code and I am willing to release it under Unlicense
  • I am not the original author of this code but it is in public domain or released under Unlicense (provide reliable evidence)

What is the purpose of your pull request?

yt_dlp/extractor/hidive.py Outdated Show resolved Hide resolved

for cc_file in rendition.get('ccFiles', []):
cc_url = url_or_none(try_get(cc_file, lambda x: x[2]))
# name is used since we cant distinguish subs with same language code
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pls explain

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the encouragement to look into that!
I improved the comments in the new commit I made.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was asking to explain it to me - coz I don't understand this code/comment. You shouldn't use long name as language code. subtitles have a separate name field

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With it as-is, here's what --list-subs shows:

Language     Formats
english-caps vtt
english-subs vtt

I have removed that comment in the latest version of this file -- it didn't make sense to me, either. Instead, I listed out example values in that data structure.

Do you still want me to make any changes here? This comment is on an outdated version.

yt_dlp/extractor/hidive.py Outdated Show resolved Hide resolved
yt_dlp/extractor/hidive.py Outdated Show resolved Hide resolved
yt_dlp/extractor/hidive.py Outdated Show resolved Hide resolved
yt_dlp/extractor/hidive.py Outdated Show resolved Hide resolved
yt_dlp/extractor/hidive.py Outdated Show resolved Hide resolved
@pukkandan pukkandan added site-bug Issue with a specific website pending-fixes PR has had changes requested labels Dec 19, 2022
@chexxor
Copy link
Contributor Author

chexxor commented Dec 20, 2022

@pukkandan Thank you for your code review! I'm not familiar with Python, so I apologize for the dumb mistakes I made. I'll get around to fixing them sometime soon.

@chexxor
Copy link
Contributor Author

chexxor commented Jan 1, 2023

@pukkandan Your suggestions were excellent -- thanks for the code review! I learned a lot about Python while investigating and making those improvements.

@pukkandan pukkandan removed the pending-fixes PR has had changes requested label Jan 1, 2023
yt_dlp/extractor/hidive.py Outdated Show resolved Hide resolved
yt_dlp/extractor/hidive.py Outdated Show resolved Hide resolved
yt_dlp/extractor/hidive.py Outdated Show resolved Hide resolved
yt_dlp/extractor/hidive.py Outdated Show resolved Hide resolved
yt_dlp/extractor/hidive.py Outdated Show resolved Hide resolved
yt_dlp/extractor/hidive.py Outdated Show resolved Hide resolved
yt_dlp/extractor/hidive.py Outdated Show resolved Hide resolved
@chexxor
Copy link
Contributor Author

chexxor commented Jan 9, 2023

Thanks for your suggested changes! I think you're much more experienced with regex than I am -- I'll review your changes, apply it and test it to ensure it still works as expected.

@chexxor
Copy link
Contributor Author

chexxor commented Jan 9, 2023

After I made the changes you suggested, I saw that GitHub has a button to commit your changes directly to my branch -- I didn't do that, so I'm sorry if that bothered you. I'm also not sure whether you prefer I click the "Resolve" button on the items you raised, or whether you do that.

I did test these changes, and it seems to still work as expected!

@pukkandan
Copy link
Member

After I made the changes you suggested, I saw that GitHub has a button to commit your changes directly to my branch -- I didn't do that, so I'm sorry if that bothered you.

Either is fine as long as the changes get made 👍

I'm also not sure whether you prefer I click the "Resolve" button on the items you raised, or whether you do that.

I'll resolve them

@pukkandan
Copy link
Member

Pls reply to #5828 (comment)

Everything else looks good

@pukkandan pukkandan merged commit 7708df8 into yt-dlp:master Feb 12, 2023
aalsuwaidi pushed a commit to aalsuwaidi/yt-dlp that referenced this pull request Apr 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
site-bug Issue with a specific website
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants