-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Add support for comment replies #10018
Conversation
It works good and pagination works well too, I can load 500 comments. Though, I think that "List view mode" shouldn't affect the way comments are displayed (grid view for comments doesn't look well). Probably I'm ignoring that's an impact of how the fragment is opened, but idk the technical stuff. |
Thank you for noticing, now it's fixed :-) The PR should now be ready |
a635746
to
2fdb259
Compare
Kudos, SonarCloud Quality Gate passed! |
@TobiGr this PeerTube video has nested replies, but they don't seem to be returned by the extractor: https://share.tube/w/vxu4uTstUBAUromWwXGHrq. |
This is nice but there's a behaviour I think should be fixed: when comment replies are open, the android back button should just close the replies and not get back to the last video list. |
That's an inconsistent behavior that is independent of this PR and is definitely strange. According to @opusforlife2 in Android the expected behavior for the top-left back button is to go to home, while the system back button should go to the previous fragment. This can be fixed in a separate PR. |
xD
Not me! I was just quoting! |
|
i think the approach in [Show comment replies #9410] by @xz-dev is much better because:
|
@MD77MD yes, if only it didn't have strange scrolling bugs and object cloning. I have already explained why this choice is being made. |
i hpoe this is not taken the wrong way, but this feature is implemented in another fork called pipepipe. perhaps you guys can take a look and see if they have managed to solve the scrolling problem stypox mentioned, or even improve on top of it in later updates. the link is down below |
50a3ae6
to
c92cd0f
Compare
app/src/main/java/org/schabi/newpipe/util/NavigationHelper.java
Outdated
Show resolved
Hide resolved
app/src/main/java/org/schabi/newpipe/util/NavigationHelper.java
Outdated
Show resolved
Hide resolved
app/src/main/java/org/schabi/newpipe/fragments/list/comments/CommentRepliesFragment.java
Outdated
Show resolved
Hide resolved
app/src/main/java/org/schabi/newpipe/info_list/holder/CommentInfoItemHolder.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Annotations are missing. Once added, this can be merged
I think your commit does not provide a proper behaviour:
|
d7ecc48
to
81c4153
Compare
I fixed the behaviour you pointed out above. One thing that I noticed is that the DetailFragment is scrolled to top after it is expanded from the BottomSheetBehaviour. I pushed a second commit which should help us to scroll to the last clicked comment. However, when calling |
This comment was marked as spam.
This comment was marked as spam.
Also remove some unused code
Also further simplify CommentRepliesInfo and RelatedItemsInfo
This reverts commit bb01da3. This commit was not needed
6000e23
to
fb19270
Compare
68c46be
to
aa84d6f
Compare
Quality Gate passedThe SonarCloud Quality Gate passed, but some issues were introduced. 2 New issues |
I rebased, fixed various problems with going back from the replies fragment to the video details fragment, and added the |
Update the FAQ entry according to the comment reply feature added in TeamNewPipe/NewPipe#10018
Update the FAQ entry according to the comment reply feature added in TeamNewPipe/NewPipe#10018
What is it?
Description of the changes in your PR
The most basic implementation of comment replies. The replies are shown in a fragment opened from the main activity, instead of on top of the video detail fragment. Although this might be a bit unexpected, this can be improved later, since I tried and found it too cumbersome to open fragments on top of each other*.
* Yet another broken thing in the current app architecture: opening new fragments from within one fragment. With the old Android APIs it would take something like 100 lines of code spread across many files, since you would have to create a stack of fragments in the base activity, then implement proper state saving in the stack, then push and pop items from the stack in the correct way, ... If we used the modern APIs instead, after the initial setup in the topmost activity, it would be as simple as writing something along the lines of
findNavController(...).navigate(viewId, arguments)
.Implementation notes
RelatedItemsInfo
in the same folder asRelatedItemsFragment
, since it wouldn't make sense for that class to stay inutil
. I also simplified the code in that class. Although this is an unrelated change, I did it because I also addedCommentRepliesInfo
that works in a similar way, and put it in the same folder asCommentRepliesFragment
.CommentsInfo
object, so I made bb01da3, but then I found out that the url is enough, so I reverted that change in 50a3ae6.CommentInfoItemHolder.updateFromItem
, but I would ignore this warning since I already simplified many parts of that method in 8bd42d9 and added comments to separate the various actions being performed.Before/After Screenshots/Screen Record
Note to blog post writers: I just noticed these images contain some aggressive comments in Italian, so they need to be taken again
Fixes the following issue(s)
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.
This PeerTube video has nested replies, but they don't seem to work: https://share.tube/w/vxu4uTstUBAUromWwXGHrq.
Due diligence
Notes for the website team: make sure to modify the corresponding FAQ entry when updating the page for the release.