Skip to content
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

Merged
merged 3 commits into from
Aug 1, 2021
Merged

Show hearts in comments #6741

merged 3 commits into from
Aug 1, 2021

Conversation

KalleStruik
Copy link

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

  • Show a heart next to the like count on comments that have heartedByUploader set to true.

Before/After Screenshots/Screen Record

  • Before:
    image

  • After:
    image

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

@skyGtm
Copy link

skyGtm commented Jul 23, 2021

It would be better if Heart color was RED.

Copy link
Member

@Stypox Stypox left a 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/res/values/dimens.xml Outdated Show resolved Hide resolved
app/src/main/res/layout/list_comments_item.xml Outdated Show resolved Hide resolved
@KalleStruik
Copy link
Author

I applied the changes requested. Also changed the heart color to be red as suggested by skyGtm. The below screenshot shows the red hearts working on android 4.4
image

@TobiGr TobiGr added the feature request Issue is related to a feature in the app label Jul 23, 2021
@triallax triallax added the GUI Issue is related to the graphical user interface label Jul 23, 2021
@opusforlife2
Copy link
Collaborator

Do we need a hover text on the icon saying "Liked by uploader"?

@SameenAhnaf
Copy link
Collaborator

@opusforlife2 Personally, I don't think so. We have two alternatives instead.

  1. Show uploader icon on the right bottom corner of the heart (just like in YouTube app)
  2. A toast like 'Liked by uploader' may appear when short pressed on the 'Heart' icon. (I like this as the UI will remain clean. Also, the user might be curious about the function of heart.)

@Stypox
Copy link
Member

Stypox commented Jul 24, 2021

@SameenAhnaf the second one is bad ui; the first one could be a good idea but idk if it would be simple to implement

@KalleStruik
Copy link
Author

I added the avatar to see how it would look. What do you all think about something like this?
Currently using the comment author's avatar because I have not yet found a way to get the video uploader avatar. Any help with how to do that would be appreciated.
image

@skyGtm
Copy link

skyGtm commented Jul 26, 2021

Instead of showing hearts in channel avatar,
What about replacing existing thumb-up icon with:

Capture

I prefer 2nd or 4th picture

@SameenAhnaf
Copy link
Collaborator

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

@KalleStruik
Copy link
Author

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

@Stypox
Copy link
Member

Stypox commented Jul 27, 2021

Any help with how to do that would be appreciated.

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

Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

Almost ready ;-)

@KalleStruik
Copy link
Author

I made the changes you requested, but I seem to be getting hit by #6744 now, so I can't test it.

Copy link
Member

@Stypox Stypox left a 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!

@Stypox
Copy link
Member

Stypox commented Aug 1, 2021

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 CommentsInfoItem.heartedByUploader = true for some items. I tested both on API 30 and API 19. Thank you!

@Stypox Stypox merged commit d42a534 into TeamNewPipe:dev Aug 1, 2021
@castrik
Copy link

castrik commented Aug 2, 2021

I personally prefer how the youtube app does this, they show the avatar of the uploader as the icon with a small heart beside it
Screenshot_20210802-054258

@tsiflimagas
Copy link
Contributor

@castrik as it was discussed in the above messages, that's not feasible in newpipe.

@castrik
Copy link

castrik commented Aug 3, 2021

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
is that not possible either? or was he saying this exact thing?

@Stypox
Copy link
Member

Stypox commented Aug 3, 2021

I think stypox said comments don't know about the commentor's avatar

No, obviously comments do know about the commentor's avatar, otherwise how would they be able to display it?

youtube app has the video uploader's avatar in the heart

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.

@FireMasterK
Copy link
Member

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 StreamInfo object itself.

@Stypox
Copy link
Member

Stypox commented Aug 4, 2021

@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

@Stypox Stypox mentioned this pull request Aug 4, 2021
@Stypox Stypox mentioned this pull request Aug 4, 2021
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issue is related to a feature in the app GUI Issue is related to the graphical user interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show hearts in comments