-
-
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
Show hearts in comments #6741
Show hearts in comments #6741
Conversation
…e creator of a video.
It would be better if Heart color was RED. |
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.
Thank you! :-)
Please test on Android 4.4 and check if the heart shows up
app/src/main/java/org/schabi/newpipe/info_list/holder/CommentsMiniInfoItemHolder.java
Outdated
Show resolved
Hide resolved
…s, and apply some code style suggestions.
Do we need a hover text on the icon saying "Liked by uploader"? |
@opusforlife2 Personally, I don't think so. We have two alternatives instead.
|
@SameenAhnaf the second one is bad ui; the first one could be a good idea but idk if it would be simple to implement |
@skyGtm Like and Love are completely two different actions. We should not just focus on aesthetics rather on logic as well. That way, other users won't get confused. @KalleStruik I'd personally prefer layout in #6741 (comment) over the latest layout. I'm not sure if the latest layout will pave the way for other devs to replace uploader icon there. If that's the case, I'm completely fine with it. |
@SameenAhnaf Currently I have not pushed the changes made in #6741 (comment) onto this branch yet, because I have not found a way to use the video uploader avatar instead of the commenter avatar. I was hoping someone else reading this PR would know of a way to get it, since I do not see an obvious way to do so. |
@KalleStruik I think this is not doable unfortunately, after looking into it. The comments themselves know nothing about the uploader avatar, and obtaining that info from the video detail fragment is not doable either, since comments could finish loading before video info does, so the uploader avatar url would not be available. So let's stick to just the heart with the tooltip; ignore my review above. |
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.
Almost ready ;-)
app/src/main/java/org/schabi/newpipe/info_list/holder/CommentsMiniInfoItemHolder.java
Outdated
Show resolved
Hide resolved
app/src/main/java/org/schabi/newpipe/info_list/holder/CommentsMiniInfoItemHolder.java
Outdated
Show resolved
Hide resolved
I made the changes you requested, but I seem to be getting hit by #6744 now, so I can't test it. |
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.
Code looks good. I couldn't test on YouTube either (though at least in SoundCloud and PeerTube it doesn't crash), let's wait until that problem is solved. Thank you!
It works, I finally tested by using by opening this fork by @FireMasterK with fixed comments, except that they are not completely fixed so the "heartedByUploader" part was wrong, but I was able to test anyway by debugging and manually setting |
@castrik as it was discussed in the above messages, that's not feasible in newpipe. |
I think stypox said comments don't know about the commentor's avatar as the ss posted by kalle struik does that, youtube app has the video uploader's avatar in the heart |
No, obviously comments do know about the commentor's avatar, otherwise how would they be able to display it?
Yes, but doing this would require many YouTube-only strange changes in the extractor, and I don't think it's worth it for such a little design improvement. |
I think this can be avoided by getting the channel avatar from the |
@FireMasterK that can't be done since comments can load before the stream info. I would not pospone loading comments just for a style improvement |
What is it?
Description of the changes in your PR
heartedByUploader
set to true.Before/After Screenshots/Screen Record
Before:
After:
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.
Due diligence