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.

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

Jump to bottom

feat: send and receive in-call reactions [#WPB-14254] #3759

Merged
merged 2 commits into from
Dec 23, 2024
Merged

feat: send and receive in-call reactions [#WPB-14254] #3759

merged 2 commits into from
Dec 23, 2024

Conversation

sergeibakhtiarov
Copy link
Contributor

@sergeibakhtiarov sergeibakhtiarov commented Dec 19, 2024 •
edited by github-actions bot
Loading

StoryWPB-14254 [Android] In-Call Reactions

https://wearezeta.atlassian.net/browse/WPB-14254

What's new in this PR?

  • Observe and display in-call reactions with animations
  • Show recent reaction in participant tile
  • New panel for selecting in-call reaction
  • Updated style for call controls
  • Camera flip button moved to participant tile
Showing emojis In-call reactions panel Custom emojis Camera flip button
reactions_1 reactions_2 reactions_3 reactions_4

@pull-request-size Pull Request Size
Copy link

Ups 🫰🟨

This PR is too big. Please try to break it up into smaller PRs.

@codecov-commenter
Copy link

codecov-commenter commented Dec 19, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 61.46789% with 42 lines in your changes missing coverage. Please review.

Project coverage is 45.83%. Comparing base (8847c15) to head (54aab29).

Files with missing lines Patch % Lines
...ng/ongoing/incallreactions/InCallReactionsState.kt 0.00% 27 Missing ⚠️
.../wire/android/ui/calling/SharedCallingViewModel.kt 68.29% 10 Missing and 3 partials ⚠️
...om/wire/android/ui/calling/model/InCallReaction.kt 83.33% 1 Missing ⚠️
...calling/ongoing/incallreactions/InCallReactions.kt 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff             @@
##           develop    #3759      +/-   ##
===========================================
+ Coverage    45.70%   45.83%   +0.12%     
===========================================
  Files          477      481       +4     
  Lines        16223    16321      +98     
  Branches      2746     2753       +7     
===========================================
+ Hits          7415     7480      +65     
- Misses        8038     8068      +30     
- Partials       770      773       +3
Files with missing lines Coverage Δ
...com/wire/android/mapper/UICallParticipantMapper.kt 93.33% <100.00%> (+0.47%) ⬆️
...wire/android/ui/calling/model/UICallParticipant.kt 100.00% <100.00%> (ø)
...android/ui/calling/ongoing/OngoingCallViewModel.kt 76.31% <ø> (ø)
...id/ui/calling/ongoing/fullscreen/FullScreenTile.kt 0.00% <ø> (ø)
...g/ongoing/participantsview/FloatingSelfUserTile.kt 0.00% <ø> (ø)
...alling/ongoing/participantsview/ParticipantTile.kt 0.00% <ø> (ø)
...g/ongoing/participantsview/VerticalCallingPager.kt 0.00% <ø> (ø)
...going/participantsview/gridview/CallingGridView.kt 0.00% <ø> (ø)
...c/main/kotlin/com/wire/android/util/ExpiringMap.kt 100.00% <100.00%> (ø)
...ain/kotlin/com/wire/android/util/extension/Flow.kt 40.00% <100.00%> (+40.00%) ⬆️
... and 4 more

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8847c15...54aab29. Read the comment docs.

@github-actions GitHub Actions
Copy link
Contributor

Built wire-android-staging-compat-pr-3759.apk is available for download

@github-actions GitHub Actions
Copy link
Contributor

Built wire-android-dev-debug-pr-3759.apk is available for download

@github-actions GitHub Actions
Copy link
Contributor

Built wire-android-staging-compat-pr-3759.apk is available for download

@github-actions GitHub Actions
Copy link
Contributor

Built wire-android-dev-debug-pr-3759.apk is available for download

borichellow
borichellow reviewed Dec 23, 2024
View reviewed changes
app/src/main/kotlin/com/wire/android/ui/calling/SharedCallingViewModel.kt Outdated Show resolved Hide resolved
app/src/main/kotlin/com/wire/android/ui/calling/SharedCallingViewModel.kt Outdated Show resolved Hide resolved
app/src/main/kotlin/com/wire/android/ui/calling/ongoing/fullscreen/FullScreenTile.kt Outdated
@@ -87,7 +89,7 @@ fun FullScreenTile(
.height(height)
.padding(contentPadding),
participantTitleState = it,
isSelfUser = selectedParticipant.isSelfUser,
isSelfClient = selectedParticipant.clientId == participants[0].clientId,
Copy link
Contributor

Choose a reason for hiding this comment

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

why did you change it? Os there any reason to not use SelectedParticipant.isSelfUser?

  1. in case of design changes and self user is not always in a first tile we'll have to make changes here too.
  2. It's preferred to as less logic in UI as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When you join the same call from two devices then you get two participants with isSelfUser flag set to true. So these users have incorrectly duplicated mute and flip camera buttons and video is only showing for one of them.
Needed to use a clientId to handle them correctly.

Copy link
Contributor

Choose a reason for hiding this comment

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

so maybe lets update SelectedParticipant by changing isSelfUser to isSelfClient or just adding isSelfClient and using it from there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am trying to fetch the clientId in the viewmodel. I think to make it correct we can add 'isSelfUser' flag to the UICallParticipant and use clientId to set it when mapping to UIParticipant. This way the flag will be correct across all UI code and will not depend on the participants ordering logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@borichellow please check updated logic for isSelfUser based on clientId.

app/src/main/kotlin/com/wire/android/ui/calling/controlbuttons/HangUpOngoingButton.kt Show resolved Hide resolved
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
6 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@sergeibakhtiarov sergeibakhtiarov added this pull request to the merge queue Dec 23, 2024
@github-actions GitHub Actions
Copy link
Contributor

Built wire-android-staging-compat-pr-3759.apk is available for download

@github-actions GitHub Actions
Copy link
Contributor

Built wire-android-dev-debug-pr-3759.apk is available for download

Merged via the queue into develop with commit 188bc6c Dec 23, 2024
13 of 14 checks passed
@sergeibakhtiarov sergeibakhtiarov deleted the feat/in-call-reactions branch December 23, 2024 15:09
@echoes-hq echoes-hq bot added the echoes: product-roadmap/feature Work contributing to adding a new feature as part of the product roadmap. label Jan 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MohamadJaara MohamadJaara MohamadJaara approved these changes

@borichellow borichellow borichellow approved these changes

Assignees

@sergeibakhtiarov sergeibakhtiarov

Labels
echoes: product-roadmap/feature Work contributing to adding a new feature as part of the product roadmap. size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sergeibakhtiarov @codecov-commenter @borichellow @MohamadJaara @sbakhtiarov