Skip to content

Show comment replies #7244

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

Closed
golfinq wants to merge 6 commits into from
Closed

Show comment replies #7244

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
66e999f
Added "Show Replies"
golfinq Oct 13, 2021
ead685e
Changed "Show Replies" to Resource
golfinq Oct 14, 2021
f6f7994
Remove redundant string resource
golfinq Oct 15, 2021
b11d62f
Fixed behavior when scrolling
golfinq Oct 15, 2021
8d30693
Moved downloading and reply code to new class
golfinq Oct 20, 2021
a6f68fc
Non-Blocking Downloading and Showing
golfinq Oct 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions app/src/main/java/org/schabi/newpipe/info_list/holder/CommentsInfoItemHolder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
import android.widget.ImageView;
Copy link
Member

@Stypox Stypox Nov 13, 2021

Choose a reason for hiding this comment

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

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

Copy link
Author

@golfinq golfinq Nov 21, 2021

Choose a reason for hiding this comment

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

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

import android.widget.TextView;

import androidx.recyclerview.widget.RecyclerView;

import org.schabi.newpipe.R;
import org.schabi.newpipe.extractor.InfoItem;
import org.schabi.newpipe.extractor.comments.CommentsInfoItem;
Expand Down Expand Up @@ -34,12 +36,17 @@
public class CommentsInfoItemHolder extends CommentsMiniInfoItemHolder {
public final TextView itemTitleView;
private final ImageView itemHeartView;
private final RepliesHandler repliesHandler;

public CommentsInfoItemHolder(final InfoItemBuilder infoItemBuilder, final ViewGroup parent) {
super(infoItemBuilder, R.layout.list_comments_item, parent);

itemTitleView = itemView.findViewById(R.id.itemTitleView);
itemHeartView = itemView.findViewById(R.id.detail_heart_image_view);

final TextView showReplies = itemView.findViewById(R.id.showReplies);
final RecyclerView repliesView = itemView.findViewById(R.id.replyRecycleView);
repliesHandler = new RepliesHandler(showReplies, repliesView);
}

@Override
Expand All @@ -55,5 +62,7 @@ public void updateFromItem(final InfoItem infoItem,
itemTitleView.setText(item.getUploaderName());

itemHeartView.setVisibility(item.isHeartedByUploader() ? View.VISIBLE : View.GONE);

repliesHandler.checkForReplies(item);
}
}
192 changes: 192 additions & 0 deletions app/src/main/java/org/schabi/newpipe/info_list/holder/RepliesHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,192 @@
package org.schabi.newpipe.info_list.holder;

import android.annotation.SuppressLint;
import android.view.View;
import android.view.ViewGroup;
import android.widget.TextView;

import androidx.recyclerview.widget.LinearLayoutManager;
import androidx.recyclerview.widget.RecyclerView;

import org.schabi.newpipe.extractor.ListExtractor;
import org.schabi.newpipe.extractor.comments.CommentsInfo;
import org.schabi.newpipe.extractor.comments.CommentsInfoItem;
import org.schabi.newpipe.info_list.InfoListAdapter;
import org.schabi.newpipe.util.ExtractorHelper;

import java.util.ArrayList;
import java.util.List;
import java.util.Objects;
import java.util.concurrent.Callable;

import io.reactivex.rxjava3.android.schedulers.AndroidSchedulers;
import io.reactivex.rxjava3.annotations.NonNull;
import io.reactivex.rxjava3.core.Single;
import io.reactivex.rxjava3.core.SingleObserver;
import io.reactivex.rxjava3.disposables.Disposable;
import io.reactivex.rxjava3.schedulers.Schedulers;

public class RepliesHandler {
private final List<CommentsInfoItem> cachedReplies;
private final TextView showReplies;
private final RecyclerView repliesView;

public RepliesHandler(final TextView showReplies, final RecyclerView recyclerView) {
this.repliesView = recyclerView;
repliesView.setAdapter(new InfoListAdapter(repliesView.getContext()));
repliesView.setLayoutManager(new LinearLayoutManager(repliesView.getContext()));

this.showReplies = showReplies;
this.cachedReplies = new ArrayList<>();
}

static class GetMoreItemsCallable implements
Callable<ListExtractor.InfoItemsPage<CommentsInfoItem>> {
private CommentsInfo parentCommentInfo;
private CommentsInfoItem parentInfoItem;

public void setCallableParameters(
final CommentsInfo commentInfo, final CommentsInfoItem infoItem) {
parentCommentInfo = commentInfo;
parentInfoItem = infoItem;
}

@Override
public ListExtractor.InfoItemsPage<CommentsInfoItem> call() throws Exception {
return CommentsInfo.getMoreItems(parentCommentInfo, parentInfoItem.getReplies());
}
}

private Single<ListExtractor.InfoItemsPage<CommentsInfoItem>>
repliesSingle(final CommentsInfo parentCommentInfo,
final CommentsInfoItem parentInfoItem) {

final GetMoreItemsCallable getMoreItems = new GetMoreItemsCallable();
getMoreItems.setCallableParameters(parentCommentInfo, parentInfoItem);
return Single.fromCallable(getMoreItems);

}

private SingleObserver<ListExtractor.InfoItemsPage<CommentsInfoItem>>
repliesObserver(final CommentsInfoItem parentInfoItem) {

return new SingleObserver<ListExtractor.InfoItemsPage<CommentsInfoItem>>() {

@SuppressLint("SetTextI18n")
@Override
public void onSubscribe(@NonNull final Disposable d) {
showReplies.setText("Setting up replies");
}

@Override
public void onSuccess(@NonNull final
ListExtractor.InfoItemsPage<CommentsInfoItem>
commentsInfoItemInfoItemsPage) {

final List<CommentsInfoItem> actualList
= commentsInfoItemInfoItemsPage.getItems();

cachedReplies.addAll(actualList);
addRepliesToUI(parentInfoItem);
}

@SuppressLint("SetTextI18n")
Copy link
Member

@Stypox Stypox Nov 22, 2021

Choose a reason for hiding this comment

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

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.

@Override
public void onError(@NonNull final Throwable e) {
showReplies.setText("Error getting replies");
}
};
}

private SingleObserver<CommentsInfo>
repliesInfoObserver(final CommentsInfoItem parentInfoItem) {

return new SingleObserver<CommentsInfo>() {

@SuppressLint("SetTextI18n")
@Override
public void onSubscribe(@NonNull final Disposable d) {
showReplies.setText("Downloading Replies");
}

@Override
public void onSuccess(@NonNull final CommentsInfo commentsInfo) {
final Single<ListExtractor.InfoItemsPage<CommentsInfoItem>>
getRepliesInfoSingle =
repliesSingle(commentsInfo, parentInfoItem);

final SingleObserver<ListExtractor.InfoItemsPage<CommentsInfoItem>>
getRepliesInfoObserver = repliesObserver(parentInfoItem);

getRepliesInfoSingle
.subscribeOn(Schedulers.io())
.observeOn(AndroidSchedulers.mainThread())
.subscribe(getRepliesInfoObserver);
}

@SuppressLint("SetTextI18n")
@Override
public void onError(@NonNull final Throwable e) {
showReplies.setText("Error getting replies");
}
};
}


public void addRepliesToUI(final CommentsInfoItem parentInfoItem) {
((InfoListAdapter) Objects.requireNonNull(repliesView.getAdapter()))
.setInfoItemList(cachedReplies);

final ViewGroup.MarginLayoutParams params =
(ViewGroup.MarginLayoutParams) repliesView.getLayoutParams();
params.topMargin = 45;

repliesView.setMinimumHeight(100);
repliesView.setHasFixedSize(true);
parentInfoItem.setRepliesOpen(true);
showReplies.setVisibility(View.GONE);
repliesView.setVisibility(View.VISIBLE);
}

public void downloadReplies(final CommentsInfoItem parentInfoItem) {

final Single<CommentsInfo> parentInfoSingle = ExtractorHelper.getCommentsInfo(
parentInfoItem.getServiceId(),
parentInfoItem.getUrl(),
false
);

final SingleObserver<CommentsInfo> singleInfoRepliesInfoObserver
= repliesInfoObserver(parentInfoItem);

parentInfoSingle
.subscribeOn(Schedulers.io())
.observeOn(AndroidSchedulers.mainThread())
.subscribe(singleInfoRepliesInfoObserver);
}

public void addReplies(final CommentsInfoItem parentInfoItem) {
if (parentInfoItem.getReplies() == null) {
return;
}

if (cachedReplies.isEmpty()) {
downloadReplies(parentInfoItem);
} else {
addRepliesToUI(parentInfoItem);
}
}

public void checkForReplies(final CommentsInfoItem item) {
if (item.getReplies() == null) {
repliesView.setVisibility(View.GONE);
showReplies.setVisibility(View.GONE);
} else if (item.getRepliesOpen()) {
Copy link
Member

@Stypox Stypox Nov 13, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Member

@Stypox Stypox Nov 13, 2021

Choose a reason for hiding this comment

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

@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)?

Copy link
Member

@litetex litetex Nov 14, 2021

Choose a reason for hiding this comment

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

@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?

Copy link
Member

@Stypox Stypox Nov 21, 2021

Choose a reason for hiding this comment

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

@golfinq ping ;-)

Copy link
Author

@golfinq golfinq Nov 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@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?

Copy link
Member

@Stypox Stypox Nov 22, 2021

Choose a reason for hiding this comment

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

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.

addReplies(item);
} else {
repliesView.setVisibility(View.GONE);
showReplies.setVisibility(View.VISIBLE);
showReplies.setOnClickListener(v -> addReplies(item));
}
}
}
30 changes: 30 additions & 0 deletions app/src/main/res/layout/list_comments_item.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,4 +123,34 @@
android:textSize="@dimen/video_item_search_upload_date_text_size"
tools:text="1 year ago" />

<TextView
android:id="@+id/showReplies"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:layout_below="@+id/itemCommentContentView"
android:layout_alignEnd="@+id/itemCommentContentView"
android:layout_alignBottom="@+id/itemPublishedTime"
android:layout_marginStart="21dp"
android:layout_marginTop="0sp"
android:layout_marginEnd="0sp"
android:layout_marginBottom="0sp"
android:layout_toEndOf="@+id/itemPublishedTime"
android:text="@string/show_replies"
android:textAppearance="@style/TextAppearance.AppCompat.Small"
android:textSize="@dimen/video_item_search_upload_date_text_size" />

<androidx.recyclerview.widget.RecyclerView
Copy link
Member

@Stypox Stypox Nov 13, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Author

@golfinq golfinq Nov 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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?

android:id="@+id/replyRecycleView"
android:layout_width="match_parent"
android:layout_height="wrap_content"
android:layout_below="@+id/showReplies"
android:layout_alignStart="@+id/itemCommentContentView"
android:layout_alignEnd="@+id/itemCommentContentView"
android:layout_marginStart="0dp"
android:layout_marginTop="0dp"
android:layout_marginEnd="0dp"
android:layout_marginBottom="0dp">

</androidx.recyclerview.widget.RecyclerView>

</RelativeLayout>
1 change: 1 addition & 0 deletions app/src/main/res/values/strings.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -704,4 +704,5 @@
<!-- Show Channel Details -->
<string name="error_show_channel_details">Error at Show Channel Details</string>
<string name="loading_channel_details">Loading Channel Details…</string>
<string name="show_replies">Show Replies</string>
</resources>