Fix placeholder position check (#1216)

* Fix placeholder position check

Because of the missed negation `placeholderPosition` could actually point to a different comment. As a result loaded comments would be displayed at a wrong position which could result in duplicated comments.

* Extract placeholder search code

Simple refactoring. The only notable change is that now technically `placeholderPosition` can be -1. In practice that should never happen, but I added checks anyways
This commit is contained in:
Sergei Kozelko 2022-12-04 05:05:28 +08:00 committed by GitHub
parent bd5814fd4d
commit 1c81d3e0c9
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23

View File

@ -645,16 +645,8 @@ public class CommentsRecyclerViewAdapter extends RecyclerView.Adapter<RecyclerVi
mVisibleComments.get(parentPosition).getChildren().get(mVisibleComments.get(parentPosition).getChildren().size() - 1) mVisibleComments.get(parentPosition).getChildren().get(mVisibleComments.get(parentPosition).getChildren().size() - 1)
.setLoadMoreChildrenFailed(false); .setLoadMoreChildrenFailed(false);
int placeholderPosition = commentPosition; int placeholderPosition = findLoadMoreCommentsPlaceholderPosition(parentComment.getFullName(), commentPosition);
if (mVisibleComments.get(commentPosition).getFullName().equals(parentComment.getFullName())) { if (placeholderPosition != -1) {
for (int i = parentPosition + 1; i < mVisibleComments.size(); i++) {
if (mVisibleComments.get(i).getFullName().equals(parentComment.getFullName())) {
placeholderPosition = i;
break;
}
}
}
mVisibleComments.get(placeholderPosition).setLoadingMoreChildren(false); mVisibleComments.get(placeholderPosition).setLoadingMoreChildren(false);
mVisibleComments.get(placeholderPosition).setLoadMoreChildrenFailed(false); mVisibleComments.get(placeholderPosition).setLoadMoreChildrenFailed(false);
((LoadMoreChildCommentsViewHolder) holder).placeholderTextView.setText(R.string.comment_load_more_comments); ((LoadMoreChildCommentsViewHolder) holder).placeholderTextView.setText(R.string.comment_load_more_comments);
@ -665,21 +657,14 @@ public class CommentsRecyclerViewAdapter extends RecyclerView.Adapter<RecyclerVi
} else { } else {
notifyItemRangeInserted(placeholderPosition, expandedComments.size()); notifyItemRangeInserted(placeholderPosition, expandedComments.size());
} }
}
} else { } else {
mVisibleComments.get(parentPosition).getChildren() mVisibleComments.get(parentPosition).getChildren()
.remove(mVisibleComments.get(parentPosition).getChildren().size() - 1); .remove(mVisibleComments.get(parentPosition).getChildren().size() - 1);
mVisibleComments.get(parentPosition).removeMoreChildrenIds(); mVisibleComments.get(parentPosition).removeMoreChildrenIds();
int placeholderPosition = commentPosition; int placeholderPosition = findLoadMoreCommentsPlaceholderPosition(parentComment.getFullName(), commentPosition);
if (mVisibleComments.get(commentPosition).getFullName().equals(parentComment.getFullName())) { if (placeholderPosition != -1) {
for (int i = parentPosition + 1; i < mVisibleComments.size(); i++) {
if (mVisibleComments.get(i).getFullName().equals(parentComment.getFullName())) {
placeholderPosition = i;
break;
}
}
}
mVisibleComments.remove(placeholderPosition); mVisibleComments.remove(placeholderPosition);
if (mIsSingleCommentThreadMode) { if (mIsSingleCommentThreadMode) {
notifyItemRemoved(placeholderPosition + 1); notifyItemRemoved(placeholderPosition + 1);
@ -694,6 +679,7 @@ public class CommentsRecyclerViewAdapter extends RecyclerView.Adapter<RecyclerVi
notifyItemRangeInserted(placeholderPosition, expandedComments.size()); notifyItemRangeInserted(placeholderPosition, expandedComments.size());
} }
} }
}
} else { } else {
if (mVisibleComments.get(parentPosition).hasReply() && moreChildrenIds.isEmpty()) { if (mVisibleComments.get(parentPosition).hasReply() && moreChildrenIds.isEmpty()) {
mVisibleComments.get(parentPosition).getChildren() mVisibleComments.get(parentPosition).getChildren()
@ -712,17 +698,10 @@ public class CommentsRecyclerViewAdapter extends RecyclerView.Adapter<RecyclerVi
for (int i = 0; i < mVisibleComments.size(); i++) { for (int i = 0; i < mVisibleComments.size(); i++) {
if (mVisibleComments.get(i).getFullName().equals(parentComment.getFullName())) { if (mVisibleComments.get(i).getFullName().equals(parentComment.getFullName())) {
if (mVisibleComments.get(i).isExpanded()) { if (mVisibleComments.get(i).isExpanded()) {
int placeholderPosition = i + mVisibleComments.get(i).getChildren().size(); int placeholderPositionHint = i + mVisibleComments.get(i).getChildren().size();
int placeholderPosition = findLoadMoreCommentsPlaceholderPosition(parentComment.getFullName(), placeholderPositionHint);
if (!mVisibleComments.get(i).getFullName()
.equals(mVisibleComments.get(placeholderPosition).getFullName())) {
for (int j = i + 1; j < mVisibleComments.size(); j++) {
if (mVisibleComments.get(j).getFullName().equals(mVisibleComments.get(i).getFullName())) {
placeholderPosition = j;
}
}
}
if (placeholderPosition != -1) {
mVisibleComments.get(placeholderPosition).setLoadingMoreChildren(false); mVisibleComments.get(placeholderPosition).setLoadingMoreChildren(false);
mVisibleComments.get(placeholderPosition).setLoadMoreChildrenFailed(false); mVisibleComments.get(placeholderPosition).setLoadMoreChildrenFailed(false);
((LoadMoreChildCommentsViewHolder) holder).placeholderTextView.setText(R.string.comment_load_more_comments); ((LoadMoreChildCommentsViewHolder) holder).placeholderTextView.setText(R.string.comment_load_more_comments);
@ -734,6 +713,7 @@ public class CommentsRecyclerViewAdapter extends RecyclerView.Adapter<RecyclerVi
notifyItemRangeInserted(placeholderPosition, expandedComments.size()); notifyItemRangeInserted(placeholderPosition, expandedComments.size());
} }
} }
}
mVisibleComments.get(i).getChildren().get(mVisibleComments.get(i).getChildren().size() - 1) mVisibleComments.get(i).getChildren().get(mVisibleComments.get(i).getChildren().size() - 1)
.setLoadingMoreChildren(false); .setLoadingMoreChildren(false);
@ -758,17 +738,9 @@ public class CommentsRecyclerViewAdapter extends RecyclerView.Adapter<RecyclerVi
&& parentComment.getFullName().equals(mVisibleComments.get(parentPosition).getFullName())) { && parentComment.getFullName().equals(mVisibleComments.get(parentPosition).getFullName())) {
if (mVisibleComments.get(parentPosition).isExpanded()) { if (mVisibleComments.get(parentPosition).isExpanded()) {
int commentPosition = mIsSingleCommentThreadMode ? holder.getBindingAdapterPosition() - 1 : holder.getBindingAdapterPosition(); int commentPosition = mIsSingleCommentThreadMode ? holder.getBindingAdapterPosition() - 1 : holder.getBindingAdapterPosition();
int placeholderPosition = commentPosition; int placeholderPosition = findLoadMoreCommentsPlaceholderPosition(parentComment.getFullName(), commentPosition);
if (commentPosition >= mVisibleComments.size() || commentPosition < 0 || !mVisibleComments.get(commentPosition).getFullName().equals(parentComment.getFullName())) {
for (int i = parentPosition + 1; i < mVisibleComments.size(); i++) {
if (mVisibleComments.get(i).getFullName().equals(parentComment.getFullName())) {
placeholderPosition = i;
break;
}
}
}
if (placeholderPosition >= mVisibleComments.size() || placeholderPosition < 0) { if (placeholderPosition != -1) {
mVisibleComments.get(placeholderPosition).setLoadingMoreChildren(false); mVisibleComments.get(placeholderPosition).setLoadingMoreChildren(false);
mVisibleComments.get(placeholderPosition).setLoadMoreChildrenFailed(true); mVisibleComments.get(placeholderPosition).setLoadMoreChildrenFailed(true);
} }
@ -783,18 +755,13 @@ public class CommentsRecyclerViewAdapter extends RecyclerView.Adapter<RecyclerVi
for (int i = 0; i < mVisibleComments.size(); i++) { for (int i = 0; i < mVisibleComments.size(); i++) {
if (mVisibleComments.get(i).getFullName().equals(parentComment.getFullName())) { if (mVisibleComments.get(i).getFullName().equals(parentComment.getFullName())) {
if (mVisibleComments.get(i).isExpanded()) { if (mVisibleComments.get(i).isExpanded()) {
int placeholderPosition = i + mVisibleComments.get(i).getChildren().size(); int placeholderPositionHint = i + mVisibleComments.get(i).getChildren().size();
if (!mVisibleComments.get(placeholderPosition).getFullName().equals(mVisibleComments.get(i).getFullName())) { int placeholderPosition = findLoadMoreCommentsPlaceholderPosition(parentComment.getFullName(), placeholderPositionHint);
for (int j = i + 1; j < mVisibleComments.size(); j++) {
if (mVisibleComments.get(j).getFullName().equals(mVisibleComments.get(i).getFullName())) {
placeholderPosition = j;
break;
}
}
}
if (placeholderPosition != -1) {
mVisibleComments.get(placeholderPosition).setLoadingMoreChildren(false); mVisibleComments.get(placeholderPosition).setLoadingMoreChildren(false);
mVisibleComments.get(placeholderPosition).setLoadMoreChildrenFailed(true); mVisibleComments.get(placeholderPosition).setLoadMoreChildrenFailed(true);
}
((LoadMoreChildCommentsViewHolder) holder).placeholderTextView.setText(R.string.comment_load_more_comments_failed); ((LoadMoreChildCommentsViewHolder) holder).placeholderTextView.setText(R.string.comment_load_more_comments_failed);
} }
@ -836,6 +803,28 @@ public class CommentsRecyclerViewAdapter extends RecyclerView.Adapter<RecyclerVi
return -1; return -1;
} }
/**
* Find position of comment with given {@code fullName} and
* {@link Comment#PLACEHOLDER_LOAD_MORE_COMMENTS} placeholder type
* @return position of the placeholder or -1 if not found
*/
private int findLoadMoreCommentsPlaceholderPosition(String fullName, int positionHint) {
if (0 <= positionHint && positionHint < mVisibleComments.size()
&& mVisibleComments.get(positionHint).getFullName().equals(fullName)
&& mVisibleComments.get(positionHint).getPlaceholderType() == Comment.PLACEHOLDER_LOAD_MORE_COMMENTS) {
return positionHint;
}
for (int i = 0; i < mVisibleComments.size(); i++) {
Comment comment = mVisibleComments.get(i);
if (comment.getFullName().equals(fullName) && comment.getPlaceholderType() == Comment.PLACEHOLDER_LOAD_MORE_COMMENTS) {
return i;
}
}
return -1;
}
private void expandChildren(ArrayList<Comment> comments, ArrayList<Comment> newList) { private void expandChildren(ArrayList<Comment> comments, ArrayList<Comment> newList) {
if (comments != null && comments.size() > 0) { if (comments != null && comments.size() > 0) {
for (Comment comment : comments) { for (Comment comment : comments) {