Skip to content

Changes for "Show Replies"#739

Closed
golfinq wants to merge 3 commits intoTeamNewPipe:devfrom
golfinq:dev
Closed

Changes for "Show Replies"#739
golfinq wants to merge 3 commits intoTeamNewPipe:devfrom
golfinq:dev

Conversation

@golfinq
Copy link
Contributor

@golfinq golfinq commented Oct 13, 2021

A small change needed for the associated pull request in NewPipe (TeamNewPipe/NewPipe#7244), which stores whether or not the replies have been cached to prevent re-downloading

  • 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.

@Redirion
Copy link
Member

what would speak against doing this directly in NewPipe instead?

@golfinq
Copy link
Contributor Author

golfinq commented Oct 14, 2021

@Redirion I tried it, and I ran into the NetworkOnMainThreadException and I am not familiar with android threads

@litetex
Copy link
Member

litetex commented Oct 15, 2021

@Redirion I tried it, and I ran into the NetworkOnMainThreadException and I am not familiar with android threads

https://stackoverflow.com/questions/6343166/how-can-i-fix-android-os-networkonmainthreadexception

TL;DR
Don't load network related stuff on the main thread and use someting that does it async instead 😉

Btw: I'm pretty sure that this PR will cause problems regarding the comment loading performance and resources required for holding the replies in memory.

@TobiGr
Copy link
Contributor

TobiGr commented Oct 15, 2021

@litetex We have JavaRX do to that.

@golfinq
Copy link
Contributor Author

golfinq commented Oct 17, 2021

@TobiGr Oh my god, that is what I was missing. I'll see if I can incorporate RXJava to not have to cache the replies to comments. I am doing some reading to learn about it here: https://github.com/ReactiveX/RxJava/tree/3.x/docs

Copy link
Member

@FireMasterK FireMasterK left a comment

Choose a reason for hiding this comment

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

I don't understand why you want to do it this way.

How I imagined it to be used was:

CommentsInfo info = CommentsInfo.getInfo(...);

// Only called when the user clicks on show more
CommentsInfo replies = info.getReplies() != null ? CommentsInfo.getMoreItems(info, info.getReplies()) : null;

Comment on lines 105 to 117
try {
if (resultItem.getReplies() != null && serviceId == YouTube.getServiceId()) {
final YoutubeCommentsExtractor youtubeCommentsExtractor =
(YoutubeCommentsExtractor) YouTube.getCommentsExtractor(
resultItem.getReplies().getUrl());

resultItem.setRepliesInfoList(
youtubeCommentsExtractor.getPage(extractor.getReplies()).getItems());
}
} catch (Exception e) {
addError(e);
}

Copy link
Member

Choose a reason for hiding this comment

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

Won't this eagerly get replies for every single comment?

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct.

Copy link
Member

Choose a reason for hiding this comment

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

This is a blocker. Replies to a comment should only be fetched on demand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean including during scrolling? I have changed the current behavior to download it once the user asks for replies then keep it in case of scrolling on the NewPipe Side

@golfinq golfinq force-pushed the dev branch 2 times, most recently from 2f3e19c to 2fb1aa1 Compare October 20, 2021 09:46
@golfinq
Copy link
Contributor Author

golfinq commented Oct 20, 2021

I don't understand why you want to do it this way.

How I imagined it to be used was:

CommentsInfo info = CommentsInfo.getInfo(...);

// Only called when the user clicks on show more
CommentsInfo replies = info.getReplies() != null ? CommentsInfo.getMoreItems(info, info.getReplies()) : null;

This was quite helpful in getting off the ground, thanks!

Comment on lines +153 to +158

public void setRepliesOpen(boolean repliesOpen) {
this.repliesOpen = repliesOpen;
}

public boolean getRepliesOpen() {return this.repliesOpen; }
Copy link
Member

Choose a reason for hiding this comment

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

This should not be done in the Extractor code, since this is just a UI thing

@opusforlife2
Copy link
Collaborator

opusforlife2 commented May 8, 2022

Closing this for the same reason as TeamNewPipe/NewPipe#7244.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants