Skip to content

Commit 2a347ea

Browse files
mahibimahibi
authored
Merge pull request #6243 from nextcloud/backport/6087/stable-24.0
[stable-24.0] fix: HPB partial updates no longer drop the call when a participant joins
2 parents 5c3c294 + c8f019e commit 2a347ea

2 files changed

Lines changed: 104 additions & 34 deletions

File tree

‎app/src/main/java/com/nextcloud/talk/call/CallParticipantList.java‎

Lines changed: 44 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -41,21 +41,54 @@ void onCallParticipantsChanged(Collection<Participant> joined, Collection<Partic
4141

4242
@Override
4343
public void onUsersInRoom(List<Participant> participants) {
44-
processParticipantList(participants);
44+
// Internal signaling: full participant list; participants absent from the list have left.
45+
Collection<Participant> joined = new ArrayList<>();
46+
Collection<Participant> updated = new ArrayList<>();
47+
Collection<Participant> left = new ArrayList<>();
48+
Collection<Participant> unchanged = new ArrayList<>();
49+
50+
Collection<Participant> notFound = classifyParticipants(participants, joined, updated, left, unchanged);
51+
52+
for (Participant callParticipant : notFound) {
53+
callParticipants.remove(callParticipant.getSessionId());
54+
// No need to copy it, as it will be no longer used.
55+
callParticipant.setInCall(Participant.InCallFlags.DISCONNECTED);
56+
}
57+
left.addAll(notFound);
58+
59+
notifyIfChanged(joined, updated, left, unchanged);
4560
}
4661

4762
@Override
4863
public void onParticipantsUpdate(List<Participant> participants) {
49-
processParticipantList(participants);
50-
}
51-
52-
private void processParticipantList(List<Participant> participants) {
64+
// HPB external signaling: partial update containing only changed participants.
65+
// Participants absent from this list are still in the call — move them to "unchanged"
66+
// so that observers (e.g. CallActivity) see the full set of active participants.
5367
Collection<Participant> joined = new ArrayList<>();
5468
Collection<Participant> updated = new ArrayList<>();
5569
Collection<Participant> left = new ArrayList<>();
5670
Collection<Participant> unchanged = new ArrayList<>();
5771

58-
Collection<Participant> knownCallParticipantsNotFound = new ArrayList<>(callParticipants.values());
72+
Collection<Participant> notFound = classifyParticipants(participants, joined, updated, left, unchanged);
73+
74+
for (Participant callParticipant : notFound) {
75+
unchanged.add(copyParticipant(callParticipant));
76+
}
77+
78+
notifyIfChanged(joined, updated, left, unchanged);
79+
}
80+
81+
/**
82+
* Classifies each participant in the given list into joined/updated/left/unchanged based on
83+
* the current known call participants. Returns the set of previously known participants that
84+
* were not present in the list (callers decide what to do with them).
85+
*/
86+
private Collection<Participant> classifyParticipants(List<Participant> participants,
87+
Collection<Participant> joined,
88+
Collection<Participant> updated,
89+
Collection<Participant> left,
90+
Collection<Participant> unchanged) {
91+
Collection<Participant> notFound = new ArrayList<>(callParticipants.values());
5992

6093
for (Participant participant : participants) {
6194
String sessionId = participant.getSessionId();
@@ -78,17 +111,15 @@ private void processParticipantList(List<Participant> participants) {
78111
}
79112

80113
if (knownCallParticipant) {
81-
knownCallParticipantsNotFound.remove(callParticipant);
114+
notFound.remove(callParticipant);
82115
}
83116
}
84117

85-
for (Participant callParticipant : knownCallParticipantsNotFound) {
86-
callParticipants.remove(callParticipant.getSessionId());
87-
// No need to copy it, as it will be no longer used.
88-
callParticipant.setInCall(Participant.InCallFlags.DISCONNECTED);
89-
}
90-
left.addAll(knownCallParticipantsNotFound);
118+
return notFound;
119+
}
91120

121+
private void notifyIfChanged(Collection<Participant> joined, Collection<Participant> updated,
122+
Collection<Participant> left, Collection<Participant> unchanged) {
92123
if (!joined.isEmpty() || !updated.isEmpty() || !left.isEmpty()) {
93124
callParticipantListNotifier.notifyChanged(joined, updated, left, unchanged);
94125
}

‎app/src/test/java/com/nextcloud/talk/call/CallParticipantListExternalSignalingTest.java‎

Lines changed: 60 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -53,15 +53,20 @@ public class CallParticipantListExternalSignalingTest {
5353
private Collection<Participant> expectedLeft;
5454
private Collection<Participant> expectedUnchanged;
5555

56-
// The order of the left participants in some tests depends on how they are internally sorted by the map, so the
57-
// list of left participants needs to be checked ignoring the sorting (or, rather, sorting by session ID as in
58-
// expectedLeft).
56+
// The order of the left/unchanged participants in some tests depends on how they are internally sorted by the map,
57+
// so the lists need to be checked ignoring the sorting (or, rather, sorting by session ID as in expectedLeft/
58+
// expectedUnchanged).
5959
// Other tests can just relay on the not guaranteed, but known internal sorting of the elements.
6060
private final ArgumentMatcher<List<Participant>> matchesExpectedLeftIgnoringOrder = left -> {
6161
Collections.sort(left, Comparator.comparing(Participant::getSessionId));
6262
return expectedLeft.equals(left);
6363
};
6464

65+
private final ArgumentMatcher<List<Participant>> matchesExpectedUnchangedIgnoringOrder = unchanged -> {
66+
Collections.sort(unchanged, Comparator.comparing(Participant::getSessionId));
67+
return expectedUnchanged.equals(unchanged);
68+
};
69+
6570
private static class ParticipantsUpdateParticipantBuilder {
6671
private Participant newUser(long inCall, long lastPing, String sessionId, Participant.ParticipantType type,
6772
String userId) {
@@ -485,26 +490,25 @@ public void testParticipantsUpdateLeaveCallThenLeaveRoomSeveralParticipants() {
485490
}
486491

487492
@Test
488-
public void testParticipantsUpdateLeaveCallAndRoom() {
493+
public void testParticipantsUpdateEmptyListDoesNotEvictExistingParticipants() {
494+
// With HPB, onParticipantsUpdate is a partial update. An empty list means "nothing changed", not
495+
// "everyone left". Absent participants must remain in the call.
489496
List<Participant> participants = new ArrayList<>();
490497
participants.add(builder.newUser(IN_CALL | WITH_AUDIO, 1, "theSessionId1", MODERATOR, "theUserId1"));
491498

492499
participantListMessageListener.onParticipantsUpdate(participants);
493500

494501
callParticipantList.addObserver(mockedCallParticipantListObserver);
495502

496-
participants = new ArrayList<>();
497-
498-
participantListMessageListener.onParticipantsUpdate(participants);
503+
participantListMessageListener.onParticipantsUpdate(new ArrayList<>());
499504

500-
expectedLeft.add(builder.newUser(DISCONNECTED, 1, "theSessionId1", MODERATOR, "theUserId1"));
501-
502-
verify(mockedCallParticipantListObserver, only()).onCallParticipantsChanged(expectedJoined, expectedUpdated,
503-
expectedLeft, expectedUnchanged);
505+
verifyNoInteractions(mockedCallParticipantListObserver);
504506
}
505507

506508
@Test
507-
public void testParticipantsUpdateLeaveCallAndRoomSeveralParticipants() {
509+
public void testParticipantsUpdatePartialListDoesNotEvictAbsentParticipants() {
510+
// With HPB, onParticipantsUpdate is a partial update. Participants absent from the list are still
511+
// in the call — they just have no state change to report in this update.
508512
List<Participant> participants = new ArrayList<>();
509513
participants.add(builder.newUser(IN_CALL | WITH_AUDIO, 1, "theSessionId1", MODERATOR, "theUserId1"));
510514
participants.add(builder.newGuest(IN_CALL, 2, "theSessionId2", GUEST));
@@ -515,18 +519,16 @@ public void testParticipantsUpdateLeaveCallAndRoomSeveralParticipants() {
515519

516520
callParticipantList.addObserver(mockedCallParticipantListObserver);
517521

522+
// Partial update: only sessions 3 and 4 are included; sessions 1 and 2 are absent but still in call.
518523
participants = new ArrayList<>();
519524
participants.add(builder.newUser(DISCONNECTED, 3, "theSessionId3", USER, "theUserId3"));
520525
participants.add(builder.newUser(IN_CALL, 4, "theSessionId4", USER, "theUserId4"));
521526

522527
participantListMessageListener.onParticipantsUpdate(participants);
523528

524-
expectedLeft.add(builder.newUser(DISCONNECTED, 1, "theSessionId1", MODERATOR, "theUserId1"));
525-
expectedLeft.add(builder.newGuest(DISCONNECTED, 2, "theSessionId2", GUEST));
526-
expectedUnchanged.add(builder.newUser(IN_CALL, 4, "theSessionId4", USER, "theUserId4"));
527-
528-
verify(mockedCallParticipantListObserver).onCallParticipantsChanged(eq(expectedJoined), eq(expectedUpdated),
529-
argThat(matchesExpectedLeftIgnoringOrder), eq(expectedUnchanged));
529+
// Sessions 1 and 2 must NOT appear in left. Session 4 is unchanged.
530+
// No joined/updated/left → no notification.
531+
verifyNoInteractions(mockedCallParticipantListObserver);
530532
}
531533

532534
@Test
@@ -547,7 +549,7 @@ public void testParticipantsUpdateSeveralEventsSeveralParticipants() {
547549
callParticipantList.addObserver(mockedCallParticipantListObserver);
548550

549551
participants = new ArrayList<>();
550-
// theSessionId1 is gone.
552+
// theSessionId1 is absent from this partial update but is still in the call (not explicitly disconnected).
551553
participants.add(builder.newGuest(DISCONNECTED, 2, "theSessionId2", GUEST));
552554
participants.add(builder.newUser(DISCONNECTED, 3, "theSessionId3", USER, "theUserId3"));
553555
participants.add(builder.newUser(IN_CALL, 4, "theSessionId4", USER, "theUserId4"));
@@ -563,14 +565,51 @@ public void testParticipantsUpdateSeveralEventsSeveralParticipants() {
563565
expectedJoined.add(builder.newUser(IN_CALL, 8, "theSessionId8", USER, "theUserId8"));
564566
expectedUpdated.add(builder.newUser(IN_CALL | WITH_AUDIO | WITH_VIDEO, 5, "theSessionId5", OWNER, "theUserId5"));
565567
expectedUpdated.add(builder.newGuest(IN_CALL, 7, "theSessionId7", GUEST));
566-
expectedLeft.add(builder.newUser(DISCONNECTED, 1, "theSessionId1", MODERATOR, "theUserId1"));
568+
// theSessionId1 absent from partial update → NOT in left, but in unchanged.
569+
// theSessionId2 explicitly DISCONNECTED → left.
567570
expectedLeft.add(builder.newGuest(DISCONNECTED, 2, "theSessionId2", GUEST));
571+
// Sorted by sessionId for matchesExpectedUnchangedIgnoringOrder.
572+
expectedUnchanged.add(builder.newUser(IN_CALL | WITH_AUDIO, 1, "theSessionId1", MODERATOR, "theUserId1"));
568573
expectedUnchanged.add(builder.newUser(IN_CALL, 4, "theSessionId4", USER, "theUserId4"));
569574
// Last ping and participant type are not seen as changed, even if they did.
570575
expectedUnchanged.add(builder.newUser(IN_CALL | WITH_AUDIO, 42, "theSessionId9", USER, "theUserId9"));
571576

572577
verify(mockedCallParticipantListObserver).onCallParticipantsChanged(eq(expectedJoined), eq(expectedUpdated),
573-
argThat(matchesExpectedLeftIgnoringOrder), eq(expectedUnchanged));
578+
argThat(matchesExpectedLeftIgnoringOrder),
579+
argThat(matchesExpectedUnchangedIgnoringOrder));
580+
}
581+
582+
/**
583+
* Regression test for HPB partial update bug: when a new participant joins (partial update containing only that
584+
* participant), existing participants absent from the update must NOT be moved to "left".
585+
*
586+
* With internal signaling (usersInRoom), an absent participant means they left. With HPB external signaling
587+
* (participantsUpdate), the update is partial — only changed participants are included; absent ones are still
588+
* in the call.
589+
*/
590+
@Test
591+
public void testParticipantsUpdateNewParticipantJoinsDoesNotEvictExisting() {
592+
// Participant 1 (self / Android) joins the call first.
593+
List<Participant> participants = new ArrayList<>();
594+
participants.add(builder.newUser(IN_CALL | WITH_AUDIO, 1, "theSessionId1", MODERATOR, "theUserId1"));
595+
596+
participantListMessageListener.onParticipantsUpdate(participants);
597+
598+
callParticipantList.addObserver(mockedCallParticipantListObserver);
599+
600+
// Participant 2 (browser) joins: HPB sends a partial update containing ONLY participant 2.
601+
// Participant 1 is absent from this update but is still in the call.
602+
participants = new ArrayList<>();
603+
participants.add(builder.newUser(IN_CALL | WITH_AUDIO, 2, "theSessionId2", USER, "theUserId2"));
604+
605+
participantListMessageListener.onParticipantsUpdate(participants);
606+
607+
// Participant 2 should be in "joined"; participant 1 must NOT appear in "left" but in "unchanged".
608+
expectedJoined.add(builder.newUser(IN_CALL | WITH_AUDIO, 2, "theSessionId2", USER, "theUserId2"));
609+
expectedUnchanged.add(builder.newUser(IN_CALL | WITH_AUDIO, 1, "theSessionId1", MODERATOR, "theUserId1"));
610+
611+
verify(mockedCallParticipantListObserver, only()).onCallParticipantsChanged(expectedJoined, expectedUpdated,
612+
expectedLeft, expectedUnchanged);
574613
}
575614

576615
@Test

0 commit comments

Comments
 (0)