Show comment replies #7244

golfinq · 2021-10-14T05:45:32Z

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

Adds a "Show Replies" for the comments with replies for Youtube; when clicked, the first page of replies is displayed below the comment.

Before/After Screenshots/Screen Record

(Ignore the different themes)

Before:

After:

Before Clicking:

After Clicking:

AudricV

This review is from a quick look at your changes. I will not review your PR.

app/src/main/res/layout/list_comments_item.xml

TobiGr · 2021-10-14T14:40:20Z

How do you load more reply pages? If I understand the code correctly, the current implementation only shows the first one.

app/src/main/res/values-en-rGB/strings.xml

Co-authored-by: Mohammed Anas <[email protected]>

golfinq · 2021-10-15T03:23:14Z

How do you load more reply pages? If I understand the code correctly, the current implementation only shows the first one.

@TobiGr That is correct, I just implemented showing the first page of replies. I am not sure how I would go about showing the next page in actual implementation. I don't know where the next page URL is and I wouldn't be able to download it without running into NetworkOnMainThreadException

I figured out how to avoid the NetworkOnMainThreadException, but haven't figured out a good way to load additional pages

golfinq · 2021-10-22T17:57:14Z

I have the core functionality up and running

BratishkaErik · 2021-11-02T18:07:44Z

Can't wait to see this in app!

opusforlife2 · 2021-11-03T19:32:18Z

@BratishkaErik if you have nothing to add to the discussion, please just react with a thumbs up to the PR.

golfinq · 2021-11-04T15:33:59Z

Is there anything that reviewers/maintainers want from me/this pull request in order to get merged? I don't have a free time until next weekend to work on anything, but if there is anything to help this PR, it would be appreciated to know.

opusforlife2 · 2021-11-04T15:46:12Z

@golfinq We just need a team dev to get enough free time to review this PR.

Stypox

As I explain in the comments the structure needs some work, but it's nothing too difficult after all. Thank you for the PR :-)

Stypox · 2021-11-13T20:22:55Z

app/src/main/res/layout/list_comments_item.xml

+        android:textAppearance="@style/TextAppearance.AppCompat.Small"
+        android:textSize="@dimen/video_item_search_upload_date_text_size" />
+
+    <androidx.recyclerview.widget.RecyclerView


If I understood correctly TeamNewPipe/NewPipeExtractor#703, a reply-comment could have replies itself, but this PR introduces support only for 1 level of reply depth. This would work fine for YouTube, but it would be problematic for other services, so the replyRecyclerView should be added dynamically. Doing this might not be best practice, since the layouts recycled in recycler views should not be built dynamically, but since the amount of comments we expect users to open is much smaller than the total number of comments, I don't think creating a few more views every time an expanded comment is shown would hurt performance whatsoever. Note: if you do this remember to cleanup the recycler view previously created, if there is one, before creating a new one.

I was hoping that by reusing the 'InfoListAdapter` the comment views which were created would automatically have the reply handling code inside of it. How are the comment views which are being created here different than the ones which are created in the main comment holding view?

Stypox · 2021-11-13T20:31:38Z

app/src/main/java/org/schabi/newpipe/info_list/holder/RepliesHandler.java

+        if (item.getReplies() == null) {
+            repliesView.setVisibility(View.GONE);
+            showReplies.setVisibility(View.GONE);
+        } else if (item.getRepliesOpen()) {


As I said in the extractor, this method should be removed from there and implemented here in the frontend. The usual approach would be to create an item to use in the RecyclerView that wraps around CommentsInfoItem but also has a field isExpanded. This option is not viable, though, since in NewPipe lists use a common pattern to load, reload, etc., so changing the type of data fed to the recycler view without changing the type of data returned by the extractor is not doable without huge code changes (and we don't want those). A solution that would also be suitable for the multiple-depth replies implementation (see my other comment), would be to just keep a static Set<CommentsInfoItem> somewhere storing a common list of expanded CommentsInfoItems (common meaning that it will be accessed by all (even nested) RecyclerViews' CommentsInfoItem.updateFromItem()) . When the user expands a comment view the related CommentsInfoItem should be added to the Set, when the user unexpands the same comment it should be removed and when the RecyclerView tries to load a comment using CommentsInfoItemHolder.updateFromItem(item) it is expanded based on whether item belongs to the set.

@litetex do you have any idea whether this can be achieved in a better way, without storing any separate data in RecyclerViews (since that data would be lost when the comment view is recycled)?

@Stypox At first: Welcome back.
Whoa that's a lot of info.

do you have any idea whethe ...

No I don't - currently.

I think I would have to look at this in more detail to get an answer to that, however some ideas from my side to make nested commets working:

ViewModel which holds the data?

Write a custom Custom component for this?

@golfinq ping ;-)

@Stypox @litetex

I guess I am not understanding the issue, is it moving the code which stores whether or not replies were opened should move here and that in turn would cause issues requiring lots of changes or that as it currently stands this code can't handle recursive levels of replies?

as it currently stands this code can't handle recursive levels of replies

This is what I could guess by looking at the code, but I might be wrong since I couldn't test. But the problem is also the fact that you shouldn't store data about whether the replies menu is open or not in an object coming from the extractor. The extractor shouldn't have anything to do with the UI.

Stypox · 2021-11-13T20:44:15Z

app/src/main/java/org/schabi/newpipe/info_list/holder/CommentsInfoItemHolder.java


 import android.view.View;
 import android.view.ViewGroup;
 import android.widget.ImageView;


Is there a particular reason you added this code in CommentsInfoItemHolder and not CommentsMiniInfoItemHolder like all other comments code?

It was probably left over from something else I was trying to do, should be straight forward to move it to where it is used

Stypox · 2021-11-22T09:00:59Z

Oh, btw @golfinq, could you fix the build so that I can take a look at the apk? I think you forgot to update the extractor commit

Stypox · 2021-11-22T09:04:17Z

app/src/main/java/org/schabi/newpipe/info_list/holder/RepliesHandler.java

+                addRepliesToUI(parentInfoItem);
+            }
+
+            @SuppressLint("SetTextI18n")


There is a reason why Android Studio gives you this warning: you shouldn't hardcode strings, put them in the strings.xml resource file instead.

Stypox · 2021-11-22T09:06:20Z

app/src/main/java/org/schabi/newpipe/info_list/holder/RepliesHandler.java

+        if (item.getReplies() == null) {
+            repliesView.setVisibility(View.GONE);
+            showReplies.setVisibility(View.GONE);
+        } else if (item.getRepliesOpen()) {


as it currently stands this code can't handle recursive levels of replies

This is what I could guess by looking at the code, but I might be wrong since I couldn't test. But the problem is also the fact that you shouldn't store data about whether the replies menu is open or not in an object coming from the extractor. The extractor shouldn't have anything to do with the UI.

golfinq · 2021-11-28T18:44:31Z

I don't really have time at the moment to work on this, so I am going to convert it to a draft until I can properly work on everyone's suggestions.

golfinq · 2022-03-12T05:49:46Z

Honestly, I am not sure. This will require time I don't currently have IRL. I would also have to re-familiarize myself with java and the code to get back up to speed, which is a non-trivial task.

…

On Wed, Mar 2, 2022 at 6:56 PM Prince Cooper ***@***.***> wrote: Undraft when 😂 — Reply to this email directly, view it on GitHub <#7244 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AOMH7IJVAR4E6YKBONG7JP3U575ZLANCNFSM5F6ZLSGA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

opusforlife2 · 2022-03-12T08:52:16Z

@golfinq Then closing this PR for now. You can reopen it if/when you resume working on it.

DUOLabs333 · 2022-05-27T15:35:30Z

I'm willing to work on this. Does Newpipe have a way for forks to build APKs with your CI service or does the build have to be local? What are the things that need to be fixed?

triallax · 2022-05-27T16:05:21Z

You don't have to do anything to get the CI to build APKs for you; pushing a branch to your fork should be sufficient.

DUOLabs333 · 2022-05-27T16:06:45Z

So, I just push, and it builds? Cool. So, what are the problems?

triallax · 2022-05-27T16:08:59Z

I'm not too sure myself, but reading the comments and reviews here should give you an idea.

DUOLabs333 · 2022-05-27T16:10:05Z

I'm going to start out with a rebase, just to get myself situated.

DUOLabs333 · 2022-05-27T16:25:02Z

Will the actions be counted towards my limit or the team's limit?

triallax · 2022-05-27T16:25:59Z

I believe there's no limit on public repositories.

DUOLabs333 · 2022-05-27T16:27:14Z

I had to manually run it, it doesn't work on push.

triallax · 2022-05-27T16:28:46Z

Hmm, actually, I think it runs automatically only on push to a pull request branch or to master or dev. If you create a pull request from your branch to this repo, I think it will be run automatically.

DUOLabs333 · 2022-05-27T16:29:50Z

Oh, ok. I'm getting some style errors, so I'm trying to fix that.

DUOLabs333 · 2022-05-27T16:42:37Z

I somehow got public void addReplies after my rebase in CommentsInfoItemHolder.java when it doesn't exist in either dev or comment-replies.

triallax · 2022-05-27T16:44:03Z

It's right here though:

https://github.com/TeamNewPipe/NewPipe/pull/7244/files#diff-b39431223efada70b3fe4eaf18c5b6c309df4cd804227f54334388da9a07e0a5R168

DUOLabs333 · 2022-05-27T16:45:07Z

But it's not here: https://github.com/golfinq/NewPipe/blob/comment-replies/app/src/main/java/org/schabi/newpipe/info_list/holder/CommentsInfoItemHolder.java

DUOLabs333 · 2022-05-27T16:52:16Z

Oh, I messed up the rebase.

DUOLabs333 · 2022-05-27T17:14:48Z

I'll open up a new PR.

DUOLabs333 · 2022-05-27T17:18:50Z

I just need approval to start running CI tests.

Mhowser · 2022-09-23T18:23:39Z

@golfinq can this pull request be resurrected? Or are you still too busy?

golfinq · 2022-09-23T18:38:46Z

There was some good work done on cleaning it up, but it really really needs someone who is familiar with the newpipe infrastructure (or someone who is willing to dig through it and learn how) to find out how to extract and implement the extraction for the 2nd page of replies. There was discussuon that the ui should be changed to a page which pulls up from the bottom which I like a lot more but I wouldnt know how to implement. As it stands the core stuff in this PR is okay, but async/java stuff isnt my strength right now and the people who would know the core code stuff are difficult to get responses from. I've kinda moved on from this and while I still do use newpipe; its kind of a mess to dig through the source code and figure out "okay to do this I use this class and then input this into this and need to call all of these functions and this data goes here and can be accessed with this class". If I was to work on this again I would want through documentation of everything, which is a lot of work. Dont blame NewPipe devs though as supporting youtube and everything else makes it very very difficult to do what NewPipe does and that requires complicated code.

Mhowser · 2022-09-23T18:46:10Z

I appreciate the update on the current situation, thank you! There are some people at #2277 who might be able to finish what was started here.

golfinq · 2022-09-23T19:42:31Z

I am glad to see people still trying to implement this! It's why my NewPipe Repo will never be deleted - as I think this PR was the furthest anyone got.

Just to add on my previous post: If anyone reading this has any android dev knowledge (or is willing to learn it) please write some API documentation for NewPipe. Even if you are a complete beginner, if you can read code and are willing to follow the "open source" trail for something in android studio - you can write documentation. Even basic, but organized, documentation help people become developers which means more active development overall.

Added "Show Replies"

66e999f

golfinq mentioned this pull request Oct 14, 2021

Added "Show Replies" to each comment #7242

Closed

3 tasks

AudricV requested changes Oct 14, 2021

View reviewed changes

app/src/main/res/layout/list_comments_item.xml Outdated Show resolved Hide resolved

Changed "Show Replies" to Resource

ead685e

golfinq force-pushed the comment-replies branch from 9c8b01f to ead685e Compare October 14, 2021 11:51

triallax suggested changes Oct 14, 2021

View reviewed changes

app/src/main/res/values-en-rGB/strings.xml Outdated Show resolved Hide resolved

Remove redundant string resource

f6f7994

Co-authored-by: Mohammed Anas <[email protected]>

Fixed behavior when scrolling

b11d62f

triallax changed the title ~~Added "Show Replies"~~ Show comment replies Oct 15, 2021

triallax added the feature request Issue is related to a feature in the app label Oct 15, 2021

Moved downloading and reply code to new class

8d30693

golfinq mentioned this pull request Oct 20, 2021

Changes for "Show Replies" TeamNewPipe/NewPipeExtractor#739

Closed

3 tasks

Non-Blocking Downloading and Showing

a6f68fc

golfinq force-pushed the comment-replies branch from 5795138 to a6f68fc Compare October 22, 2021 03:53

Stypox requested changes Nov 13, 2021

View reviewed changes

Stypox reviewed Nov 22, 2021

View reviewed changes

golfinq marked this pull request as draft November 28, 2021 18:43

opusforlife2 closed this Mar 12, 2022

nicoursi mentioned this pull request May 8, 2022

Display comment replies #2277

Closed

opusforlife2 mentioned this pull request May 8, 2022

[HELP NEEDED] Major planned/missing features #6448

Open

34 tasks

DUOLabs333 mentioned this pull request May 27, 2022

Comment replies #8443

Closed

5 tasks

Uh oh!

Show comment replies #7244

Show comment replies #7244

Uh oh!

Conversation

golfinq commented Oct 14, 2021 • edited by opusforlife2 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is it?

Description of the changes in your PR

Before/After Screenshots/Screen Record

Fixes the following issue(s)

Relies on the following changes

Due diligence

Uh oh!

AudricV left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TobiGr commented Oct 14, 2021

Uh oh!

Uh oh!

golfinq commented Oct 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

golfinq commented Oct 22, 2021

Uh oh!

BratishkaErik commented Nov 2, 2021

Uh oh!

opusforlife2 commented Nov 3, 2021

Uh oh!

golfinq commented Nov 4, 2021

Uh oh!

opusforlife2 commented Nov 4, 2021

Uh oh!

Stypox left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

golfinq Nov 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

golfinq Nov 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Stypox commented Nov 22, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

golfinq commented Nov 28, 2021

Uh oh!

golfinq commented Mar 12, 2022 via email

Uh oh!

opusforlife2 commented Mar 12, 2022

Uh oh!

DUOLabs333 commented May 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

triallax commented May 27, 2022

Uh oh!

DUOLabs333 commented May 27, 2022

Uh oh!

golfinq commented Oct 14, 2021 •

edited by opusforlife2

Loading

golfinq commented Oct 15, 2021 •

edited

Loading

golfinq Nov 21, 2021 •

edited

Loading

golfinq Nov 21, 2021 •

edited

Loading

DUOLabs333 commented May 27, 2022 •

edited

Loading

triallax commented May 27, 2022 •

edited

Loading

DUOLabs333 commented May 27, 2022 •

edited

Loading

DUOLabs333 commented May 27, 2022 •

edited

Loading

DUOLabs333 commented May 27, 2022 •

edited

Loading