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

Comment replies #8443

Closed
wants to merge 35 commits into from
Closed

Comment replies #8443

wants to merge 35 commits into from

Conversation

DUOLabs333
Copy link

What is it?

  • Bugfix (user facing)
  • Feature (user facing)
  • Codebase improvement (dev facing)
  • Meta improvement to the project (dev facing)

Description of the changes in your PR

A continuation of #7244

Before/After Screenshots/Screen Record

  • Before:
  • After:

Fixes the following issue(s)

  • Fixes #

Relies on the following changes

APK testing

The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR.

Due diligence

@Stypox
Copy link
Member

Stypox commented May 27, 2022

@DUOLabs333 I think you should write code and build it in a proper IDE, like Android Studio or Intellij. The build process already tells you of checkstyle issues, for example, and I doubt it is doable to do the big requested changes by changing files not within an IDE. But thank you for the PR :-)

@DUOLabs333
Copy link
Author

Here's one --- https://github.com/DUOLabs333/NewPipe/suites/6825870032/artifacts/262855249. It is nowhere near ready though.

@InfinityLoop1308
Copy link

It is nowhere near ready though.

I tried it and it works well.

However, even if we don't use a separate UI, there are still some big problems with this --- one, it is possible to click and no replies show up, and two, it often doesn't list all comments. I am not sure if this is due to some limit. I did try to not load all comments' replies at once, but only when you click on them.

It is probably caused by the wrong implement in Youtube service in NewPipeExtractor, since the "getReplies" function is written long ago and may need adaption to fit in your code. I tried it in my service and did not encounter such problems.

@DUOLabs333
Copy link
Author

I tried it and it works well.

What I meant is that the planned UI hasn't been implemented yet.

@zeref-dragneel
Copy link

zeref-dragneel commented Jun 17, 2022

It would be great if this feature was available in the new version. Sometimes I want to see the response to a comment on NewPipe.

@DUOLabs333
Copy link
Author

Sorry, it will come, but probably not for the next few versions (unless someone want to implement the comments page UI seen on YouTube).

@zeref-dragneel
Copy link

zeref-dragneel commented Jun 17, 2022

The idea is that adding more lines is great

@DUOLabs333
Copy link
Author

DUOLabs333 commented Jun 17, 2022

What I meant is that the replies will be on its own page (like the Youtube app, not like the Youtube website, or to be honest, most other mobile apps).

@opusforlife2
Copy link
Collaborator

@DUOLabs333 No time to work on this? Or some other reason?

@DUOLabs333
Copy link
Author

No, it's just without the UI implementation, the PR is stuck indefinitely.

@foliagecanine
Copy link

@DUOLabs333
Context (you can skip):
I've been following along with a few PRs that come and go about the comment replies, since this is a feature I use all the time on YouTube.
For my own personal use, I fixed up golfinq's changes a bit to fix the lack of a "Hide Replies" button (which it looks like you did too) and RecyclerView having incorrect replies tacked onto other comments due to the replies being recycled to a new comment.
I daily drive this version, and am pretty happy with it, but I'll have to merge in the changes from 0.23.1 soon (I need DASH).

But anyways, my question is did you ever get more than 10 replies to load? If so, could you point me to the function/lines?
Once I have this, it'll have all the functionality I need for my fork. Maybe I'll submit a PR too.

Thanks!

@DUOLabs333
Copy link
Author

DUOLabs333 commented Jul 5, 2022

No, as that is a problem with the Extractor (and would probably have to implement lazy loading in the frontend).

@golfinq
Copy link

golfinq commented Jul 6, 2022

To load the next page of comments, you make a request with the same url as the replies with the continuation header replaced by the continuationcommand token from the "show more replies" field in the JSON response.

Here is the token source
replies_source

and here it is being used to get more replies
replies_request_r

The extractor might already recognize this and put it somewhere, but otherwise getting the next page will require editing of the extractor

@DUOLabs333
Copy link
Author

DUOLabs333 commented Jul 6, 2022

If someone was to hypothetically work on this, what would be the best way to implement this? You obviously can't just get any arbitrary page --- maybe commentsInfoItemInfoItemsPage.getItems() gets an argument that will get this and the next page, so you can just loop until you get an empty list? Not sure how you would keep the page you're on, though.

@golfinq
Copy link

golfinq commented Jul 6, 2022

Hmmm, maybe. The code seems to already mostly exists here:

public Page getReplies() throws ParsingException {
    try {
        final String id = JsonUtils.getString(
                JsonUtils.getArray(json, "replies.commentRepliesRenderer.contents")
                        .getObject(0),
                "continuationItemRenderer.continuationEndpoint.continuationCommand.token");
        return new Page(url, id);
    } catch (final Exception e) {
        return null;
    }

So it looks like you can create a new Page just from the URL and token, then feed the Page object to the ListExtractor to exploit the machinary here.
It also looks like the actual parsing happens here with the creation of the actualList variable so it's possible everything got preserved and the last element in that list already has that data. I would want more experienced opinions on what the code is doing though.

@DUOLabs333
Copy link
Author

I'll try to work on the Extractor to get more replies (it seem relatively simple, and something that can get merged in a reasonable amount of time).

@DUOLabs333
Copy link
Author

@golfinq From the look of the code mentioned, it seems that all of the work is already done --- just call getReplies multiple times to get more replies. Is there something I'm missing?

@golfinq
Copy link

golfinq commented Sep 25, 2022 via email

@golfinq
Copy link

golfinq commented Sep 25, 2022 via email

@DUOLabs333
Copy link
Author

DUOLabs333 commented Sep 25, 2022

Oh, I see the problem. Page returns its own thing, so there's no way to get the JSON of a returned page. What needs to happen is that there should be a new variable (continuation maybe), that is updated at each getReplies. Can someone from the team comment to see if this is a good idea?

@DUOLabs333
Copy link
Author

I've been thinking about this, and a way to move forward with this without having to build a new UI would be to have at the bottom of the replies list tow buttons: "Load more replies", and "Collapse"/"Hide". This would allow an easy integration of continuation without having to implement lazy-loading, as well as prevent having a big list of un-collapsable replies. What does the team think?

@DUOLabs333
Copy link
Author

For those that are not aware, this PR has been superseded by #9410 by the original author.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issue is related to a feature in the app
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants