-
Notifications
You must be signed in to change notification settings - Fork 155
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
Cancel ringing call notification on call cancellation #3047
Cancel ringing call notification on call cancellation #3047
Conversation
Quality Gate passedIssues Measures |
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3047 +/- ##
========================================
Coverage 76.14% 76.15%
========================================
Files 1645 1645
Lines 38809 38829 +20
Branches 7516 7523 +7
========================================
+ Hits 29552 29570 +18
- Misses 5356 5357 +1
- Partials 3901 3902 +1 ☔ View full report in Codecov by Sentry. |
Hello @jmartinesp what is the status of the PR? I mean can it become "Ready for review" or does it need any update? |
So I created this PR as a draft so everyone could test it (especially Timo, who suggested the implementation), it's functional and it could be merged (maybe after some cleanup, I can't remember) but we don't have a counterpart on iOS and I don't think we have a plan to do this work there any time soon. It depends on whether we want this even if iOS doesn't have it or not. @manuroe ? |
We (well @stefanceriu ) worked through this on iOS in this commit with the same approach as @jmartinesp here. We can merge this PR. |
1581573
to
be4f0db
Compare
be4f0db
to
0813fe9
Compare
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.
Just one remark, else LGTM!
.onEach { roomHasActiveCall -> | ||
if (!roomHasActiveCall) { | ||
// The call was cancelled | ||
incomingCallTimedOut() |
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.
You may want to add timedOutCallJob?.cancel()
too, even if it should not change anything.
// We only want to check if the room active call status changes | ||
.distinctUntilChanged() | ||
// Skip the first one, we're not interested in it (if the check below passes, it had to be active anyway) | ||
.drop(1) |
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.
I am a bit scared about this drop
. If it is not mandatory, can we just remove it?
Or maybe replace by
// We only want to check if the room active call status changes
.distinctUntilChanged()
.filter { roomHasActiveCall -> !roomHasActiveCall }
.onEach {
// The call was cancelled
incomingCallTimedOut()
}
.launchIn(coroutineScope)
(Unit tests are still passing with this)
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.
IIRC we can't, because in a real life situation this flow will start before we get the room info, and by default the roomHasActiveCall
will be false
, instantly cancelling any incoming call as a result.
We could replace it by some operator that takes pairs of events instead so we can compare hadCall == true && newValue == false
, but that's technically doing exactly the same as the drop as it wouldn't trigger until there are 2 elements in the flow.
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.
ok
Quality Gate passedIssues Measures |
Content
Listen to active call state for DMs with ringing calls so if the other user hangs up before you've replied the ringing stops and the call is cancelled.
Motivation and context
Improve the call ringing implementation. Fixes #3014.
Tests
Tested devices
Checklist