-
Notifications
You must be signed in to change notification settings - Fork 420
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
[SoundCloud] Add support for comment replies #993
base: dev
Are you sure you want to change the base?
Conversation
75d875f
to
9ae1be7
Compare
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.
Looks mostly good to me, but it looks like at SoundCloud they overcomplicated comment replies 😅
...bi/newpipe/extractor/services/soundcloud/extractors/SoundcloudCommentsInfoItemExtractor.java
Outdated
Show resolved
Hide resolved
} | ||
} | ||
replyCount = replies.size(); | ||
if (collector.getItems().isEmpty()) { |
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.
I guess you are collecting items here to make sure not to show that there are replies when actually all replies give an error when collected.
- Since you are already collecting replies here, can't you set the items as the
repliesPage
content directly instead of passing thejson
again? Or maybe make aSerializable
structure of some sort. To avoid recollecting them again later. - Or even better, I would skip this check completely and avoid collecting items beforehand. If one of the comments yields an error while being collected, then the error would be shown in the frontend and it is expected that in case of error on an item the list of replies might be empty.
When getting a page which is not the initial page there it is possible that the first comments are replies to a comment from a previous page.
9ae1be7
to
e5be686
Compare
Implemented a cache. TODO: Do not store in cache when viewing replies....
I agree to create a pull request for NewPipe as soon as possible to make it compatible with the changed API.Not needed. Is done in Show comment replies NewPipe#9410
Follow up on #936 and #941
To Do: