Skip to content

Add ActiveRoomHolder to manage the active room for a session #4758

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

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import io.element.android.libraries.matrix.api.core.RoomId
import io.element.android.libraries.matrix.api.core.UserId
import io.element.android.libraries.matrix.api.permalink.PermalinkData
import io.element.android.libraries.matrix.api.room.JoinedRoom
import io.element.android.services.appnavstate.api.ActiveRoomHolder
import io.element.android.services.appnavstate.api.AppNavigationStateService
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.launch
Expand All @@ -51,6 +52,7 @@ class JoinedRoomLoadedFlowNode @AssistedInject constructor(
private val appNavigationStateService: AppNavigationStateService,
private val appCoroutineScope: CoroutineScope,
private val matrixClient: MatrixClient,
private val activeRoomHolder: ActiveRoomHolder,
roomComponentFactory: RoomComponentFactory,
) : BaseFlowNode<JoinedRoomLoadedFlowNode.NavTarget>(
backstack = BackStack(
Expand Down Expand Up @@ -85,6 +87,7 @@ class JoinedRoomLoadedFlowNode @AssistedInject constructor(
onCreate = {
Timber.v("OnCreate => ${inputs.room.roomId}")
appNavigationStateService.onNavigateToRoom(id, inputs.room.roomId)
activeRoomHolder.addRoom(inputs.room)
fetchRoomMembers()
trackVisitedRoom()
},
Expand All @@ -95,6 +98,7 @@ class JoinedRoomLoadedFlowNode @AssistedInject constructor(
},
onDestroy = {
Timber.v("OnDestroy")
activeRoomHolder.removeRoom(inputs.room.sessionId)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not work well with permalink. If a room A opens a room B, when going back to room A, the holder will be empty.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still believe that this would be useful to provide the roomId to the removeRoom api, to double check that we are removing the correct room, and not the last added one.

Also one question, what will happen if there is an active call in the room, but the user closes the timeline? I guess the only issue is that there will be no cache, so not a real problem.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still believe that this would be useful to provide the roomId to the removeRoom api, to double check that we are removing the correct room, and not the last added one.

Makes sense.

Also one question, what will happen if there is an active call in the room, but the user closes the timeline? I guess the only issue is that there will be no cache, so not a real problem.

I'd say nothing really. A call needs the sync to work and an active timeline to start the call, but if it's already running there should be no issues, at least with the current features.

inputs.room.destroy()
appNavigationStateService.onLeavingRoom(id)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,16 +24,18 @@ import io.element.android.features.messages.api.MessagesEntryPoint
import io.element.android.features.roomdetails.api.RoomDetailsEntryPoint
import io.element.android.libraries.architecture.childNode
import io.element.android.libraries.matrix.api.room.JoinedRoom
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.room.FakeBaseRoom
import io.element.android.libraries.matrix.test.room.FakeJoinedRoom
import io.element.android.services.appnavstate.api.ActiveRoomHolder
import io.element.android.services.appnavstate.test.FakeAppNavigationStateService
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.test.runTest
import org.junit.Rule
import org.junit.Test

class JoinBaseRoomLoadedFlowNodeTest {
class JoinedRoomLoadedFlowNodeTest {
@get:Rule
val instantTaskExecutorRule = InstantTaskExecutorRule()

Expand Down Expand Up @@ -100,6 +102,7 @@ class JoinBaseRoomLoadedFlowNodeTest {
plugins: List<Plugin>,
messagesEntryPoint: MessagesEntryPoint = FakeMessagesEntryPoint(),
roomDetailsEntryPoint: RoomDetailsEntryPoint = FakeRoomDetailsEntryPoint(),
activeRoomHolder: ActiveRoomHolder = ActiveRoomHolder(),
coroutineScope: CoroutineScope,
) = JoinedRoomLoadedFlowNode(
buildContext = BuildContext.root(savedStateMap = null),
Expand All @@ -110,6 +113,7 @@ class JoinBaseRoomLoadedFlowNodeTest {
appCoroutineScope = coroutineScope,
roomComponentFactory = FakeRoomComponentFactory(),
matrixClient = FakeMatrixClient(),
activeRoomHolder = activeRoomHolder,
)

@Test
Expand Down Expand Up @@ -154,4 +158,55 @@ class JoinBaseRoomLoadedFlowNodeTest {
val roomDetailsNode = roomFlowNode.childNode(JoinedRoomLoadedFlowNode.NavTarget.RoomDetails)!!
assertThat(roomDetailsNode.id).isEqualTo(fakeRoomDetailsEntryPoint.nodeId)
}

@Test
fun `the ActiveRoomHolder will be updated with the loaded room on create`() = runTest {
// GIVEN
val room = FakeJoinedRoom(baseRoom = FakeBaseRoom(updateMembersResult = {}))
val fakeMessagesEntryPoint = FakeMessagesEntryPoint()
val fakeRoomDetailsEntryPoint = FakeRoomDetailsEntryPoint()
val inputs = JoinedRoomLoadedFlowNode.Inputs(room, RoomNavigationTarget.Messages())
val activeRoomHolder = ActiveRoomHolder()
val roomFlowNode = createJoinedRoomLoadedFlowNode(
plugins = listOf(inputs),
messagesEntryPoint = fakeMessagesEntryPoint,
roomDetailsEntryPoint = fakeRoomDetailsEntryPoint,
coroutineScope = this,
activeRoomHolder = activeRoomHolder,
)

assertThat(activeRoomHolder.getActiveRoom(A_SESSION_ID)).isNull()
val roomFlowNodeTestHelper = roomFlowNode.parentNodeTestHelper()
// WHEN
roomFlowNodeTestHelper.assertChildHasLifecycle(JoinedRoomLoadedFlowNode.NavTarget.Messages(null), Lifecycle.State.CREATED)
// THEN
assertThat(activeRoomHolder.getActiveRoom(A_SESSION_ID)).isNotNull()
}

@Test
fun `the ActiveRoomHolder will be removed on destroy`() = runTest {
// GIVEN
val room = FakeJoinedRoom(baseRoom = FakeBaseRoom(updateMembersResult = {}))
val fakeMessagesEntryPoint = FakeMessagesEntryPoint()
val fakeRoomDetailsEntryPoint = FakeRoomDetailsEntryPoint()
val inputs = JoinedRoomLoadedFlowNode.Inputs(room, RoomNavigationTarget.Messages())
val activeRoomHolder = ActiveRoomHolder().apply {
addRoom(room)
}
val roomFlowNode = createJoinedRoomLoadedFlowNode(
plugins = listOf(inputs),
messagesEntryPoint = fakeMessagesEntryPoint,
roomDetailsEntryPoint = fakeRoomDetailsEntryPoint,
coroutineScope = this,
activeRoomHolder = activeRoomHolder,
)
val roomFlowNodeTestHelper = roomFlowNode.parentNodeTestHelper()
roomFlowNodeTestHelper.assertChildHasLifecycle(JoinedRoomLoadedFlowNode.NavTarget.Messages(null), Lifecycle.State.CREATED)
assertThat(activeRoomHolder.getActiveRoom(A_SESSION_ID)).isNotNull()
// WHEN
roomFlowNode.updateLifecycleState(Lifecycle.State.DESTROYED)
// THEN
roomFlowNodeTestHelper.assertChildHasLifecycle(JoinedRoomLoadedFlowNode.NavTarget.Messages(null), Lifecycle.State.DESTROYED)
assertThat(activeRoomHolder.getActiveRoom(A_SESSION_ID)).isNull()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
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.appnavstate.api.ActiveRoomHolder
import io.element.android.services.appnavstate.api.AppForegroundStateService
import io.element.android.services.toolbox.api.systemclock.SystemClock
import kotlinx.coroutines.CoroutineScope
Expand All @@ -62,6 +63,7 @@
private val activeCallManager: ActiveCallManager,
private val languageTagProvider: LanguageTagProvider,
private val appForegroundStateService: AppForegroundStateService,
private val activeRoomHolder: ActiveRoomHolder,
private val appCoroutineScope: CoroutineScope,
) : Presenter<CallScreenState> {
@AssistedFactory
Expand Down Expand Up @@ -241,8 +243,13 @@

private suspend fun MatrixClient.notifyCallStartIfNeeded(roomId: RoomId) {
if (!notifiedCallStart) {
getJoinedRoom(roomId)?.use { it.sendCallNotificationIfNeeded() }
?.onSuccess { notifiedCallStart = true }
val activeRoomForSession = activeRoomHolder.getActiveRoom(sessionId)
val sendCallNotificationResult = if (activeRoomForSession?.roomId == roomId) {
activeRoomForSession.sendCallNotificationIfNeeded()

Check warning on line 248 in features/call/impl/src/main/kotlin/io/element/android/features/call/impl/ui/CallScreenPresenter.kt

View check run for this annotation

Codecov / codecov/patch

features/call/impl/src/main/kotlin/io/element/android/features/call/impl/ui/CallScreenPresenter.kt#L248

Added line #L248 was not covered by tests
} else {
getJoinedRoom(roomId)?.use { it.sendCallNotificationIfNeeded() }
}
sendCallNotificationResult?.onSuccess { notifiedCallStart = true }
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import io.element.android.libraries.matrix.api.core.RoomId
import io.element.android.libraries.matrix.api.core.SessionId
import io.element.android.libraries.matrix.api.widget.CallWidgetSettingsProvider
import io.element.android.libraries.preferences.api.store.AppPreferencesStore
import io.element.android.services.appnavstate.api.ActiveRoomHolder
import kotlinx.coroutines.flow.firstOrNull
import javax.inject.Inject

Expand All @@ -24,6 +25,7 @@ class DefaultCallWidgetProvider @Inject constructor(
private val matrixClientsProvider: MatrixClientProvider,
private val appPreferencesStore: AppPreferencesStore,
private val callWidgetSettingsProvider: CallWidgetSettingsProvider,
private val activeRoomHolder: ActiveRoomHolder,
) : CallWidgetProvider {
override suspend fun getWidget(
sessionId: SessionId,
Expand All @@ -33,7 +35,9 @@ class DefaultCallWidgetProvider @Inject constructor(
theme: String?,
): Result<CallWidgetProvider.GetWidgetResult> = runCatching {
val matrixClient = matrixClientsProvider.getOrRestore(sessionId).getOrThrow()
val room = matrixClient.getJoinedRoom(roomId) ?: error("Room not found")
val room = activeRoomHolder.getActiveRoom(sessionId)?.takeIf { it.roomId == roomId }
?: matrixClient.getJoinedRoom(roomId)
?: error("Room not found")

val customBaseUrl = appPreferencesStore.getCustomElementCallBaseUrlFlow().firstOrNull()
val baseUrl = customBaseUrl ?: EMBEDDED_CALL_WIDGET_BASE_URL
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ 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
import io.element.android.services.analytics.test.FakeScreenTracker
import io.element.android.services.appnavstate.api.ActiveRoomHolder
import io.element.android.services.appnavstate.test.FakeAppForegroundStateService
import io.element.android.services.toolbox.api.systemclock.SystemClock
import io.element.android.tests.testutils.WarmUpRule
Expand Down Expand Up @@ -367,6 +368,7 @@ import kotlin.time.Duration.Companion.seconds
activeCallManager: FakeActiveCallManager = FakeActiveCallManager(),
screenTracker: ScreenTracker = FakeScreenTracker(),
appForegroundStateService: FakeAppForegroundStateService = FakeAppForegroundStateService(),
activeRoomHolder: ActiveRoomHolder = ActiveRoomHolder(),
): CallScreenPresenter {
val userAgentProvider = object : UserAgentProvider {
override fun provide(): String {
Expand All @@ -387,6 +389,7 @@ import kotlin.time.Duration.Companion.seconds
languageTagProvider = FakeLanguageTagProvider("en-US"),
appForegroundStateService = appForegroundStateService,
appCoroutineScope = backgroundScope,
activeRoomHolder = activeRoomHolder,
)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,13 @@ 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.FakeBaseRoom
import io.element.android.libraries.matrix.test.room.FakeJoinedRoom
import io.element.android.libraries.matrix.test.widget.FakeCallWidgetSettingsProvider
import io.element.android.libraries.matrix.test.widget.FakeMatrixWidgetDriver
import io.element.android.libraries.preferences.api.store.AppPreferencesStore
import io.element.android.libraries.preferences.test.InMemoryAppPreferencesStore
import io.element.android.services.appnavstate.api.ActiveRoomHolder
import kotlinx.coroutines.test.runTest
import org.junit.Test

Expand Down Expand Up @@ -77,6 +79,23 @@ class DefaultCallWidgetProviderTest {
assertThat(provider.getWidget(A_SESSION_ID, A_ROOM_ID, "clientId", "languageTag", "theme").getOrNull()).isNotNull()
}

@Test
fun `getWidget - reuses the active room if possible`() = runTest {
val client = FakeMatrixClient().apply {
// No room from the client
givenGetRoomResult(A_ROOM_ID, null)
}
val activeRoomHolder = ActiveRoomHolder().apply {
// A current active room with the same room id
addRoom(FakeJoinedRoom(baseRoom = FakeBaseRoom(roomId = A_ROOM_ID)))
}
val provider = createProvider(
matrixClientProvider = FakeMatrixClientProvider { Result.success(client) },
activeRoomHolder = activeRoomHolder
)
assertThat(provider.getWidget(A_SESSION_ID, A_ROOM_ID, "clientId", "languageTag", "theme").isFailure).isTrue()
}

@Test
fun `getWidget - will use a custom base url if it exists`() = runTest {
val room = FakeJoinedRoom(
Expand Down Expand Up @@ -104,9 +123,11 @@ class DefaultCallWidgetProviderTest {
matrixClientProvider: MatrixClientProvider = FakeMatrixClientProvider(),
appPreferencesStore: AppPreferencesStore = InMemoryAppPreferencesStore(),
callWidgetSettingsProvider: CallWidgetSettingsProvider = FakeCallWidgetSettingsProvider(),
activeRoomHolder: ActiveRoomHolder = ActiveRoomHolder(),
) = DefaultCallWidgetProvider(
matrixClientsProvider = matrixClientProvider,
appPreferencesStore = appPreferencesStore,
callWidgetSettingsProvider = callWidgetSettingsProvider,
activeRoomHolder = activeRoomHolder,
)
}
1 change: 1 addition & 0 deletions features/preferences/impl/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ dependencies {
implementation(projects.features.roomlist.api)
implementation(projects.services.analytics.api)
implementation(projects.services.analytics.compose)
implementation(projects.services.appnavstate.api)
implementation(projects.services.toolbox.api)
implementation(libs.datetime)
implementation(libs.coil.compose)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import io.element.android.libraries.di.ApplicationContext
import io.element.android.libraries.di.SessionScope
import io.element.android.libraries.matrix.api.MatrixClient
import io.element.android.libraries.push.api.PushService
import io.element.android.services.appnavstate.api.ActiveRoomHolder
import kotlinx.coroutines.withContext
import okhttp3.OkHttpClient
import javax.inject.Inject
Expand All @@ -37,8 +38,11 @@ class DefaultClearCacheUseCase @Inject constructor(
private val ftueService: FtueService,
private val pushService: PushService,
private val seenInvitesStore: SeenInvitesStore,
private val activeRoomHolder: ActiveRoomHolder,
) : ClearCacheUseCase {
override suspend fun invoke() = withContext(coroutineDispatchers.io) {
// Active rooms should be disposed of before clearing the cache
activeRoomHolder.removeRoom(matrixClient.sessionId)?.destroy()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that activeRoomHolder stores a list of rooms, I think this code needs to be updated to destroy all the rooms.

// Clear Matrix cache
matrixClient.clearCache()
// Clear Coil cache
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,11 @@ import io.element.android.features.invite.test.InMemorySeenInvitesStore
import io.element.android.features.preferences.impl.DefaultCacheService
import io.element.android.libraries.matrix.api.core.SessionId
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.room.FakeJoinedRoom
import io.element.android.libraries.push.test.FakePushService
import io.element.android.services.appnavstate.api.ActiveRoomHolder
import io.element.android.tests.testutils.lambda.lambdaRecorder
import io.element.android.tests.testutils.lambda.value
import io.element.android.tests.testutils.testCoroutineDispatchers
Expand All @@ -31,8 +34,10 @@ import org.robolectric.RobolectricTestRunner
class DefaultClearCacheUseCaseTest {
@Test
fun `execute clear cache should do all the expected tasks`() = runTest {
val activeRoomHolder = ActiveRoomHolder().apply { addRoom(FakeJoinedRoom()) }
val clearCacheLambda = lambdaRecorder<Unit> { }
val matrixClient = FakeMatrixClient(
sessionId = A_SESSION_ID,
clearCacheLambda = clearCacheLambda,
)
val defaultCacheService = DefaultCacheService()
Expand All @@ -55,6 +60,7 @@ class DefaultClearCacheUseCaseTest {
ftueService = ftueService,
pushService = pushService,
seenInvitesStore = seenInvitesStore,
activeRoomHolder = activeRoomHolder,
)
defaultCacheService.clearedCacheEventFlow.test {
sut.invoke()
Expand All @@ -64,6 +70,7 @@ class DefaultClearCacheUseCaseTest {
.with(value(matrixClient.sessionId), value(false))
assertThat(awaitItem()).isEqualTo(matrixClient.sessionId)
assertThat(seenInvitesStore.seenRoomIds().first()).isEmpty()
assertThat(activeRoomHolder.getActiveRoom(A_SESSION_ID)).isNull()
}
}
}
1 change: 1 addition & 0 deletions features/share/impl/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ dependencies {
implementation(projects.libraries.roomselect.api)
implementation(projects.libraries.uiStrings)
implementation(projects.libraries.testtags)
implementation(projects.services.appnavstate.api)
api(libs.statemachine)
api(projects.features.share.api)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,11 @@ import io.element.android.libraries.architecture.runCatchingUpdatingState
import io.element.android.libraries.core.bool.orFalse
import io.element.android.libraries.matrix.api.MatrixClient
import io.element.android.libraries.matrix.api.core.RoomId
import io.element.android.libraries.matrix.api.room.JoinedRoom
import io.element.android.libraries.mediaupload.api.MediaPreProcessor
import io.element.android.libraries.mediaupload.api.MediaSender
import io.element.android.libraries.preferences.api.store.SessionPreferencesStore
import io.element.android.services.appnavstate.api.ActiveRoomHolder
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.launch

Expand All @@ -33,6 +35,7 @@ class SharePresenter @AssistedInject constructor(
private val matrixClient: MatrixClient,
private val mediaPreProcessor: MediaPreProcessor,
private val sessionPreferencesStore: SessionPreferencesStore,
private val activeRoomHolder: ActiveRoomHolder,
) : Presenter<ShareState> {
@AssistedFactory
interface Factory {
Expand All @@ -59,6 +62,12 @@ class SharePresenter @AssistedInject constructor(
)
}

private suspend fun getJoinedRoom(roomId: RoomId): JoinedRoom? {
return activeRoomHolder.getActiveRoom(matrixClient.sessionId)
?.takeIf { it.roomId == roomId }
?: matrixClient.getJoinedRoom(roomId)
}

private fun CoroutineScope.share(
intent: Intent,
roomIds: List<RoomId>,
Expand All @@ -72,7 +81,7 @@ class SharePresenter @AssistedInject constructor(
} else {
roomIds
.map { roomId ->
val room = matrixClient.getJoinedRoom(roomId) ?: return@map false
val room = getJoinedRoom(roomId) ?: return@map false
val mediaSender = MediaSender(
preProcessor = mediaPreProcessor,
room = room,
Expand All @@ -94,7 +103,7 @@ class SharePresenter @AssistedInject constructor(
onPlainText = { text ->
roomIds
.map { roomId ->
matrixClient.getJoinedRoom(roomId)?.liveTimeline?.sendMessage(
getJoinedRoom(roomId)?.liveTimeline?.sendMessage(
body = text,
htmlBody = null,
intentionalMentions = emptyList(),
Expand Down
Loading
Loading