From d77ff60f03a1032cf7e5a8fefd765a3c1a1e892c Mon Sep 17 00:00:00 2001 From: ganfra Date: Tue, 11 Jul 2023 11:40:55 +0200 Subject: [PATCH 01/20] Coroutine: remove diffUpdateDispatcher, not used anymore --- app/src/main/kotlin/io/element/android/x/di/AppModule.kt | 3 --- .../android/libraries/core/coroutine/CoroutineDispatchers.kt | 1 - .../kotlin/io/element/android/samples/minimal/Singleton.kt | 3 --- .../android/tests/testutils/TestCoroutineDispatchers.kt | 2 -- 4 files changed, 9 deletions(-) diff --git a/app/src/main/kotlin/io/element/android/x/di/AppModule.kt b/app/src/main/kotlin/io/element/android/x/di/AppModule.kt index 89ab50d32a..a1d0b50522 100644 --- a/app/src/main/kotlin/io/element/android/x/di/AppModule.kt +++ b/app/src/main/kotlin/io/element/android/x/di/AppModule.kt @@ -37,10 +37,8 @@ import kotlinx.coroutines.CoroutineName import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.MainScope -import kotlinx.coroutines.asCoroutineDispatcher import kotlinx.coroutines.plus import java.io.File -import java.util.concurrent.Executors @Module @ContributesTo(AppScope::class) @@ -99,7 +97,6 @@ object AppModule { io = Dispatchers.IO, computation = Dispatchers.Default, main = Dispatchers.Main, - diffUpdateDispatcher = Executors.newSingleThreadExecutor().asCoroutineDispatcher() ) } diff --git a/libraries/core/src/main/kotlin/io/element/android/libraries/core/coroutine/CoroutineDispatchers.kt b/libraries/core/src/main/kotlin/io/element/android/libraries/core/coroutine/CoroutineDispatchers.kt index a3f1586070..918b6cda8a 100644 --- a/libraries/core/src/main/kotlin/io/element/android/libraries/core/coroutine/CoroutineDispatchers.kt +++ b/libraries/core/src/main/kotlin/io/element/android/libraries/core/coroutine/CoroutineDispatchers.kt @@ -22,5 +22,4 @@ data class CoroutineDispatchers( val io: CoroutineDispatcher, val computation: CoroutineDispatcher, val main: CoroutineDispatcher, - val diffUpdateDispatcher: CoroutineDispatcher, ) diff --git a/samples/minimal/src/main/kotlin/io/element/android/samples/minimal/Singleton.kt b/samples/minimal/src/main/kotlin/io/element/android/samples/minimal/Singleton.kt index 006ca8d9e4..5f8c6555a5 100644 --- a/samples/minimal/src/main/kotlin/io/element/android/samples/minimal/Singleton.kt +++ b/samples/minimal/src/main/kotlin/io/element/android/samples/minimal/Singleton.kt @@ -22,10 +22,8 @@ import io.element.android.libraries.matrix.api.tracing.TracingConfigurations import kotlinx.coroutines.CoroutineName import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.MainScope -import kotlinx.coroutines.asCoroutineDispatcher import kotlinx.coroutines.plus import timber.log.Timber -import java.util.concurrent.Executors object Singleton { @@ -39,6 +37,5 @@ object Singleton { io = Dispatchers.IO, computation = Dispatchers.Default, main = Dispatchers.Main, - diffUpdateDispatcher = Executors.newSingleThreadExecutor().asCoroutineDispatcher(), ) } diff --git a/tests/testutils/src/main/kotlin/io/element/android/tests/testutils/TestCoroutineDispatchers.kt b/tests/testutils/src/main/kotlin/io/element/android/tests/testutils/TestCoroutineDispatchers.kt index b8376ddf6f..1fdb5879fc 100644 --- a/tests/testutils/src/main/kotlin/io/element/android/tests/testutils/TestCoroutineDispatchers.kt +++ b/tests/testutils/src/main/kotlin/io/element/android/tests/testutils/TestCoroutineDispatchers.kt @@ -37,12 +37,10 @@ fun TestScope.testCoroutineDispatchers( io = UnconfinedTestDispatcher(testScheduler), computation = UnconfinedTestDispatcher(testScheduler), main = UnconfinedTestDispatcher(testScheduler), - diffUpdateDispatcher = UnconfinedTestDispatcher(testScheduler), ) false -> CoroutineDispatchers( io = StandardTestDispatcher(testScheduler), computation = StandardTestDispatcher(testScheduler), main = StandardTestDispatcher(testScheduler), - diffUpdateDispatcher = StandardTestDispatcher(testScheduler), ) } From 4012317e4039d1a58f40822cc4f0107796944465 Mon Sep 17 00:00:00 2001 From: ganfra Date: Tue, 11 Jul 2023 11:41:24 +0200 Subject: [PATCH 02/20] Coroutine: introduce scoped dispatcher with limitedParalellism --- .../libraries/matrix/impl/RustMatrixClient.kt | 31 +++++---- .../matrix/impl/media/RustMediaLoader.kt | 11 ++-- .../matrix/impl/room/RustMatrixRoom.kt | 65 ++++++++++--------- .../impl/room/RustRoomSummaryDataSource.kt | 6 +- .../impl/timeline/RustMatrixTimeline.kt | 10 +-- 5 files changed, 68 insertions(+), 55 deletions(-) diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/RustMatrixClient.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/RustMatrixClient.kt index 7a2f7ea6b8..75beba0901 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/RustMatrixClient.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/RustMatrixClient.kt @@ -54,7 +54,7 @@ import io.element.android.libraries.matrix.impl.verification.RustSessionVerifica import io.element.android.libraries.sessionstorage.api.SessionStore import io.element.android.services.toolbox.api.systemclock.SystemClock import kotlinx.coroutines.CoroutineScope -import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.cancel import kotlinx.coroutines.flow.filter import kotlinx.coroutines.flow.first @@ -73,6 +73,7 @@ import org.matrix.rustcomponents.sdk.CreateRoomParameters as RustCreateRoomParam import org.matrix.rustcomponents.sdk.RoomPreset as RustRoomPreset import org.matrix.rustcomponents.sdk.RoomVisibility as RustRoomVisibility +@OptIn(ExperimentalCoroutinesApi::class) class RustMatrixClient constructor( private val client: Client, private val sessionStore: SessionStore, @@ -85,6 +86,7 @@ class RustMatrixClient constructor( override val sessionId: UserId = UserId(client.userId()) private val roomListService = client.roomListServiceWithEncryption() + private val sessionDispatcher = dispatchers.io.limitedParallelism(64) private val sessionCoroutineScope = appCoroutineScope.childScope(dispatchers.main, "Session-${sessionId}") private val verificationService = RustSessionVerificationService() private val syncService = RustSyncService(roomListService, sessionCoroutineScope) @@ -92,6 +94,7 @@ class RustMatrixClient constructor( client = client, dispatchers = dispatchers, ) + private val notificationService = RustNotificationService(client) private val clientDelegate = object : ClientDelegate { @@ -105,7 +108,7 @@ class RustMatrixClient constructor( RustRoomSummaryDataSource( roomListService = roomListService, sessionCoroutineScope = sessionCoroutineScope, - coroutineDispatchers = dispatchers, + dispatcher = sessionDispatcher, ) override val roomSummaryDataSource: RoomSummaryDataSource @@ -150,7 +153,7 @@ class RustMatrixClient constructor( ) } - private suspend fun pairOfRoom(roomId: RoomId): Pair? = withContext(dispatchers.io) { + private suspend fun pairOfRoom(roomId: RoomId): Pair? = withContext(sessionDispatcher) { val cachedRoomListItem = roomListService.roomOrNull(roomId.value) val fullRoom = cachedRoomListItem?.fullRoom() if (cachedRoomListItem == null || fullRoom == null) { @@ -165,19 +168,19 @@ class RustMatrixClient constructor( return roomId?.let { getRoom(it) } } - override suspend fun ignoreUser(userId: UserId): Result = withContext(dispatchers.io) { + override suspend fun ignoreUser(userId: UserId): Result = withContext(sessionDispatcher) { runCatching { client.ignoreUser(userId.value) } } - override suspend fun unignoreUser(userId: UserId): Result = withContext(dispatchers.io) { + override suspend fun unignoreUser(userId: UserId): Result = withContext(sessionDispatcher) { runCatching { client.unignoreUser(userId.value) } } - override suspend fun createRoom(createRoomParams: CreateRoomParameters): Result = withContext(dispatchers.io) { + override suspend fun createRoom(createRoomParams: CreateRoomParameters): Result = withContext(sessionDispatcher) { runCatching { val rustParams = RustCreateRoomParameters( name = createRoomParams.name, @@ -221,14 +224,14 @@ class RustMatrixClient constructor( return createRoom(createRoomParams) } - override suspend fun getProfile(userId: UserId): Result = withContext(Dispatchers.IO) { + override suspend fun getProfile(userId: UserId): Result = withContext(sessionDispatcher) { runCatching { client.getProfile(userId.value).let(UserProfileMapper::map) } } override suspend fun searchUsers(searchTerm: String, limit: Long): Result = - withContext(dispatchers.io) { + withContext(sessionDispatcher) { runCatching { client.searchUsers(searchTerm, limit.toULong()).let(UserSearchResultMapper::map) } @@ -260,7 +263,7 @@ class RustMatrixClient constructor( baseDirectory.deleteSessionDirectory(userID = sessionId.value, deleteCryptoDb = false) } - override suspend fun logout() = withContext(dispatchers.io) { + override suspend fun logout() = withContext(sessionDispatcher) { try { client.logout() } catch (failure: Throwable) { @@ -271,20 +274,20 @@ class RustMatrixClient constructor( sessionStore.removeSession(sessionId.value) } - override suspend fun loadUserDisplayName(): Result = withContext(dispatchers.io) { + override suspend fun loadUserDisplayName(): Result = withContext(sessionDispatcher) { runCatching { client.displayName() } } - override suspend fun loadUserAvatarURLString(): Result = withContext(dispatchers.io) { + override suspend fun loadUserAvatarURLString(): Result = withContext(sessionDispatcher) { runCatching { client.avatarUrl() } } @OptIn(ExperimentalUnsignedTypes::class) - override suspend fun uploadMedia(mimeType: String, data: ByteArray, progressCallback: ProgressCallback?): Result = withContext(dispatchers.io) { + override suspend fun uploadMedia(mimeType: String, data: ByteArray, progressCallback: ProgressCallback?): Result = withContext(sessionDispatcher) { runCatching { client.uploadMedia(mimeType, data.toUByteArray().toList(), progressCallback?.toProgressWatcher()) } @@ -305,7 +308,7 @@ class RustMatrixClient constructor( private suspend fun File.getCacheSize( userID: String, includeCryptoDb: Boolean = false, - ): Long = withContext(dispatchers.io) { + ): Long = withContext(sessionDispatcher) { // Rust sanitises the user ID replacing invalid characters with an _ val sanitisedUserID = userID.replace(":", "_") val sessionDirectory = File(this@getCacheSize, sanitisedUserID) @@ -327,7 +330,7 @@ class RustMatrixClient constructor( private suspend fun File.deleteSessionDirectory( userID: String, deleteCryptoDb: Boolean = false, - ): Boolean = withContext(dispatchers.io) { + ): Boolean = withContext(sessionDispatcher) { // Rust sanitises the user ID replacing invalid characters with an _ val sanitisedUserID = userID.replace(":", "_") val sessionDirectory = File(this@deleteSessionDirectory, sanitisedUserID) diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/media/RustMediaLoader.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/media/RustMediaLoader.kt index 213cab2256..c958810f5e 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/media/RustMediaLoader.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/media/RustMediaLoader.kt @@ -20,6 +20,7 @@ import io.element.android.libraries.core.coroutine.CoroutineDispatchers import io.element.android.libraries.matrix.api.media.MatrixMediaLoader import io.element.android.libraries.matrix.api.media.MediaFile import io.element.android.libraries.matrix.api.media.MediaSource +import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.withContext import org.matrix.rustcomponents.sdk.Client import org.matrix.rustcomponents.sdk.mediaSourceFromUrl @@ -29,10 +30,12 @@ import org.matrix.rustcomponents.sdk.MediaSource as RustMediaSource class RustMediaLoader( baseCacheDirectory: File, - private val dispatchers: CoroutineDispatchers, + dispatchers: CoroutineDispatchers, private val innerClient: Client, ) : MatrixMediaLoader { + @OptIn(ExperimentalCoroutinesApi::class) + private val mediaDispatcher = dispatchers.io.limitedParallelism(32) private val cacheDirectory = File(baseCacheDirectory, "temp/media").apply { if (!exists()) { mkdirs() @@ -41,7 +44,7 @@ class RustMediaLoader( @OptIn(ExperimentalUnsignedTypes::class) override suspend fun loadMediaContent(source: MediaSource): Result = - withContext(dispatchers.io) { + withContext(mediaDispatcher) { runCatching { source.toRustMediaSource().use { source -> innerClient.getMediaContent(source).toUByteArray().toByteArray() @@ -55,7 +58,7 @@ class RustMediaLoader( width: Long, height: Long ): Result = - withContext(dispatchers.io) { + withContext(mediaDispatcher) { runCatching { source.toRustMediaSource().use { mediaSource -> innerClient.getMediaThumbnail( @@ -68,7 +71,7 @@ class RustMediaLoader( } override suspend fun downloadMediaFile(source: MediaSource, mimeType: String?, body: String?): Result = - withContext(dispatchers.io) { + withContext(mediaDispatcher) { runCatching { source.toRustMediaSource().use { mediaSource -> val mediaFile = innerClient.getMediaFile( diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/room/RustMatrixRoom.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/room/RustMatrixRoom.kt index 378e9ae372..11d75641c5 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/room/RustMatrixRoom.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/room/RustMatrixRoom.kt @@ -23,7 +23,6 @@ import io.element.android.libraries.matrix.api.core.ProgressCallback 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.core.UserId -import io.element.android.libraries.matrix.api.room.location.AssetType import io.element.android.libraries.matrix.api.media.AudioInfo import io.element.android.libraries.matrix.api.media.FileInfo import io.element.android.libraries.matrix.api.media.ImageInfo @@ -32,17 +31,19 @@ import io.element.android.libraries.matrix.api.room.MatrixRoom import io.element.android.libraries.matrix.api.room.MatrixRoomMembersState import io.element.android.libraries.matrix.api.room.MessageEventType import io.element.android.libraries.matrix.api.room.StateEventType +import io.element.android.libraries.matrix.api.room.location.AssetType import io.element.android.libraries.matrix.api.room.roomMembers import io.element.android.libraries.matrix.api.timeline.MatrixTimeline import io.element.android.libraries.matrix.api.timeline.item.event.EventType import io.element.android.libraries.matrix.impl.core.toProgressWatcher -import io.element.android.libraries.matrix.impl.room.location.toInner import io.element.android.libraries.matrix.impl.media.map +import io.element.android.libraries.matrix.impl.room.location.toInner import io.element.android.libraries.matrix.impl.timeline.RustMatrixTimeline import io.element.android.libraries.matrix.impl.timeline.backPaginationStatusFlow import io.element.android.libraries.matrix.impl.timeline.timelineDiffFlow import io.element.android.services.toolbox.api.systemclock.SystemClock import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.cancel import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow @@ -62,6 +63,7 @@ import org.matrix.rustcomponents.sdk.messageEventContentFromMarkdown import timber.log.Timber import java.io.File +@OptIn(ExperimentalCoroutinesApi::class) class RustMatrixRoom( override val sessionId: SessionId, private val roomListItem: RoomListItem, @@ -74,6 +76,11 @@ class RustMatrixRoom( override val roomId = RoomId(innerRoom.id()) + // Create a dispatcher for all room methods... + private val roomDispatcher = coroutineDispatchers.io.limitedParallelism(32) + //...except getMember methods as it could quickly fill the roomDispatcher... + private val roomMembersDispatcher = coroutineDispatchers.io.limitedParallelism(8) + private val roomCoroutineScope = sessionCoroutineScope.childScope(coroutineDispatchers.main, "RoomScope-$roomId") private val _membersStateFlow = MutableStateFlow(MatrixRoomMembersState.Unknown) private val isInit = MutableStateFlow(false) @@ -83,7 +90,7 @@ class RustMatrixRoom( matrixRoom = this, innerRoom = innerRoom, roomCoroutineScope = roomCoroutineScope, - coroutineDispatchers = coroutineDispatchers + dispatcher = roomDispatcher ) } @@ -105,7 +112,7 @@ class RustMatrixRoom( timelineLimit = null ) roomListItem.subscribe(settings) - roomCoroutineScope.launch(coroutineDispatchers.computation) { + roomCoroutineScope.launch(roomDispatcher) { innerRoom.timelineDiffFlow { initialList -> _timeline.postItems(initialList) }.onEach { @@ -175,7 +182,7 @@ class RustMatrixRoom( override val activeMemberCount: Long get() = innerRoom.activeMembersCount().toLong() - override suspend fun updateMembers(): Result = withContext(coroutineDispatchers.io) { + override suspend fun updateMembers(): Result = withContext(roomMembersDispatcher) { val currentState = _membersStateFlow.value val currentMembers = currentState.roomMembers() _membersStateFlow.value = MatrixRoomMembersState.Pending(prevRoomMembers = currentMembers) @@ -189,20 +196,20 @@ class RustMatrixRoom( } override suspend fun userDisplayName(userId: UserId): Result = - withContext(coroutineDispatchers.io) { + withContext(roomDispatcher) { runCatching { innerRoom.memberDisplayName(userId.value) } } override suspend fun userAvatarUrl(userId: UserId): Result = - withContext(coroutineDispatchers.io) { + withContext(roomDispatcher) { runCatching { innerRoom.memberAvatarUrl(userId.value) } } - override suspend fun sendMessage(message: String): Result = withContext(coroutineDispatchers.io) { + override suspend fun sendMessage(message: String): Result = withContext(roomDispatcher) { val transactionId = genTransactionId() messageEventContentFromMarkdown(message).use { content -> runCatching { @@ -211,7 +218,7 @@ class RustMatrixRoom( } } - override suspend fun editMessage(originalEventId: EventId?, transactionId: String?, message: String): Result = withContext(coroutineDispatchers.io) { + override suspend fun editMessage(originalEventId: EventId?, transactionId: String?, message: String): Result = withContext(roomDispatcher) { if (originalEventId != null) { runCatching { innerRoom.edit(/* TODO use content */ message, originalEventId.value, transactionId) @@ -224,7 +231,7 @@ class RustMatrixRoom( } } - override suspend fun replyMessage(eventId: EventId, message: String): Result = withContext(coroutineDispatchers.io) { + override suspend fun replyMessage(eventId: EventId, message: String): Result = withContext(roomDispatcher) { val transactionId = genTransactionId() // val content = messageEventContentFromMarkdown(message) runCatching { @@ -232,50 +239,50 @@ class RustMatrixRoom( } } - override suspend fun redactEvent(eventId: EventId, reason: String?) = withContext(coroutineDispatchers.io) { + override suspend fun redactEvent(eventId: EventId, reason: String?) = withContext(roomDispatcher) { val transactionId = genTransactionId() runCatching { innerRoom.redact(eventId.value, reason, transactionId) } } - override suspend fun leave(): Result = withContext(coroutineDispatchers.io) { + override suspend fun leave(): Result = withContext(roomDispatcher) { runCatching { innerRoom.leave() } } - override suspend fun acceptInvitation(): Result = withContext(coroutineDispatchers.io) { + override suspend fun acceptInvitation(): Result = withContext(roomDispatcher) { runCatching { innerRoom.acceptInvitation() } } - override suspend fun rejectInvitation(): Result = withContext(coroutineDispatchers.io) { + override suspend fun rejectInvitation(): Result = withContext(roomDispatcher) { runCatching { innerRoom.rejectInvitation() } } - override suspend fun inviteUserById(id: UserId): Result = withContext(coroutineDispatchers.io) { + override suspend fun inviteUserById(id: UserId): Result = withContext(roomDispatcher) { runCatching { innerRoom.inviteUserById(id.value) } } - override suspend fun canInvite(): Result = withContext(coroutineDispatchers.io) { + override suspend fun canInvite(): Result = withContext(roomMembersDispatcher) { runCatching { innerRoom.member(sessionId.value).use(RoomMember::canInvite) } } - override suspend fun canSendStateEvent(type: StateEventType): Result = withContext(coroutineDispatchers.io) { + override suspend fun canSendStateEvent(type: StateEventType): Result = withContext(roomMembersDispatcher) { runCatching { innerRoom.member(sessionId.value).use { it.canSendState(type.map()) } } } - override suspend fun canSendEvent(type: MessageEventType): Result = withContext(coroutineDispatchers.io) { + override suspend fun canSendEvent(type: MessageEventType): Result = withContext(roomMembersDispatcher) { runCatching { innerRoom.member(sessionId.value).use { it.canSendMessage(type.map()) } } @@ -305,13 +312,13 @@ class RustMatrixRoom( } } - override suspend fun toggleReaction(emoji: String, eventId: EventId): Result = withContext(coroutineDispatchers.io) { + override suspend fun toggleReaction(emoji: String, eventId: EventId): Result = withContext(roomDispatcher) { runCatching { innerRoom.toggleReaction(key = emoji, eventId = eventId.value) } } - override suspend fun forwardEvent(eventId: EventId, roomIds: List): Result = withContext(coroutineDispatchers.io) { + override suspend fun forwardEvent(eventId: EventId, roomIds: List): Result = withContext(roomDispatcher) { runCatching { roomContentForwarder.forward(fromRoom = innerRoom, eventId = eventId, toRoomIds = roomIds) }.onFailure { @@ -320,14 +327,14 @@ class RustMatrixRoom( } override suspend fun retrySendMessage(transactionId: String): Result = - withContext(coroutineDispatchers.io) { + withContext(roomDispatcher) { runCatching { innerRoom.retrySend(transactionId) } } override suspend fun cancelSend(transactionId: String): Result = - withContext(coroutineDispatchers.io) { + withContext(roomDispatcher) { runCatching { innerRoom.cancelSend(transactionId) } @@ -335,40 +342,40 @@ class RustMatrixRoom( @OptIn(ExperimentalUnsignedTypes::class) override suspend fun updateAvatar(mimeType: String, data: ByteArray): Result = - withContext(coroutineDispatchers.io) { + withContext(roomDispatcher) { runCatching { innerRoom.uploadAvatar(mimeType, data.toUByteArray().toList()) } } override suspend fun removeAvatar(): Result = - withContext(coroutineDispatchers.io) { + withContext(roomDispatcher) { runCatching { innerRoom.removeAvatar() } } override suspend fun setName(name: String): Result = - withContext(coroutineDispatchers.io) { + withContext(roomDispatcher) { runCatching { innerRoom.setName(name) } } override suspend fun setTopic(topic: String): Result = - withContext(coroutineDispatchers.io) { + withContext(roomDispatcher) { runCatching { innerRoom.setTopic(topic) } } - private suspend fun fetchMembers() = withContext(coroutineDispatchers.io) { + private suspend fun fetchMembers() = withContext(roomDispatcher) { runCatching { innerRoom.fetchMembers() } } - override suspend fun reportContent(eventId: EventId, reason: String, blockUserId: UserId?): Result = withContext(coroutineDispatchers.io) { + override suspend fun reportContent(eventId: EventId, reason: String, blockUserId: UserId?): Result = withContext(roomDispatcher) { runCatching { innerRoom.reportContent(eventId = eventId.value, score = null, reason = reason) if (blockUserId != null) { @@ -383,7 +390,7 @@ class RustMatrixRoom( description: String?, zoomLevel: Int?, assetType: AssetType?, - ): Result = withContext(coroutineDispatchers.io) { + ): Result = withContext(roomDispatcher) { runCatching { innerRoom.sendLocation( body = body, diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/room/RustRoomSummaryDataSource.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/room/RustRoomSummaryDataSource.kt index 3da4c560e8..efdbcf34ad 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/room/RustRoomSummaryDataSource.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/room/RustRoomSummaryDataSource.kt @@ -16,9 +16,9 @@ package io.element.android.libraries.matrix.impl.room -import io.element.android.libraries.core.coroutine.CoroutineDispatchers import io.element.android.libraries.matrix.api.room.RoomSummary import io.element.android.libraries.matrix.api.room.RoomSummaryDataSource +import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableStateFlow @@ -41,7 +41,7 @@ import timber.log.Timber internal class RustRoomSummaryDataSource( private val roomListService: RoomListService, private val sessionCoroutineScope: CoroutineScope, - coroutineDispatchers: CoroutineDispatchers, + dispatcher: CoroutineDispatcher, roomSummaryDetailsFactory: RoomSummaryDetailsFactory = RoomSummaryDetailsFactory(), ) : RoomSummaryDataSource { @@ -53,7 +53,7 @@ internal class RustRoomSummaryDataSource( private val inviteRoomsListProcessor = RoomSummaryListProcessor(inviteRooms, roomListService, roomSummaryDetailsFactory, shouldFetchFullRoom = true) init { - sessionCoroutineScope.launch(coroutineDispatchers.computation) { + sessionCoroutineScope.launch(dispatcher) { val allRooms = roomListService.allRooms() allRooms .observeEntriesWithProcessor(allRoomsListProcessor) diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/RustMatrixTimeline.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/RustMatrixTimeline.kt index c7335ba10b..2c3a579b2f 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/RustMatrixTimeline.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/RustMatrixTimeline.kt @@ -16,7 +16,6 @@ package io.element.android.libraries.matrix.impl.timeline -import io.element.android.libraries.core.coroutine.CoroutineDispatchers import io.element.android.libraries.matrix.api.core.EventId import io.element.android.libraries.matrix.api.room.MatrixRoom import io.element.android.libraries.matrix.api.timeline.MatrixTimeline @@ -25,6 +24,7 @@ import io.element.android.libraries.matrix.impl.timeline.item.event.EventMessage import io.element.android.libraries.matrix.impl.timeline.item.event.EventTimelineItemMapper import io.element.android.libraries.matrix.impl.timeline.item.event.TimelineEventContentMapper import io.element.android.libraries.matrix.impl.timeline.item.virtual.VirtualTimelineItemMapper +import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.FlowPreview import kotlinx.coroutines.flow.Flow @@ -45,7 +45,7 @@ class RustMatrixTimeline( roomCoroutineScope: CoroutineScope, private val matrixRoom: MatrixRoom, private val innerRoom: Room, - private val coroutineDispatchers: CoroutineDispatchers, + private val dispatcher: CoroutineDispatcher, ) : MatrixTimeline { private val _timelineItems: MutableStateFlow> = @@ -109,13 +109,13 @@ class RustMatrixTimeline( } } - override suspend fun fetchDetailsForEvent(eventId: EventId): Result = withContext(coroutineDispatchers.io) { + override suspend fun fetchDetailsForEvent(eventId: EventId): Result = withContext(dispatcher) { runCatching { innerRoom.fetchDetailsForEvent(eventId.value) } } - override suspend fun paginateBackwards(requestSize: Int, untilNumberOfItems: Int): Result = withContext(coroutineDispatchers.io) { + override suspend fun paginateBackwards(requestSize: Int, untilNumberOfItems: Int): Result = withContext(dispatcher) { runCatching { Timber.v("Start back paginating for room ${matrixRoom.roomId} ") val paginationOptions = PaginationOptions.UntilNumItems( @@ -131,7 +131,7 @@ class RustMatrixTimeline( } } - override suspend fun sendReadReceipt(eventId: EventId) = withContext(coroutineDispatchers.io) { + override suspend fun sendReadReceipt(eventId: EventId) = withContext(dispatcher) { runCatching { innerRoom.sendReadReceipt(eventId = eventId.value) } From d56c66866338869d91221aaa3d613326b189a1db Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Tue, 11 Jul 2023 17:48:31 +0200 Subject: [PATCH 03/20] Improve UX on Block/Unblock user action. Add loading and error case. And make the value (a bit more) live. --- .../impl/blockuser/BlockUserSection.kt | 63 +++++++++++++++---- .../details/RoomMemberDetailsEvents.kt | 1 + .../details/RoomMemberDetailsPresenter.kt | 46 ++++++++++---- .../members/details/RoomMemberDetailsState.kt | 4 +- .../details/RoomMemberDetailsStateProvider.kt | 6 +- .../RoomMemberDetailsPresenterTests.kt | 31 ++++++++- .../libraries/matrix/test/FakeMatrixClient.kt | 5 +- 7 files changed, 122 insertions(+), 34 deletions(-) diff --git a/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/blockuser/BlockUserSection.kt b/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/blockuser/BlockUserSection.kt index e70a7d0533..de171b06fa 100644 --- a/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/blockuser/BlockUserSection.kt +++ b/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/blockuser/BlockUserSection.kt @@ -25,30 +25,67 @@ import androidx.compose.ui.res.stringResource import io.element.android.features.roomdetails.impl.R import io.element.android.features.roomdetails.impl.members.details.RoomMemberDetailsEvents import io.element.android.features.roomdetails.impl.members.details.RoomMemberDetailsState +import io.element.android.libraries.architecture.Async +import io.element.android.libraries.core.bool.orFalse import io.element.android.libraries.designsystem.components.dialogs.ConfirmationDialog +import io.element.android.libraries.designsystem.components.dialogs.RetryDialog import io.element.android.libraries.designsystem.components.preferences.PreferenceCategory import io.element.android.libraries.designsystem.components.preferences.PreferenceText +import io.element.android.libraries.ui.strings.CommonStrings @Composable internal fun BlockUserSection(state: RoomMemberDetailsState, modifier: Modifier = Modifier) { PreferenceCategory(showDivider = false, modifier = modifier) { - if (state.isBlocked) { - PreferenceText( - title = stringResource(R.string.screen_dm_details_unblock_user), - icon = Icons.Outlined.Block, - onClick = { state.eventSink(RoomMemberDetailsEvents.UnblockUser(needsConfirmation = true)) }, - ) - } else { - PreferenceText( - title = stringResource(R.string.screen_dm_details_block_user), - icon = Icons.Outlined.Block, - tintColor = MaterialTheme.colorScheme.error, - onClick = { state.eventSink(RoomMemberDetailsEvents.BlockUser(needsConfirmation = true)) }, - ) + when (state.isBlocked) { + is Async.Failure -> { + PreferenceBlockUser(state.isBlocked.prevData, false, state.eventSink, modifier) + RetryDialog( + content = stringResource(CommonStrings.error_unknown), + onDismiss = { state.eventSink(RoomMemberDetailsEvents.ClearBlockUserError) }, + onRetry = { + val event = when (state.isBlocked.prevData) { + true -> RoomMemberDetailsEvents.UnblockUser(needsConfirmation = false) + false -> RoomMemberDetailsEvents.BlockUser(needsConfirmation = false) + null -> /*Should not happen */ RoomMemberDetailsEvents.ClearBlockUserError + } + state.eventSink(event) + }, + ) + } + is Async.Loading -> PreferenceBlockUser(state.isBlocked.prevData, true, state.eventSink, modifier) + is Async.Success -> PreferenceBlockUser(state.isBlocked.data, false, state.eventSink, modifier) + Async.Uninitialized -> PreferenceBlockUser(null, true, state.eventSink, modifier) } } } +@Composable +private fun PreferenceBlockUser( + isBlocked: Boolean?, + isLoading: Boolean, + eventSink: (RoomMemberDetailsEvents) -> Unit, + modifier: Modifier = Modifier, +) { + if (isBlocked.orFalse()) { + PreferenceText( + title = stringResource(R.string.screen_dm_details_unblock_user), + icon = Icons.Outlined.Block, + onClick = { if (!isLoading) eventSink(RoomMemberDetailsEvents.UnblockUser(needsConfirmation = true)) }, + loadingCurrentValue = isLoading, + modifier = modifier, + ) + } else { + PreferenceText( + title = stringResource(R.string.screen_dm_details_block_user), + icon = Icons.Outlined.Block, + tintColor = MaterialTheme.colorScheme.error, + onClick = { if (!isLoading) eventSink(RoomMemberDetailsEvents.BlockUser(needsConfirmation = true)) }, + loadingCurrentValue = isLoading, + modifier = modifier, + ) + } +} + @Composable internal fun BlockUserDialogs(state: RoomMemberDetailsState) { when (state.displayConfirmationDialog) { diff --git a/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/members/details/RoomMemberDetailsEvents.kt b/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/members/details/RoomMemberDetailsEvents.kt index 5848561f3e..c09d9a1f70 100644 --- a/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/members/details/RoomMemberDetailsEvents.kt +++ b/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/members/details/RoomMemberDetailsEvents.kt @@ -19,5 +19,6 @@ package io.element.android.features.roomdetails.impl.members.details sealed interface RoomMemberDetailsEvents { data class BlockUser(val needsConfirmation: Boolean = false) : RoomMemberDetailsEvents data class UnblockUser(val needsConfirmation: Boolean = false) : RoomMemberDetailsEvents + object ClearBlockUserError : RoomMemberDetailsEvents object ClearConfirmationDialog : RoomMemberDetailsEvents } diff --git a/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/members/details/RoomMemberDetailsPresenter.kt b/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/members/details/RoomMemberDetailsPresenter.kt index 9d3c391ace..3be83a2fef 100644 --- a/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/members/details/RoomMemberDetailsPresenter.kt +++ b/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/members/details/RoomMemberDetailsPresenter.kt @@ -28,6 +28,7 @@ import androidx.compose.runtime.setValue import dagger.assisted.Assisted import dagger.assisted.AssistedInject import io.element.android.features.roomdetails.impl.members.details.RoomMemberDetailsState.ConfirmationDialog +import io.element.android.libraries.architecture.Async import io.element.android.libraries.architecture.Presenter import io.element.android.libraries.core.bool.orFalse import io.element.android.libraries.matrix.api.MatrixClient @@ -53,8 +54,13 @@ class RoomMemberDetailsPresenter @AssistedInject constructor( var confirmationDialog by remember { mutableStateOf(null) } val roomMember by room.getRoomMemberAsState(roomMemberId) // the room member is not really live... - val isBlocked = remember { - mutableStateOf(roomMember?.isIgnored.orFalse()) + val isBlocked: MutableState> = remember(roomMember) { + val isIgnored = roomMember?.isIgnored + if (isIgnored == null) { + mutableStateOf(Async.Uninitialized) + } else { + mutableStateOf(Async.Success(isIgnored)) + } } LaunchedEffect(Unit) { room.updateMembers() @@ -79,6 +85,9 @@ class RoomMemberDetailsPresenter @AssistedInject constructor( } } RoomMemberDetailsEvents.ClearConfirmationDialog -> confirmationDialog = null + RoomMemberDetailsEvents.ClearBlockUserError -> { + isBlocked.value = Async.Success(isBlocked.value.dataOrNull().orFalse()) + } } } @@ -105,20 +114,31 @@ class RoomMemberDetailsPresenter @AssistedInject constructor( ) } - private fun CoroutineScope.blockUser(userId: UserId, isBlockedState: MutableState) = launch { + private fun CoroutineScope.blockUser(userId: UserId, isBlockedState: MutableState>) = launch { + isBlockedState.value = Async.Loading(false) client.ignoreUser(userId) - .map { - isBlockedState.value = true - room.updateMembers() - } - + .fold( + onSuccess = { + isBlockedState.value = Async.Success(true) + room.updateMembers() + }, + onFailure = { + isBlockedState.value = Async.Failure(it, false) + } + ) } - private fun CoroutineScope.unblockUser(userId: UserId, isBlockedState: MutableState) = launch { + private fun CoroutineScope.unblockUser(userId: UserId, isBlockedState: MutableState>) = launch { + isBlockedState.value = Async.Loading(true) client.unignoreUser(userId) - .map { - isBlockedState.value = false - room.updateMembers() - } + .fold( + onSuccess = { + isBlockedState.value = Async.Success(false) + room.updateMembers() + }, + onFailure = { + isBlockedState.value = Async.Failure(it, true) + } + ) } } diff --git a/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/members/details/RoomMemberDetailsState.kt b/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/members/details/RoomMemberDetailsState.kt index 0a2895db09..0d3423e179 100644 --- a/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/members/details/RoomMemberDetailsState.kt +++ b/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/members/details/RoomMemberDetailsState.kt @@ -16,11 +16,13 @@ package io.element.android.features.roomdetails.impl.members.details +import io.element.android.libraries.architecture.Async + data class RoomMemberDetailsState( val userId: String, val userName: String?, val avatarUrl: String?, - val isBlocked: Boolean, + val isBlocked: Async, val displayConfirmationDialog: ConfirmationDialog? = null, val isCurrentUser: Boolean, val eventSink: (RoomMemberDetailsEvents) -> Unit diff --git a/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/members/details/RoomMemberDetailsStateProvider.kt b/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/members/details/RoomMemberDetailsStateProvider.kt index d8e7ce5ad3..6883b20898 100644 --- a/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/members/details/RoomMemberDetailsStateProvider.kt +++ b/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/members/details/RoomMemberDetailsStateProvider.kt @@ -17,15 +17,17 @@ package io.element.android.features.roomdetails.impl.members.details import androidx.compose.ui.tooling.preview.PreviewParameterProvider +import io.element.android.libraries.architecture.Async open class RoomMemberDetailsStateProvider : PreviewParameterProvider { override val values: Sequence get() = sequenceOf( aRoomMemberDetailsState(), aRoomMemberDetailsState().copy(userName = null), - aRoomMemberDetailsState().copy(isBlocked = true), + aRoomMemberDetailsState().copy(isBlocked = Async.Success(true)), aRoomMemberDetailsState().copy(displayConfirmationDialog = RoomMemberDetailsState.ConfirmationDialog.Block), aRoomMemberDetailsState().copy(displayConfirmationDialog = RoomMemberDetailsState.ConfirmationDialog.Unblock), + aRoomMemberDetailsState().copy(isBlocked = Async.Loading(true)), // Add other states here ) } @@ -34,7 +36,7 @@ fun aRoomMemberDetailsState() = RoomMemberDetailsState( userId = "@daniel:domain.com", userName = "Daniel", avatarUrl = null, - isBlocked = false, + isBlocked = Async.Success(false), isCurrentUser = false, eventSink = {}, ) diff --git a/features/roomdetails/impl/src/test/kotlin/io/element/android/features/roomdetails/members/details/RoomMemberDetailsPresenterTests.kt b/features/roomdetails/impl/src/test/kotlin/io/element/android/features/roomdetails/members/details/RoomMemberDetailsPresenterTests.kt index 912f354c89..94b940bb17 100644 --- a/features/roomdetails/impl/src/test/kotlin/io/element/android/features/roomdetails/members/details/RoomMemberDetailsPresenterTests.kt +++ b/features/roomdetails/impl/src/test/kotlin/io/element/android/features/roomdetails/members/details/RoomMemberDetailsPresenterTests.kt @@ -25,7 +25,9 @@ import io.element.android.features.roomdetails.impl.members.aRoomMember import io.element.android.features.roomdetails.impl.members.details.RoomMemberDetailsEvents import io.element.android.features.roomdetails.impl.members.details.RoomMemberDetailsPresenter import io.element.android.features.roomdetails.impl.members.details.RoomMemberDetailsState +import io.element.android.libraries.architecture.Async import io.element.android.libraries.matrix.api.room.MatrixRoomMembersState +import io.element.android.libraries.matrix.test.A_THROWABLE import io.element.android.libraries.matrix.test.FakeMatrixClient import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.test.runTest @@ -50,7 +52,7 @@ class RoomMemberDetailsPresenterTests { Truth.assertThat(initialState.userId).isEqualTo(roomMember.userId.value) Truth.assertThat(initialState.userName).isEqualTo(roomMember.displayName) Truth.assertThat(initialState.avatarUrl).isEqualTo(roomMember.avatarUrl) - Truth.assertThat(initialState.isBlocked).isEqualTo(roomMember.isIgnored) + Truth.assertThat(initialState.isBlocked).isEqualTo(Async.Success(roomMember.isIgnored)) skipItems(1) val loadedState = awaitItem() Truth.assertThat(loadedState.userName).isEqualTo("A custom name") @@ -129,10 +131,33 @@ class RoomMemberDetailsPresenterTests { }.test { val initialState = awaitItem() initialState.eventSink(RoomMemberDetailsEvents.BlockUser(needsConfirmation = false)) - Truth.assertThat(awaitItem().isBlocked).isTrue() + Truth.assertThat(awaitItem().isBlocked.isLoading()).isTrue() + Truth.assertThat(awaitItem().isBlocked.dataOrNull()).isTrue() initialState.eventSink(RoomMemberDetailsEvents.UnblockUser(needsConfirmation = false)) - Truth.assertThat(awaitItem().isBlocked).isFalse() + Truth.assertThat(awaitItem().isBlocked.isLoading()).isTrue() + Truth.assertThat(awaitItem().isBlocked.dataOrNull()).isFalse() + } + } + + @Test + fun `present - BlockUser with error`() = runTest { + val room = aMatrixRoom() + val roomMember = aRoomMember() + val matrixClient = FakeMatrixClient() + matrixClient.givenIgnoreUserResult(Result.failure(A_THROWABLE)) + val presenter = RoomMemberDetailsPresenter(matrixClient, room, roomMember.userId) + moleculeFlow(RecompositionClock.Immediate) { + presenter.present() + }.test { + val initialState = awaitItem() + initialState.eventSink(RoomMemberDetailsEvents.BlockUser(needsConfirmation = false)) + Truth.assertThat(awaitItem().isBlocked.isLoading()).isTrue() + val errorState = awaitItem() + Truth.assertThat(errorState.isBlocked.errorOrNull()).isEqualTo(A_THROWABLE) + // Clear error + initialState.eventSink(RoomMemberDetailsEvents.ClearBlockUserError) + Truth.assertThat(awaitItem().isBlocked).isEqualTo(Async.Success(false)) } } diff --git a/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/FakeMatrixClient.kt b/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/FakeMatrixClient.kt index fcd2f161e9..1a654ac8d4 100644 --- a/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/FakeMatrixClient.kt +++ b/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/FakeMatrixClient.kt @@ -38,6 +38,7 @@ import io.element.android.libraries.matrix.test.room.FakeMatrixRoom import io.element.android.libraries.matrix.test.room.FakeRoomSummaryDataSource import io.element.android.libraries.matrix.test.sync.FakeSyncService import io.element.android.libraries.matrix.test.verification.FakeSessionVerificationService +import io.element.android.tests.testutils.simulateLongTask import kotlinx.coroutines.delay class FakeMatrixClient( @@ -72,11 +73,11 @@ class FakeMatrixClient( return findDmResult } - override suspend fun ignoreUser(userId: UserId): Result { + override suspend fun ignoreUser(userId: UserId): Result = simulateLongTask { return ignoreUserResult } - override suspend fun unignoreUser(userId: UserId): Result { + override suspend fun unignoreUser(userId: UserId): Result = simulateLongTask { return unignoreUserResult } From 2cc548f14511a73dcbdffc926bcdda81578425ed Mon Sep 17 00:00:00 2001 From: ElementBot Date: Tue, 11 Jul 2023 16:19:22 +0000 Subject: [PATCH 04/20] Update screenshots --- ...emberDetailsViewDarkPreview--3_3_null_5,NEXUS_5,1.0,en].png | 3 +++ ...mberDetailsViewLightPreview--2_2_null_5,NEXUS_5,1.0,en].png | 3 +++ 2 files changed, 6 insertions(+) create mode 100644 tests/uitests/src/test/snapshots/images/io.element.android.tests.uitests_ScreenshotTest_preview_tests[io.element.android.features.roomdetails.impl.members.details_null_DefaultGroup_RoomMemberDetailsViewDarkPreview--3_3_null_5,NEXUS_5,1.0,en].png create mode 100644 tests/uitests/src/test/snapshots/images/io.element.android.tests.uitests_ScreenshotTest_preview_tests[io.element.android.features.roomdetails.impl.members.details_null_DefaultGroup_RoomMemberDetailsViewLightPreview--2_2_null_5,NEXUS_5,1.0,en].png diff --git a/tests/uitests/src/test/snapshots/images/io.element.android.tests.uitests_ScreenshotTest_preview_tests[io.element.android.features.roomdetails.impl.members.details_null_DefaultGroup_RoomMemberDetailsViewDarkPreview--3_3_null_5,NEXUS_5,1.0,en].png b/tests/uitests/src/test/snapshots/images/io.element.android.tests.uitests_ScreenshotTest_preview_tests[io.element.android.features.roomdetails.impl.members.details_null_DefaultGroup_RoomMemberDetailsViewDarkPreview--3_3_null_5,NEXUS_5,1.0,en].png new file mode 100644 index 0000000000..50217fd9f2 --- /dev/null +++ b/tests/uitests/src/test/snapshots/images/io.element.android.tests.uitests_ScreenshotTest_preview_tests[io.element.android.features.roomdetails.impl.members.details_null_DefaultGroup_RoomMemberDetailsViewDarkPreview--3_3_null_5,NEXUS_5,1.0,en].png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:c7ca068387cff8faf728a989488e7c4b5b07983c4b24162ff82a14fd90b82d05 +size 20609 diff --git a/tests/uitests/src/test/snapshots/images/io.element.android.tests.uitests_ScreenshotTest_preview_tests[io.element.android.features.roomdetails.impl.members.details_null_DefaultGroup_RoomMemberDetailsViewLightPreview--2_2_null_5,NEXUS_5,1.0,en].png b/tests/uitests/src/test/snapshots/images/io.element.android.tests.uitests_ScreenshotTest_preview_tests[io.element.android.features.roomdetails.impl.members.details_null_DefaultGroup_RoomMemberDetailsViewLightPreview--2_2_null_5,NEXUS_5,1.0,en].png new file mode 100644 index 0000000000..cab17c4d00 --- /dev/null +++ b/tests/uitests/src/test/snapshots/images/io.element.android.tests.uitests_ScreenshotTest_preview_tests[io.element.android.features.roomdetails.impl.members.details_null_DefaultGroup_RoomMemberDetailsViewLightPreview--2_2_null_5,NEXUS_5,1.0,en].png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:e1599be0c5a37083c015378ee47a78a90dcf670f4df5d85dd25d39f4b2edbdf6 +size 21147 From 38b91a75929ecf7d6c3f7da30c3a30bcbe8ea77e Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 12 Jul 2023 09:37:13 +0200 Subject: [PATCH 05/20] Fix issue about modifier. --- .../impl/blockuser/BlockUserSection.kt | 36 +++++++++---------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/blockuser/BlockUserSection.kt b/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/blockuser/BlockUserSection.kt index de171b06fa..cccf682c9c 100644 --- a/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/blockuser/BlockUserSection.kt +++ b/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/blockuser/BlockUserSection.kt @@ -37,26 +37,26 @@ import io.element.android.libraries.ui.strings.CommonStrings internal fun BlockUserSection(state: RoomMemberDetailsState, modifier: Modifier = Modifier) { PreferenceCategory(showDivider = false, modifier = modifier) { when (state.isBlocked) { - is Async.Failure -> { - PreferenceBlockUser(state.isBlocked.prevData, false, state.eventSink, modifier) - RetryDialog( - content = stringResource(CommonStrings.error_unknown), - onDismiss = { state.eventSink(RoomMemberDetailsEvents.ClearBlockUserError) }, - onRetry = { - val event = when (state.isBlocked.prevData) { - true -> RoomMemberDetailsEvents.UnblockUser(needsConfirmation = false) - false -> RoomMemberDetailsEvents.BlockUser(needsConfirmation = false) - null -> /*Should not happen */ RoomMemberDetailsEvents.ClearBlockUserError - } - state.eventSink(event) - }, - ) - } - is Async.Loading -> PreferenceBlockUser(state.isBlocked.prevData, true, state.eventSink, modifier) - is Async.Success -> PreferenceBlockUser(state.isBlocked.data, false, state.eventSink, modifier) - Async.Uninitialized -> PreferenceBlockUser(null, true, state.eventSink, modifier) + is Async.Failure -> PreferenceBlockUser(isBlocked = state.isBlocked.prevData, isLoading = false, eventSink = state.eventSink) + is Async.Loading -> PreferenceBlockUser(isBlocked = state.isBlocked.prevData, isLoading = true, eventSink = state.eventSink) + is Async.Success -> PreferenceBlockUser(isBlocked = state.isBlocked.data, isLoading = false, eventSink = state.eventSink) + Async.Uninitialized -> PreferenceBlockUser(isBlocked = null, isLoading = true, eventSink = state.eventSink) } } + if (state.isBlocked is Async.Failure) { + RetryDialog( + content = stringResource(CommonStrings.error_unknown), + onDismiss = { state.eventSink(RoomMemberDetailsEvents.ClearBlockUserError) }, + onRetry = { + val event = when (state.isBlocked.prevData) { + true -> RoomMemberDetailsEvents.UnblockUser(needsConfirmation = false) + false -> RoomMemberDetailsEvents.BlockUser(needsConfirmation = false) + null -> /*Should not happen */ RoomMemberDetailsEvents.ClearBlockUserError + } + state.eventSink(event) + }, + ) + } } @Composable From a2b84ac6177b6769571214b932687b71419e5c53 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 12 Jul 2023 09:40:02 +0200 Subject: [PATCH 06/20] Ensure CI run all the tests. There were some failing tests, but the CI does not see it. It seems that koverMergedReport does not run all the tests (?). --- .github/workflows/nightlyReports.yml | 2 +- .github/workflows/tests.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/nightlyReports.yml b/.github/workflows/nightlyReports.yml index 5cd92d6cb1..4805bf44d1 100644 --- a/.github/workflows/nightlyReports.yml +++ b/.github/workflows/nightlyReports.yml @@ -27,7 +27,7 @@ jobs: java-version: '17' - name: ⚙️ Run unit & screenshot tests, generate kover report - run: ./gradlew koverMergedReport $CI_GRADLE_ARG_PROPERTIES -Pci-build=true + run: ./gradlew test koverMergedReport $CI_GRADLE_ARG_PROPERTIES -Pci-build=true - name: ✅ Upload kover report if: always() diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 2926159a00..44fdf58323 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -38,7 +38,7 @@ jobs: cache-read-only: ${{ github.ref != 'refs/heads/develop' }} - name: ⚙️ Run unit & screenshot tests, generate kover report - run: ./gradlew koverMergedReport $CI_GRADLE_ARG_PROPERTIES -Pci-build=true + run: ./gradlew test koverMergedReport $CI_GRADLE_ARG_PROPERTIES -Pci-build=true - name: 📈 Verify coverage run: ./gradlew koverMergedVerify $CI_GRADLE_ARG_PROPERTIES -Pci-build=true From af520ddc0071a927b2c37bbfae07410e028d1f2e Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 12 Jul 2023 09:43:18 +0200 Subject: [PATCH 07/20] Fix failing test. Code is now aligned with the comment. --- .../android/libraries/core/extensions/BasicExtensions.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/core/src/main/kotlin/io/element/android/libraries/core/extensions/BasicExtensions.kt b/libraries/core/src/main/kotlin/io/element/android/libraries/core/extensions/BasicExtensions.kt index 562f62de6d..db07432df0 100644 --- a/libraries/core/src/main/kotlin/io/element/android/libraries/core/extensions/BasicExtensions.kt +++ b/libraries/core/src/main/kotlin/io/element/android/libraries/core/extensions/BasicExtensions.kt @@ -64,7 +64,7 @@ fun String?.insertBeforeLast(insert: String, delimiter: String = "."): String { * Throws if length is < 1. */ fun String.ellipsize(length: Int): String { - require(length > 1) + require(length >= 1) if (this.length <= length) { return this From e85de6b3007b9f5edfe0dfc5b5fe48aeed1d477e Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 12 Jul 2023 09:59:52 +0200 Subject: [PATCH 08/20] Rework DeeplinkParser to fix a test (and fix a bug in release mode). The test was failing in release mode because there is not check on `RoomId` format, so INVITE_LIST value ("invites") is seen as a valid RoomId. First check for known paths, then try to parse as RoomId. The tryOrNull will return null only in debug mode, so I think we can remove it. Error was: value of: getFromIntent(...) expected: InviteList(sessionId=@alice:server.org) but was : Room(sessionId=@alice:server.org, roomId=invites, threadId=null) at io.element.android.libraries.deeplink.DeeplinkParserTest.nominal cases(DeeplinkParserTest.kt:54) --- .../libraries/deeplink/DeeplinkParser.kt | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/libraries/deeplink/src/main/kotlin/io/element/android/libraries/deeplink/DeeplinkParser.kt b/libraries/deeplink/src/main/kotlin/io/element/android/libraries/deeplink/DeeplinkParser.kt index 7d2c8af135..7a5f9d5772 100644 --- a/libraries/deeplink/src/main/kotlin/io/element/android/libraries/deeplink/DeeplinkParser.kt +++ b/libraries/deeplink/src/main/kotlin/io/element/android/libraries/deeplink/DeeplinkParser.kt @@ -18,7 +18,6 @@ package io.element.android.libraries.deeplink import android.content.Intent import android.net.Uri -import io.element.android.libraries.core.data.tryOrNull 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.core.ThreadId @@ -37,21 +36,15 @@ class DeeplinkParser @Inject constructor() { if (host != HOST) return null val pathBits = path.orEmpty().split("/").drop(1) val sessionId = pathBits.elementAtOrNull(0)?.let(::SessionId) ?: return null - val screenPathComponent = pathBits.elementAtOrNull(1) - val roomId = tryOrNull { screenPathComponent?.let(::RoomId) } - return when { - roomId != null -> { + return when (val screenPathComponent = pathBits.elementAtOrNull(1)) { + null -> DeeplinkData.Root(sessionId) + DeepLinkPaths.INVITE_LIST -> DeeplinkData.InviteList(sessionId) + else -> { + val roomId = screenPathComponent.let(::RoomId) val threadId = pathBits.elementAtOrNull(2)?.let(::ThreadId) DeeplinkData.Room(sessionId, roomId, threadId) } - screenPathComponent == DeepLinkPaths.INVITE_LIST -> { - DeeplinkData.InviteList(sessionId) - } - screenPathComponent == null -> { - DeeplinkData.Root(sessionId) - } - else -> null } } } From bb1991fe4acbe41c2137c7aa5042b82300c3acfc Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 12 Jul 2023 11:26:13 +0200 Subject: [PATCH 09/20] More log about Node lifecycle. Will help to track user navigation. --- .../android/appnav/NotLoggedInFlowNode.kt | 9 ------ .../libraries/architecture/BackstackNode.kt | 14 ++++++++- .../libraries/architecture/LifecycleExt.kt | 30 +++++++++++++++++++ 3 files changed, 43 insertions(+), 10 deletions(-) create mode 100644 libraries/architecture/src/main/kotlin/io/element/android/libraries/architecture/LifecycleExt.kt diff --git a/appnav/src/main/kotlin/io/element/android/appnav/NotLoggedInFlowNode.kt b/appnav/src/main/kotlin/io/element/android/appnav/NotLoggedInFlowNode.kt index 4b89b442d7..f9a0c00ec5 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/NotLoggedInFlowNode.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/NotLoggedInFlowNode.kt @@ -20,7 +20,6 @@ import android.os.Parcelable import androidx.compose.runtime.Composable import androidx.compose.ui.Modifier import com.bumble.appyx.core.composable.Children -import com.bumble.appyx.core.lifecycle.subscribe import com.bumble.appyx.core.modality.BuildContext import com.bumble.appyx.core.node.Node import com.bumble.appyx.core.plugin.Plugin @@ -35,7 +34,6 @@ import io.element.android.libraries.architecture.BackstackNode import io.element.android.libraries.architecture.animation.rememberDefaultTransitionHandler import io.element.android.libraries.di.AppScope import kotlinx.parcelize.Parcelize -import timber.log.Timber @ContributesNode(AppScope::class) class NotLoggedInFlowNode @AssistedInject constructor( @@ -51,13 +49,6 @@ class NotLoggedInFlowNode @AssistedInject constructor( buildContext = buildContext, plugins = plugins, ) { - init { - lifecycle.subscribe( - onCreate = { Timber.v("OnCreate") }, - onDestroy = { Timber.v("OnDestroy") } - ) - } - sealed interface NavTarget : Parcelable { @Parcelize object OnBoarding : NavTarget diff --git a/libraries/architecture/src/main/kotlin/io/element/android/libraries/architecture/BackstackNode.kt b/libraries/architecture/src/main/kotlin/io/element/android/libraries/architecture/BackstackNode.kt index ec607e9281..ec22c5e21f 100644 --- a/libraries/architecture/src/main/kotlin/io/element/android/libraries/architecture/BackstackNode.kt +++ b/libraries/architecture/src/main/kotlin/io/element/android/libraries/architecture/BackstackNode.kt @@ -19,6 +19,7 @@ package io.element.android.libraries.architecture import androidx.compose.runtime.Stable import com.bumble.appyx.core.children.ChildEntry import com.bumble.appyx.core.modality.BuildContext +import com.bumble.appyx.core.node.Node import com.bumble.appyx.core.node.ParentNode import com.bumble.appyx.core.plugin.Plugin import com.bumble.appyx.navmodel.backstack.BackStack @@ -39,4 +40,15 @@ abstract class BackstackNode( buildContext = buildContext, plugins = plugins, childKeepMode = childKeepMode, -) +) { + override fun onBuilt() { + super.onBuilt() + lifecycle.logLifecycle(this::class.java.simpleName) + whenChildAttached { _, child -> + // BackstackNode will be logged by their parent. + if (child !is BackstackNode<*>) { + child.lifecycle.logLifecycle(child::class.java.simpleName) + } + } + } +} diff --git a/libraries/architecture/src/main/kotlin/io/element/android/libraries/architecture/LifecycleExt.kt b/libraries/architecture/src/main/kotlin/io/element/android/libraries/architecture/LifecycleExt.kt new file mode 100644 index 0000000000..7c42c4cfe3 --- /dev/null +++ b/libraries/architecture/src/main/kotlin/io/element/android/libraries/architecture/LifecycleExt.kt @@ -0,0 +1,30 @@ +/* + * Copyright (c) 2023 New Vector Ltd + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.element.android.libraries.architecture + +import androidx.lifecycle.Lifecycle +import com.bumble.appyx.core.lifecycle.subscribe +import timber.log.Timber + +fun Lifecycle.logLifecycle(name: String) { + subscribe( + onCreate = { Timber.tag("Lifecycle").d("onCreate $name") }, + onPause = { Timber.tag("Lifecycle").d("onPause $name") }, + onResume = { Timber.tag("Lifecycle").d("onResume $name") }, + onDestroy = { Timber.tag("Lifecycle").d("onDestroy $name") }, + ) +} From 5622517dffb6fcf7e97b4d2613c026dcd0f34958 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 12 Jul 2023 12:05:31 +0200 Subject: [PATCH 10/20] Fix image not loading after a clear cache. --- .../io/element/android/appnav/LoggedInFlowNode.kt | 2 -- .../element/android/appnav/NotLoggedInFlowNode.kt | 14 ++++++++++++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/appnav/src/main/kotlin/io/element/android/appnav/LoggedInFlowNode.kt b/appnav/src/main/kotlin/io/element/android/appnav/LoggedInFlowNode.kt index bc73efde71..3380dd91db 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/LoggedInFlowNode.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/LoggedInFlowNode.kt @@ -154,8 +154,6 @@ class LoggedInFlowNode @AssistedInject constructor( syncService.stopSync() }, onDestroy = { - val imageLoaderFactory = bindings().notLoggedInImageLoaderFactory() - Coil.setImageLoader(imageLoaderFactory) plugins().forEach { it.onFlowReleased(id, inputs.matrixClient) } appNavigationStateService.onLeavingSpace(id) appNavigationStateService.onLeavingSession(id) diff --git a/appnav/src/main/kotlin/io/element/android/appnav/NotLoggedInFlowNode.kt b/appnav/src/main/kotlin/io/element/android/appnav/NotLoggedInFlowNode.kt index f9a0c00ec5..ac17f6ad00 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/NotLoggedInFlowNode.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/NotLoggedInFlowNode.kt @@ -19,7 +19,9 @@ package io.element.android.appnav import android.os.Parcelable import androidx.compose.runtime.Composable import androidx.compose.ui.Modifier +import coil.Coil import com.bumble.appyx.core.composable.Children +import com.bumble.appyx.core.lifecycle.subscribe import com.bumble.appyx.core.modality.BuildContext import com.bumble.appyx.core.node.Node import com.bumble.appyx.core.plugin.Plugin @@ -32,7 +34,9 @@ import io.element.android.features.login.api.LoginEntryPoint import io.element.android.features.onboarding.api.OnBoardingEntryPoint import io.element.android.libraries.architecture.BackstackNode import io.element.android.libraries.architecture.animation.rememberDefaultTransitionHandler +import io.element.android.libraries.architecture.bindings import io.element.android.libraries.di.AppScope +import io.element.android.libraries.matrix.ui.di.MatrixUIBindings import kotlinx.parcelize.Parcelize @ContributesNode(AppScope::class) @@ -49,6 +53,16 @@ class NotLoggedInFlowNode @AssistedInject constructor( buildContext = buildContext, plugins = plugins, ) { + override fun onBuilt() { + super.onBuilt() + lifecycle.subscribe( + onCreate = { + val imageLoaderFactory = bindings().notLoggedInImageLoaderFactory() + Coil.setImageLoader(imageLoaderFactory) + }, + ) + } + sealed interface NavTarget : Parcelable { @Parcelize object OnBoarding : NavTarget From 47b684f7247099e152cf9891e04d5550247085d8 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 12 Jul 2023 12:23:46 +0200 Subject: [PATCH 11/20] Let RootFlowNode manage MatrixClientsHolder save and restoration. --- .../io/element/android/x/MainActivity.kt | 8 +---- .../io/element/android/x/di/AppBindings.kt | 2 -- .../io/element/android/appnav/RootFlowNode.kt | 7 +++++ .../android/appnav/di/MatrixClientsHolder.kt | 31 +++++++++---------- 4 files changed, 23 insertions(+), 25 deletions(-) diff --git a/app/src/main/kotlin/io/element/android/x/MainActivity.kt b/app/src/main/kotlin/io/element/android/x/MainActivity.kt index be9f6134ba..cf37b22159 100644 --- a/app/src/main/kotlin/io/element/android/x/MainActivity.kt +++ b/app/src/main/kotlin/io/element/android/x/MainActivity.kt @@ -52,8 +52,7 @@ class MainActivity : NodeComponentActivity() { Timber.tag(loggerTag.value).w("onCreate, with savedInstanceState: ${savedInstanceState != null}") installSplashScreen() super.onCreate(savedInstanceState) - appBindings = bindings() - appBindings.matrixClientsHolder().restore(savedInstanceState) + appBindings = bindings() WindowCompat.setDecorFitsSystemWindows(window, false) setContent { MainContent(appBindings) @@ -125,9 +124,4 @@ class MainActivity : NodeComponentActivity() { super.onDestroy() Timber.tag(loggerTag.value).w("onDestroy") } - - override fun onSaveInstanceState(outState: Bundle) { - super.onSaveInstanceState(outState) - bindings().matrixClientsHolder().onSaveInstanceState(outState) - } } diff --git a/app/src/main/kotlin/io/element/android/x/di/AppBindings.kt b/app/src/main/kotlin/io/element/android/x/di/AppBindings.kt index 59f7e98d20..4d75d8601e 100644 --- a/app/src/main/kotlin/io/element/android/x/di/AppBindings.kt +++ b/app/src/main/kotlin/io/element/android/x/di/AppBindings.kt @@ -17,13 +17,11 @@ package io.element.android.x.di import com.squareup.anvil.annotations.ContributesTo -import io.element.android.appnav.di.MatrixClientsHolder import io.element.android.libraries.designsystem.utils.SnackbarDispatcher import io.element.android.libraries.di.AppScope @ContributesTo(AppScope::class) interface AppBindings { - fun matrixClientsHolder(): MatrixClientsHolder fun mainDaggerComponentOwner(): MainDaggerComponentsOwner fun snackbarDispatcher(): SnackbarDispatcher } diff --git a/appnav/src/main/kotlin/io/element/android/appnav/RootFlowNode.kt b/appnav/src/main/kotlin/io/element/android/appnav/RootFlowNode.kt index 8803d574d9..4150bcaebc 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/RootFlowNode.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/RootFlowNode.kt @@ -30,6 +30,7 @@ import com.bumble.appyx.core.node.Node import com.bumble.appyx.core.node.node import com.bumble.appyx.core.plugin.Plugin import com.bumble.appyx.core.plugin.plugins +import com.bumble.appyx.core.state.MutableSavedStateMap import com.bumble.appyx.navmodel.backstack.BackStack import com.bumble.appyx.navmodel.backstack.operation.pop import com.bumble.appyx.navmodel.backstack.operation.push @@ -90,10 +91,16 @@ class RootFlowNode @AssistedInject constructor( ) { override fun onBuilt() { + matrixClientsHolder.restore(buildContext.savedStateMap) super.onBuilt() observeLoggedInState() } + override fun onSaveInstanceState(state: MutableSavedStateMap) { + super.onSaveInstanceState(state) + matrixClientsHolder.save(state) + } + private fun observeLoggedInState() { combine( cacheService.onClearedCacheEventFlow(), diff --git a/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixClientsHolder.kt b/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixClientsHolder.kt index 092cffd630..24c082627f 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixClientsHolder.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixClientsHolder.kt @@ -16,13 +16,11 @@ package io.element.android.appnav.di -import android.os.Bundle -import io.element.android.libraries.di.AppScope -import io.element.android.libraries.di.SingleIn +import com.bumble.appyx.core.state.MutableSavedStateMap +import com.bumble.appyx.core.state.SavedStateMap import io.element.android.libraries.matrix.api.MatrixClient import io.element.android.libraries.matrix.api.auth.MatrixAuthenticationService import io.element.android.libraries.matrix.api.core.SessionId -import io.element.android.libraries.matrix.api.core.UserId import kotlinx.coroutines.runBlocking import timber.log.Timber import java.util.concurrent.ConcurrentHashMap @@ -30,7 +28,6 @@ import javax.inject.Inject private const val SAVE_INSTANCE_KEY = "io.element.android.x.di.MatrixClientsHolder.SaveInstanceKey" -@SingleIn(AppScope::class) class MatrixClientsHolder @Inject constructor(private val authenticationService: MatrixAuthenticationService) { private val sessionIdsToMatrixClient = ConcurrentHashMap() @@ -55,16 +52,18 @@ class MatrixClientsHolder @Inject constructor(private val authenticationService: return sessionIdsToMatrixClient[sessionId] } - @Suppress("DEPRECATION") - fun restore(savedInstanceState: Bundle?) { - if (savedInstanceState == null || sessionIdsToMatrixClient.isNotEmpty()) return - val userIds = savedInstanceState.getSerializable(SAVE_INSTANCE_KEY) as? Array - if (userIds.isNullOrEmpty()) return + @Suppress("UNCHECKED_CAST") + fun restore(state: SavedStateMap?) { + if (state == null || sessionIdsToMatrixClient.isNotEmpty()) return Unit.also { + Timber.w("Restore with non-empty map") + } + val sessionIds = state[SAVE_INSTANCE_KEY] as? Array + if (sessionIds.isNullOrEmpty()) return // Not ideal but should only happens in case of process recreation. This ensure we restore all the active sessions before restoring the node graphs. runBlocking { - userIds.forEach { userId -> - Timber.v("Restore matrix session: $userId") - authenticationService.restoreSession(userId) + sessionIds.forEach { sessionId -> + Timber.d("Restore matrix session: $sessionId") + authenticationService.restoreSession(sessionId) .onSuccess { matrixClient -> add(matrixClient) } @@ -75,9 +74,9 @@ class MatrixClientsHolder @Inject constructor(private val authenticationService: } } - fun onSaveInstanceState(outState: Bundle) { + fun save(state: MutableSavedStateMap) { val sessionKeys = sessionIdsToMatrixClient.keys.toTypedArray() - Timber.v("Save matrix session keys = $sessionKeys") - outState.putSerializable(SAVE_INSTANCE_KEY, sessionKeys) + Timber.d("Save matrix session keys = $sessionKeys") + state[SAVE_INSTANCE_KEY] = sessionKeys } } From 1627dbfd279988def218ef49fac59177fb9cca33 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 12 Jul 2023 12:39:41 +0200 Subject: [PATCH 12/20] Improve logs. --- .../io/element/android/appnav/di/MatrixClientsHolder.kt | 4 +++- .../libraries/matrix/api/room/RoomSummaryDataSource.kt | 3 ++- .../android/libraries/matrix/impl/room/RoomListExtensions.kt | 2 +- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixClientsHolder.kt b/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixClientsHolder.kt index 24c082627f..bbb14d4d29 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixClientsHolder.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixClientsHolder.kt @@ -54,10 +54,12 @@ class MatrixClientsHolder @Inject constructor(private val authenticationService: @Suppress("UNCHECKED_CAST") fun restore(state: SavedStateMap?) { + Timber.d("Restore state") if (state == null || sessionIdsToMatrixClient.isNotEmpty()) return Unit.also { Timber.w("Restore with non-empty map") } val sessionIds = state[SAVE_INSTANCE_KEY] as? Array + Timber.d("Restore matrix session keys = ${sessionIds?.map { it.value }}") if (sessionIds.isNullOrEmpty()) return // Not ideal but should only happens in case of process recreation. This ensure we restore all the active sessions before restoring the node graphs. runBlocking { @@ -76,7 +78,7 @@ class MatrixClientsHolder @Inject constructor(private val authenticationService: fun save(state: MutableSavedStateMap) { val sessionKeys = sessionIdsToMatrixClient.keys.toTypedArray() - Timber.d("Save matrix session keys = $sessionKeys") + Timber.d("Save matrix session keys = ${sessionKeys.map { it.value }}") state[SAVE_INSTANCE_KEY] = sessionKeys } } diff --git a/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/room/RoomSummaryDataSource.kt b/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/room/RoomSummaryDataSource.kt index e6c7b34056..d677d56ed9 100644 --- a/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/room/RoomSummaryDataSource.kt +++ b/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/room/RoomSummaryDataSource.kt @@ -38,12 +38,13 @@ interface RoomSummaryDataSource { suspend fun RoomSummaryDataSource.awaitAllRoomsAreLoaded(timeout: Duration = Duration.INFINITE) { try { + Timber.d("awaitAllRoomsAreLoaded: wait") withTimeout(timeout) { allRoomsLoadingState().firstOrNull { it is RoomSummaryDataSource.LoadingState.Loaded } } } catch (timeoutException: TimeoutCancellationException) { - Timber.v("AwaitAllRooms: no response after $timeout") + Timber.d("awaitAllRoomsAreLoaded: no response after $timeout") } } diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/room/RoomListExtensions.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/room/RoomListExtensions.kt index 824d477fd3..84c4eeaefb 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/room/RoomListExtensions.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/room/RoomListExtensions.kt @@ -62,7 +62,7 @@ fun RoomListService.roomOrNull(roomId: String): RoomListItem? { return try { room(roomId) } catch (exception: RoomListException) { - Timber.e(exception, "Failed finding room with id=$roomId") + Timber.d(exception, "Failed finding room with id=$roomId.") return null } } From 67fd2ebba95ef6cb53502c14e3c4927293f24003 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 12 Jul 2023 14:11:26 +0200 Subject: [PATCH 13/20] Fix warning (rename the base parameter name). --- .../io/element/android/libraries/matrix/api/room/MatrixRoom.kt | 2 +- .../android/libraries/matrix/test/room/FakeMatrixRoom.kt | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/room/MatrixRoom.kt b/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/room/MatrixRoom.kt index 3ad391ead8..05dfda714c 100644 --- a/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/room/MatrixRoom.kt +++ b/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/room/MatrixRoom.kt @@ -86,7 +86,7 @@ interface MatrixRoom : Closeable { suspend fun toggleReaction(emoji: String, eventId: EventId): Result - suspend fun forwardEvent(eventId: EventId, rooms: List): Result + suspend fun forwardEvent(eventId: EventId, roomIds: List): Result suspend fun retrySendMessage(transactionId: String): Result diff --git a/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/room/FakeMatrixRoom.kt b/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/room/FakeMatrixRoom.kt index 9392fb7985..3f265f392a 100644 --- a/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/room/FakeMatrixRoom.kt +++ b/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/room/FakeMatrixRoom.kt @@ -239,7 +239,7 @@ class FakeMatrixRoom( override suspend fun sendFile(file: File, fileInfo: FileInfo, progressCallback: ProgressCallback?): Result = fakeSendMedia(progressCallback) - override suspend fun forwardEvent(eventId: EventId, rooms: List): Result = simulateLongTask { + override suspend fun forwardEvent(eventId: EventId, roomIds: List): Result = simulateLongTask { forwardEventResult } From 92f5c96936be6a509c61c91fb47a553f42b1dbb2 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 12 Jul 2023 14:14:44 +0200 Subject: [PATCH 14/20] Use the param (bad copy paste) --- .../libraries/push/impl/notifications/NotificationFactory.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/NotificationFactory.kt b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/NotificationFactory.kt index 0addc1276a..9dd6c7d4cd 100644 --- a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/NotificationFactory.kt +++ b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/NotificationFactory.kt @@ -123,7 +123,7 @@ class NotificationFactory @Inject constructor( val roomMeta = roomNotifications.filterIsInstance().map { it.meta } val invitationMeta = invitationNotifications.filterIsInstance().map { it.meta } val simpleMeta = simpleNotifications.filterIsInstance().map { it.meta } - val fallbackMeta = simpleNotifications.filterIsInstance().map { it.meta } + val fallbackMeta = fallbackNotifications.filterIsInstance().map { it.meta } return when { roomMeta.isEmpty() && invitationMeta.isEmpty() && simpleMeta.isEmpty() -> SummaryNotification.Removed else -> SummaryNotification.Update( From 19fc90385c8abd8846274387639b51bcb0b62370 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 12 Jul 2023 14:15:25 +0200 Subject: [PATCH 15/20] Fix another warning. --- .../features/messages/forward/ForwardMessagesPresenterTests.kt | 1 - 1 file changed, 1 deletion(-) diff --git a/features/messages/impl/src/test/kotlin/io/element/android/features/messages/forward/ForwardMessagesPresenterTests.kt b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/forward/ForwardMessagesPresenterTests.kt index 82526e100d..502305ab1d 100644 --- a/features/messages/impl/src/test/kotlin/io/element/android/features/messages/forward/ForwardMessagesPresenterTests.kt +++ b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/forward/ForwardMessagesPresenterTests.kt @@ -65,7 +65,6 @@ class ForwardMessagesPresenterTests { }.test { val initialState = awaitItem() skipItems(1) - val summary = aRoomSummaryDetail() initialState.eventSink(ForwardMessagesEvents.ToggleSearchActive) assertThat(awaitItem().isSearchActive).isTrue() From c8912060fb349bd4e7087f7844997c26aa67e542 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 12 Jul 2023 14:16:10 +0200 Subject: [PATCH 16/20] Fix another warning. --- .../eventformatter/impl/RoomMembershipContentFormatter.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/eventformatter/impl/src/main/kotlin/io/element/android/libraries/eventformatter/impl/RoomMembershipContentFormatter.kt b/libraries/eventformatter/impl/src/main/kotlin/io/element/android/libraries/eventformatter/impl/RoomMembershipContentFormatter.kt index f8ac3b491c..6a65a9bd1e 100644 --- a/libraries/eventformatter/impl/src/main/kotlin/io/element/android/libraries/eventformatter/impl/RoomMembershipContentFormatter.kt +++ b/libraries/eventformatter/impl/src/main/kotlin/io/element/android/libraries/eventformatter/impl/RoomMembershipContentFormatter.kt @@ -34,7 +34,7 @@ class RoomMembershipContentFormatter @Inject constructor( ): CharSequence? { val userId = membershipContent.userId val memberIsYou = matrixClient.isMe(userId) - return when (val change = membershipContent.change) { + return when (membershipContent.change) { MembershipChange.JOINED -> if (memberIsYou) { sp.getString(R.string.state_event_room_join_by_you) } else { From 0d45096b5935690cb8a024c816de8b8b4eb3efb7 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 12 Jul 2023 15:11:40 +0200 Subject: [PATCH 17/20] Split task in 2, due to the fact that when we run kover on the CI, run only debug test variants. Error was: Some problems were found with the configuration of task ':koverMergedHtmlReport' (type 'KoverHtmlTask'). - Gradle detected a problem with the following location: '/home/runner/work/element-x-android/element-x-android/features/analytics/api/build/tmp/kotlin-classes/release'. Reason: Task ':koverMergedHtmlReport' uses this output of task ':features:analytics:api:compileReleaseKotlin' without declaring an explicit or implicit dependency. This can lead to incorrect results being produced, depending on what order the tasks are executed. Possible solutions: 1. Declare task ':features:analytics:api:compileReleaseKotlin' as an input of ':koverMergedHtmlReport'. 2. Declare an explicit dependency on ':features:analytics:api:compileReleaseKotlin' from ':koverMergedHtmlReport' using Task#dependsOn. 3. Declare an explicit dependency on ':features:analytics:api:compileReleaseKotlin' from ':koverMergedHtmlReport' using Task#mustRunAfter. ... --- .github/workflows/nightlyReports.yml | 5 ++++- .github/workflows/tests.yml | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/.github/workflows/nightlyReports.yml b/.github/workflows/nightlyReports.yml index 4805bf44d1..cbe9935fc7 100644 --- a/.github/workflows/nightlyReports.yml +++ b/.github/workflows/nightlyReports.yml @@ -26,8 +26,11 @@ jobs: distribution: 'temurin' # See 'Supported distributions' for available options java-version: '17' + - name: ⚙️ Run unit & screenshot tests, debug and release + run: ./gradlew test $CI_GRADLE_ARG_PROPERTIES -Pci-build=true + - name: ⚙️ Run unit & screenshot tests, generate kover report - run: ./gradlew test koverMergedReport $CI_GRADLE_ARG_PROPERTIES -Pci-build=true + run: ./gradlew koverMergedReport $CI_GRADLE_ARG_PROPERTIES -Pci-build=true - name: ✅ Upload kover report if: always() diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 44fdf58323..f5df6b1362 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -37,8 +37,11 @@ jobs: with: cache-read-only: ${{ github.ref != 'refs/heads/develop' }} + - name: ⚙️ Run unit & screenshot tests, debug and release + run: ./gradlew test $CI_GRADLE_ARG_PROPERTIES -Pci-build=true + - name: ⚙️ Run unit & screenshot tests, generate kover report - run: ./gradlew test koverMergedReport $CI_GRADLE_ARG_PROPERTIES -Pci-build=true + run: ./gradlew koverMergedReport $CI_GRADLE_ARG_PROPERTIES -Pci-build=true - name: 📈 Verify coverage run: ./gradlew koverMergedVerify $CI_GRADLE_ARG_PROPERTIES -Pci-build=true From d3a95afe863945ee5640a87c1c4d93023b43d2c1 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 12 Jul 2023 15:32:00 +0200 Subject: [PATCH 18/20] Fix crash at first startup. Inject NotLoggedInImageLoaderFactory directly to NotLoggedInFlowNode --- .../io/element/android/appnav/NotLoggedInFlowNode.kt | 7 +++---- .../android/libraries/matrix/ui/di/MatrixUIBindings.kt | 2 -- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/appnav/src/main/kotlin/io/element/android/appnav/NotLoggedInFlowNode.kt b/appnav/src/main/kotlin/io/element/android/appnav/NotLoggedInFlowNode.kt index ac17f6ad00..1ed1aec678 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/NotLoggedInFlowNode.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/NotLoggedInFlowNode.kt @@ -34,9 +34,8 @@ import io.element.android.features.login.api.LoginEntryPoint import io.element.android.features.onboarding.api.OnBoardingEntryPoint import io.element.android.libraries.architecture.BackstackNode import io.element.android.libraries.architecture.animation.rememberDefaultTransitionHandler -import io.element.android.libraries.architecture.bindings import io.element.android.libraries.di.AppScope -import io.element.android.libraries.matrix.ui.di.MatrixUIBindings +import io.element.android.libraries.matrix.ui.media.NotLoggedInImageLoaderFactory import kotlinx.parcelize.Parcelize @ContributesNode(AppScope::class) @@ -45,6 +44,7 @@ class NotLoggedInFlowNode @AssistedInject constructor( @Assisted plugins: List, private val onBoardingEntryPoint: OnBoardingEntryPoint, private val loginEntryPoint: LoginEntryPoint, + private val notLoggedInImageLoaderFactory: NotLoggedInImageLoaderFactory, ) : BackstackNode( backstack = BackStack( initialElement = NavTarget.OnBoarding, @@ -57,8 +57,7 @@ class NotLoggedInFlowNode @AssistedInject constructor( super.onBuilt() lifecycle.subscribe( onCreate = { - val imageLoaderFactory = bindings().notLoggedInImageLoaderFactory() - Coil.setImageLoader(imageLoaderFactory) + Coil.setImageLoader(notLoggedInImageLoaderFactory) }, ) } diff --git a/libraries/matrixui/src/main/kotlin/io/element/android/libraries/matrix/ui/di/MatrixUIBindings.kt b/libraries/matrixui/src/main/kotlin/io/element/android/libraries/matrix/ui/di/MatrixUIBindings.kt index a5734f5b9c..919971b307 100644 --- a/libraries/matrixui/src/main/kotlin/io/element/android/libraries/matrix/ui/di/MatrixUIBindings.kt +++ b/libraries/matrixui/src/main/kotlin/io/element/android/libraries/matrix/ui/di/MatrixUIBindings.kt @@ -19,10 +19,8 @@ package io.element.android.libraries.matrix.ui.di import com.squareup.anvil.annotations.ContributesTo import io.element.android.libraries.di.SessionScope import io.element.android.libraries.matrix.ui.media.LoggedInImageLoaderFactory -import io.element.android.libraries.matrix.ui.media.NotLoggedInImageLoaderFactory @ContributesTo(SessionScope::class) interface MatrixUIBindings { fun loggedInImageLoaderFactory(): LoggedInImageLoaderFactory - fun notLoggedInImageLoaderFactory(): NotLoggedInImageLoaderFactory } From 86a2c340ee4e1b86e821c794cda348d0f8580e50 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 12 Jul 2023 17:28:53 +0200 Subject: [PATCH 19/20] Ensure pending intent data are unique. --- .../push/impl/notifications/factories/PendingIntentFactory.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/factories/PendingIntentFactory.kt b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/factories/PendingIntentFactory.kt index fc7586cbd7..2fe02a8958 100644 --- a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/factories/PendingIntentFactory.kt +++ b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/factories/PendingIntentFactory.kt @@ -106,7 +106,7 @@ class PendingIntentFactory @Inject constructor( fun createDismissEventPendingIntent(sessionId: SessionId, roomId: RoomId, eventId: EventId): PendingIntent { val intent = Intent(context, NotificationBroadcastReceiver::class.java) intent.action = actionIds.dismissEvent - intent.data = createIgnoredUri("deleteEvent/$sessionId/$roomId") + intent.data = createIgnoredUri("deleteEvent/$sessionId/$roomId/$eventId") intent.putExtra(NotificationBroadcastReceiver.KEY_SESSION_ID, sessionId.value) intent.putExtra(NotificationBroadcastReceiver.KEY_ROOM_ID, roomId.value) intent.putExtra(NotificationBroadcastReceiver.KEY_EVENT_ID, eventId.value) From 32f86b272526f5b06026c7b5e5842cd053864490 Mon Sep 17 00:00:00 2001 From: Kat Gerasimova Date: Tue, 11 Jul 2023 14:24:38 +0100 Subject: [PATCH 20/20] Try to debug project automation column issues The error I get from my private test repo is different from this one. Need to check what the first step returns --- .github/workflows/triage-labelled.yml | 35 ++++++++++++++------------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/.github/workflows/triage-labelled.yml b/.github/workflows/triage-labelled.yml index 138708552f..acbae99bb4 100644 --- a/.github/workflows/triage-labelled.yml +++ b/.github/workflows/triage-labelled.yml @@ -17,23 +17,24 @@ jobs: project-url: https://github.com/orgs/vector-im/projects/43 github-token: ${{ secrets.ELEMENT_BOT_TOKEN }} -# move_needs_info: -# name: Move triaged needs info issues on board -# runs-on: ubuntu-latest -# steps: -# - uses: actions/add-to-project@main -# id: addItem -# with: -# project-url: https://github.com/orgs/vector-im/projects/91 -# github-token: ${{ secrets.ELEMENT_BOT_TOKEN }} -# labeled: X-Needs-Info -# -# - uses: kalgurn/update-project-item-status@main -# with: -# project-url: https://github.com/orgs/vector-im/projects/91 -# github-token: ${{ secrets.ELEMENT_BOT_TOKEN }} -# item-id: ${{ steps.addItem.outputs.itemId }} -# status: "Needs info" + move_needs_info: + name: Move triaged needs info issues on board + runs-on: ubuntu-latest + steps: + - uses: actions/add-to-project@main + id: addItem + with: + project-url: https://github.com/orgs/vector-im/projects/91 + github-token: ${{ secrets.ELEMENT_BOT_TOKEN }} + labeled: X-Needs-Info + - name: Print itemId + run: echo ${{ steps.addItem.outputs.itemId }} + - uses: kalgurn/update-project-item-status@main + with: + project-url: https://github.com/orgs/vector-im/projects/91 + github-token: ${{ secrets.ELEMENT_BOT_TOKEN }} + item-id: ${{ steps.addItem.outputs.itemId }} + status: "Needs info" ex_plorers: name: Add labelled issues to X-Plorer project