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 long playlists (100+ videos) #1911

Merged
merged 11 commits into from
Mar 23, 2021

Conversation

SamantazFox
Copy link
Member

This should fix issue #1883.
I've also made some changes in src/invidious/channels.cr because both channel and playlist videos require a call to [YT]/youtubei/v1/browse, so I made a common wrapper (it will ease future updates if the key or client version strings change).

@SamantazFox

This comment has been minimized.

@TheFrenchGhosty
Copy link
Member

What are the differences with #1909 ?

PS: CI must be fixed.

@TheFrenchGhosty TheFrenchGhosty added the unfinished More work is needed on this PR, or on something this PR uses. label Mar 21, 2021
@SamantazFox
Copy link
Member Author

SamantazFox commented Mar 21, 2021

What are the differences with #1909 ?

All playlists mentonned there work, being 300 or 5k videos ones and the continuation token was fixed too (so we can have pages >= 2)

PS: CI must be fixed.

Yup, I know. I'll go on for the "dummy request index" (option 1), then, so if YT does take that into account in the future, that won't be a problem.

PS: Also I don't care about the bounty.

@TheFrenchGhosty
Copy link
Member

All playlists mentonned there work, being 300 or 5k videos ones and the continuation token was fixed too (so we can have pages >= 2)

Well, this is great then!

Yup, I know. I'll go on for the "dummy request index" (option 1), then, so if YT does take that into account in the future, that won't be a problem.

Alright, I'll wait for this to be done before I ask saltycrys to review.

PS: Also I don't care about the bounty.

You're sure? You deserve it. If you really don't want it, we can send it to someone else you choose if you want.

@TheFrenchGhosty TheFrenchGhosty added need-code-review A crystal developper need to check if the code is correct. need-testing This feature needs to be deployed and tested to see if it's working, and doesn't break something and removed unfinished More work is needed on this PR, or on something this PR uses. labels Mar 21, 2021
@SamantazFox
Copy link
Member Author

SamantazFox commented Mar 21, 2021

Alright, I'll wait for this to be done before I ask saltycrys to review.

It's fixed, all CI tests pass now :)

You're sure? You deserve it. If you really don't want it, we can send it to someone else you choose if you want.

I don't really like cryptos, tbh. Plus I stardted working on it before any bounty were added.

I know someone who was very excited about that bounty, tho :P

@TheFrenchGhosty TheFrenchGhosty removed the request for review from saltycrys March 21, 2021 21:53
src/invidious/channels.cr Outdated Show resolved Hide resolved
@TheFrenchGhosty
Copy link
Member

I'll surely test this PR tonight, if everything works I'll merge it.

@SamantazFox what is your decision about the reward?

@TheFrenchGhosty TheFrenchGhosty removed the need-code-review A crystal developper need to check if the code is correct. label Mar 22, 2021
@TheFrenchGhosty TheFrenchGhosty added unfinished More work is needed on this PR, or on something this PR uses. and removed need-testing This feature needs to be deployed and tested to see if it's working, and doesn't break something labels Mar 22, 2021
@SamantazFox
Copy link
Member Author

SamantazFox commented Mar 22, 2021

@TheFrenchGhosty Ah, yeah, I didn't fix the HTML template nor the paging code.

Oh, and BTW, I don't have page 11. It stops at page 10 for me. Maybe some videos are geo-blocked?

@SamantazFox
Copy link
Member Author

Here we go. That's fixed :)

@SamantazFox SamantazFox requested a review from saltycrys March 23, 2021 02:28
@TheFrenchGhosty TheFrenchGhosty added need-code-review A crystal developper need to check if the code is correct. need-testing This feature needs to be deployed and tested to see if it's working, and doesn't break something and removed unfinished More work is needed on this PR, or on something this PR uses. labels Mar 23, 2021
@TheFrenchGhosty TheFrenchGhosty added in-testing This feature has been deployed and is being tested and removed need-code-review A crystal developper need to check if the code is correct. need-testing This feature needs to be deployed and tested to see if it's working, and doesn't break something labels Mar 23, 2021
@TheFrenchGhosty
Copy link
Member

Oh, and BTW, I don't have page 11. It stops at page 10 for me. Maybe some videos are geo-blocked?

I test that on my instance on https://pussthecat.org so it's in Germany. It's possible that it's geo-bocked in France yeah.

@TheFrenchGhosty
Copy link
Member

TheFrenchGhosty commented Mar 23, 2021

Tested: works perfectly!

The fact that when going over the page number you get redirect to the last page:

if page > page_count
is really cool too.

@TheFrenchGhosty TheFrenchGhosty merged commit c481ca9 into iv-org:master Mar 23, 2021
@TheFrenchGhosty
Copy link
Member

Merged.

Thanks for the review @saltycrys

Thanks @SamantazFox and @tenpura-shrimp

If you really don't want the reward we'll surely just use this money for another bounty.

@TheFrenchGhosty TheFrenchGhosty removed the in-testing This feature has been deployed and is being tested label Mar 23, 2021
@SamantazFox
Copy link
Member Author

The fact that when going over the page number you get redirect to the last page is really cool too.

glad you like it ^^

@TheFrenchGhosty
Copy link
Member

TheFrenchGhosty commented Mar 23, 2021

@SamantazFox I mean, it's a clever way to limit it without displaying an "error" page, and actually useful. So yeah, it's cool! :)

@TheFrenchGhosty
Copy link
Member

TheFrenchGhosty commented Mar 24, 2021

@SamantazFox I think this PR is breaking comments by adding ?format=html&hl=en-US&thin_mode=false at the end of the API url.

#1926

@github-actions
Copy link

github-actions bot commented Jun 8, 2021

This pull request has been automatically locked since there has not been any activity in it in the last 30 days. If you want to tell us about needed or wanted changes or if problems related to this code are discovered, feel free to open an issue or a new pull request.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 8, 2021
@TheFrenchGhosty TheFrenchGhosty added the bounty:refused This bounty's reward has been refused by the person who were supposed to receive it label Oct 31, 2021
@SamantazFox SamantazFox deleted the fix-long-playlists branch November 21, 2021 14:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bounty:refused This bounty's reward has been refused by the person who were supposed to receive it bounty
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants