Improve how active calls work (#3029)

* Improve how active calls work: - 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.
element-hq · Jun 18, 2024 · 2e32adf · 2e32adf
1 parent 2b5ea96
commit 2e32adf
Show file tree

Hide file tree

Showing 9 changed files with 128 additions and 86 deletions.
diff --git a/changelog.d/3029.misc b/changelog.d/3029.misc
@@ -0,0 +1 @@
+Improve how active calls work by also taking into account external url calls and waiting for the sync process to start before sending the `m.call.notify` event.
diff --git a/...lin/io/element/android/features/call/impl/notifications/RingingCallNotificationCreator.kt b/...lin/io/element/android/features/call/impl/notifications/RingingCallNotificationCreator.kt
@@ -84,11 +84,24 @@ class RingingCallNotificationCreator @Inject constructor(
             .build()
 
         val answerIntent = IntentProvider.getPendingIntent(context, CallType.RoomCall(sessionId, roomId))
+        val notificationData = CallNotificationData(
+            sessionId = sessionId,
+            roomId = roomId,
+            eventId = eventId,
+            senderId = senderId,
+            roomName = roomName,
+            senderName = senderDisplayName,
+            avatarUrl = roomAvatarUrl,
+            notificationChannelId = notificationChannelId,
+            timestamp = timestamp
+        )
 
         val declineIntent = PendingIntentCompat.getBroadcast(
             context,
             DECLINE_REQUEST_CODE,
-            Intent(context, DeclineCallBroadcastReceiver::class.java),
+            Intent(context, DeclineCallBroadcastReceiver::class.java).apply {
+                putExtra(DeclineCallBroadcastReceiver.EXTRA_NOTIFICATION_DATA, notificationData)
+            },
             PendingIntent.FLAG_CANCEL_CURRENT,
             false,
         )!!
@@ -97,10 +110,7 @@ class RingingCallNotificationCreator @Inject constructor(
             context,
             FULL_SCREEN_INTENT_REQUEST_CODE,
             Intent(context, IncomingCallActivity::class.java).apply {
-                putExtra(
-                    IncomingCallActivity.EXTRA_NOTIFICATION_DATA,
-                    CallNotificationData(sessionId, roomId, eventId, senderId, roomName, senderDisplayName, roomAvatarUrl, notificationChannelId, timestamp)
-                )
+                putExtra(IncomingCallActivity.EXTRA_NOTIFICATION_DATA, notificationData)
             },
             PendingIntent.FLAG_CANCEL_CURRENT,
             false

diff --git a/...in/kotlin/io/element/android/features/call/impl/receivers/DeclineCallBroadcastReceiver.kt b/...in/kotlin/io/element/android/features/call/impl/receivers/DeclineCallBroadcastReceiver.kt
@@ -19,7 +19,10 @@ package io.element.android.features.call.impl.receivers
 import android.content.BroadcastReceiver
 import android.content.Context
 import android.content.Intent
+import androidx.core.content.IntentCompat
+import io.element.android.features.call.api.CallType
 import io.element.android.features.call.impl.di.CallBindings
+import io.element.android.features.call.impl.notifications.CallNotificationData
 import io.element.android.features.call.impl.utils.ActiveCallManager
 import io.element.android.libraries.architecture.bindings
 import javax.inject.Inject
@@ -28,10 +31,15 @@ import javax.inject.Inject
  * Broadcast receiver to decline the incoming call.
  */
 class DeclineCallBroadcastReceiver : BroadcastReceiver() {
+    companion object {
+        const val EXTRA_NOTIFICATION_DATA = "EXTRA_NOTIFICATION_DATA"
+    }
     @Inject
     lateinit var activeCallManager: ActiveCallManager
     override fun onReceive(context: Context, intent: Intent?) {
+        val notificationData = intent?.let { IntentCompat.getParcelableExtra(it, EXTRA_NOTIFICATION_DATA, CallNotificationData::class.java) }
+            ?: return
         context.bindings<CallBindings>().inject(this)
-        activeCallManager.hungUpCall()
+        activeCallManager.hungUpCall(callType = CallType.RoomCall(notificationData.sessionId, notificationData.roomId))
     }
 }
diff --git a/...call/impl/src/main/kotlin/io/element/android/features/call/impl/ui/CallScreenPresenter.kt b/...call/impl/src/main/kotlin/io/element/android/features/call/impl/ui/CallScreenPresenter.kt
@@ -40,14 +40,15 @@ import io.element.android.libraries.architecture.AsyncData
 import io.element.android.libraries.architecture.Presenter
 import io.element.android.libraries.architecture.runCatchingUpdatingState
 import io.element.android.libraries.core.coroutine.CoroutineDispatchers
+import io.element.android.libraries.matrix.api.MatrixClient
 import io.element.android.libraries.matrix.api.MatrixClientProvider
+import io.element.android.libraries.matrix.api.core.RoomId
 import io.element.android.libraries.matrix.api.sync.SyncState
 import io.element.android.libraries.matrix.api.widget.MatrixWidgetDriver
 import io.element.android.libraries.network.useragent.UserAgentProvider
 import io.element.android.services.analytics.api.ScreenTracker
 import io.element.android.services.toolbox.api.systemclock.SystemClock
 import kotlinx.coroutines.CoroutineScope
-import kotlinx.coroutines.flow.collect
 import kotlinx.coroutines.flow.launchIn
 import kotlinx.coroutines.flow.onEach
 import kotlinx.coroutines.launch
@@ -75,6 +76,7 @@ class CallScreenPresenter @AssistedInject constructor(
 
     private val isInWidgetMode = callType is CallType.RoomCall
     private val userAgent = userAgentProvider.provide()
+    private var notifiedCallStart = false
 
     @Composable
     override fun present(): CallScreenState {
@@ -84,11 +86,14 @@ class CallScreenPresenter @AssistedInject constructor(
         val messageInterceptor = remember { mutableStateOf<WidgetMessageInterceptor?>(null) }
         var isJoinedCall by rememberSaveable { mutableStateOf(false) }
 
-        LaunchedEffect(Unit) {
-            loadUrl(callType, urlState, callWidgetDriver)
-
-            if (callType is CallType.RoomCall) {
-                activeCallManager.joinedCall(callType.sessionId, callType.roomId)
+        DisposableEffect(Unit) {
+            coroutineScope.launch {
+                // Sets the call as joined
+                activeCallManager.joinedCall(callType)
+                loadUrl(callType, urlState, callWidgetDriver)
+            }
+            onDispose {
+                activeCallManager.hungUpCall(callType)
             }
         }
 
@@ -140,14 +145,6 @@ class CallScreenPresenter @AssistedInject constructor(
             }
         }
 
-        DisposableEffect(Unit) {
-            onDispose {
-                if (callType is CallType.RoomCall) {
-                    activeCallManager.hungUpCall()
-                }
-            }
-        }
-
         fun handleEvents(event: CallScreenEvents) {
             when (event) {
                 is CallScreenEvents.Hangup -> {
@@ -173,15 +170,15 @@ class CallScreenPresenter @AssistedInject constructor(
             urlState = urlState.value,
             userAgent = userAgent,
             isInWidgetMode = isInWidgetMode,
-            eventSink = ::handleEvents,
+            eventSink = { handleEvents(it) },
         )
     }
 
-    private fun CoroutineScope.loadUrl(
+    private suspend fun loadUrl(
         inputs: CallType,
         urlState: MutableState<AsyncData<String>>,
         callWidgetDriver: MutableState<MatrixWidgetDriver?>,
-    ) = launch {
+    ) {
         urlState.runCatchingUpdatingState {
             when (inputs) {
                 is CallType.ExternalUrl -> {
@@ -209,12 +206,13 @@ class CallScreenPresenter @AssistedInject constructor(
             } ?: return@DisposableEffect onDispose { }
             coroutineScope.launch {
                 client.syncService().syncState
-                    .onEach { state ->
-                        if (state != SyncState.Running) {
+                    .collect { state ->
+                        if (state == SyncState.Running) {
+                            client.notifyCallStartIfNeeded(callType.roomId)
+                        } else {
                             client.syncService().startSync()
                         }
                     }
-                    .collect()
             }
             onDispose {
                 // We can't use the local coroutine scope here because it will be disposed before this effect
@@ -229,6 +227,13 @@ class CallScreenPresenter @AssistedInject constructor(
         }
     }
 
+    private suspend fun MatrixClient.notifyCallStartIfNeeded(roomId: RoomId) {
+        if (!notifiedCallStart) {
+            getRoom(roomId)?.sendCallNotificationIfNeeded()
+                ?.onSuccess { notifiedCallStart = true }
+        }
+    }
+
     private fun parseMessage(message: String): WidgetMessage? {
         return WidgetMessageSerializer.deserialize(message).getOrNull()
     }

diff --git a/...all/impl/src/main/kotlin/io/element/android/features/call/impl/ui/IncomingCallActivity.kt b/...all/impl/src/main/kotlin/io/element/android/features/call/impl/ui/IncomingCallActivity.kt
@@ -91,6 +91,7 @@ class IncomingCallActivity : AppCompatActivity() {
     }
 
     private fun onCancel() {
-        activeCallManager.hungUpCall()
+        val activeCall = activeCallManager.activeCall.value ?: return
+        activeCallManager.hungUpCall(callType = activeCall.callType)
     }
 }
diff --git a/...all/impl/src/main/kotlin/io/element/android/features/call/impl/utils/ActiveCallManager.kt b/...all/impl/src/main/kotlin/io/element/android/features/call/impl/utils/ActiveCallManager.kt
@@ -20,13 +20,11 @@ import android.annotation.SuppressLint
 import androidx.core.app.NotificationManagerCompat
 import com.squareup.anvil.annotations.ContributesBinding
 import io.element.android.appconfig.ElementCallConfig
+import io.element.android.features.call.api.CallType
 import io.element.android.features.call.impl.notifications.CallNotificationData
 import io.element.android.features.call.impl.notifications.RingingCallNotificationCreator
 import io.element.android.libraries.di.AppScope
 import io.element.android.libraries.di.SingleIn
-import io.element.android.libraries.matrix.api.MatrixClientProvider
-import io.element.android.libraries.matrix.api.core.RoomId
-import io.element.android.libraries.matrix.api.core.SessionId
 import io.element.android.libraries.push.api.notifications.ForegroundServiceType
 import io.element.android.libraries.push.api.notifications.NotificationIdProvider
 import io.element.android.libraries.push.api.notifications.OnMissedCallNotificationHandler
@@ -61,24 +59,23 @@ interface ActiveCallManager {
     fun incomingCallTimedOut()
 
     /**
-     * Hangs up the active call and removes any associated UI.
+     * Called when the active call has been hung up. It will remove any existing UI and the active call.
+     * @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)
 
     /**
-     * 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].
      *
-     * @param sessionId The session ID of the user joining the call.
-     * @param roomId The room ID of the call.
+     * @param callType The type of call that the user joined, either an external url one or a room one.
      */
-    fun joinedCall(sessionId: SessionId, roomId: RoomId)
+    fun joinedCall(callType: CallType)
 }
 
 @SingleIn(AppScope::class)
 @ContributesBinding(AppScope::class)
 class DefaultActiveCallManager @Inject constructor(
     private val coroutineScope: CoroutineScope,
-    private val matrixClientProvider: MatrixClientProvider,
     private val onMissedCallNotificationHandler: OnMissedCallNotificationHandler,
     private val ringingCallNotificationCreator: RingingCallNotificationCreator,
     private val notificationManagerCompat: NotificationManagerCompat,
@@ -94,15 +91,17 @@ class DefaultActiveCallManager @Inject constructor(
             return
         }
         activeCall.value = ActiveCall(
-            sessionId = notificationData.sessionId,
-            roomId = notificationData.roomId,
+            callType = CallType.RoomCall(
+                sessionId = notificationData.sessionId,
+                roomId = notificationData.roomId,
+            ),
             callState = CallState.Ringing(notificationData),
         )
 
         timedOutCallJob = coroutineScope.launch {
             showIncomingCallNotification(notificationData)
 
-            // Wait for the call to end
+            // Wait for the ringing call to time out
             delay(ElementCallConfig.RINGING_CALL_DURATION_SECONDS.seconds)
             incomingCallTimedOut()
         }
@@ -118,28 +117,24 @@ class DefaultActiveCallManager @Inject constructor(
         displayMissedCallNotification(notificationData)
     }
 
-    override fun hungUpCall() {
+    override fun hungUpCall(callType: CallType) {
+        if (activeCall.value?.callType != callType) {
+            Timber.w("Call type $callType does not match the active call type, ignoring")
+            return
+        }
         cancelIncomingCallNotification()
         timedOutCallJob?.cancel()
         activeCall.value = null
     }
 
-    override fun joinedCall(sessionId: SessionId, roomId: RoomId) {
+    override fun joinedCall(callType: CallType) {
         cancelIncomingCallNotification()
         timedOutCallJob?.cancel()
 
         activeCall.value = ActiveCall(
-            sessionId = sessionId,
-            roomId = roomId,
+            callType = callType,
             callState = CallState.InCall,
         )
-        // Send call notification to the room
-        coroutineScope.launch {
-            matrixClientProvider.getOrRestore(sessionId)
-                .getOrNull()
-                ?.getRoom(roomId)
-                ?.sendCallNotificationIfNeeded()
-        }
     }
 
     @SuppressLint("MissingPermission")
@@ -184,8 +179,7 @@ class DefaultActiveCallManager @Inject constructor(
  * Represents an active call.
  */
 data class ActiveCall(
-    val sessionId: SessionId,
-    val roomId: RoomId,
+    val callType: CallType,
     val callState: CallState,
 )
 

diff --git a/.../call/impl/src/test/kotlin/io/element/android/features/call/ui/CallScreenPresenterTest.kt b/.../call/impl/src/test/kotlin/io/element/android/features/call/ui/CallScreenPresenterTest.kt
@@ -35,6 +35,7 @@ import io.element.android.libraries.matrix.test.A_ROOM_ID
 import io.element.android.libraries.matrix.test.A_SESSION_ID
 import io.element.android.libraries.matrix.test.FakeMatrixClient
 import io.element.android.libraries.matrix.test.FakeMatrixClientProvider
+import io.element.android.libraries.matrix.test.room.FakeMatrixRoom
 import io.element.android.libraries.matrix.test.widget.FakeMatrixWidgetDriver
 import io.element.android.libraries.network.useragent.UserAgentProvider
 import io.element.android.services.analytics.api.ScreenTracker
@@ -61,11 +62,13 @@ class CallScreenPresenterTest {
     val warmUpRule = WarmUpRule()
 
     @Test
-    fun `present - with CallType ExternalUrl just loads the URL`() = runTest {
-        val analyticsLambda = lambdaRecorder<MobileScreen.ScreenName, Unit> { }
+    fun `present - with CallType ExternalUrl just loads the URL and sets the call as active`() = runTest {
+        val analyticsLambda = lambdaRecorder<MobileScreen.ScreenName, Unit> {}
+        val joinedCallLambda = lambdaRecorder<CallType, Unit> {}
         val presenter = createCallScreenPresenter(
             callType = CallType.ExternalUrl("https://call.element.io"),
-            screenTracker = FakeScreenTracker(analyticsLambda)
+            screenTracker = FakeScreenTracker(analyticsLambda),
+            activeCallManager = FakeActiveCallManager(joinedCallResult = joinedCallLambda),
         )
         moleculeFlow(RecompositionMode.Immediate) {
             presenter.present()
@@ -76,25 +79,35 @@ class CallScreenPresenterTest {
             assertThat(initialState.urlState).isEqualTo(AsyncData.Success("https://call.element.io"))
             assertThat(initialState.isInWidgetMode).isFalse()
             analyticsLambda.assertions().isNeverCalled()
+            joinedCallLambda.assertions().isCalledOnce()
         }
     }
 
     @Test
-    fun `present - with CallType RoomCall loads URL and runs WidgetDriver`() = runTest {
+    fun `present - with CallType RoomCall sets call as active, loads URL, runs WidgetDriver and notifies the other clients a call started`() = runTest {
+        val sendCallNotificationIfNeededLambda = lambdaRecorder<Result<Unit>> { Result.success(Unit) }
+        val fakeRoom = FakeMatrixRoom(sendCallNotificationIfNeededResult = sendCallNotificationIfNeededLambda)
+        val client = FakeMatrixClient().apply {
+            givenGetRoomResult(A_ROOM_ID, fakeRoom)
+        }
         val widgetDriver = FakeMatrixWidgetDriver()
         val widgetProvider = FakeCallWidgetProvider(widgetDriver)
-        val analyticsLambda = lambdaRecorder<MobileScreen.ScreenName, Unit> { }
+        val analyticsLambda = lambdaRecorder<MobileScreen.ScreenName, Unit> {}
+        val joinedCallLambda = lambdaRecorder<CallType, Unit> {}
         val presenter = createCallScreenPresenter(
+            matrixClientsProvider = FakeMatrixClientProvider(getClient = { Result.success(client) }),
             callType = CallType.RoomCall(A_SESSION_ID, A_ROOM_ID),
             widgetDriver = widgetDriver,
             widgetProvider = widgetProvider,
-            screenTracker = FakeScreenTracker(analyticsLambda)
+            screenTracker = FakeScreenTracker(analyticsLambda),
+            activeCallManager = FakeActiveCallManager(joinedCallResult = joinedCallLambda),
         )
         moleculeFlow(RecompositionMode.Immediate) {
             presenter.present()
         }.test {
             // Wait until the URL is loaded
             skipItems(1)
+            joinedCallLambda.assertions().isCalledOnce()
             val initialState = awaitItem()
             assertThat(initialState.urlState).isInstanceOf(AsyncData.Success::class.java)
             assertThat(initialState.isInWidgetMode).isTrue()
@@ -106,6 +119,7 @@ class CallScreenPresenterTest {
                     listOf(value(MobileScreen.ScreenName.RoomCall)),
                     listOf(value(MobileScreen.ScreenName.RoomCall))
                 )
+            sendCallNotificationIfNeededLambda.assertions().isCalledOnce()
         }
     }
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		Improve how active calls work by also taking into account external url calls and waiting for the sync process to start before sending the `m.call.notify` event.