Skip to content

Commit b1782f7

Browse files
authored
Merge pull request #6241 from nextcloud/backport/6086/stable-24.0
[stable-24.0] fix: send local state when ICE connects instead of at participant-add time
2 parents a755efe + b2a31cf commit b1782f7

7 files changed

Lines changed: 656 additions & 69 deletions

File tree

app/build.gradle.kts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -382,6 +382,9 @@ dependencies {
382382
testImplementation("com.squareup.okhttp3:mockwebserver:$okhttpVersion")
383383
testImplementation("com.google.dagger:hilt-android-testing:2.59.2")
384384
testImplementation("org.robolectric:robolectric:4.16.1")
385+
// conscrypt-android provides Android JNI libs only; the openjdk-uber variant bundles
386+
// JVM host natives so Robolectric can initialise the security provider without crashing.
387+
testImplementation("org.conscrypt:conscrypt-openjdk-uber:2.5.2")
385388
}
386389

387390
tasks.register<Copy>("installGitHooks") {

app/src/main/java/com/nextcloud/talk/activities/CallActivity.kt

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2132,9 +2132,10 @@ class CallActivity : CallBaseActivity() {
21322132
) {
21332133
Log.d(TAG, "handleCallParticipantsChanged")
21342134

2135-
// The signaling session is the same as the Nextcloud session only when the MCU is not used.
2135+
// The signaling session is the same as the Nextcloud session only when internal signaling is used.
2136+
// With external signaling (HPB), participant session IDs are HPB session IDs regardless of MCU.
21362137
var currentSessionId = callSession
2137-
if (hasMCU) {
2138+
if (webSocketClient != null) {
21382139
currentSessionId = webSocketClient!!.sessionId
21392140
}
21402141
Log.d(TAG, " currentSessionId is $currentSessionId")
@@ -2439,7 +2440,9 @@ class CallActivity : CallBaseActivity() {
24392440
signalingMessageReceiver!!
24402441
)
24412442

2442-
localStateBroadcaster!!.handleCallParticipantAdded(callViewModel.getParticipant(sessionId)?.uiState?.value)
2443+
// Pass the live StateFlow so LocalStateBroadcasterNoMcu can observe ICE state changes
2444+
// and send the local state exactly when the data channel becomes ready.
2445+
localStateBroadcaster!!.handleCallParticipantAdded(callViewModel.getParticipant(sessionId)!!.uiState)
24432446

24442447
initPipMode()
24452448
}

app/src/main/java/com/nextcloud/talk/call/LocalStateBroadcaster.java

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,8 @@
1313

1414
import java.util.Objects;
1515

16+
import kotlinx.coroutines.flow.StateFlow;
17+
1618
/**
1719
* Helper class to send the local participant state to the other participants in the call.
1820
* <p>
@@ -82,6 +84,17 @@ public void destroy() {
8284
this.localCallParticipantModel.removeObserver(localCallParticipantModelObserver);
8385
}
8486

87+
/**
88+
* Passes the live StateFlow for the given participant so the broadcaster can react to each
89+
* connection-state change. The default implementation takes a snapshot of the current value
90+
* and delegates to {@link #handleCallParticipantAdded(ParticipantUiState)}; subclasses that
91+
* need to observe future emissions (e.g. {@code LocalStateBroadcasterNoMcu}) should override
92+
* this method instead.
93+
*/
94+
public void handleCallParticipantAdded(StateFlow<ParticipantUiState> uiStateFlow) {
95+
handleCallParticipantAdded(uiStateFlow.getValue());
96+
}
97+
8598
public abstract void handleCallParticipantAdded(ParticipantUiState uiState);
8699
public abstract void handleCallParticipantRemoved(String sessionId);
87100

app/src/main/java/com/nextcloud/talk/call/LocalStateBroadcasterNoMcu.kt

Lines changed: 48 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -14,26 +14,25 @@ package com.nextcloud.talk.call
1414
* state when a remote participant is added.
1515
*
1616
*
17-
* The state is sent when a connection with another participant is first established (which implicitly broadcasts the
18-
* initial state when the local participant joins the call, as a connection is established with all the remote
19-
* participants). Note that, as long as that participant stays in the call, the initial state is not sent again, even
20-
* after a temporary disconnection; data channels use a reliable transport by default, so even if the state changes
21-
* while the connection is temporarily interrupted the normal state update messages should be received by the other
22-
* participant once the connection is restored.
17+
* The state is sent the first time the ICE connection reaches a "connected" state for a given participant
18+
* (isConnected transitions from false/unknown to true). The observer collects the participant's
19+
* uiState StateFlow so that if the connection is briefly lost and then restored (e.g. an ICE restart),
20+
* the state is re-sent on the next false → true transition.
2321
*
2422
*
25-
* Nevertheless, in case of a failed connection and an ICE restart it is unclear whether the data channel messages
26-
* would be received or not (as the data channel transport may be the one that failed and needs to be restarted).
27-
* However, the state (except the speaking state) is also sent through signaling messages, which need to be
28-
* explicitly fetched from the internal signaling server, so even in case of a failed connection they will be
29-
* eventually received once the remote participant connects again.
23+
* Note that, as long as a participant stays in the call, the state is sent each time the ICE connection
24+
* goes from disconnected to connected; data channels use a reliable transport by default, so even if the
25+
* state changes while the connection is temporarily interrupted the normal state update messages should be
26+
* received by the other participant once the connection is restored.
3027
*/
3128
import com.nextcloud.talk.activities.ParticipantUiState
3229
import kotlinx.coroutines.CoroutineScope
3330
import kotlinx.coroutines.Dispatchers
3431
import kotlinx.coroutines.Job
3532
import kotlinx.coroutines.SupervisorJob
36-
import org.webrtc.PeerConnection.IceConnectionState
33+
import kotlinx.coroutines.flow.MutableStateFlow
34+
import kotlinx.coroutines.flow.StateFlow
35+
import kotlinx.coroutines.launch
3736
import java.util.concurrent.ConcurrentHashMap
3837

3938
class LocalStateBroadcasterNoMcu(
@@ -42,39 +41,25 @@ class LocalStateBroadcasterNoMcu(
4241
private val scope: CoroutineScope = CoroutineScope(Dispatchers.Main.immediate + SupervisorJob())
4342
) : LocalStateBroadcaster(localCallParticipantModel, messageSender) {
4443

45-
// Map sessionId -> observer wrapper (Flow collector job)
4644
private val iceConnectionStateObservers = ConcurrentHashMap<String, IceConnectionStateObserver>()
4745

48-
private inner class IceConnectionStateObserver(val uiState: ParticipantUiState) {
49-
private var job: Job? = null
50-
51-
init {
52-
handleStateChange(uiState)
53-
}
54-
55-
private fun handleStateChange(uiState: ParticipantUiState) {
56-
// Determine ICE connection state
57-
val iceState = if (uiState.isConnected) IceConnectionState.CONNECTED else IceConnectionState.NEW
58-
59-
if (iceState == IceConnectionState.CONNECTED) {
60-
remove()
61-
sendState(uiState.sessionKey)
62-
}
63-
}
64-
65-
fun remove() {
66-
job?.cancel()
67-
iceConnectionStateObservers.remove(uiState.sessionKey)
68-
}
46+
/**
47+
* Primary entry point called by CallActivity. Starts (or restarts) collecting the live
48+
* StateFlow so that the local state is sent every time the ICE connection transitions from
49+
* disconnected to connected.
50+
*/
51+
override fun handleCallParticipantAdded(uiStateFlow: StateFlow<ParticipantUiState>) {
52+
val sessionId = uiStateFlow.value.sessionKey ?: return
53+
iceConnectionStateObservers[sessionId]?.remove()
54+
iceConnectionStateObservers[sessionId] = IceConnectionStateObserver(sessionId, uiStateFlow)
6955
}
7056

57+
/**
58+
* Fallback for callers that only have a snapshot (e.g. tests that pre-date the StateFlow API).
59+
* Wraps the snapshot in a single-value StateFlow and delegates to the primary overload.
60+
*/
7161
override fun handleCallParticipantAdded(uiState: ParticipantUiState) {
72-
uiState.sessionKey?.let {
73-
iceConnectionStateObservers[it]?.remove()
74-
75-
iceConnectionStateObservers[it] =
76-
IceConnectionStateObserver(uiState)
77-
}
62+
handleCallParticipantAdded(MutableStateFlow(uiState) as StateFlow<ParticipantUiState>)
7863
}
7964

8065
override fun handleCallParticipantRemoved(sessionId: String) {
@@ -83,13 +68,35 @@ class LocalStateBroadcasterNoMcu(
8368

8469
override fun destroy() {
8570
super.destroy()
86-
// Cancel all collectors safely
8771
val observersCopy = iceConnectionStateObservers.values.toList()
8872
for (observer in observersCopy) {
8973
observer.remove()
9074
}
9175
}
9276

77+
private inner class IceConnectionStateObserver(
78+
private val sessionId: String,
79+
uiStateFlow: StateFlow<ParticipantUiState>
80+
) {
81+
private val job: Job = scope.launch {
82+
var previousIsConnected: Boolean? = null
83+
uiStateFlow.collect { uiState ->
84+
val currentIsConnected = uiState.isConnected
85+
// Send state on every false → true (or null → true) transition so that the
86+
// remote participant receives the local state as soon as the data channel is ready.
87+
if (currentIsConnected && previousIsConnected != true) {
88+
sendState(uiState.sessionKey)
89+
}
90+
previousIsConnected = currentIsConnected
91+
}
92+
}
93+
94+
fun remove() {
95+
job.cancel()
96+
iceConnectionStateObservers.remove(sessionId)
97+
}
98+
}
99+
93100
private fun sendState(sessionKey: String?) {
94101
messageSender.send(getDataChannelMessageForAudioState(), sessionKey)
95102
messageSender.send(getDataChannelMessageForSpeakingState(), sessionKey)
Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,136 @@
1+
/*
2+
* Nextcloud Talk - Android Client
3+
*
4+
* SPDX-FileCopyrightText: 2026 Alain Lauzon
5+
* SPDX-License-Identifier: GPL-3.0-or-later
6+
*/
7+
package com.nextcloud.talk.activities
8+
9+
import android.app.Application
10+
import com.nextcloud.talk.call.LocalStateBroadcaster
11+
import com.nextcloud.talk.signaling.SignalingMessageReceiver
12+
import kotlinx.coroutines.flow.StateFlow
13+
import org.junit.Assert.assertSame
14+
import org.junit.Before
15+
import org.junit.Test
16+
import org.junit.runner.RunWith
17+
import org.mockito.kotlin.argumentCaptor
18+
import org.mockito.kotlin.mock
19+
import org.mockito.kotlin.verify
20+
import org.robolectric.Robolectric
21+
import org.robolectric.RobolectricTestRunner
22+
import org.robolectric.annotation.Config
23+
import java.lang.reflect.Field
24+
25+
/**
26+
* Robolectric test verifying that [CallActivity.addCallParticipant] passes the **live StateFlow**
27+
* from [ParticipantHandler.uiState] to [LocalStateBroadcaster.handleCallParticipantAdded].
28+
*
29+
* Before the fix, the method passed only the snapshot value [StateFlow.value], meaning
30+
* [LocalStateBroadcasterNoMcu] could not observe future ICE state changes and videoOn/audioOn
31+
* were never sent once the data channel was ready.
32+
*
33+
* This test:
34+
* - creates [CallActivity] via Robolectric WITHOUT calling [Activity.onCreate] (avoiding native
35+
* WebRTC and Dagger initialisation)
36+
* - injects the required fields via reflection
37+
* - calls [addCallParticipant] via reflection
38+
* - verifies that the argument passed to [LocalStateBroadcaster.handleCallParticipantAdded] is
39+
* the exact same [StateFlow] object as [CallViewModel.getParticipant]!!.uiState
40+
*
41+
* If the buggy snapshot pattern is used (`uiState.value` instead of `uiState`), the verification
42+
* fails because the [StateFlow] overload is never called.
43+
*/
44+
@RunWith(RobolectricTestRunner::class)
45+
@Config(
46+
// Use the plain Application to avoid NextcloudTalkApplication.onCreate() which initialises
47+
// Dagger, WebRTC, and WorkManager — none of which are needed for this focused test.
48+
application = Application::class,
49+
sdk = [33]
50+
)
51+
class CallActivityAddParticipantTest {
52+
53+
private val mockLocalStateBroadcaster: LocalStateBroadcaster = mock()
54+
private val mockSignalingMessageReceiver: SignalingMessageReceiver = mock()
55+
56+
private lateinit var callViewModel: CallViewModel
57+
private lateinit var activity: CallActivity
58+
59+
@Before
60+
fun setUp() {
61+
callViewModel = CallViewModel()
62+
63+
// Build the Activity without triggering onCreate — this avoids Dagger injection,
64+
// EglBase.create() (native), and all network calls.
65+
activity = Robolectric.buildActivity(CallActivity::class.java).get()
66+
67+
// Inject the minimum set of fields required by addCallParticipant.
68+
setField("callViewModel", callViewModel)
69+
setField("localStateBroadcaster", mockLocalStateBroadcaster)
70+
setField("signalingMessageReceiver", mockSignalingMessageReceiver)
71+
setField("baseUrl", "https://test.example.com")
72+
setField("roomToken", "testRoom")
73+
// hasExternalSignalingServer = true skips the OfferAnswerNickProvider branch
74+
setField("hasExternalSignalingServer", true)
75+
}
76+
77+
// -----------------------------------------------------------------------------------------
78+
// Tests
79+
// -----------------------------------------------------------------------------------------
80+
81+
/**
82+
* The live [StateFlow] from [ParticipantHandler.uiState] must be passed to
83+
* [LocalStateBroadcaster.handleCallParticipantAdded], not just the current snapshot.
84+
*
85+
* Fails with the buggy code because the snapshot call goes to the
86+
* [handleCallParticipantAdded(ParticipantUiState)] overload, not the StateFlow one.
87+
*/
88+
@Test
89+
fun `addCallParticipant passes the live StateFlow to handleCallParticipantAdded`() {
90+
val sessionId = "robolectric-session-1"
91+
92+
invokeAddCallParticipant(sessionId)
93+
94+
val captor = argumentCaptor<StateFlow<ParticipantUiState>>()
95+
verify(mockLocalStateBroadcaster).handleCallParticipantAdded(captor.capture())
96+
97+
val capturedFlow = captor.firstValue
98+
99+
// The captured argument must be the SAME StateFlow object that ParticipantHandler
100+
// exposes — not a copy, not a one-shot MutableStateFlow wrapping the snapshot.
101+
assertSame(
102+
"Expected the live ParticipantHandler.uiState StateFlow, got a different object",
103+
callViewModel.getParticipant(sessionId)!!.uiState,
104+
capturedFlow
105+
)
106+
}
107+
108+
// -----------------------------------------------------------------------------------------
109+
// Reflection helpers
110+
// -----------------------------------------------------------------------------------------
111+
112+
private fun invokeAddCallParticipant(sessionId: String) {
113+
val method = CallActivity::class.java.getDeclaredMethod("addCallParticipant", String::class.java)
114+
method.isAccessible = true
115+
method.invoke(activity, sessionId)
116+
}
117+
118+
private fun setField(fieldName: String, value: Any?) {
119+
val field = findField(CallActivity::class.java, fieldName)
120+
?: error("Field '$fieldName' not found in CallActivity hierarchy")
121+
field.isAccessible = true
122+
field.set(activity, value)
123+
}
124+
125+
private fun findField(clazz: Class<*>, fieldName: String): Field? {
126+
var current: Class<*>? = clazz
127+
while (current != null) {
128+
try {
129+
return current.getDeclaredField(fieldName)
130+
} catch (e: NoSuchFieldException) {
131+
current = current.superclass
132+
}
133+
}
134+
return null
135+
}
136+
}

0 commit comments

Comments
 (0)