-
Notifications
You must be signed in to change notification settings - Fork 380
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
[Bandcamp] Support loading additional comments #1030
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.
Thank you very much for this PR!
Please apply the following changes and add @Nonnull
and @Nullable
annotations where that's relevant in the code you added and changed.
You will also have to wait a review from @fynngodau.
The Bandcamp tests failures from the CI doesn't seem to be related to your changes (timeouts).
...ava/org/schabi/newpipe/extractor/services/bandcamp/extractors/BandcampCommentsExtractor.java
Outdated
Show resolved
Hide resolved
...ava/org/schabi/newpipe/extractor/services/bandcamp/extractors/BandcampCommentsExtractor.java
Outdated
Show resolved
Hide resolved
...ava/org/schabi/newpipe/extractor/services/bandcamp/extractors/BandcampCommentsExtractor.java
Outdated
Show resolved
Hide resolved
...ava/org/schabi/newpipe/extractor/services/bandcamp/extractors/BandcampCommentsExtractor.java
Outdated
Show resolved
Hide resolved
...ava/org/schabi/newpipe/extractor/services/bandcamp/extractors/BandcampCommentsExtractor.java
Outdated
Show resolved
Hide resolved
Hello @petlyh, many thanks for opening this PR. I'm happy to see that you have taken a look at this old Bandcamp issue! Finding a track, not an album, with many comments is indeed quite tricky! I found this one, with three pages (one initial + two): https://laurashigihara.bandcamp.com/track/cube-land |
Thank you so much! I was now able to verify that loading more pages after the second page works as expected :-) |
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.
Code looks good to me, except for the things already pointed out. I don't know anything about Bandcamp though.
...ava/org/schabi/newpipe/extractor/services/bandcamp/extractors/BandcampCommentsExtractor.java
Outdated
Show resolved
Hide resolved
(FYI, I'm going to post an approving review once the points from the other reviews are addressed) |
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.
Thank you for applying the review comments, and many thanks from my side yet again!
No NewPipe changes required.
The Bandcamp service now supports loading multiple pages of comments.
Closes #621
Initial comments are now extracted from the
data-blob
attribute of the#collectors-data
element, with subsequent pages of comments being fetched from the API. The way initial comments are being extracted was changed to:CommentsInfoItemExtractor
for both the initial page and subsequent pages.Note: I was unable to find a track with more than 2 pages of comments to test.
Before:
![](https://user-images.githubusercontent.com/88139840/220964281-acb58848-29a7-4f96-9c8b-28f098c0075e.png)
After:
after.mp4