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

fix: Active speaker highlighting [WPB-11040] #3045

Merged
merged 3 commits into from
Oct 1, 2024

Conversation

borichellow
Copy link
Contributor

@borichellow borichellow commented Sep 27, 2024

BugWPB-11040 [Android] Active speaker not indicated anymore on latest beta

What's new in this PR?

Issues

Highlighting of speaking user during a call doesn't work.

Causes (Optional)

Small mistake in refactoring:
when we want to know if some participant is speaking during a call we need to check if that participant (pair of userId and clientId) is in a activeSpeakers list. The problem is that we were using participantId instead of userId.

Solutions

fix the mistake

@borichellow borichellow self-assigned this Sep 27, 2024
@echoes-hq echoes-hq bot added the echoes: unplanned Any work item that isn’t part of the product or technical roadmap. label Sep 27, 2024
Copy link
Contributor

github-actions bot commented Sep 27, 2024

Test Results

3 210 tests  +2   3 104 ✅ +3   4m 16s ⏱️ +22s
  551 suites ±0     106 💤  - 1 
  551 files   ±0       0 ❌ ±0 

Results for commit ce766e1. ± Comparison against base commit 025b3f7.

♻️ This comment has been updated with latest results.

Copy link
Contributor

github-actions bot commented Sep 27, 2024

🐰 Bencher Report

Branchfix/active_speaker_highlighting
Testbedubuntu-latest

⚠️ WARNING: The following Measure does not have a Threshold. Without a Threshold, no Alerts will ever be generated!

Click here to create a new Threshold
For more information, see the Threshold documentation.
To only post results if a Threshold exists, set the --ci-only-thresholds CLI flag.

Click to view all benchmark results
BenchmarkLatencynanoseconds (ns)
com.wire.kalium.benchmarks.logic.CoreLogicBenchmark.createObjectInFiles📈 view plot
⚠️ NO THRESHOLD
697,737.61
com.wire.kalium.benchmarks.logic.CoreLogicBenchmark.createObjectInMemory📈 view plot
⚠️ NO THRESHOLD
543,318,818.64
com.wire.kalium.benchmarks.persistence.MessagesNoPragmaTuneBenchmark.messageInsertionBenchmark📈 view plot
⚠️ NO THRESHOLD
943,780,384.33
com.wire.kalium.benchmarks.persistence.MessagesNoPragmaTuneBenchmark.queryMessagesBenchmark📈 view plot
⚠️ NO THRESHOLD
21,241,698.89
🐰 View full continuous benchmarking report in Bencher

@codecov-commenter
Copy link

codecov-commenter commented Sep 27, 2024

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 52.56%. Comparing base (025b3f7) to head (ce766e1).
Report is 3 commits behind head on develop.

Files with missing lines Patch % Lines
...wire/kalium/logic/data/call/CallMetadataProfile.kt 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3045      +/-   ##
===========================================
+ Coverage    52.52%   52.56%   +0.04%     
===========================================
  Files         1301     1303       +2     
  Lines        50044    50104      +60     
  Branches      4663     4671       +8     
===========================================
+ Hits         26284    26337      +53     
- Misses       21883    21885       +2     
- Partials      1877     1882       +5     
Files with missing lines Coverage Δ
...wire/kalium/logic/data/call/CallMetadataProfile.kt 0.00% <0.00%> (ø)

... and 15 files with indirect coverage changes


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 025b3f7...ce766e1. Read the comment docs.

@datadog-wireapp
Copy link

datadog-wireapp bot commented Sep 27, 2024

Datadog Report

Branch report: fix/active_speaker_highlighting
Commit report: 61c26cb
Test service: kalium-jvm

✅ 0 Failed, 3104 Passed, 106 Skipped, 13.15s Total Time

Copy link
Member

@vitorhugods vitorhugods left a comment

Choose a reason for hiding this comment

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

There could be a couple of tests for this.

// Given
Create a CallMetadata with some users. Put some in ActiveSpeakers.

// When
Call getFullParticipants.

// Then
Only the active speakers have isSpeaking = true


// Given
Create a CallMetadata with some users.
Put some in ActiveSpeakers.
Mute some of these.

// When
Call getFullParticipants.

// Then
Only the active speakers not muted have isSpeaking = true

Copy link

@borichellow borichellow added this pull request to the merge queue Oct 1, 2024
Merged via the queue into develop with commit 27fc353 Oct 1, 2024
22 checks passed
@borichellow borichellow deleted the fix/active_speaker_highlighting branch October 1, 2024 09:39
MohamadJaara pushed a commit that referenced this pull request Oct 18, 2024
* fix: Active speaker highlighting

* Added tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
echoes: unplanned Any work item that isn’t part of the product or technical roadmap. 👕 size: S type: bug / fix 🐞
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants