Improve how active calls work #3029

jmartinesp · 2024-06-13T14:36:25Z

Type of change

Feature
Bugfix
Technical
Other :

Content

Sending the m.call.notify event is now done in CallScreenPresenter once we know the sync is running.
You can mark a call of both external url or room type as joined.
Hanging up checks the current active call type and will only remove it if it matches.

Motivation and context

@toger5 mentioned some ways in which the EXA implementation of ringing calls could be improved.

Tests

Just tests calling between clients in several scenarios.

Tested devices

Physical
Emulator
OS version(s): 14.

Checklist

Changes have been tested on an Android device or Android emulator with API 23
UI change has been tested on both light and dark themes
Accessibility has been taken into account. See https://github.com/element-hq/element-x-android/blob/develop/CONTRIBUTING.md#accessibility
Pull request is based on the develop branch
Pull request includes a new file under ./changelog.d. See https://github.com/element-hq/element-x-android/blob/develop/CONTRIBUTING.md#changelog
Pull request includes screenshots or videos if containing UI changes
Pull request includes a sign off
You've made a self review of your PR

- Sending the `m.call.notify` event is now done in `CallScreenPresenter` once we know the sync is running. - You can mark a call of both external url or room type as joined. - Hanging up checks the current active call type and will only remove it if it matches.

github-actions · 2024-06-13T14:45:26Z

📱 Scan the QR code below to install the build (arm64 only) for this PR.

If you can't scan the QR code you can install the build via this link: https://i.diawi.com/JVHNQD

codecov · 2024-06-18T07:57:54Z

Codecov Report

Attention: Patch coverage is 78.12500% with 7 lines in your changes missing coverage. Please review.

Project coverage is 75.80%. Comparing base (2b5ea96) to head (2ff3ee9).

Files	Patch %	Lines
...all/impl/receivers/DeclineCallBroadcastReceiver.kt	0.00%	2 Missing ⚠️
...droid/features/call/impl/ui/CallScreenPresenter.kt	71.42%	0 Missing and 2 partials ⚠️
...roid/features/call/impl/ui/IncomingCallActivity.kt	0.00%	2 Missing ⚠️
...roid/features/call/impl/utils/ActiveCallManager.kt	85.71%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3029      +/-   ##
===========================================
- Coverage    75.80%   75.80%   -0.01%     
===========================================
  Files         1623     1623              
  Lines        38230    38237       +7     
  Branches      7369     7372       +3     
===========================================
+ Hits         28982    28987       +5     
- Misses        5395     5397       +2     
  Partials      3853     3853

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bmarty · 2024-06-18T08:09:58Z

I am currently testing the feature.

I think this commit e18c8d0 can be dropped, I have fixed the failing test in #3036

bmarty

The changes make sense, and my tests went well, thanks!

…ow-active-calls-work

toger5 · 2024-06-18T09:33:19Z

...s/call/impl/src/main/kotlin/io/element/android/features/call/impl/utils/ActiveCallManager.kt

     * Hangs up the active call and removes any associated UI.
+     * @param callType The type of call that the user hung up, either an external url one or a room one.
     */
-    fun hungUpCall()
+    fun hungUpCall(callType: CallType)


there seems to be a conflict between the docstring and the method name.
hangUpCall or

* Called when the active call has been hung up and all associated UI was removed.

toger5 · 2024-06-18T09:33:39Z

...s/call/impl/src/main/kotlin/io/element/android/features/call/impl/utils/ActiveCallManager.kt

     */
-    fun hungUpCall()
+    fun hungUpCall(callType: CallType)

    /**
     * Called when the user joins a call. It will remove any existing UI and set the call state as [CallState.InCall].


Suggested change

* Called when the user joins a call. It will remove any existing UI and set the call state as [CallState.InCall].

* Called after the user joined a call. It will remove any existing UI and set the call state as [CallState.InCall].

sonarcloud · 2024-06-18T09:53:34Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

jmartinesp · 2024-06-18T09:54:28Z

@toger5 let me know if any other changes are needed.

toger5 · 2024-06-18T09:56:44Z

@bmarty the rest made sense from just skimming over it.

Is this related: element-hq/element-call#2421

jmartinesp · 2024-06-18T09:58:57Z

Is this related: element-hq/element-call#2421

I don't think so. We don't really have some event we receive as a push that says 'this user hung up, the call is now over' we can use to cancel the ringing, right?

jmartinesp · 2024-06-18T10:00:35Z

Maybe we could try using the room info and observe the call status from there as long as we have an active call now that I think about it. But I'd add that on a separate PR.

toger5 · 2024-06-18T10:03:05Z

@jmartinesp yea that is how its intended to be done. When the ring starts, we want to run the sync loop and once we receive an empty call member event (room info updates to call has stopped) we update the state again.

Initially it was planned to do all this in the rust sdk directly but the app team decided this is quicker on the platform side.

jmartinesp · 2024-06-18T10:06:34Z

Ok, then I'll merge this as is and then continue the work on another PR once I'm free from my current tasks.

toger5 · 2024-06-18T10:27:22Z

This sounds really good. Can you evaluate with @stefanceriu if there is a chance to move some of that logic into the rust sdk?

Maybe its also worth to confirm with the MSC: https://github.com/matrix-org/matrix-spec-proposals/blob/toger5/matrixrtc-call-ringing/proposals/4075-call-notify-event.md
if the mobile implementation covers all the cases. (That should mention all the cases in which we should start and stop to ring.)

jmartinesp · 2024-06-18T11:37:03Z

PoC of a platform implementation for this: #3047

jmartinesp requested a review from a team as a code owner June 13, 2024 14:36

jmartinesp requested review from bmarty and removed request for a team June 13, 2024 14:36

Add changelog

9d92937

jmartinesp requested a review from toger5 June 13, 2024 14:37

jmartinesp force-pushed the misc/jme/improve-how-active-calls-work branch from f62fced to e18c8d0 Compare June 13, 2024 15:00

bmarty approved these changes Jun 18, 2024

View reviewed changes

jmartinesp added the Run-Maestro Starts a Maestro Cloud session to run integration tests label Jun 18, 2024

github-actions bot removed the Run-Maestro Starts a Maestro Cloud session to run integration tests label Jun 18, 2024

jmartinesp added 2 commits June 18, 2024 11:29

Try to fix tests

c9ce3f0

Merge remote-tracking branch 'origin/develop' into misc/jme/improve-h…

082a5e5

…ow-active-calls-work

toger5 reviewed Jun 18, 2024

View reviewed changes

Fix docs

2ff3ee9

jmartinesp force-pushed the misc/jme/improve-how-active-calls-work branch from 6538113 to 2ff3ee9 Compare June 18, 2024 09:43

jmartinesp merged commit 2e32adf into develop Jun 18, 2024
24 checks passed

jmartinesp deleted the misc/jme/improve-how-active-calls-work branch June 18, 2024 10:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve how active calls work #3029

Improve how active calls work #3029

jmartinesp commented Jun 13, 2024 •

edited

Loading

github-actions bot commented Jun 13, 2024 •

edited

Loading

codecov bot commented Jun 18, 2024 •

edited

Loading

bmarty commented Jun 18, 2024

bmarty left a comment

toger5 Jun 18, 2024

jmartinesp Jun 18, 2024

toger5 Jun 18, 2024

jmartinesp Jun 18, 2024

sonarcloud bot commented Jun 18, 2024

jmartinesp commented Jun 18, 2024

toger5 commented Jun 18, 2024

jmartinesp commented Jun 18, 2024

jmartinesp commented Jun 18, 2024

toger5 commented Jun 18, 2024

jmartinesp commented Jun 18, 2024

toger5 commented Jun 18, 2024

jmartinesp commented Jun 18, 2024

	* Called when the user joins a call. It will remove any existing UI and set the call state as [CallState.InCall].
	* Called after the user joined a call. It will remove any existing UI and set the call state as [CallState.InCall].

Improve how active calls work #3029

Improve how active calls work #3029

Conversation

jmartinesp commented Jun 13, 2024 • edited Loading

Type of change

Content

Motivation and context

Tests

Tested devices

Checklist

github-actions bot commented Jun 13, 2024 • edited Loading

codecov bot commented Jun 18, 2024 • edited Loading

Codecov Report

bmarty commented Jun 18, 2024

bmarty left a comment

Choose a reason for hiding this comment

toger5 Jun 18, 2024

Choose a reason for hiding this comment

jmartinesp Jun 18, 2024

Choose a reason for hiding this comment

toger5 Jun 18, 2024

Choose a reason for hiding this comment

jmartinesp Jun 18, 2024

Choose a reason for hiding this comment

sonarcloud bot commented Jun 18, 2024

Quality Gate passed

jmartinesp commented Jun 18, 2024

toger5 commented Jun 18, 2024

jmartinesp commented Jun 18, 2024

jmartinesp commented Jun 18, 2024

toger5 commented Jun 18, 2024

jmartinesp commented Jun 18, 2024

toger5 commented Jun 18, 2024

jmartinesp commented Jun 18, 2024

jmartinesp commented Jun 13, 2024 •

edited

Loading

github-actions bot commented Jun 13, 2024 •

edited

Loading

codecov bot commented Jun 18, 2024 •

edited

Loading