- 
          
- 
                Notifications
    You must be signed in to change notification settings 
- Fork 497
[SoundCloud] Add support for comment replies #993
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
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 repliesPagecontent directly instead of passing thejsonagain? Or maybe make aSerializablestructure 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: