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

[SoundCloud] Fix getting more comments #943

Merged
merged 1 commit into from
Oct 29, 2022
Merged

[SoundCloud] Fix getting more comments #943

merged 1 commit into from
Oct 29, 2022

Conversation

TobiGr
Copy link
Contributor

@TobiGr TobiGr commented Oct 11, 2022

  • I carefully read the contribution guidelines and agree to them.
  • I have tested the API against NewPipe.
  • I agree to create a pull request for NewPipe as soon as possible to make it compatible with the changed API.

Previously, trying to fetch the second page of comments resulted in errors like the one below:

Exception

Crash log

org.schabi.newpipe.extractor.exceptions.ParsingException: URL not accepted: https://api-v2.soundcloud.com/tracks/1358538436/comments?client_id=0B4jtZF6rFh5TG0eiQ1nuJfbsGk9FZFg&threaded=0&filter_replies=1
	at org.schabi.newpipe.extractor.linkhandler.LinkHandlerFactory.fromUrl(LinkHandlerFactory.java:79)
	at org.schabi.newpipe.extractor.linkhandler.ListLinkHandlerFactory.fromUrl(ListLinkHandlerFactory.java:40)
	at org.schabi.newpipe.extractor.linkhandler.ListLinkHandlerFactory.fromUrl(ListLinkHandlerFactory.java:34)
	at org.schabi.newpipe.extractor.StreamingService.getCommentsExtractor(StreamingService.java:278)
	at org.schabi.newpipe.extractor.comments.CommentsInfo.getMoreItems(CommentsInfo.java:74)
	at org.schabi.newpipe.extractor.comments.CommentsInfo.getMoreItems(CommentsInfo.java:67)
	at org.schabi.newpipe.util.ExtractorHelper.lambda$getMoreCommentItems$8(ExtractorHelper.java:167)
	at org.schabi.newpipe.util.ExtractorHelper.$r8$lambda$DLEFpIAjy5YVZt97lKfLttNB3h4(Unknown Source:0)
	at org.schabi.newpipe.util.ExtractorHelper$$ExternalSyntheticLambda14.call(Unknown Source:6)
	at io.reactivex.rxjava3.internal.operators.single.SingleFromCallable.subscribeActual(SingleFromCallable.java:43)
	at io.reactivex.rxjava3.core.Single.subscribe(Single.java:4813)
	at io.reactivex.rxjava3.internal.operators.single.SingleSubscribeOn$SubscribeOnObserver.run(SingleSubscribeOn.java:89)
	at io.reactivex.rxjava3.core.Scheduler$DisposeTask.run(Scheduler.java:644)
	at io.reactivex.rxjava3.internal.schedulers.ScheduledRunnable.run(ScheduledRunnable.java:65)
	at io.reactivex.rxjava3.internal.schedulers.ScheduledRunnable.call(ScheduledRunnable.java:56)
	at java.util.concurrent.FutureTask.run(FutureTask.java:264)
	at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:307)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1137)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:637)
	at java.lang.Thread.run(Thread.java:1012)


@TobiGr TobiGr added bug Issue is related to a bug soundcloud service, https://soundcloud.com/ labels Oct 11, 2022
Comment on lines +35 to +37
if (Parser.isMatch(API_URL_PATTERN, url)) {
return Parser.matchGroup1(API_URL_PATTERN, url);
}
Copy link
Member

Choose a reason for hiding this comment

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

Aren't you parsing the url with the regex twice here? I think you can return matchGroup1 and sorround it with a try-catch

Copy link
Contributor Author

@TobiGr TobiGr Oct 29, 2022

Choose a reason for hiding this comment

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

I just benchmarked both approaches, it seems that it does not make a difference:

Time needed for 100000 iterations: 224 ms using current implementation
Time needed for 100000 iterations: 227 ms using try & catch

That was a surprise for me, because throwing exceptions can be a quite heavy task. I guess that's not the case here, because we have a flat hierarchy in my tests.
If you want, I can change the implementation anyway.

Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

Ok, lgtm then, thanks!

@TobiGr TobiGr merged commit 3d31416 into dev Oct 29, 2022
@TobiGr TobiGr deleted the fix/sc/comments branch October 29, 2022 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is related to a bug soundcloud service, https://soundcloud.com/
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants