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.

Sign up for GitHub

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

Jump to bottom

Fix yt comments and add disabled comments functionallity #652

Merged
merged 5 commits into from
Jul 12, 2021
Merged

Fix yt comments and add disabled comments functionallity #652

merged 5 commits into from
Jul 12, 2021

Conversation

litetex
Copy link
Member

@litetex litetex commented Jun 13, 2021 •
edited
Loading

Fixes #577 (ASAP field was set in march lol)
Also fixes TeamNewPipe/NewPipe#5769

Changes:

  • Fixes ParsingException when comments disabled #577
    • If no continuation token is sent, the comments are disabled
    • The findValue in YoutubeCommentsExtractor was faulty: If no "start" was found (e.g. continuation":" for the continuation token) it searched nearly everything, found apple-itunes-app (from <!DOCTYPE html><head><meta name="apple-itunes-app" content="app-id=544007664, app-argume... and used that as continuation-token, which resulted in the response of an internal server error (500) by YT's servers and caused NewPipe to show the error
  • Adds a new field to the commentsExtractor disabledComments which is true, when the comments are disabled.
  • Code clean up, according to the NewPipe Checkstyle config (added a lot of missing final keywords)
  • Added comments

Notice:

This was referenced Jun 13, 2021
Closed
Merged
Stypox
Stypox requested changes Jun 15, 2021
View reviewed changes
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.

At a first glance it looks fine

.../java/org/schabi/newpipe/extractor/services/youtube/extractors/YoutubeCommentsExtractor.java Show resolved Hide resolved
B0pol
B0pol reviewed Jun 17, 2021
View reviewed changes
Copy link
Member

@B0pol B0pol left a comment •
edited
Loading

Choose a reason for hiding this comment

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

  • PeerTube: commentsEnabled field in video info
  • SoundCloud: commentable field in video info
  • BandCamp: ?
  • media.ccc.de: comments not implemented

Given SoundCloud and PeerTube give that info in video info and not comments, how should it be handled (I'm also talking to other reviewers)?

@Stypox
Copy link
Member

Stypox commented Jun 18, 2021

My proposal is to add getCommentsExtractor() to StreamInfo.

For services where comments and stream info are completely independent, like YouTube, it would just return the same thing as what is currently used, i.e. YouTubeService.getCommentsExtractor(url) (see CommentsInfo#getInfo(service, url)).

For the other services, like SoundCloud and PeerTube, it would:

  • if comments are disabled, return a custom DisabledCommentsExtractor that does nothing except returning true when isCommentsDisabled() is called.
  • otherwise, return the same CommentsExtractor as the current service.getCommentsExtractor(url)

Even though service.getCommentsExtractor(url) would lack some information (i.e. whether comments are disabled or not for that particular service), it should still return a CommentsExtractor that is able to work acceptably without that information (i.e. maybe provide an empty list of comments or try to extract whether comments are disabled or not in another way).

@litetex litetex force-pushed the fixYTCommentsAndAddDisabledComments branch from d4fd64d to 3b264f3 Compare June 19, 2021 13:08
@litetex
Copy link
Member Author

litetex commented Jun 20, 2021

@Stypox
@B0pol

Should I change anything for now?

I have no idea how to implement Stypox's comment the best way without breaking e.g. backwards compatibility, caching (I'm not sure if it's a good idea to return the extractor maybe just a supplier how to build it) and so on.
I also have not found videos/music with disabled comments at Soundcloud/Peertube.

I think a simple new field in StreamInfo e.g. Optional<Boolean> hasComments could also do the job:

  • isEmpty → We don't know if the video has comments or not → the "user" (code using this) has to ask the CommentsExtractor
  • true/false → Video has (no) comments

Anyway currently I see two options:

  1. We implement this PR for now and postpone the implementation of PeerTube, SoundCloud, etc (→ new issue and PR)
    CommentsExtractor#isCommentsDisabled could be marked as "experimental and may change".
  2. We add the changes all in once, however due to the fact that - as mentioned above - I have no idea what's best so someone else with more experience should better work on this

I personally would go with option one to not create a PR "monolith"-monster.

@litetex litetex force-pushed the fixYTCommentsAndAddDisabledComments branch from 3b264f3 to 9316f1f Compare June 20, 2021 18:30
@litetex litetex mentioned this pull request Jun 23, 2021
Closed
@litetex litetex force-pushed the fixYTCommentsAndAddDisabledComments branch from 9316f1f to 676f647 Compare June 27, 2021 12:46
@Stypox
Copy link
Member

Stypox commented Jul 5, 2021

Ok, let's go with option 1. Is this ready to be reviewed again?

@litetex
Copy link
Member Author

litetex commented Jul 6, 2021

@Stypox

Is this ready to be reviewed again?

It never was not ready 😄

Should I mark the new methods as experimental and may change in the future?

@litetex
Improved code format (final) and added isCommentsDisabled method
ca33f4f
@litetex
Fixed Exception when YT comments are disabled and added ``commentsDis…
688a1c3 
…abled`` field

* Fixed code: Added missing finals (according to NewPipes Checkstyle guide)
* Fixed ``findValue`` method in ``YoutubeCommentsExtractor``
@litetex
Fixed typos
a59fead
@litetex
Added more doc
6860543
@litetex litetex force-pushed the fixYTCommentsAndAddDisabledComments branch from 676f647 to 6860543 Compare July 6, 2021 19:16
Stypox
Stypox requested changes Jul 7, 2021
View reviewed changes
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.

Should I mark the new methods as experimental and may change in the future?

Good idea

Other than that, it looks good to me

.../java/org/schabi/newpipe/extractor/services/youtube/extractors/YoutubeCommentsExtractor.java Show resolved Hide resolved
@litetex litetex mentioned this pull request Jul 7, 2021
Closed
@litetex
Copy link
Member Author

litetex commented Jul 7, 2021

Done

@Stypox Stypox mentioned this pull request Jul 8, 2021
Merged
8 tasks
@TobiGr TobiGr merged commit b45bb41 into TeamNewPipe:dev Jul 12, 2021
@litetex litetex deleted the fixYTCommentsAndAddDisabledComments branch July 12, 2021 19:17
@litetex litetex mentioned this pull request Jul 23, 2021
Closed
3 tasks
@Stypox Stypox mentioned this pull request Aug 3, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@B0pol B0pol B0pol left review comments

@Stypox Stypox Stypox approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ParsingException when comments disabled Newpipe generates crash reports when comments are disabled
4 participants
@litetex @Stypox @B0pol @TobiGr