From 084fa780727a8ca99966435493853905fcef4f64 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Thu, 7 Nov 2024 17:05:18 +0100 Subject: [PATCH 01/15] Disable button during the "verifying" step. --- .../verifysession/impl/outgoing/VerifySelfSessionView.kt | 1 + 1 file changed, 1 insertion(+) diff --git a/features/verifysession/impl/src/main/kotlin/io/element/android/features/verifysession/impl/outgoing/VerifySelfSessionView.kt b/features/verifysession/impl/src/main/kotlin/io/element/android/features/verifysession/impl/outgoing/VerifySelfSessionView.kt index afebffea47..2a4d113236 100644 --- a/features/verifysession/impl/src/main/kotlin/io/element/android/features/verifysession/impl/outgoing/VerifySelfSessionView.kt +++ b/features/verifysession/impl/src/main/kotlin/io/element/android/features/verifysession/impl/outgoing/VerifySelfSessionView.kt @@ -335,6 +335,7 @@ private fun VerifySelfSessionBottomMenu( modifier = Modifier.fillMaxWidth(), text = positiveButtonTitle, showProgress = isVerifying, + enabled = !isVerifying, onClick = { if (!isVerifying) { eventSink(VerifySelfSessionViewEvents.ConfirmVerification) From ea32b39a98f26641b114f9791607d2b9ab44ed9c Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Thu, 7 Nov 2024 18:33:37 +0100 Subject: [PATCH 02/15] ElementCall: allow user to switch to another call. --- .../call/impl/ui/ElementCallActivity.kt | 39 ++++++++++--------- 1 file changed, 21 insertions(+), 18 deletions(-) diff --git a/features/call/impl/src/main/kotlin/io/element/android/features/call/impl/ui/ElementCallActivity.kt b/features/call/impl/src/main/kotlin/io/element/android/features/call/impl/ui/ElementCallActivity.kt index a685ef7b9a..95d8dad64c 100644 --- a/features/call/impl/src/main/kotlin/io/element/android/features/call/impl/ui/ElementCallActivity.kt +++ b/features/call/impl/src/main/kotlin/io/element/android/features/call/impl/ui/ElementCallActivity.kt @@ -35,6 +35,7 @@ import androidx.core.content.IntentCompat import androidx.core.util.Consumer import androidx.lifecycle.Lifecycle import io.element.android.features.call.api.CallType +import io.element.android.features.call.api.CallType.ExternalUrl import io.element.android.features.call.impl.DefaultElementCallEntryPoint import io.element.android.features.call.impl.di.CallBindings import io.element.android.features.call.impl.pip.PictureInPictureEvents @@ -49,6 +50,8 @@ import io.element.android.libraries.preferences.api.store.AppPreferencesStore import timber.log.Timber import javax.inject.Inject +private const val loggerTag = "ElementCallActivity" + class ElementCallActivity : AppCompatActivity(), CallScreenNavigator, @@ -132,7 +135,7 @@ class ElementCallActivity : DisposableEffect(Unit) { val listener = Runnable { if (requestPermissionCallback != null) { - Timber.w("Ignoring onUserLeaveHint event because user is asked to grant permissions") + Timber.tag(loggerTag).w("Ignoring onUserLeaveHint event because user is asked to grant permissions") } else { pipEventSink(PictureInPictureEvents.EnterPictureInPicture) } @@ -146,7 +149,7 @@ class ElementCallActivity : val onPictureInPictureModeChangedListener = Consumer { _: PictureInPictureModeChangedInfo -> pipEventSink(PictureInPictureEvents.OnPictureInPictureModeChanged(isInPictureInPictureMode)) if (!isInPictureInPictureMode && !lifecycle.currentState.isAtLeast(Lifecycle.State.STARTED)) { - Timber.d("Exiting PiP mode: Hangup the call") + Timber.tag(loggerTag).d("Exiting PiP mode: Hangup the call") eventSink?.invoke(CallScreenEvents.Hangup) } } @@ -185,23 +188,23 @@ class ElementCallActivity : private fun setCallType(intent: Intent?) { val callType = intent?.let { - IntentCompat.getParcelableExtra(it, DefaultElementCallEntryPoint.EXTRA_CALL_TYPE, CallType::class.java) + IntentCompat.getParcelableExtra(intent, DefaultElementCallEntryPoint.EXTRA_CALL_TYPE, CallType::class.java) + ?: intent.dataString?.let(::parseUrl)?.let(::ExternalUrl) } - val intentUrl = intent?.dataString?.let(::parseUrl) - when { - // Re-opened the activity but we have no url to load or a cached one, finish the activity - intent?.dataString == null && callType == null && webViewTarget.value == null -> finish() - callType != null -> { - webViewTarget.value = callType - presenter = presenterFactory.create(callType, this) - } - intentUrl != null -> { - val fallbackInputs = CallType.ExternalUrl(intentUrl) - webViewTarget.value = fallbackInputs - presenter = presenterFactory.create(fallbackInputs, this) - } - // Coming back from notification, do nothing - else -> return + val currentCallType = webViewTarget.value + if (currentCallType == null && callType == null) { + Timber.tag(loggerTag).d("Re-opened the activity but we have no url to load or a cached one, finish the activity") + finish() + } else if (currentCallType == null) { + Timber.tag(loggerTag).d("Set the call type and create the presenter") + webViewTarget.value = callType + presenter = presenterFactory.create(callType!!, this) + } else if (callType != currentCallType) { + Timber.tag(loggerTag).d("User starts another call, restart the Activity") + setIntent(intent) + recreate() + } else { + Timber.tag(loggerTag).d("Coming back from notification, do nothing") } } From 700d4c62af975b035c3a864734c601efacd77765 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Thu, 7 Nov 2024 18:39:46 +0100 Subject: [PATCH 03/15] Hide "They don't match" button when verifying. --- .../impl/outgoing/VerifySelfSessionView.kt | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/features/verifysession/impl/src/main/kotlin/io/element/android/features/verifysession/impl/outgoing/VerifySelfSessionView.kt b/features/verifysession/impl/src/main/kotlin/io/element/android/features/verifysession/impl/outgoing/VerifySelfSessionView.kt index 2a4d113236..d60986e1cb 100644 --- a/features/verifysession/impl/src/main/kotlin/io/element/android/features/verifysession/impl/outgoing/VerifySelfSessionView.kt +++ b/features/verifysession/impl/src/main/kotlin/io/element/android/features/verifysession/impl/outgoing/VerifySelfSessionView.kt @@ -342,11 +342,16 @@ private fun VerifySelfSessionBottomMenu( } }, ) - TextButton( - modifier = Modifier.fillMaxWidth(), - text = stringResource(R.string.screen_session_verification_they_dont_match), - onClick = { eventSink(VerifySelfSessionViewEvents.DeclineVerification) }, - ) + if (isVerifying) { + // Placeholder so the 1st button keeps its vertical position + Spacer(modifier = Modifier.height(40.dp)) + } else { + TextButton( + modifier = Modifier.fillMaxWidth(), + text = stringResource(R.string.screen_session_verification_they_dont_match), + onClick = { eventSink(VerifySelfSessionViewEvents.DeclineVerification) }, + ) + } } } is Step.Completed -> { From 8b21efa32a4ac9309419ad79b37beb853d09297d Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Thu, 7 Nov 2024 18:51:17 +0100 Subject: [PATCH 04/15] Ensure the invisible buttons have the correct size. --- .../impl/incoming/IncomingVerificationView.kt | 9 +++---- .../impl/outgoing/VerifySelfSessionView.kt | 18 +++++-------- .../designsystem/theme/components/Button.kt | 26 +++++++++++++------ 3 files changed, 27 insertions(+), 26 deletions(-) diff --git a/features/verifysession/impl/src/main/kotlin/io/element/android/features/verifysession/impl/incoming/IncomingVerificationView.kt b/features/verifysession/impl/src/main/kotlin/io/element/android/features/verifysession/impl/incoming/IncomingVerificationView.kt index e8f098f848..3d6adf28cd 100644 --- a/features/verifysession/impl/src/main/kotlin/io/element/android/features/verifysession/impl/incoming/IncomingVerificationView.kt +++ b/features/verifysession/impl/src/main/kotlin/io/element/android/features/verifysession/impl/incoming/IncomingVerificationView.kt @@ -10,9 +10,7 @@ package io.element.android.features.verifysession.impl.incoming import androidx.activity.compose.BackHandler import androidx.compose.foundation.layout.Arrangement import androidx.compose.foundation.layout.Column -import androidx.compose.foundation.layout.Spacer import androidx.compose.foundation.layout.fillMaxWidth -import androidx.compose.foundation.layout.height import androidx.compose.foundation.layout.padding import androidx.compose.material3.ExperimentalMaterial3Api import androidx.compose.runtime.Composable @@ -35,6 +33,7 @@ import io.element.android.libraries.designsystem.components.PageTitle import io.element.android.libraries.designsystem.preview.ElementPreview import io.element.android.libraries.designsystem.preview.PreviewsDayNight import io.element.android.libraries.designsystem.theme.components.Button +import io.element.android.libraries.designsystem.theme.components.InvisibleButton import io.element.android.libraries.designsystem.theme.components.Text import io.element.android.libraries.designsystem.theme.components.TextButton import io.element.android.libraries.designsystem.theme.components.TopAppBar @@ -166,8 +165,7 @@ private fun IncomingVerificationBottomMenu( enabled = false, showProgress = true, ) - // Placeholder so the 1st button keeps its vertical position - Spacer(modifier = Modifier.height(40.dp)) + InvisibleButton() } } else { VerificationBottomMenu { @@ -194,8 +192,7 @@ private fun IncomingVerificationBottomMenu( enabled = false, showProgress = true, ) - // Placeholder so the 1st button keeps its vertical position - Spacer(modifier = Modifier.height(40.dp)) + InvisibleButton() } } else { VerificationBottomMenu { diff --git a/features/verifysession/impl/src/main/kotlin/io/element/android/features/verifysession/impl/outgoing/VerifySelfSessionView.kt b/features/verifysession/impl/src/main/kotlin/io/element/android/features/verifysession/impl/outgoing/VerifySelfSessionView.kt index d60986e1cb..e1e22eda33 100644 --- a/features/verifysession/impl/src/main/kotlin/io/element/android/features/verifysession/impl/outgoing/VerifySelfSessionView.kt +++ b/features/verifysession/impl/src/main/kotlin/io/element/android/features/verifysession/impl/outgoing/VerifySelfSessionView.kt @@ -12,10 +12,8 @@ import androidx.compose.foundation.clickable import androidx.compose.foundation.layout.Arrangement import androidx.compose.foundation.layout.Box import androidx.compose.foundation.layout.Row -import androidx.compose.foundation.layout.Spacer import androidx.compose.foundation.layout.fillMaxSize import androidx.compose.foundation.layout.fillMaxWidth -import androidx.compose.foundation.layout.height import androidx.compose.foundation.layout.padding import androidx.compose.material3.ExperimentalMaterial3Api import androidx.compose.runtime.Composable @@ -44,6 +42,7 @@ import io.element.android.libraries.designsystem.preview.ElementPreview import io.element.android.libraries.designsystem.preview.PreviewsDayNight import io.element.android.libraries.designsystem.theme.components.Button import io.element.android.libraries.designsystem.theme.components.CircularProgressIndicator +import io.element.android.libraries.designsystem.theme.components.InvisibleButton import io.element.android.libraries.designsystem.theme.components.OutlinedButton import io.element.android.libraries.designsystem.theme.components.Text import io.element.android.libraries.designsystem.theme.components.TextButton @@ -282,8 +281,7 @@ private fun VerifySelfSessionBottomMenu( text = stringResource(CommonStrings.action_start_verification), onClick = { eventSink(VerifySelfSessionViewEvents.RequestVerification) }, ) - // Placeholder so the 1st button keeps its vertical position - Spacer(modifier = Modifier.height(40.dp)) + InvisibleButton() } } is Step.Canceled -> { @@ -293,8 +291,7 @@ private fun VerifySelfSessionBottomMenu( text = stringResource(CommonStrings.action_done), onClick = onCancelClick, ) - // Placeholder so the 1st button keeps its vertical position - Spacer(modifier = Modifier.height(40.dp)) + InvisibleButton() } } is Step.Ready -> { @@ -320,8 +317,7 @@ private fun VerifySelfSessionBottomMenu( showProgress = true, enabled = false, ) - // Placeholder so the 1st button keeps its vertical position - Spacer(modifier = Modifier.height(40.dp)) + InvisibleButton() } } is Step.Verifying -> { @@ -343,8 +339,7 @@ private fun VerifySelfSessionBottomMenu( }, ) if (isVerifying) { - // Placeholder so the 1st button keeps its vertical position - Spacer(modifier = Modifier.height(40.dp)) + InvisibleButton() } else { TextButton( modifier = Modifier.fillMaxWidth(), @@ -361,8 +356,7 @@ private fun VerifySelfSessionBottomMenu( text = stringResource(CommonStrings.action_continue), onClick = onContinueClick, ) - // Placeholder so the 1st button keeps its vertical position - Spacer(modifier = Modifier.height(48.dp)) + InvisibleButton() } } is Step.Skipped -> return diff --git a/libraries/designsystem/src/main/kotlin/io/element/android/libraries/designsystem/theme/components/Button.kt b/libraries/designsystem/src/main/kotlin/io/element/android/libraries/designsystem/theme/components/Button.kt index 01182343f8..9f49bca0bb 100644 --- a/libraries/designsystem/src/main/kotlin/io/element/android/libraries/designsystem/theme/components/Button.kt +++ b/libraries/designsystem/src/main/kotlin/io/element/android/libraries/designsystem/theme/components/Button.kt @@ -17,6 +17,7 @@ import androidx.compose.foundation.layout.PaddingValues import androidx.compose.foundation.layout.Row import androidx.compose.foundation.layout.Spacer import androidx.compose.foundation.layout.fillMaxWidth +import androidx.compose.foundation.layout.height import androidx.compose.foundation.layout.heightIn import androidx.compose.foundation.layout.padding import androidx.compose.foundation.layout.size @@ -118,6 +119,14 @@ fun TextButton( leadingIcon = leadingIcon ) +@Composable +fun InvisibleButton( + modifier: Modifier = Modifier, + size: ButtonSize = ButtonSize.Large, +) { + Spacer(modifier = modifier.height(size.toMinHeight())) +} + @Composable private fun ButtonInternal( text: String, @@ -131,14 +140,7 @@ private fun ButtonInternal( showProgress: Boolean = false, leadingIcon: IconSource? = null, ) { - val minHeight = when (size) { - ButtonSize.Small -> 32.dp - ButtonSize.Medium, - ButtonSize.MediumLowPadding -> 40.dp - ButtonSize.Large, - ButtonSize.LargeLowPadding -> 48.dp - } - + val minHeight = size.toMinHeight() val hasStartDrawable = showProgress || leadingIcon != null val contentPadding = when (size) { @@ -253,6 +255,14 @@ private fun ButtonInternal( } } +private fun ButtonSize.toMinHeight() = when (this) { + ButtonSize.Small -> 32.dp + ButtonSize.Medium, + ButtonSize.MediumLowPadding -> 40.dp + ButtonSize.Large, + ButtonSize.LargeLowPadding -> 48.dp +} + @Immutable sealed interface IconSource { val contentDescription: String? From 31f153469cf795e5d133b6416cc42d0753783f2e Mon Sep 17 00:00:00 2001 From: ElementBot Date: Thu, 7 Nov 2024 18:02:45 +0000 Subject: [PATCH 05/15] Update screenshots --- ...ession.impl.incoming_IncomingVerificationView_Day_1_en.png | 4 ++-- ...ession.impl.incoming_IncomingVerificationView_Day_3_en.png | 4 ++-- ...sion.impl.incoming_IncomingVerificationView_Night_1_en.png | 4 ++-- ...sion.impl.incoming_IncomingVerificationView_Night_3_en.png | 4 ++-- ...ysession.impl.outgoing_VerifySelfSessionView_Day_13_en.png | 4 ++-- ...fysession.impl.outgoing_VerifySelfSessionView_Day_1_en.png | 4 ++-- ...fysession.impl.outgoing_VerifySelfSessionView_Day_3_en.png | 4 ++-- ...fysession.impl.outgoing_VerifySelfSessionView_Day_4_en.png | 4 ++-- ...ession.impl.outgoing_VerifySelfSessionView_Night_13_en.png | 4 ++-- ...session.impl.outgoing_VerifySelfSessionView_Night_1_en.png | 4 ++-- ...session.impl.outgoing_VerifySelfSessionView_Night_3_en.png | 4 ++-- ...session.impl.outgoing_VerifySelfSessionView_Night_4_en.png | 4 ++-- 12 files changed, 24 insertions(+), 24 deletions(-) diff --git a/tests/uitests/src/test/snapshots/images/features.verifysession.impl.incoming_IncomingVerificationView_Day_1_en.png b/tests/uitests/src/test/snapshots/images/features.verifysession.impl.incoming_IncomingVerificationView_Day_1_en.png index 5a39ab5ef2..f55850de1d 100644 --- a/tests/uitests/src/test/snapshots/images/features.verifysession.impl.incoming_IncomingVerificationView_Day_1_en.png +++ b/tests/uitests/src/test/snapshots/images/features.verifysession.impl.incoming_IncomingVerificationView_Day_1_en.png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:dbf6f78ad928bcc9878e546345a98d334ae92c9b81d4d8404892a16d19b446c3 -size 41534 +oid sha256:bea78fb1bb813bedce30e5b13257892bcc23a7d6eb1a404d24e764337568d6cd +size 41596 diff --git a/tests/uitests/src/test/snapshots/images/features.verifysession.impl.incoming_IncomingVerificationView_Day_3_en.png b/tests/uitests/src/test/snapshots/images/features.verifysession.impl.incoming_IncomingVerificationView_Day_3_en.png index b219c98625..21f135beca 100644 --- a/tests/uitests/src/test/snapshots/images/features.verifysession.impl.incoming_IncomingVerificationView_Day_3_en.png +++ b/tests/uitests/src/test/snapshots/images/features.verifysession.impl.incoming_IncomingVerificationView_Day_3_en.png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:dfc69dc6d93a62e23df2f817ad5a167b1e94d7fb0d408d6ec0051d666b6cf175 -size 44869 +oid sha256:5d8163be5e84aa851df9666822a4d9bc7a40d3a99d6fa8498243c256b729d58f +size 44724 diff --git a/tests/uitests/src/test/snapshots/images/features.verifysession.impl.incoming_IncomingVerificationView_Night_1_en.png b/tests/uitests/src/test/snapshots/images/features.verifysession.impl.incoming_IncomingVerificationView_Night_1_en.png index 85072965e9..4731f5a297 100644 --- a/tests/uitests/src/test/snapshots/images/features.verifysession.impl.incoming_IncomingVerificationView_Night_1_en.png +++ b/tests/uitests/src/test/snapshots/images/features.verifysession.impl.incoming_IncomingVerificationView_Night_1_en.png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:b76212b5942484621b7a58044a958203d838807d50687e8f4e2f9c8bdb6ad37c -size 40232 +oid sha256:db8b048d11f766a8e3db28bac99f3e8dea34e6f37fba1eacd9e6f98f6127462b +size 40383 diff --git a/tests/uitests/src/test/snapshots/images/features.verifysession.impl.incoming_IncomingVerificationView_Night_3_en.png b/tests/uitests/src/test/snapshots/images/features.verifysession.impl.incoming_IncomingVerificationView_Night_3_en.png index a8b63f04d2..4a519862f3 100644 --- a/tests/uitests/src/test/snapshots/images/features.verifysession.impl.incoming_IncomingVerificationView_Night_3_en.png +++ b/tests/uitests/src/test/snapshots/images/features.verifysession.impl.incoming_IncomingVerificationView_Night_3_en.png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:1033af0fc84e2819509fc798c17e8f0b07d74a08da99d0e059b7ff19db2ce56a -size 43674 +oid sha256:c1098ef994ad9552aca2f9ad9efe7b1239133544e561e73eced6fa96d08eaa3b +size 43785 diff --git a/tests/uitests/src/test/snapshots/images/features.verifysession.impl.outgoing_VerifySelfSessionView_Day_13_en.png b/tests/uitests/src/test/snapshots/images/features.verifysession.impl.outgoing_VerifySelfSessionView_Day_13_en.png index 567462d90f..c13d05d4b3 100644 --- a/tests/uitests/src/test/snapshots/images/features.verifysession.impl.outgoing_VerifySelfSessionView_Day_13_en.png +++ b/tests/uitests/src/test/snapshots/images/features.verifysession.impl.outgoing_VerifySelfSessionView_Day_13_en.png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:18dadaebe7a32aacde31afa0352a343913955b099ca4a07851e3ffe75e88b4d6 -size 31012 +oid sha256:104d1d386398aac789420babf33755e03d163f40ae596cd8d24f35da0363fac6 +size 30952 diff --git a/tests/uitests/src/test/snapshots/images/features.verifysession.impl.outgoing_VerifySelfSessionView_Day_1_en.png b/tests/uitests/src/test/snapshots/images/features.verifysession.impl.outgoing_VerifySelfSessionView_Day_1_en.png index 29667b296d..2f77b586a2 100644 --- a/tests/uitests/src/test/snapshots/images/features.verifysession.impl.outgoing_VerifySelfSessionView_Day_1_en.png +++ b/tests/uitests/src/test/snapshots/images/features.verifysession.impl.outgoing_VerifySelfSessionView_Day_1_en.png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:71b9f32b26b391ff3bf231ca9f364a157f535c1e6ff52ee3e3ead3630bb1b239 -size 30007 +oid sha256:145c85570217621cc576f305ed72e27205381c438e434205e07b7363cfd67f04 +size 30069 diff --git a/tests/uitests/src/test/snapshots/images/features.verifysession.impl.outgoing_VerifySelfSessionView_Day_3_en.png b/tests/uitests/src/test/snapshots/images/features.verifysession.impl.outgoing_VerifySelfSessionView_Day_3_en.png index 528c702f24..21f135beca 100644 --- a/tests/uitests/src/test/snapshots/images/features.verifysession.impl.outgoing_VerifySelfSessionView_Day_3_en.png +++ b/tests/uitests/src/test/snapshots/images/features.verifysession.impl.outgoing_VerifySelfSessionView_Day_3_en.png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:c5435926f4a54e99902b78881c59d9d816110e401bcb92baf3b2fca844338f31 -size 48182 +oid sha256:5d8163be5e84aa851df9666822a4d9bc7a40d3a99d6fa8498243c256b729d58f +size 44724 diff --git a/tests/uitests/src/test/snapshots/images/features.verifysession.impl.outgoing_VerifySelfSessionView_Day_4_en.png b/tests/uitests/src/test/snapshots/images/features.verifysession.impl.outgoing_VerifySelfSessionView_Day_4_en.png index 1faa04af50..6a6ac768f0 100644 --- a/tests/uitests/src/test/snapshots/images/features.verifysession.impl.outgoing_VerifySelfSessionView_Day_4_en.png +++ b/tests/uitests/src/test/snapshots/images/features.verifysession.impl.outgoing_VerifySelfSessionView_Day_4_en.png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:000374157cb5fbf6670b4af041fc385538df78bf4aecaf83ea24d39dfec84f23 -size 24278 +oid sha256:781d869dff205f99d5a9bc9986abbe489bef24551e6fb695280541741895a508 +size 24238 diff --git a/tests/uitests/src/test/snapshots/images/features.verifysession.impl.outgoing_VerifySelfSessionView_Night_13_en.png b/tests/uitests/src/test/snapshots/images/features.verifysession.impl.outgoing_VerifySelfSessionView_Night_13_en.png index bebfa94f6d..36c7cdb859 100644 --- a/tests/uitests/src/test/snapshots/images/features.verifysession.impl.outgoing_VerifySelfSessionView_Night_13_en.png +++ b/tests/uitests/src/test/snapshots/images/features.verifysession.impl.outgoing_VerifySelfSessionView_Night_13_en.png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:4dd274f8c2ade6213a13a47400ec3a571844eafcc82b292b1c813bd9aa098236 -size 30241 +oid sha256:85b92d23ab2f690c3c15ded7f14c513371cd36482fbeda1478f62c82650f1829 +size 30274 diff --git a/tests/uitests/src/test/snapshots/images/features.verifysession.impl.outgoing_VerifySelfSessionView_Night_1_en.png b/tests/uitests/src/test/snapshots/images/features.verifysession.impl.outgoing_VerifySelfSessionView_Night_1_en.png index af8d7eadbe..49a47a746b 100644 --- a/tests/uitests/src/test/snapshots/images/features.verifysession.impl.outgoing_VerifySelfSessionView_Night_1_en.png +++ b/tests/uitests/src/test/snapshots/images/features.verifysession.impl.outgoing_VerifySelfSessionView_Night_1_en.png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:994b39ba25e011cce97504a1f3d4ed0c420b7bce87daafae77ba481cc629cdae -size 29051 +oid sha256:b848d50320f1494c9d0edbc21bddcc773de0cf9b1c36ce034235fd3fbbd77cf1 +size 29210 diff --git a/tests/uitests/src/test/snapshots/images/features.verifysession.impl.outgoing_VerifySelfSessionView_Night_3_en.png b/tests/uitests/src/test/snapshots/images/features.verifysession.impl.outgoing_VerifySelfSessionView_Night_3_en.png index 79198e05f9..4a519862f3 100644 --- a/tests/uitests/src/test/snapshots/images/features.verifysession.impl.outgoing_VerifySelfSessionView_Night_3_en.png +++ b/tests/uitests/src/test/snapshots/images/features.verifysession.impl.outgoing_VerifySelfSessionView_Night_3_en.png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:a6c90e5be60738ded8312822f95279aa76d3ec8d86265164f4e3e31dbcce61c5 -size 47355 +oid sha256:c1098ef994ad9552aca2f9ad9efe7b1239133544e561e73eced6fa96d08eaa3b +size 43785 diff --git a/tests/uitests/src/test/snapshots/images/features.verifysession.impl.outgoing_VerifySelfSessionView_Night_4_en.png b/tests/uitests/src/test/snapshots/images/features.verifysession.impl.outgoing_VerifySelfSessionView_Night_4_en.png index d9b8a3a0de..7ff37748ea 100644 --- a/tests/uitests/src/test/snapshots/images/features.verifysession.impl.outgoing_VerifySelfSessionView_Night_4_en.png +++ b/tests/uitests/src/test/snapshots/images/features.verifysession.impl.outgoing_VerifySelfSessionView_Night_4_en.png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:3a3f08002e805fe5f7a4c96aa4b73c2fcd6e8b79e6a9e82bcc7bd3df50d8c22d -size 24015 +oid sha256:5931dfac0337cf856282d9c62983aa185fe208f2926868097238d5b95e222fe6 +size 23953 From 0dbfab99d1f97de32f02a75ca03f24e64e66b554 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Thu, 7 Nov 2024 20:40:19 +0100 Subject: [PATCH 06/15] Fix Konsist test failing --- .../io/element/android/tests/konsist/KonsistComposableTest.kt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/konsist/src/test/kotlin/io/element/android/tests/konsist/KonsistComposableTest.kt b/tests/konsist/src/test/kotlin/io/element/android/tests/konsist/KonsistComposableTest.kt index 7d5ea75f2e..098ec0c9f7 100644 --- a/tests/konsist/src/test/kotlin/io/element/android/tests/konsist/KonsistComposableTest.kt +++ b/tests/konsist/src/test/kotlin/io/element/android/tests/konsist/KonsistComposableTest.kt @@ -32,9 +32,10 @@ class KonsistComposableTest { .withoutReceiverType() .withoutName( // Add some exceptions... + "InvisibleButton", "OutlinedButton", - "TextButton", "SimpleAlertDialogContent", + "TextButton", ) .assertTrue( additionalMessage = From 2873a6829bcb75b7b25611e3b98609fc0917361c Mon Sep 17 00:00:00 2001 From: Jorge Martin Espinosa Date: Fri, 8 Nov 2024 08:40:38 +0100 Subject: [PATCH 07/15] Use in-memory thumbnail APIs when possible (#3817) * Use in-memory thumbnail APIs when possible * Make an exception for animated image types. Also add `TimelineItemImageContent.thumbnailMediaRequestData` lazy property. * Try simplifying the logic a bit more. --- .../components/event/TimelineItemImageView.kt | 9 +----- .../components/event/TimelineItemVideoView.kt | 8 +++-- .../TimelineItemContentMessageFactory.kt | 4 +++ .../model/event/TimelineItemImageContent.kt | 29 +++++++++++++++---- .../event/TimelineItemImageContentProvider.kt | 2 ++ .../model/event/TimelineItemVideoContent.kt | 2 ++ .../event/TimelineItemVideoContentProvider.kt | 4 ++- .../messages/impl/MessagesPresenterTest.kt | 4 +++ .../TimelineItemContentMessageFactoryTest.kt | 8 +++++ .../libraries/core/mimetype/MimeTypes.kt | 1 + .../matrix/impl/media/RustMediaLoader.kt | 4 +-- .../matrix/ui/media/MediaRequestData.kt | 6 ++++ 12 files changed, 62 insertions(+), 19 deletions(-) diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/components/event/TimelineItemImageView.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/components/event/TimelineItemImageView.kt index abedcf4c50..63bbac7f6f 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/components/event/TimelineItemImageView.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/components/event/TimelineItemImageView.kt @@ -51,7 +51,6 @@ import io.element.android.libraries.designsystem.components.blurhash.blurHashBac import io.element.android.libraries.designsystem.preview.ElementPreview import io.element.android.libraries.designsystem.preview.PreviewsDayNight import io.element.android.libraries.matrix.api.timeline.item.event.MessageFormat -import io.element.android.libraries.matrix.ui.media.MediaRequestData import io.element.android.libraries.textcomposer.ElementRichTextEditorStyle import io.element.android.libraries.ui.strings.CommonStrings import io.element.android.wysiwyg.compose.EditorStyledText @@ -86,13 +85,7 @@ fun TimelineItemImageView( modifier = Modifier .fillMaxWidth() .then(if (isLoaded) Modifier.background(Color.White) else Modifier), - model = MediaRequestData( - source = content.preferredMediaSource, - kind = MediaRequestData.Kind.File( - fileName = content.filename, - mimeType = content.mimeType, - ), - ), + model = content.thumbnailMediaRequestData, contentScale = ContentScale.Fit, alignment = Alignment.Center, contentDescription = description, diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/components/event/TimelineItemVideoView.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/components/event/TimelineItemVideoView.kt index 849084ea1c..64e6d00d71 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/components/event/TimelineItemVideoView.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/components/event/TimelineItemVideoView.kt @@ -57,6 +57,8 @@ import io.element.android.libraries.designsystem.modifiers.roundedBackground import io.element.android.libraries.designsystem.preview.ElementPreview import io.element.android.libraries.designsystem.preview.PreviewsDayNight import io.element.android.libraries.matrix.api.timeline.item.event.MessageFormat +import io.element.android.libraries.matrix.ui.media.MAX_THUMBNAIL_HEIGHT +import io.element.android.libraries.matrix.ui.media.MAX_THUMBNAIL_WIDTH import io.element.android.libraries.matrix.ui.media.MediaRequestData import io.element.android.libraries.textcomposer.ElementRichTextEditorStyle import io.element.android.libraries.ui.strings.CommonStrings @@ -97,9 +99,9 @@ fun TimelineItemVideoView( .then(if (isLoaded) Modifier.background(Color.White) else Modifier), model = MediaRequestData( source = content.thumbnailSource, - kind = MediaRequestData.Kind.File( - fileName = content.filename, - mimeType = content.mimeType + kind = MediaRequestData.Kind.Thumbnail( + width = content.thumbnailWidth?.toLong() ?: MAX_THUMBNAIL_WIDTH, + height = content.thumbnailHeight?.toLong() ?: MAX_THUMBNAIL_HEIGHT, ) ), contentScale = ContentScale.Fit, diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/factories/event/TimelineItemContentMessageFactory.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/factories/event/TimelineItemContentMessageFactory.kt index 3eb0c66594..001d40b254 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/factories/event/TimelineItemContentMessageFactory.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/factories/event/TimelineItemContentMessageFactory.kt @@ -93,6 +93,8 @@ class TimelineItemContentMessageFactory @Inject constructor( blurhash = messageType.info?.blurhash, width = messageType.info?.width?.toInt(), height = messageType.info?.height?.toInt(), + thumbnailWidth = messageType.info?.thumbnailInfo?.width?.toInt(), + thumbnailHeight = messageType.info?.thumbnailInfo?.height?.toInt(), aspectRatio = aspectRatio, formattedFileSize = fileSizeFormatter.format(messageType.info?.size ?: 0), fileExtension = fileExtensionExtractor.extractFromName(messageType.filename) @@ -146,6 +148,8 @@ class TimelineItemContentMessageFactory @Inject constructor( mimeType = messageType.info?.mimetype ?: MimeTypes.OctetStream, width = messageType.info?.width?.toInt(), height = messageType.info?.height?.toInt(), + thumbnailWidth = messageType.info?.thumbnailInfo?.width?.toInt(), + thumbnailHeight = messageType.info?.thumbnailInfo?.height?.toInt(), duration = messageType.info?.duration ?: Duration.ZERO, blurHash = messageType.info?.blurhash, aspectRatio = aspectRatio, diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/model/event/TimelineItemImageContent.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/model/event/TimelineItemImageContent.kt index efc2d4a100..e6e4bffb9b 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/model/event/TimelineItemImageContent.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/model/event/TimelineItemImageContent.kt @@ -7,9 +7,12 @@ package io.element.android.features.messages.impl.timeline.model.event -import io.element.android.libraries.core.mimetype.MimeTypes +import io.element.android.libraries.core.mimetype.MimeTypes.isMimeTypeAnimatedImage import io.element.android.libraries.matrix.api.media.MediaSource import io.element.android.libraries.matrix.api.timeline.item.event.FormattedBody +import io.element.android.libraries.matrix.ui.media.MAX_THUMBNAIL_HEIGHT +import io.element.android.libraries.matrix.ui.media.MAX_THUMBNAIL_WIDTH +import io.element.android.libraries.matrix.ui.media.MediaRequestData data class TimelineItemImageContent( override val filename: String, @@ -23,15 +26,31 @@ data class TimelineItemImageContent( val blurhash: String?, val width: Int?, val height: Int?, + val thumbnailWidth: Int?, + val thumbnailHeight: Int?, val aspectRatio: Float? ) : TimelineItemEventContentWithAttachment { override val type: String = "TimelineItemImageContent" val showCaption = caption != null - val preferredMediaSource = if (mimeType == MimeTypes.Gif) { - mediaSource - } else { - thumbnailSource ?: mediaSource + val thumbnailMediaRequestData: MediaRequestData by lazy { + if (mimeType.isMimeTypeAnimatedImage()) { + MediaRequestData( + source = mediaSource, + kind = MediaRequestData.Kind.File( + fileName = filename, + mimeType = mimeType + ) + ) + } else { + MediaRequestData( + source = thumbnailSource ?: mediaSource, + kind = MediaRequestData.Kind.Thumbnail( + width = thumbnailWidth?.toLong() ?: MAX_THUMBNAIL_WIDTH, + height = thumbnailHeight?.toLong() ?: MAX_THUMBNAIL_HEIGHT + ), + ) + } } } diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/model/event/TimelineItemImageContentProvider.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/model/event/TimelineItemImageContentProvider.kt index 8c645ab901..60edb0e6d7 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/model/event/TimelineItemImageContentProvider.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/model/event/TimelineItemImageContentProvider.kt @@ -37,6 +37,8 @@ fun aTimelineItemImageContent( blurhash = blurhash, width = null, height = 300, + thumbnailWidth = null, + thumbnailHeight = 150, aspectRatio = aspectRatio, formattedFileSize = "4MB", fileExtension = "jpg" diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/model/event/TimelineItemVideoContent.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/model/event/TimelineItemVideoContent.kt index 3b2e6c1f21..5c0e601708 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/model/event/TimelineItemVideoContent.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/model/event/TimelineItemVideoContent.kt @@ -22,6 +22,8 @@ data class TimelineItemVideoContent( val blurHash: String?, val height: Int?, val width: Int?, + val thumbnailWidth: Int?, + val thumbnailHeight: Int?, val mimeType: String, val formattedFileSize: String, val fileExtension: String, diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/model/event/TimelineItemVideoContentProvider.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/model/event/TimelineItemVideoContentProvider.kt index 39d104b2af..b9390b4e52 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/model/event/TimelineItemVideoContentProvider.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/model/event/TimelineItemVideoContentProvider.kt @@ -35,8 +35,10 @@ fun aTimelineItemVideoContent( aspectRatio = aspectRatio, duration = 100.milliseconds, videoSource = MediaSource(""), - height = 300, width = 150, + height = 300, + thumbnailWidth = 150, + thumbnailHeight = 300, mimeType = MimeTypes.Mp4, formattedFileSize = "14MB", fileExtension = "mp4" diff --git a/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/MessagesPresenterTest.kt b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/MessagesPresenterTest.kt index 8d143ff179..9ca5c0b98e 100644 --- a/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/MessagesPresenterTest.kt +++ b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/MessagesPresenterTest.kt @@ -324,6 +324,8 @@ class MessagesPresenterTest { blurhash = null, width = 20, height = 20, + thumbnailWidth = null, + thumbnailHeight = null, aspectRatio = 1.0f, fileExtension = "jpg", formattedFileSize = "4MB" @@ -364,6 +366,8 @@ class MessagesPresenterTest { blurHash = null, width = 20, height = 20, + thumbnailWidth = 20, + thumbnailHeight = 20, aspectRatio = 1.0f, fileExtension = "mp4", formattedFileSize = "50MB" diff --git a/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/timeline/factories/event/TimelineItemContentMessageFactoryTest.kt b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/timeline/factories/event/TimelineItemContentMessageFactoryTest.kt index 337ca9ab7e..e9343bf21d 100644 --- a/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/timeline/factories/event/TimelineItemContentMessageFactoryTest.kt +++ b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/timeline/factories/event/TimelineItemContentMessageFactoryTest.kt @@ -246,6 +246,8 @@ class TimelineItemContentMessageFactoryTest { width = null, mimeType = MimeTypes.OctetStream, formattedFileSize = "0 Bytes", + thumbnailWidth = null, + thumbnailHeight = null, fileExtension = "", ) assertThat(result).isEqualTo(expected) @@ -294,6 +296,8 @@ class TimelineItemContentMessageFactoryTest { width = 300, mimeType = MimeTypes.Mp4, formattedFileSize = "555 Bytes", + thumbnailWidth = 5, + thumbnailHeight = 10, fileExtension = "mp4", ) assertThat(result).isEqualTo(expected) @@ -458,6 +462,8 @@ class TimelineItemContentMessageFactoryTest { blurhash = null, width = null, height = null, + thumbnailWidth = null, + thumbnailHeight = null, aspectRatio = null ) assertThat(result).isEqualTo(expected) @@ -531,6 +537,8 @@ class TimelineItemContentMessageFactoryTest { blurhash = A_BLUR_HASH, width = 5, height = 10, + thumbnailWidth = 5, + thumbnailHeight = 10, aspectRatio = 0.5f, ) assertThat(result).isEqualTo(expected) diff --git a/libraries/core/src/main/kotlin/io/element/android/libraries/core/mimetype/MimeTypes.kt b/libraries/core/src/main/kotlin/io/element/android/libraries/core/mimetype/MimeTypes.kt index 8dc47d06d8..cacdfddc00 100644 --- a/libraries/core/src/main/kotlin/io/element/android/libraries/core/mimetype/MimeTypes.kt +++ b/libraries/core/src/main/kotlin/io/element/android/libraries/core/mimetype/MimeTypes.kt @@ -38,6 +38,7 @@ object MimeTypes { fun String?.normalizeMimeType() = if (this == BadJpg) Jpeg else this fun String?.isMimeTypeImage() = this?.startsWith("image/").orFalse() + fun String?.isMimeTypeAnimatedImage() = this == Gif || this == WebP fun String?.isMimeTypeVideo() = this?.startsWith("video/").orFalse() fun String?.isMimeTypeAudio() = this?.startsWith("audio/").orFalse() fun String?.isMimeTypeApplication() = this?.startsWith("application/").orFalse() 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 9604d6af6d..17ba1d2c4d 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 @@ -37,7 +37,7 @@ class RustMediaLoader( withContext(mediaDispatcher) { runCatching { source.toRustMediaSource().use { source -> - innerClient.getMediaContent(source).toUByteArray().toByteArray() + innerClient.getMediaContent(source) } } } @@ -55,7 +55,7 @@ class RustMediaLoader( mediaSource = mediaSource, width = width.toULong(), height = height.toULong() - ).toUByteArray().toByteArray() + ) } } } diff --git a/libraries/matrixui/src/main/kotlin/io/element/android/libraries/matrix/ui/media/MediaRequestData.kt b/libraries/matrixui/src/main/kotlin/io/element/android/libraries/matrix/ui/media/MediaRequestData.kt index 38499d15fb..79f2d6a0b6 100644 --- a/libraries/matrixui/src/main/kotlin/io/element/android/libraries/matrix/ui/media/MediaRequestData.kt +++ b/libraries/matrixui/src/main/kotlin/io/element/android/libraries/matrix/ui/media/MediaRequestData.kt @@ -37,3 +37,9 @@ data class MediaRequestData( } } } + +/** Max width a thumbnail can have according to [the spec](https://spec.matrix.org/v1.10/client-server-api/#thumbnails). */ +const val MAX_THUMBNAIL_WIDTH = 800L + +/** Max height a thumbnail can have according to [the spec](https://spec.matrix.org/v1.10/client-server-api/#thumbnails). */ +const val MAX_THUMBNAIL_HEIGHT = 600L From d09df1bcc3e12dde3fa4249bc65520f0bb85a189 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Fri, 8 Nov 2024 09:46:56 +0100 Subject: [PATCH 08/15] Use LoggerTag. --- .../features/call/impl/ui/ElementCallActivity.kt | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/features/call/impl/src/main/kotlin/io/element/android/features/call/impl/ui/ElementCallActivity.kt b/features/call/impl/src/main/kotlin/io/element/android/features/call/impl/ui/ElementCallActivity.kt index 95d8dad64c..0495f8ec7d 100644 --- a/features/call/impl/src/main/kotlin/io/element/android/features/call/impl/ui/ElementCallActivity.kt +++ b/features/call/impl/src/main/kotlin/io/element/android/features/call/impl/ui/ElementCallActivity.kt @@ -45,12 +45,13 @@ import io.element.android.features.call.impl.pip.PipView import io.element.android.features.call.impl.services.CallForegroundService import io.element.android.features.call.impl.utils.CallIntentDataParser import io.element.android.libraries.architecture.bindings +import io.element.android.libraries.core.log.logger.LoggerTag import io.element.android.libraries.designsystem.theme.ElementThemeApp import io.element.android.libraries.preferences.api.store.AppPreferencesStore import timber.log.Timber import javax.inject.Inject -private const val loggerTag = "ElementCallActivity" +private val loggerTag = LoggerTag("ElementCallActivity") class ElementCallActivity : AppCompatActivity(), @@ -135,7 +136,7 @@ class ElementCallActivity : DisposableEffect(Unit) { val listener = Runnable { if (requestPermissionCallback != null) { - Timber.tag(loggerTag).w("Ignoring onUserLeaveHint event because user is asked to grant permissions") + Timber.tag(loggerTag.value).w("Ignoring onUserLeaveHint event because user is asked to grant permissions") } else { pipEventSink(PictureInPictureEvents.EnterPictureInPicture) } @@ -149,7 +150,7 @@ class ElementCallActivity : val onPictureInPictureModeChangedListener = Consumer { _: PictureInPictureModeChangedInfo -> pipEventSink(PictureInPictureEvents.OnPictureInPictureModeChanged(isInPictureInPictureMode)) if (!isInPictureInPictureMode && !lifecycle.currentState.isAtLeast(Lifecycle.State.STARTED)) { - Timber.tag(loggerTag).d("Exiting PiP mode: Hangup the call") + Timber.tag(loggerTag.value).d("Exiting PiP mode: Hangup the call") eventSink?.invoke(CallScreenEvents.Hangup) } } @@ -193,18 +194,18 @@ class ElementCallActivity : } val currentCallType = webViewTarget.value if (currentCallType == null && callType == null) { - Timber.tag(loggerTag).d("Re-opened the activity but we have no url to load or a cached one, finish the activity") + Timber.tag(loggerTag.value).d("Re-opened the activity but we have no url to load or a cached one, finish the activity") finish() } else if (currentCallType == null) { - Timber.tag(loggerTag).d("Set the call type and create the presenter") + Timber.tag(loggerTag.value).d("Set the call type and create the presenter") webViewTarget.value = callType presenter = presenterFactory.create(callType!!, this) } else if (callType != currentCallType) { - Timber.tag(loggerTag).d("User starts another call, restart the Activity") + Timber.tag(loggerTag.value).d("User starts another call, restart the Activity") setIntent(intent) recreate() } else { - Timber.tag(loggerTag).d("Coming back from notification, do nothing") + Timber.tag(loggerTag.value).d("Coming back from notification, do nothing") } } From 13ec1838c0b0ea0c8dce81db72967feef3e8ea66 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 6 Nov 2024 11:46:05 +0100 Subject: [PATCH 09/15] MediaPreProcessor: remove default value of parameter `deleteOriginal`. No functional change here. --- .../impl/configureroom/ConfigureRoomPresenter.kt | 7 ++++++- .../impl/user/editprofile/EditUserProfilePresenter.kt | 7 ++++++- .../roomdetails/impl/edit/RoomDetailsEditPresenter.kt | 7 ++++++- .../android/libraries/mediaupload/api/MediaPreProcessor.kt | 4 ++-- 4 files changed, 20 insertions(+), 5 deletions(-) diff --git a/features/createroom/impl/src/main/kotlin/io/element/android/features/createroom/impl/configureroom/ConfigureRoomPresenter.kt b/features/createroom/impl/src/main/kotlin/io/element/android/features/createroom/impl/configureroom/ConfigureRoomPresenter.kt index ad19d2cb9b..bbefc987ef 100644 --- a/features/createroom/impl/src/main/kotlin/io/element/android/features/createroom/impl/configureroom/ConfigureRoomPresenter.kt +++ b/features/createroom/impl/src/main/kotlin/io/element/android/features/createroom/impl/configureroom/ConfigureRoomPresenter.kt @@ -175,7 +175,12 @@ class ConfigureRoomPresenter @Inject constructor( } private suspend fun uploadAvatar(avatarUri: Uri): String { - val preprocessed = mediaPreProcessor.process(avatarUri, MimeTypes.Jpeg, compressIfPossible = false).getOrThrow() + val preprocessed = mediaPreProcessor.process( + uri = avatarUri, + mimeType = MimeTypes.Jpeg, + deleteOriginal = false, + compressIfPossible = false, + ).getOrThrow() val byteArray = preprocessed.file.readBytes() return matrixClient.uploadMedia(MimeTypes.Jpeg, byteArray, null).getOrThrow() } diff --git a/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/user/editprofile/EditUserProfilePresenter.kt b/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/user/editprofile/EditUserProfilePresenter.kt index 14d0bd7dcd..5f9c20dfdb 100644 --- a/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/user/editprofile/EditUserProfilePresenter.kt +++ b/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/user/editprofile/EditUserProfilePresenter.kt @@ -155,7 +155,12 @@ class EditUserProfilePresenter @AssistedInject constructor( private suspend fun updateAvatar(avatarUri: Uri?): Result { return runCatching { if (avatarUri != null) { - val preprocessed = mediaPreProcessor.process(avatarUri, MimeTypes.Jpeg, compressIfPossible = false).getOrThrow() + val preprocessed = mediaPreProcessor.process( + uri = avatarUri, + mimeType = MimeTypes.Jpeg, + deleteOriginal = false, + compressIfPossible = false, + ).getOrThrow() matrixClient.uploadAvatar(MimeTypes.Jpeg, preprocessed.file.readBytes()).getOrThrow() } else { matrixClient.removeAvatar().getOrThrow() diff --git a/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/edit/RoomDetailsEditPresenter.kt b/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/edit/RoomDetailsEditPresenter.kt index 8ad7eed8f4..fec062c7c8 100644 --- a/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/edit/RoomDetailsEditPresenter.kt +++ b/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/edit/RoomDetailsEditPresenter.kt @@ -202,7 +202,12 @@ class RoomDetailsEditPresenter @Inject constructor( private suspend fun updateAvatar(avatarUri: Uri?): Result { return runCatching { if (avatarUri != null) { - val preprocessed = mediaPreProcessor.process(avatarUri, MimeTypes.Jpeg, compressIfPossible = false).getOrThrow() + val preprocessed = mediaPreProcessor.process( + uri = avatarUri, + mimeType = MimeTypes.Jpeg, + deleteOriginal = false, + compressIfPossible = false, + ).getOrThrow() room.updateAvatar(MimeTypes.Jpeg, preprocessed.file.readBytes()).getOrThrow() } else { room.removeAvatar().getOrThrow() diff --git a/libraries/mediaupload/api/src/main/kotlin/io/element/android/libraries/mediaupload/api/MediaPreProcessor.kt b/libraries/mediaupload/api/src/main/kotlin/io/element/android/libraries/mediaupload/api/MediaPreProcessor.kt index 19b0a0700d..75a4be1cf3 100644 --- a/libraries/mediaupload/api/src/main/kotlin/io/element/android/libraries/mediaupload/api/MediaPreProcessor.kt +++ b/libraries/mediaupload/api/src/main/kotlin/io/element/android/libraries/mediaupload/api/MediaPreProcessor.kt @@ -18,8 +18,8 @@ interface MediaPreProcessor { suspend fun process( uri: Uri, mimeType: String, - deleteOriginal: Boolean = false, - compressIfPossible: Boolean + deleteOriginal: Boolean, + compressIfPossible: Boolean, ): Result data class Failure(override val cause: Throwable?) : Exception(cause) From 58a0875c5d4d8fffdd8dcb38988a4e96c1633c12 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 6 Nov 2024 11:57:51 +0100 Subject: [PATCH 10/15] Do not delete the original file when sending a media. Fixes #3800. --- .../io/element/android/libraries/mediaupload/api/MediaSender.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/mediaupload/api/src/main/kotlin/io/element/android/libraries/mediaupload/api/MediaSender.kt b/libraries/mediaupload/api/src/main/kotlin/io/element/android/libraries/mediaupload/api/MediaSender.kt index 54f886d302..09b8de4b41 100644 --- a/libraries/mediaupload/api/src/main/kotlin/io/element/android/libraries/mediaupload/api/MediaSender.kt +++ b/libraries/mediaupload/api/src/main/kotlin/io/element/android/libraries/mediaupload/api/MediaSender.kt @@ -39,7 +39,7 @@ class MediaSender @Inject constructor( .process( uri = uri, mimeType = mimeType, - deleteOriginal = true, + deleteOriginal = false, compressIfPossible = compressIfPossible, ) .flatMapCatching { info -> From 0c841442d9cafb78144618f9de8d58e7dae75100 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 6 Nov 2024 11:59:33 +0100 Subject: [PATCH 11/15] Add a log when deleting a file. --- .../libraries/mediaupload/impl/AndroidMediaPreProcessor.kt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/libraries/mediaupload/impl/src/main/kotlin/io/element/android/libraries/mediaupload/impl/AndroidMediaPreProcessor.kt b/libraries/mediaupload/impl/src/main/kotlin/io/element/android/libraries/mediaupload/impl/AndroidMediaPreProcessor.kt index 288ba988ef..b219da5350 100644 --- a/libraries/mediaupload/impl/src/main/kotlin/io/element/android/libraries/mediaupload/impl/AndroidMediaPreProcessor.kt +++ b/libraries/mediaupload/impl/src/main/kotlin/io/element/android/libraries/mediaupload/impl/AndroidMediaPreProcessor.kt @@ -36,6 +36,7 @@ import kotlinx.coroutines.flow.filterIsInstance import kotlinx.coroutines.flow.first import kotlinx.coroutines.flow.onEach import kotlinx.coroutines.withContext +import timber.log.Timber import java.io.File import java.io.InputStream import javax.inject.Inject @@ -82,6 +83,7 @@ class AndroidMediaPreProcessor @Inject constructor( } if (deleteOriginal) { tryOrNull { + Timber.w("Deleting original uri $uri") contentResolver.delete(uri, null, null) } } From 585b6a94f3857eb3efb05fa022a673e2475c9854 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 6 Nov 2024 14:30:42 +0100 Subject: [PATCH 12/15] Delete temporary created files. --- .../preview/AttachmentsPreviewPresenter.kt | 17 ++ .../AttachmentsPreviewPresenterTest.kt | 4 + .../editprofile/EditUserProfilePresenter.kt | 21 +- .../EditUserProfilePresenterTest.kt | 75 ++++++- .../impl/edit/RoomDetailsEditPresenter.kt | 22 +- .../edit/RoomDetailsEditPresenterTest.kt | 199 +++++++++++------- .../androidutils/file/TemporaryUriDeleter.kt | 39 ++++ .../impl/AndroidMediaPreProcessor.kt | 4 + .../impl/AndroidMediaPreProcessorTest.kt | 16 +- .../testutils/fake/FakeTemporaryUriDeleter.kt | 20 ++ 10 files changed, 328 insertions(+), 89 deletions(-) create mode 100644 libraries/androidutils/src/main/kotlin/io/element/android/libraries/androidutils/file/TemporaryUriDeleter.kt create mode 100644 tests/testutils/src/main/kotlin/io/element/android/tests/testutils/fake/FakeTemporaryUriDeleter.kt diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewPresenter.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewPresenter.kt index 57e2ee3ad8..a3ba509a15 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewPresenter.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewPresenter.kt @@ -8,6 +8,7 @@ package io.element.android.features.messages.impl.attachments.preview import androidx.compose.runtime.Composable +import androidx.compose.runtime.DisposableEffect import androidx.compose.runtime.MutableState import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableStateOf @@ -18,6 +19,7 @@ import dagger.assisted.Assisted import dagger.assisted.AssistedFactory import dagger.assisted.AssistedInject import io.element.android.features.messages.impl.attachments.Attachment +import io.element.android.libraries.androidutils.file.TemporaryUriDeleter import io.element.android.libraries.architecture.Presenter import io.element.android.libraries.matrix.api.core.ProgressCallback import io.element.android.libraries.matrix.api.permalink.PermalinkBuilder @@ -36,6 +38,7 @@ class AttachmentsPreviewPresenter @AssistedInject constructor( @Assisted private val attachment: Attachment, private val mediaSender: MediaSender, private val permalinkBuilder: PermalinkBuilder, + private val temporaryUriDeleter: TemporaryUriDeleter, ) : Presenter { @AssistedFactory interface Factory { @@ -57,6 +60,20 @@ class AttachmentsPreviewPresenter @AssistedInject constructor( val ongoingSendAttachmentJob = remember { mutableStateOf(null) } + DisposableEffect(Unit) { + onDispose { + // Delete the temporary file when the composable is disposed, in case it was not sent + if (sendActionState.value == SendActionState.Idle) { + // Attachment has not been sent, maybe delete it + when (attachment) { + is Attachment.Media -> { + temporaryUriDeleter.delete(attachment.localMedia.uri) + } + } + } + } + } + fun handleEvents(attachmentsPreviewEvents: AttachmentsPreviewEvents) { when (attachmentsPreviewEvents) { is AttachmentsPreviewEvents.SendAttachment -> { diff --git a/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/attachments/AttachmentsPreviewPresenterTest.kt b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/attachments/AttachmentsPreviewPresenterTest.kt index 51318454eb..85acefb5fe 100644 --- a/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/attachments/AttachmentsPreviewPresenterTest.kt +++ b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/attachments/AttachmentsPreviewPresenterTest.kt @@ -18,6 +18,7 @@ import io.element.android.features.messages.impl.attachments.preview.Attachments import io.element.android.features.messages.impl.attachments.preview.AttachmentsPreviewPresenter import io.element.android.features.messages.impl.attachments.preview.SendActionState import io.element.android.features.messages.impl.fixtures.aMediaAttachment +import io.element.android.libraries.androidutils.file.TemporaryUriDeleter import io.element.android.libraries.matrix.api.core.ProgressCallback import io.element.android.libraries.matrix.api.media.FileInfo import io.element.android.libraries.matrix.api.media.ImageInfo @@ -35,6 +36,7 @@ import io.element.android.libraries.mediaviewer.api.local.LocalMedia import io.element.android.libraries.mediaviewer.test.viewer.aLocalMedia import io.element.android.libraries.preferences.test.InMemorySessionPreferencesStore import io.element.android.tests.testutils.WarmUpRule +import io.element.android.tests.testutils.fake.FakeTemporaryUriDeleter import io.element.android.tests.testutils.lambda.any import io.element.android.tests.testutils.lambda.lambdaRecorder import io.element.android.tests.testutils.lambda.value @@ -207,11 +209,13 @@ class AttachmentsPreviewPresenterTest { room: MatrixRoom = FakeMatrixRoom(), permalinkBuilder: PermalinkBuilder = FakePermalinkBuilder(), mediaPreProcessor: MediaPreProcessor = FakeMediaPreProcessor(), + temporaryUriDeleter: TemporaryUriDeleter = FakeTemporaryUriDeleter(), ): AttachmentsPreviewPresenter { return AttachmentsPreviewPresenter( attachment = aMediaAttachment(localMedia), mediaSender = MediaSender(mediaPreProcessor, room, InMemorySessionPreferencesStore()), permalinkBuilder = permalinkBuilder, + temporaryUriDeleter = temporaryUriDeleter, ) } } diff --git a/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/user/editprofile/EditUserProfilePresenter.kt b/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/user/editprofile/EditUserProfilePresenter.kt index 5f9c20dfdb..0e5a05be56 100644 --- a/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/user/editprofile/EditUserProfilePresenter.kt +++ b/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/user/editprofile/EditUserProfilePresenter.kt @@ -22,6 +22,7 @@ import androidx.core.net.toUri import dagger.assisted.Assisted import dagger.assisted.AssistedFactory import dagger.assisted.AssistedInject +import io.element.android.libraries.androidutils.file.TemporaryUriDeleter import io.element.android.libraries.architecture.AsyncAction import io.element.android.libraries.architecture.Presenter import io.element.android.libraries.architecture.runCatchingUpdatingState @@ -43,6 +44,7 @@ class EditUserProfilePresenter @AssistedInject constructor( private val matrixClient: MatrixClient, private val mediaPickerProvider: PickerProvider, private val mediaPreProcessor: MediaPreProcessor, + private val temporaryUriDeleter: TemporaryUriDeleter, permissionsPresenterFactory: PermissionsPresenter.Factory, ) : Presenter { private val cameraPermissionPresenter: PermissionsPresenter = permissionsPresenterFactory.create(android.Manifest.permission.CAMERA) @@ -59,10 +61,20 @@ class EditUserProfilePresenter @AssistedInject constructor( var userAvatarUri by rememberSaveable { mutableStateOf(matrixUser.avatarUrl?.let { Uri.parse(it) }) } var userDisplayName by rememberSaveable { mutableStateOf(matrixUser.displayName) } val cameraPhotoPicker = mediaPickerProvider.registerCameraPhotoPicker( - onResult = { uri -> if (uri != null) userAvatarUri = uri } + onResult = { uri -> + if (uri != null) { + temporaryUriDeleter.delete(userAvatarUri) + userAvatarUri = uri + } + } ) val galleryImagePicker = mediaPickerProvider.registerGalleryImagePicker( - onResult = { uri -> if (uri != null) userAvatarUri = uri } + onResult = { uri -> + if (uri != null) { + temporaryUriDeleter.delete(userAvatarUri) + userAvatarUri = uri + } + } ) val avatarActions by remember(userAvatarUri) { @@ -96,7 +108,10 @@ class EditUserProfilePresenter @AssistedInject constructor( pendingPermissionRequest = true cameraPermissionState.eventSink(PermissionsEvents.RequestPermissions) } - AvatarAction.Remove -> userAvatarUri = null + AvatarAction.Remove -> { + temporaryUriDeleter.delete(userAvatarUri) + userAvatarUri = null + } } } diff --git a/features/preferences/impl/src/test/kotlin/io/element/android/features/preferences/impl/user/editprofile/EditUserProfilePresenterTest.kt b/features/preferences/impl/src/test/kotlin/io/element/android/features/preferences/impl/user/editprofile/EditUserProfilePresenterTest.kt index 5fae451e0f..77b86ce4c7 100644 --- a/features/preferences/impl/src/test/kotlin/io/element/android/features/preferences/impl/user/editprofile/EditUserProfilePresenterTest.kt +++ b/features/preferences/impl/src/test/kotlin/io/element/android/features/preferences/impl/user/editprofile/EditUserProfilePresenterTest.kt @@ -12,6 +12,7 @@ import app.cash.molecule.RecompositionMode import app.cash.molecule.moleculeFlow import app.cash.turbine.test import com.google.common.truth.Truth.assertThat +import io.element.android.libraries.androidutils.file.TemporaryUriDeleter import io.element.android.libraries.architecture.AsyncAction import io.element.android.libraries.matrix.api.MatrixClient import io.element.android.libraries.matrix.api.user.MatrixUser @@ -29,6 +30,9 @@ import io.element.android.libraries.permissions.test.FakePermissionsPresenterFac import io.element.android.tests.testutils.WarmUpRule import io.element.android.tests.testutils.consumeItemsUntilPredicate import io.element.android.tests.testutils.consumeItemsUntilTimeout +import io.element.android.tests.testutils.fake.FakeTemporaryUriDeleter +import io.element.android.tests.testutils.lambda.lambdaRecorder +import io.element.android.tests.testutils.lambda.value import io.mockk.every import io.mockk.mockk import io.mockk.mockkStatic @@ -73,12 +77,14 @@ class EditUserProfilePresenterTest { matrixClient: MatrixClient = FakeMatrixClient(), matrixUser: MatrixUser = aMatrixUser(), permissionsPresenter: PermissionsPresenter = FakePermissionsPresenter(), + temporaryUriDeleter: TemporaryUriDeleter = FakeTemporaryUriDeleter(), ): EditUserProfilePresenter { return EditUserProfilePresenter( matrixClient = matrixClient, matrixUser = matrixUser, mediaPickerProvider = fakePickerProvider, mediaPreProcessor = fakeMediaPreProcessor, + temporaryUriDeleter = temporaryUriDeleter, permissionsPresenterFactory = FakePermissionsPresenterFactory(permissionsPresenter), ) } @@ -107,7 +113,12 @@ class EditUserProfilePresenterTest { @Test fun `present - updates state in response to changes`() = runTest { val user = aMatrixUser(id = A_USER_ID.value, displayName = "Name", avatarUrl = AN_AVATAR_URL) - val presenter = createEditUserProfilePresenter(matrixUser = user) + val presenter = createEditUserProfilePresenter( + matrixUser = user, + temporaryUriDeleter = FakeTemporaryUriDeleter( + deleteCallback = { assertThat(it).isEqualTo(userAvatarUri) } + ), + ) moleculeFlow(RecompositionMode.Immediate) { presenter.present() }.test { @@ -136,7 +147,12 @@ class EditUserProfilePresenterTest { fun `present - obtains avatar uris from gallery`() = runTest { val user = aMatrixUser(id = A_USER_ID.value, displayName = "Name", avatarUrl = AN_AVATAR_URL) fakePickerProvider.givenResult(anotherAvatarUri) - val presenter = createEditUserProfilePresenter(matrixUser = user) + val presenter = createEditUserProfilePresenter( + matrixUser = user, + temporaryUriDeleter = FakeTemporaryUriDeleter( + deleteCallback = { assertThat(it).isEqualTo(userAvatarUri) } + ), + ) moleculeFlow(RecompositionMode.Immediate) { presenter.present() }.test { @@ -154,9 +170,13 @@ class EditUserProfilePresenterTest { val user = aMatrixUser(id = A_USER_ID.value, displayName = "Name", avatarUrl = AN_AVATAR_URL) fakePickerProvider.givenResult(anotherAvatarUri) val fakePermissionsPresenter = FakePermissionsPresenter() + val deleteCallback = lambdaRecorder {} val presenter = createEditUserProfilePresenter( matrixUser = user, permissionsPresenter = fakePermissionsPresenter, + temporaryUriDeleter = FakeTemporaryUriDeleter( + deleteCallback = deleteCallback, + ), ) moleculeFlow(RecompositionMode.Immediate) { presenter.present() @@ -177,6 +197,10 @@ class EditUserProfilePresenterTest { stateWithNewAvatar.eventSink(EditUserProfileEvents.HandleAvatarAction(AvatarAction.TakePhoto)) val stateWithNewAvatar2 = awaitItem() assertThat(stateWithNewAvatar2.userAvatarUrl).isEqualTo(userAvatarUri) + deleteCallback.assertions().isCalledExactly(2).withSequence( + listOf(value(userAvatarUri)), + listOf(value(anotherAvatarUri)), + ) } } @@ -184,7 +208,13 @@ class EditUserProfilePresenterTest { fun `present - updates save button state`() = runTest { val user = aMatrixUser(id = A_USER_ID.value, displayName = "Name", avatarUrl = AN_AVATAR_URL) fakePickerProvider.givenResult(userAvatarUri) - val presenter = createEditUserProfilePresenter(matrixUser = user) + val deleteCallback = lambdaRecorder {} + val presenter = createEditUserProfilePresenter( + matrixUser = user, + temporaryUriDeleter = FakeTemporaryUriDeleter( + deleteCallback = deleteCallback + ), + ) moleculeFlow(RecompositionMode.Immediate) { presenter.present() }.test { @@ -210,6 +240,10 @@ class EditUserProfilePresenterTest { awaitItem().apply { assertThat(saveButtonEnabled).isFalse() } + deleteCallback.assertions().isCalledExactly(2).withSequence( + listOf(value(userAvatarUri)), + listOf(value(null)), + ) } } @@ -217,7 +251,13 @@ class EditUserProfilePresenterTest { fun `present - updates save button state when initial values are null`() = runTest { val user = aMatrixUser(id = A_USER_ID.value, displayName = "Name", avatarUrl = null) fakePickerProvider.givenResult(userAvatarUri) - val presenter = createEditUserProfilePresenter(matrixUser = user) + val deleteCallback = lambdaRecorder {} + val presenter = createEditUserProfilePresenter( + matrixUser = user, + temporaryUriDeleter = FakeTemporaryUriDeleter( + deleteCallback = deleteCallback + ), + ) moleculeFlow(RecompositionMode.Immediate) { presenter.present() }.test { @@ -243,6 +283,10 @@ class EditUserProfilePresenterTest { awaitItem().apply { assertThat(saveButtonEnabled).isFalse() } + deleteCallback.assertions().isCalledExactly(2).withSequence( + listOf(value(null)), + listOf(value(userAvatarUri)), + ) } } @@ -252,7 +296,10 @@ class EditUserProfilePresenterTest { val user = aMatrixUser(id = A_USER_ID.value, displayName = "Name", avatarUrl = AN_AVATAR_URL) val presenter = createEditUserProfilePresenter( matrixClient = matrixClient, - matrixUser = user + matrixUser = user, + temporaryUriDeleter = FakeTemporaryUriDeleter( + deleteCallback = { assertThat(it).isEqualTo(userAvatarUri) } + ), ) moleculeFlow(RecompositionMode.Immediate) { presenter.present() @@ -318,7 +365,10 @@ class EditUserProfilePresenterTest { givenPickerReturnsFile() val presenter = createEditUserProfilePresenter( matrixClient = matrixClient, - matrixUser = user + matrixUser = user, + temporaryUriDeleter = FakeTemporaryUriDeleter( + deleteCallback = { assertThat(it).isEqualTo(userAvatarUri) } + ), ) moleculeFlow(RecompositionMode.Immediate) { presenter.present() @@ -337,7 +387,10 @@ class EditUserProfilePresenterTest { val user = aMatrixUser(id = A_USER_ID.value, displayName = "Name", avatarUrl = AN_AVATAR_URL) val presenter = createEditUserProfilePresenter( matrixClient = matrixClient, - matrixUser = user + matrixUser = user, + temporaryUriDeleter = FakeTemporaryUriDeleter( + deleteCallback = { assertThat(it).isEqualTo(userAvatarUri) } + ), ) fakePickerProvider.givenResult(anotherAvatarUri) fakeMediaPreProcessor.givenResult(Result.failure(Throwable("Oh no"))) @@ -403,7 +456,13 @@ class EditUserProfilePresenterTest { } private suspend fun saveAndAssertFailure(matrixUser: MatrixUser, matrixClient: MatrixClient, event: EditUserProfileEvents) { - val presenter = createEditUserProfilePresenter(matrixUser = matrixUser, matrixClient = matrixClient) + val presenter = createEditUserProfilePresenter( + matrixUser = matrixUser, + matrixClient = matrixClient, + temporaryUriDeleter = FakeTemporaryUriDeleter( + deleteCallback = { assertThat(it).isEqualTo(userAvatarUri) } + ), + ) moleculeFlow(RecompositionMode.Immediate) { presenter.present() }.test { diff --git a/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/edit/RoomDetailsEditPresenter.kt b/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/edit/RoomDetailsEditPresenter.kt index fec062c7c8..df220f0d50 100644 --- a/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/edit/RoomDetailsEditPresenter.kt +++ b/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/edit/RoomDetailsEditPresenter.kt @@ -20,6 +20,7 @@ import androidx.compose.runtime.rememberCoroutineScope import androidx.compose.runtime.saveable.rememberSaveable import androidx.compose.runtime.setValue import androidx.core.net.toUri +import io.element.android.libraries.androidutils.file.TemporaryUriDeleter import io.element.android.libraries.architecture.AsyncAction import io.element.android.libraries.architecture.Presenter import io.element.android.libraries.architecture.runCatchingUpdatingState @@ -45,6 +46,7 @@ class RoomDetailsEditPresenter @Inject constructor( private val room: MatrixRoom, private val mediaPickerProvider: PickerProvider, private val mediaPreProcessor: MediaPreProcessor, + private val temporaryUriDeleter: TemporaryUriDeleter, permissionsPresenterFactory: PermissionsPresenter.Factory, ) : Presenter { private val cameraPermissionPresenter = permissionsPresenterFactory.create(android.Manifest.permission.CAMERA) @@ -59,6 +61,7 @@ class RoomDetailsEditPresenter @Inject constructor( var roomAvatarUriEdited by rememberSaveable { mutableStateOf(null) } LaunchedEffect(roomAvatarUri) { // Every time the roomAvatar change (from sync), we can set the new avatar. + temporaryUriDeleter.delete(roomAvatarUriEdited) roomAvatarUriEdited = roomAvatarUri } @@ -98,10 +101,20 @@ class RoomDetailsEditPresenter @Inject constructor( } val cameraPhotoPicker = mediaPickerProvider.registerCameraPhotoPicker( - onResult = { uri -> if (uri != null) roomAvatarUriEdited = uri } + onResult = { uri -> + if (uri != null) { + temporaryUriDeleter.delete(roomAvatarUriEdited) + roomAvatarUriEdited = uri + } + } ) val galleryImagePicker = mediaPickerProvider.registerGalleryImagePicker( - onResult = { uri -> if (uri != null) roomAvatarUriEdited = uri } + onResult = { uri -> + if (uri != null) { + temporaryUriDeleter.delete(roomAvatarUriEdited) + roomAvatarUriEdited = uri + } + } ) LaunchedEffect(cameraPermissionState.permissionGranted) { @@ -143,7 +156,10 @@ class RoomDetailsEditPresenter @Inject constructor( pendingPermissionRequest = true cameraPermissionState.eventSink(PermissionsEvents.RequestPermissions) } - AvatarAction.Remove -> roomAvatarUriEdited = null + AvatarAction.Remove -> { + temporaryUriDeleter.delete(roomAvatarUriEdited) + roomAvatarUriEdited = null + } } } diff --git a/features/roomdetails/impl/src/test/kotlin/io/element/android/features/roomdetails/edit/RoomDetailsEditPresenterTest.kt b/features/roomdetails/impl/src/test/kotlin/io/element/android/features/roomdetails/edit/RoomDetailsEditPresenterTest.kt index 3b40e3bf3c..b1edabbbff 100644 --- a/features/roomdetails/impl/src/test/kotlin/io/element/android/features/roomdetails/edit/RoomDetailsEditPresenterTest.kt +++ b/features/roomdetails/impl/src/test/kotlin/io/element/android/features/roomdetails/edit/RoomDetailsEditPresenterTest.kt @@ -8,14 +8,12 @@ package io.element.android.features.roomdetails.edit import android.net.Uri -import app.cash.molecule.RecompositionMode -import app.cash.molecule.moleculeFlow import app.cash.turbine.ReceiveTurbine -import app.cash.turbine.test import com.google.common.truth.Truth.assertThat import io.element.android.features.roomdetails.aMatrixRoom import io.element.android.features.roomdetails.impl.edit.RoomDetailsEditEvents import io.element.android.features.roomdetails.impl.edit.RoomDetailsEditPresenter +import io.element.android.libraries.androidutils.file.TemporaryUriDeleter import io.element.android.libraries.architecture.AsyncAction import io.element.android.libraries.core.mimetype.MimeTypes import io.element.android.libraries.matrix.api.room.MatrixRoom @@ -31,9 +29,11 @@ import io.element.android.libraries.permissions.api.PermissionsPresenter import io.element.android.libraries.permissions.test.FakePermissionsPresenter import io.element.android.libraries.permissions.test.FakePermissionsPresenterFactory import io.element.android.tests.testutils.WarmUpRule +import io.element.android.tests.testutils.fake.FakeTemporaryUriDeleter import io.element.android.tests.testutils.lambda.lambdaError import io.element.android.tests.testutils.lambda.lambdaRecorder import io.element.android.tests.testutils.lambda.value +import io.element.android.tests.testutils.test import io.mockk.every import io.mockk.mockk import io.mockk.mockkStatic @@ -46,6 +46,7 @@ import org.junit.Rule import org.junit.Test import java.io.File +@Suppress("LargeClass") @ExperimentalCoroutinesApi class RoomDetailsEditPresenterTest { @get:Rule @@ -77,12 +78,14 @@ class RoomDetailsEditPresenterTest { private fun createRoomDetailsEditPresenter( room: MatrixRoom, permissionsPresenter: PermissionsPresenter = FakePermissionsPresenter(), + temporaryUriDeleter: TemporaryUriDeleter = FakeTemporaryUriDeleter(), ): RoomDetailsEditPresenter { return RoomDetailsEditPresenter( room = room, mediaPickerProvider = fakePickerProvider, mediaPreProcessor = fakeMediaPreProcessor, permissionsPresenterFactory = FakePermissionsPresenterFactory(permissionsPresenter), + temporaryUriDeleter = temporaryUriDeleter, ) } @@ -95,10 +98,12 @@ class RoomDetailsEditPresenterTest { emitRoomInfo = true, canSendStateResult = { _, _ -> Result.success(true) } ) - val presenter = createRoomDetailsEditPresenter(room) - moleculeFlow(RecompositionMode.Immediate) { - presenter.present() - }.test { + val deleteCallback = lambdaRecorder {} + val presenter = createRoomDetailsEditPresenter( + room = room, + temporaryUriDeleter = FakeTemporaryUriDeleter(deleteCallback), + ) + presenter.test { val initialState = awaitFirstItem() assertThat(initialState.roomId).isEqualTo(room.roomId) assertThat(initialState.roomRawName).isEqualTo(A_ROOM_RAW_NAME) @@ -127,10 +132,12 @@ class RoomDetailsEditPresenterTest { } }, ) - val presenter = createRoomDetailsEditPresenter(room) - moleculeFlow(RecompositionMode.Immediate) { - presenter.present() - }.test { + val deleteCallback = lambdaRecorder {} + val presenter = createRoomDetailsEditPresenter( + room = room, + temporaryUriDeleter = FakeTemporaryUriDeleter(deleteCallback), + ) + presenter.test { // Initially false val initialState = awaitItem() assertThat(initialState.canChangeName).isFalse() @@ -141,6 +148,7 @@ class RoomDetailsEditPresenterTest { assertThat(settledState.canChangeName).isTrue() assertThat(settledState.canChangeAvatar).isFalse() assertThat(settledState.canChangeTopic).isFalse() + deleteCallback.assertions().isCalledOnce().with(value(null)) } } @@ -157,10 +165,12 @@ class RoomDetailsEditPresenterTest { } } ) - val presenter = createRoomDetailsEditPresenter(room) - moleculeFlow(RecompositionMode.Immediate) { - presenter.present() - }.test { + val deleteCallback = lambdaRecorder {} + val presenter = createRoomDetailsEditPresenter( + room = room, + temporaryUriDeleter = FakeTemporaryUriDeleter(deleteCallback), + ) + presenter.test { // Initially false val initialState = awaitItem() assertThat(initialState.canChangeName).isFalse() @@ -187,10 +197,12 @@ class RoomDetailsEditPresenterTest { } } ) - val presenter = createRoomDetailsEditPresenter(room) - moleculeFlow(RecompositionMode.Immediate) { - presenter.present() - }.test { + val deleteCallback = lambdaRecorder {} + val presenter = createRoomDetailsEditPresenter( + room = room, + temporaryUriDeleter = FakeTemporaryUriDeleter(deleteCallback), + ) + presenter.test { // Initially false val initialState = awaitItem() assertThat(initialState.canChangeName).isFalse() @@ -213,10 +225,12 @@ class RoomDetailsEditPresenterTest { emitRoomInfo = true, canSendStateResult = { _, _ -> Result.success(true) } ) - val presenter = createRoomDetailsEditPresenter(room) - moleculeFlow(RecompositionMode.Immediate) { - presenter.present() - }.test { + val deleteCallback = lambdaRecorder {} + val presenter = createRoomDetailsEditPresenter( + room = room, + temporaryUriDeleter = FakeTemporaryUriDeleter(deleteCallback), + ) + presenter.test { val initialState = awaitFirstItem() assertThat(initialState.roomTopic).isEqualTo("My topic") assertThat(initialState.roomRawName).isEqualTo("Name") @@ -258,10 +272,12 @@ class RoomDetailsEditPresenterTest { canSendStateResult = { _, _ -> Result.success(true) } ) fakePickerProvider.givenResult(anotherAvatarUri) - val presenter = createRoomDetailsEditPresenter(room) - moleculeFlow(RecompositionMode.Immediate) { - presenter.present() - }.test { + val deleteCallback = lambdaRecorder {} + val presenter = createRoomDetailsEditPresenter( + room = room, + temporaryUriDeleter = FakeTemporaryUriDeleter(deleteCallback), + ) + presenter.test { val initialState = awaitFirstItem() assertThat(initialState.roomAvatarUrl).isEqualTo(roomAvatarUri) initialState.eventSink(RoomDetailsEditEvents.HandleAvatarAction(AvatarAction.ChoosePhoto)) @@ -282,13 +298,13 @@ class RoomDetailsEditPresenterTest { ) fakePickerProvider.givenResult(anotherAvatarUri) val fakePermissionsPresenter = FakePermissionsPresenter() + val deleteCallback = lambdaRecorder {} val presenter = createRoomDetailsEditPresenter( room = room, permissionsPresenter = fakePermissionsPresenter, + temporaryUriDeleter = FakeTemporaryUriDeleter(deleteCallback), ) - moleculeFlow(RecompositionMode.Immediate) { - presenter.present() - }.test { + presenter.test { val initialState = awaitFirstItem() assertThat(initialState.roomAvatarUrl).isEqualTo(roomAvatarUri) assertThat(initialState.cameraPermissionState.permissionGranted).isFalse() @@ -305,6 +321,12 @@ class RoomDetailsEditPresenterTest { stateWithNewAvatar.eventSink(RoomDetailsEditEvents.HandleAvatarAction(AvatarAction.TakePhoto)) val stateWithNewAvatar2 = awaitItem() assertThat(stateWithNewAvatar2.roomAvatarUrl).isEqualTo(roomAvatarUri) + deleteCallback.assertions().isCalledExactly(4).withSequence( + listOf(value(null)), + listOf(value(null)), + listOf(value(roomAvatarUri)), + listOf(value(anotherAvatarUri)), + ) } } @@ -318,10 +340,12 @@ class RoomDetailsEditPresenterTest { canSendStateResult = { _, _ -> Result.success(true) } ) fakePickerProvider.givenResult(roomAvatarUri) - val presenter = createRoomDetailsEditPresenter(room) - moleculeFlow(RecompositionMode.Immediate) { - presenter.present() - }.test { + val deleteCallback = lambdaRecorder {} + val presenter = createRoomDetailsEditPresenter( + room = room, + temporaryUriDeleter = FakeTemporaryUriDeleter(deleteCallback), + ) + presenter.test { val initialState = awaitFirstItem() assertThat(initialState.saveButtonEnabled).isFalse() // Once a change is made, the save button is enabled @@ -367,10 +391,12 @@ class RoomDetailsEditPresenterTest { canSendStateResult = { _, _ -> Result.success(true) } ) fakePickerProvider.givenResult(roomAvatarUri) - val presenter = createRoomDetailsEditPresenter(room) - moleculeFlow(RecompositionMode.Immediate) { - presenter.present() - }.test { + val deleteCallback = lambdaRecorder {} + val presenter = createRoomDetailsEditPresenter( + room = room, + temporaryUriDeleter = FakeTemporaryUriDeleter(deleteCallback), + ) + presenter.test { val initialState = awaitFirstItem() assertThat(initialState.saveButtonEnabled).isFalse() // Once a change is made, the save button is enabled @@ -421,10 +447,12 @@ class RoomDetailsEditPresenterTest { removeAvatarResult = removeAvatarResult, canSendStateResult = { _, _ -> Result.success(true) } ) - val presenter = createRoomDetailsEditPresenter(room) - moleculeFlow(RecompositionMode.Immediate) { - presenter.present() - }.test { + val deleteCallback = lambdaRecorder {} + val presenter = createRoomDetailsEditPresenter( + room = room, + temporaryUriDeleter = FakeTemporaryUriDeleter(deleteCallback), + ) + presenter.test { val initialState = awaitFirstItem() initialState.eventSink(RoomDetailsEditEvents.UpdateRoomName("New name")) initialState.eventSink(RoomDetailsEditEvents.UpdateRoomTopic("New topic")) @@ -445,10 +473,12 @@ class RoomDetailsEditPresenterTest { avatarUrl = AN_AVATAR_URL, canSendStateResult = { _, _ -> Result.success(true) } ) - val presenter = createRoomDetailsEditPresenter(room) - moleculeFlow(RecompositionMode.Immediate) { - presenter.present() - }.test { + val deleteCallback = lambdaRecorder {} + val presenter = createRoomDetailsEditPresenter( + room = room, + temporaryUriDeleter = FakeTemporaryUriDeleter(deleteCallback), + ) + presenter.test { val initialState = awaitItem() initialState.eventSink(RoomDetailsEditEvents.UpdateRoomName(" Name ")) initialState.eventSink(RoomDetailsEditEvents.UpdateRoomTopic(" My topic ")) @@ -465,14 +495,17 @@ class RoomDetailsEditPresenterTest { avatarUrl = AN_AVATAR_URL, canSendStateResult = { _, _ -> Result.success(true) } ) - val presenter = createRoomDetailsEditPresenter(room) - moleculeFlow(RecompositionMode.Immediate) { - presenter.present() - }.test { + val deleteCallback = lambdaRecorder {} + val presenter = createRoomDetailsEditPresenter( + room = room, + temporaryUriDeleter = FakeTemporaryUriDeleter(deleteCallback), + ) + presenter.test { val initialState = awaitItem() initialState.eventSink(RoomDetailsEditEvents.UpdateRoomTopic("")) initialState.eventSink(RoomDetailsEditEvents.Save) cancelAndIgnoreRemainingEvents() + deleteCallback.assertions().isCalledOnce().with(value(null)) } } @@ -484,14 +517,17 @@ class RoomDetailsEditPresenterTest { avatarUrl = AN_AVATAR_URL, canSendStateResult = { _, _ -> Result.success(true) } ) - val presenter = createRoomDetailsEditPresenter(room) - moleculeFlow(RecompositionMode.Immediate) { - presenter.present() - }.test { + val deleteCallback = lambdaRecorder {} + val presenter = createRoomDetailsEditPresenter( + room = room, + temporaryUriDeleter = FakeTemporaryUriDeleter(deleteCallback), + ) + presenter.test { val initialState = awaitItem() initialState.eventSink(RoomDetailsEditEvents.UpdateRoomName("")) initialState.eventSink(RoomDetailsEditEvents.Save) cancelAndIgnoreRemainingEvents() + deleteCallback.assertions().isCalledOnce().with(value(null)) } } @@ -506,15 +542,21 @@ class RoomDetailsEditPresenterTest { canSendStateResult = { _, _ -> Result.success(true) } ) givenPickerReturnsFile() - val presenter = createRoomDetailsEditPresenter(room) - moleculeFlow(RecompositionMode.Immediate) { - presenter.present() - }.test { + val deleteCallback = lambdaRecorder {} + val presenter = createRoomDetailsEditPresenter( + room = room, + temporaryUriDeleter = FakeTemporaryUriDeleter(deleteCallback), + ) + presenter.test { val initialState = awaitItem() initialState.eventSink(RoomDetailsEditEvents.HandleAvatarAction(AvatarAction.ChoosePhoto)) initialState.eventSink(RoomDetailsEditEvents.Save) skipItems(4) updateAvatarResult.assertions().isCalledOnce().with(value(MimeTypes.Jpeg), value(fakeFileContents)) + deleteCallback.assertions().isCalledExactly(2).withSequence( + listOf(value(null)), + listOf(value(null)), + ) } } @@ -528,10 +570,12 @@ class RoomDetailsEditPresenterTest { ) fakePickerProvider.givenResult(anotherAvatarUri) fakeMediaPreProcessor.givenResult(Result.failure(Throwable("Oh no"))) - val presenter = createRoomDetailsEditPresenter(room) - moleculeFlow(RecompositionMode.Immediate) { - presenter.present() - }.test { + val deleteCallback = lambdaRecorder {} + val presenter = createRoomDetailsEditPresenter( + room = room, + temporaryUriDeleter = FakeTemporaryUriDeleter(deleteCallback), + ) + presenter.test { val initialState = awaitItem() initialState.eventSink(RoomDetailsEditEvents.HandleAvatarAction(AvatarAction.ChoosePhoto)) initialState.eventSink(RoomDetailsEditEvents.Save) @@ -576,7 +620,7 @@ class RoomDetailsEditPresenterTest { removeAvatarResult = { Result.failure(Throwable("!")) }, canSendStateResult = { _, _ -> Result.success(true) } ) - saveAndAssertFailure(room, RoomDetailsEditEvents.HandleAvatarAction(AvatarAction.Remove)) + saveAndAssertFailure(room, RoomDetailsEditEvents.HandleAvatarAction(AvatarAction.Remove), deleteCallbackNumberOfInvocation = 3) } @Test @@ -590,7 +634,7 @@ class RoomDetailsEditPresenterTest { updateAvatarResult = { _, _ -> Result.failure(Throwable("!")) }, canSendStateResult = { _, _ -> Result.success(true) } ) - saveAndAssertFailure(room, RoomDetailsEditEvents.HandleAvatarAction(AvatarAction.ChoosePhoto)) + saveAndAssertFailure(room, RoomDetailsEditEvents.HandleAvatarAction(AvatarAction.ChoosePhoto), deleteCallbackNumberOfInvocation = 3) } @Test @@ -603,10 +647,12 @@ class RoomDetailsEditPresenterTest { setTopicResult = { Result.failure(Throwable("!")) }, canSendStateResult = { _, _ -> Result.success(true) } ) - val presenter = createRoomDetailsEditPresenter(room) - moleculeFlow(RecompositionMode.Immediate) { - presenter.present() - }.test { + val deleteCallback = lambdaRecorder {} + val presenter = createRoomDetailsEditPresenter( + room = room, + temporaryUriDeleter = FakeTemporaryUriDeleter(deleteCallback), + ) + presenter.test { val initialState = awaitItem() initialState.eventSink(RoomDetailsEditEvents.UpdateRoomTopic("foo")) initialState.eventSink(RoomDetailsEditEvents.Save) @@ -617,17 +663,24 @@ class RoomDetailsEditPresenterTest { } } - private suspend fun saveAndAssertFailure(room: MatrixRoom, event: RoomDetailsEditEvents) { - val presenter = createRoomDetailsEditPresenter(room) - moleculeFlow(RecompositionMode.Immediate) { - presenter.present() - }.test { + private suspend fun saveAndAssertFailure( + room: MatrixRoom, + event: RoomDetailsEditEvents, + deleteCallbackNumberOfInvocation: Int = 2, + ) { + val deleteCallback = lambdaRecorder {} + val presenter = createRoomDetailsEditPresenter( + room = room, + temporaryUriDeleter = FakeTemporaryUriDeleter(deleteCallback), + ) + presenter.test { val initialState = awaitFirstItem() initialState.eventSink(event) initialState.eventSink(RoomDetailsEditEvents.Save) skipItems(1) assertThat(awaitItem().saveAction).isInstanceOf(AsyncAction.Loading::class.java) assertThat(awaitItem().saveAction).isInstanceOf(AsyncAction.Failure::class.java) + deleteCallback.assertions().isCalledExactly(deleteCallbackNumberOfInvocation) } } diff --git a/libraries/androidutils/src/main/kotlin/io/element/android/libraries/androidutils/file/TemporaryUriDeleter.kt b/libraries/androidutils/src/main/kotlin/io/element/android/libraries/androidutils/file/TemporaryUriDeleter.kt new file mode 100644 index 0000000000..4eabda8972 --- /dev/null +++ b/libraries/androidutils/src/main/kotlin/io/element/android/libraries/androidutils/file/TemporaryUriDeleter.kt @@ -0,0 +1,39 @@ +/* + * Copyright 2024 New Vector Ltd. + * + * SPDX-License-Identifier: AGPL-3.0-only + * Please see LICENSE in the repository root for full details. + */ + +package io.element.android.libraries.androidutils.file + +import android.content.Context +import android.net.Uri +import com.squareup.anvil.annotations.ContributesBinding +import io.element.android.libraries.di.AppScope +import io.element.android.libraries.di.ApplicationContext +import timber.log.Timber +import javax.inject.Inject + +interface TemporaryUriDeleter { + /** + * Delete the Uri only if it is a temporary one. + */ + fun delete(uri: Uri?) +} + +@ContributesBinding(AppScope::class) +class DefaultTemporaryUriDeleter @Inject constructor( + @ApplicationContext private val context: Context, +) : TemporaryUriDeleter { + private val baseCacheUri = "content://${context.packageName}.fileprovider/cache" + + override fun delete(uri: Uri?) { + uri ?: return + if (uri.toString().startsWith(baseCacheUri)) { + context.contentResolver.delete(uri, null, null) + } else { + Timber.d("Do not delete the uri") + } + } +} diff --git a/libraries/mediaupload/impl/src/main/kotlin/io/element/android/libraries/mediaupload/impl/AndroidMediaPreProcessor.kt b/libraries/mediaupload/impl/src/main/kotlin/io/element/android/libraries/mediaupload/impl/AndroidMediaPreProcessor.kt index b219da5350..54b880369b 100644 --- a/libraries/mediaupload/impl/src/main/kotlin/io/element/android/libraries/mediaupload/impl/AndroidMediaPreProcessor.kt +++ b/libraries/mediaupload/impl/src/main/kotlin/io/element/android/libraries/mediaupload/impl/AndroidMediaPreProcessor.kt @@ -13,6 +13,7 @@ import android.media.MediaMetadataRetriever import android.net.Uri import androidx.exifinterface.media.ExifInterface import com.squareup.anvil.annotations.ContributesBinding +import io.element.android.libraries.androidutils.file.TemporaryUriDeleter import io.element.android.libraries.androidutils.file.createTmpFile import io.element.android.libraries.androidutils.file.getFileName import io.element.android.libraries.androidutils.file.safeRenameTo @@ -50,6 +51,7 @@ class AndroidMediaPreProcessor @Inject constructor( private val imageCompressor: ImageCompressor, private val videoCompressor: VideoCompressor, private val coroutineDispatchers: CoroutineDispatchers, + private val temporaryUriDeleter: TemporaryUriDeleter, ) : MediaPreProcessor { companion object { /** @@ -86,6 +88,8 @@ class AndroidMediaPreProcessor @Inject constructor( Timber.w("Deleting original uri $uri") contentResolver.delete(uri, null, null) } + } else { + temporaryUriDeleter.delete(uri) } result.postProcess(uri) } diff --git a/libraries/mediaupload/impl/src/test/kotlin/io/element/android/libraries/mediaupload/impl/AndroidMediaPreProcessorTest.kt b/libraries/mediaupload/impl/src/test/kotlin/io/element/android/libraries/mediaupload/impl/AndroidMediaPreProcessorTest.kt index 09e316bd2f..a9e9c3f7c2 100644 --- a/libraries/mediaupload/impl/src/test/kotlin/io/element/android/libraries/mediaupload/impl/AndroidMediaPreProcessorTest.kt +++ b/libraries/mediaupload/impl/src/test/kotlin/io/element/android/libraries/mediaupload/impl/AndroidMediaPreProcessorTest.kt @@ -8,10 +8,12 @@ package io.element.android.libraries.mediaupload.impl import android.content.Context +import android.net.Uri import android.os.Build import androidx.core.net.toUri import androidx.test.platform.app.InstrumentationRegistry import com.google.common.truth.Truth.assertThat +import io.element.android.libraries.androidutils.file.TemporaryUriDeleter import io.element.android.libraries.core.mimetype.MimeTypes import io.element.android.libraries.matrix.api.media.AudioInfo import io.element.android.libraries.matrix.api.media.FileInfo @@ -21,6 +23,8 @@ import io.element.android.libraries.matrix.api.media.VideoInfo import io.element.android.libraries.mediaupload.api.MediaPreProcessor import io.element.android.libraries.mediaupload.api.MediaUploadInfo import io.element.android.services.toolbox.test.sdk.FakeBuildVersionSdkIntProvider +import io.element.android.tests.testutils.fake.FakeTemporaryUriDeleter +import io.element.android.tests.testutils.lambda.lambdaRecorder import io.element.android.tests.testutils.testCoroutineDispatchers import kotlinx.coroutines.test.TestScope import kotlinx.coroutines.test.runTest @@ -42,7 +46,12 @@ class AndroidMediaPreProcessorTest { deleteOriginal: Boolean = false, ): MediaUploadInfo { val context = InstrumentationRegistry.getInstrumentation().context - val sut = createAndroidMediaPreProcessor(context, sdkIntVersion) + val deleteCallback = lambdaRecorder {} + val sut = createAndroidMediaPreProcessor( + context = context, + sdkIntVersion = sdkIntVersion, + temporaryUriDeleter = FakeTemporaryUriDeleter(deleteCallback), + ) val file = getFileFromAssets(context, asset.filename) val result = sut.process( uri = file.toUri(), @@ -52,6 +61,7 @@ class AndroidMediaPreProcessorTest { ) val data = result.getOrThrow() assertThat(data.file.path).endsWith(asset.filename) + deleteCallback.assertions().isCalledExactly(if (deleteOriginal) 0 else 1) return data } @@ -356,13 +366,15 @@ class AndroidMediaPreProcessorTest { private fun TestScope.createAndroidMediaPreProcessor( context: Context, - sdkIntVersion: Int = Build.VERSION_CODES.P + sdkIntVersion: Int = Build.VERSION_CODES.P, + temporaryUriDeleter: TemporaryUriDeleter = FakeTemporaryUriDeleter(), ) = AndroidMediaPreProcessor( context = context, thumbnailFactory = ThumbnailFactory(context, FakeBuildVersionSdkIntProvider(sdkIntVersion)), imageCompressor = ImageCompressor(context, testCoroutineDispatchers()), videoCompressor = VideoCompressor(context), coroutineDispatchers = testCoroutineDispatchers(), + temporaryUriDeleter = temporaryUriDeleter, ) @Throws(IOException::class) diff --git a/tests/testutils/src/main/kotlin/io/element/android/tests/testutils/fake/FakeTemporaryUriDeleter.kt b/tests/testutils/src/main/kotlin/io/element/android/tests/testutils/fake/FakeTemporaryUriDeleter.kt new file mode 100644 index 0000000000..e530fdbc0b --- /dev/null +++ b/tests/testutils/src/main/kotlin/io/element/android/tests/testutils/fake/FakeTemporaryUriDeleter.kt @@ -0,0 +1,20 @@ +/* + * Copyright 2024 New Vector Ltd. + * + * SPDX-License-Identifier: AGPL-3.0-only + * Please see LICENSE in the repository root for full details. + */ + +package io.element.android.tests.testutils.fake + +import android.net.Uri +import io.element.android.libraries.androidutils.file.TemporaryUriDeleter +import io.element.android.tests.testutils.lambda.lambdaError + +class FakeTemporaryUriDeleter( + val deleteCallback: (uri: Uri?) -> Unit = { lambdaError() } +) : TemporaryUriDeleter { + override fun delete(uri: Uri?) { + deleteCallback(uri) + } +} From 43053de5fb194a63521d7469b780f320db9c59d4 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Thu, 7 Nov 2024 09:20:48 +0100 Subject: [PATCH 13/15] Delete the temporary file only when the user explicitly cancel the upload. --- .../preview/AttachmentsPreviewEvents.kt | 1 + .../preview/AttachmentsPreviewNode.kt | 10 ++++- .../preview/AttachmentsPreviewPresenter.kt | 38 +++++++++------- .../preview/AttachmentsPreviewState.kt | 1 - .../preview/AttachmentsPreviewView.kt | 19 ++++---- .../attachments/preview/OnDoneListener.kt | 12 +++++ .../AttachmentsPreviewPresenterTest.kt | 45 ++++++++++++++++--- 7 files changed, 88 insertions(+), 38 deletions(-) create mode 100644 features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/OnDoneListener.kt diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewEvents.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewEvents.kt index 28cc95eaf0..742e78e8e0 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewEvents.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewEvents.kt @@ -12,5 +12,6 @@ import androidx.compose.runtime.Immutable @Immutable sealed interface AttachmentsPreviewEvents { data object SendAttachment : AttachmentsPreviewEvents + data object Cancel : AttachmentsPreviewEvents data object ClearSendState : AttachmentsPreviewEvents } diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewNode.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewNode.kt index a435821197..2417d8346e 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewNode.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewNode.kt @@ -31,7 +31,14 @@ class AttachmentsPreviewNode @AssistedInject constructor( private val inputs: Inputs = inputs() - private val presenter = presenterFactory.create(inputs.attachment) + private val onDoneListener = OnDoneListener { + navigateUp() + } + + private val presenter = presenterFactory.create( + attachment = inputs.attachment, + onDoneListener = onDoneListener, + ) @Composable override fun View(modifier: Modifier) { @@ -39,7 +46,6 @@ class AttachmentsPreviewNode @AssistedInject constructor( val state = presenter.present() AttachmentsPreviewView( state = state, - onDismiss = this::navigateUp, modifier = modifier ) } diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewPresenter.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewPresenter.kt index a3ba509a15..bc3e121070 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewPresenter.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewPresenter.kt @@ -8,7 +8,6 @@ package io.element.android.features.messages.impl.attachments.preview import androidx.compose.runtime.Composable -import androidx.compose.runtime.DisposableEffect import androidx.compose.runtime.MutableState import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableStateOf @@ -36,13 +35,17 @@ import kotlin.coroutines.coroutineContext class AttachmentsPreviewPresenter @AssistedInject constructor( @Assisted private val attachment: Attachment, + @Assisted private val onDoneListener: OnDoneListener, private val mediaSender: MediaSender, private val permalinkBuilder: PermalinkBuilder, private val temporaryUriDeleter: TemporaryUriDeleter, ) : Presenter { @AssistedFactory interface Factory { - fun create(attachment: Attachment): AttachmentsPreviewPresenter + fun create( + attachment: Attachment, + onDoneListener: OnDoneListener, + ): AttachmentsPreviewPresenter } @Composable @@ -60,20 +63,6 @@ class AttachmentsPreviewPresenter @AssistedInject constructor( val ongoingSendAttachmentJob = remember { mutableStateOf(null) } - DisposableEffect(Unit) { - onDispose { - // Delete the temporary file when the composable is disposed, in case it was not sent - if (sendActionState.value == SendActionState.Idle) { - // Attachment has not been sent, maybe delete it - when (attachment) { - is Attachment.Media -> { - temporaryUriDeleter.delete(attachment.localMedia.uri) - } - } - } - } - } - fun handleEvents(attachmentsPreviewEvents: AttachmentsPreviewEvents) { when (attachmentsPreviewEvents) { is AttachmentsPreviewEvents.SendAttachment -> { @@ -85,6 +74,9 @@ class AttachmentsPreviewPresenter @AssistedInject constructor( sendActionState = sendActionState, ) } + AttachmentsPreviewEvents.Cancel -> { + coroutineScope.cancel(attachment) + } AttachmentsPreviewEvents.ClearSendState -> { ongoingSendAttachmentJob.value?.let { it.cancel() @@ -119,6 +111,18 @@ class AttachmentsPreviewPresenter @AssistedInject constructor( } } + private fun CoroutineScope.cancel( + attachment: Attachment, + ) = launch { + // Delete the temporary file + when (attachment) { + is Attachment.Media -> { + temporaryUriDeleter.delete(attachment.localMedia.uri) + } + } + onDoneListener() + } + private suspend fun sendMedia( mediaAttachment: Attachment.Media, caption: String?, @@ -141,7 +145,7 @@ class AttachmentsPreviewPresenter @AssistedInject constructor( ).getOrThrow() }.fold( onSuccess = { - sendActionState.value = SendActionState.Done + onDoneListener() }, onFailure = { error -> Timber.e(error, "Failed to send attachment") diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewState.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewState.kt index 72ea0a2098..5ffe9364ff 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewState.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewState.kt @@ -36,5 +36,4 @@ sealed interface SendActionState { } data class Failure(val error: Throwable) : SendActionState - data object Done : SendActionState } diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewView.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewView.kt index 2906f26b3c..1380add329 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewView.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewView.kt @@ -7,6 +7,7 @@ package io.element.android.features.messages.impl.attachments.preview +import androidx.activity.compose.BackHandler import androidx.compose.foundation.background import androidx.compose.foundation.layout.Box import androidx.compose.foundation.layout.IntrinsicSize @@ -17,9 +18,6 @@ import androidx.compose.foundation.layout.imePadding import androidx.compose.foundation.layout.navigationBarsPadding import androidx.compose.material3.ExperimentalMaterial3Api import androidx.compose.runtime.Composable -import androidx.compose.runtime.LaunchedEffect -import androidx.compose.runtime.getValue -import androidx.compose.runtime.rememberUpdatedState import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier import androidx.compose.ui.res.stringResource @@ -50,22 +48,22 @@ import me.saket.telephoto.zoomable.rememberZoomableState @Composable fun AttachmentsPreviewView( state: AttachmentsPreviewState, - onDismiss: () -> Unit, modifier: Modifier = Modifier, ) { fun postSendAttachment() { state.eventSink(AttachmentsPreviewEvents.SendAttachment) } + fun postCancel() { + state.eventSink(AttachmentsPreviewEvents.Cancel) + } + fun postClearSendState() { state.eventSink(AttachmentsPreviewEvents.ClearSendState) } - if (state.sendActionState is SendActionState.Done) { - val latestOnDismiss by rememberUpdatedState(onDismiss) - LaunchedEffect(state.sendActionState) { - latestOnDismiss() - } + BackHandler(enabled = state.sendActionState !is SendActionState.Sending) { + postCancel() } Scaffold( @@ -75,7 +73,7 @@ fun AttachmentsPreviewView( navigationIcon = { BackButton( imageVector = CompoundIcons.Close(), - onClick = onDismiss, + onClick = ::postCancel, ) }, title = {}, @@ -202,6 +200,5 @@ private fun AttachmentsPreviewBottomActions( internal fun AttachmentsPreviewViewPreview(@PreviewParameter(AttachmentsPreviewStateProvider::class) state: AttachmentsPreviewState) = ElementPreviewDark { AttachmentsPreviewView( state = state, - onDismiss = {}, ) } diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/OnDoneListener.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/OnDoneListener.kt new file mode 100644 index 0000000000..2e53ab2c50 --- /dev/null +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/OnDoneListener.kt @@ -0,0 +1,12 @@ +/* + * Copyright 2024 New Vector Ltd. + * + * SPDX-License-Identifier: AGPL-3.0-only + * Please see LICENSE in the repository root for full details. + */ + +package io.element.android.features.messages.impl.attachments.preview + +fun interface OnDoneListener { + operator fun invoke() +} diff --git a/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/attachments/AttachmentsPreviewPresenterTest.kt b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/attachments/AttachmentsPreviewPresenterTest.kt index 85acefb5fe..ac753f6cdb 100644 --- a/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/attachments/AttachmentsPreviewPresenterTest.kt +++ b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/attachments/AttachmentsPreviewPresenterTest.kt @@ -16,6 +16,7 @@ import app.cash.turbine.test import com.google.common.truth.Truth.assertThat import io.element.android.features.messages.impl.attachments.preview.AttachmentsPreviewEvents import io.element.android.features.messages.impl.attachments.preview.AttachmentsPreviewPresenter +import io.element.android.features.messages.impl.attachments.preview.OnDoneListener import io.element.android.features.messages.impl.attachments.preview.SendActionState import io.element.android.features.messages.impl.fixtures.aMediaAttachment import io.element.android.libraries.androidutils.file.TemporaryUriDeleter @@ -42,6 +43,7 @@ import io.element.android.tests.testutils.lambda.lambdaRecorder import io.element.android.tests.testutils.lambda.value import io.mockk.mockk import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.test.advanceUntilIdle import kotlinx.coroutines.test.runTest import org.junit.Rule import org.junit.Test @@ -69,7 +71,11 @@ class AttachmentsPreviewPresenterTest { ), sendFileResult = sendFileResult, ) - val presenter = createAttachmentsPreviewPresenter(room = room) + val onDoneListener = lambdaRecorder { } + val presenter = createAttachmentsPreviewPresenter( + room = room, + onDoneListener = { onDoneListener() }, + ) moleculeFlow(RecompositionMode.Immediate) { presenter.present() }.test { @@ -80,9 +86,28 @@ class AttachmentsPreviewPresenterTest { assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Sending.Uploading(0f)) assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Sending.Uploading(0.5f)) assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Sending.Uploading(1f)) - val successState = awaitItem() - assertThat(successState.sendActionState).isEqualTo(SendActionState.Done) + advanceUntilIdle() sendFileResult.assertions().isCalledOnce() + onDoneListener.assertions().isCalledOnce() + } + } + + @Test + fun `present - cancel scenario`() = runTest { + val onDoneListener = lambdaRecorder { } + val deleteCallback = lambdaRecorder {} + val presenter = createAttachmentsPreviewPresenter( + temporaryUriDeleter = FakeTemporaryUriDeleter(deleteCallback), + onDoneListener = { onDoneListener() }, + ) + moleculeFlow(RecompositionMode.Immediate) { + presenter.present() + }.test { + val initialState = awaitItem() + assertThat(initialState.sendActionState).isEqualTo(SendActionState.Idle) + initialState.eventSink(AttachmentsPreviewEvents.Cancel) + deleteCallback.assertions().isCalledOnce() + onDoneListener.assertions().isCalledOnce() } } @@ -98,9 +123,11 @@ class AttachmentsPreviewPresenterTest { val room = FakeMatrixRoom( sendImageResult = sendImageResult, ) + val onDoneListener = lambdaRecorder { } val presenter = createAttachmentsPreviewPresenter( room = room, mediaPreProcessor = mediaPreProcessor, + onDoneListener = { onDoneListener() }, ) moleculeFlow(RecompositionMode.Immediate) { presenter.present() @@ -110,8 +137,7 @@ class AttachmentsPreviewPresenterTest { initialState.textEditorState.setMarkdown(A_CAPTION) initialState.eventSink(AttachmentsPreviewEvents.SendAttachment) assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Sending.Processing) - val successState = awaitItem() - assertThat(successState.sendActionState).isEqualTo(SendActionState.Done) + advanceUntilIdle() sendImageResult.assertions().isCalledOnce().with( any(), any(), @@ -120,6 +146,7 @@ class AttachmentsPreviewPresenterTest { any(), any(), ) + onDoneListener.assertions().isCalledOnce() } } @@ -135,9 +162,11 @@ class AttachmentsPreviewPresenterTest { val room = FakeMatrixRoom( sendVideoResult = sendVideoResult, ) + val onDoneListener = lambdaRecorder { } val presenter = createAttachmentsPreviewPresenter( room = room, mediaPreProcessor = mediaPreProcessor, + onDoneListener = { onDoneListener() }, ) moleculeFlow(RecompositionMode.Immediate) { presenter.present() @@ -147,8 +176,7 @@ class AttachmentsPreviewPresenterTest { initialState.textEditorState.setMarkdown(A_CAPTION) initialState.eventSink(AttachmentsPreviewEvents.SendAttachment) assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Sending.Processing) - val successState = awaitItem() - assertThat(successState.sendActionState).isEqualTo(SendActionState.Done) + advanceUntilIdle() sendVideoResult.assertions().isCalledOnce().with( any(), any(), @@ -157,6 +185,7 @@ class AttachmentsPreviewPresenterTest { any(), any(), ) + onDoneListener.assertions().isCalledOnce() } } @@ -210,9 +239,11 @@ class AttachmentsPreviewPresenterTest { permalinkBuilder: PermalinkBuilder = FakePermalinkBuilder(), mediaPreProcessor: MediaPreProcessor = FakeMediaPreProcessor(), temporaryUriDeleter: TemporaryUriDeleter = FakeTemporaryUriDeleter(), + onDoneListener: OnDoneListener = OnDoneListener {}, ): AttachmentsPreviewPresenter { return AttachmentsPreviewPresenter( attachment = aMediaAttachment(localMedia), + onDoneListener = onDoneListener, mediaSender = MediaSender(mediaPreProcessor, room, InMemorySessionPreferencesStore()), permalinkBuilder = permalinkBuilder, temporaryUriDeleter = temporaryUriDeleter, From 8dfe5300b893a7a2484750df136f818ab692a041 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Thu, 7 Nov 2024 09:36:36 +0100 Subject: [PATCH 14/15] Rename parameter. --- .../EditUserProfilePresenterTest.kt | 18 +++++++++--------- .../testutils/fake/FakeTemporaryUriDeleter.kt | 4 ++-- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/features/preferences/impl/src/test/kotlin/io/element/android/features/preferences/impl/user/editprofile/EditUserProfilePresenterTest.kt b/features/preferences/impl/src/test/kotlin/io/element/android/features/preferences/impl/user/editprofile/EditUserProfilePresenterTest.kt index 77b86ce4c7..0f39ab491c 100644 --- a/features/preferences/impl/src/test/kotlin/io/element/android/features/preferences/impl/user/editprofile/EditUserProfilePresenterTest.kt +++ b/features/preferences/impl/src/test/kotlin/io/element/android/features/preferences/impl/user/editprofile/EditUserProfilePresenterTest.kt @@ -116,7 +116,7 @@ class EditUserProfilePresenterTest { val presenter = createEditUserProfilePresenter( matrixUser = user, temporaryUriDeleter = FakeTemporaryUriDeleter( - deleteCallback = { assertThat(it).isEqualTo(userAvatarUri) } + deleteLambda = { assertThat(it).isEqualTo(userAvatarUri) } ), ) moleculeFlow(RecompositionMode.Immediate) { @@ -150,7 +150,7 @@ class EditUserProfilePresenterTest { val presenter = createEditUserProfilePresenter( matrixUser = user, temporaryUriDeleter = FakeTemporaryUriDeleter( - deleteCallback = { assertThat(it).isEqualTo(userAvatarUri) } + deleteLambda = { assertThat(it).isEqualTo(userAvatarUri) } ), ) moleculeFlow(RecompositionMode.Immediate) { @@ -175,7 +175,7 @@ class EditUserProfilePresenterTest { matrixUser = user, permissionsPresenter = fakePermissionsPresenter, temporaryUriDeleter = FakeTemporaryUriDeleter( - deleteCallback = deleteCallback, + deleteLambda = deleteCallback, ), ) moleculeFlow(RecompositionMode.Immediate) { @@ -212,7 +212,7 @@ class EditUserProfilePresenterTest { val presenter = createEditUserProfilePresenter( matrixUser = user, temporaryUriDeleter = FakeTemporaryUriDeleter( - deleteCallback = deleteCallback + deleteLambda = deleteCallback ), ) moleculeFlow(RecompositionMode.Immediate) { @@ -255,7 +255,7 @@ class EditUserProfilePresenterTest { val presenter = createEditUserProfilePresenter( matrixUser = user, temporaryUriDeleter = FakeTemporaryUriDeleter( - deleteCallback = deleteCallback + deleteLambda = deleteCallback ), ) moleculeFlow(RecompositionMode.Immediate) { @@ -298,7 +298,7 @@ class EditUserProfilePresenterTest { matrixClient = matrixClient, matrixUser = user, temporaryUriDeleter = FakeTemporaryUriDeleter( - deleteCallback = { assertThat(it).isEqualTo(userAvatarUri) } + deleteLambda = { assertThat(it).isEqualTo(userAvatarUri) } ), ) moleculeFlow(RecompositionMode.Immediate) { @@ -367,7 +367,7 @@ class EditUserProfilePresenterTest { matrixClient = matrixClient, matrixUser = user, temporaryUriDeleter = FakeTemporaryUriDeleter( - deleteCallback = { assertThat(it).isEqualTo(userAvatarUri) } + deleteLambda = { assertThat(it).isEqualTo(userAvatarUri) } ), ) moleculeFlow(RecompositionMode.Immediate) { @@ -389,7 +389,7 @@ class EditUserProfilePresenterTest { matrixClient = matrixClient, matrixUser = user, temporaryUriDeleter = FakeTemporaryUriDeleter( - deleteCallback = { assertThat(it).isEqualTo(userAvatarUri) } + deleteLambda = { assertThat(it).isEqualTo(userAvatarUri) } ), ) fakePickerProvider.givenResult(anotherAvatarUri) @@ -460,7 +460,7 @@ class EditUserProfilePresenterTest { matrixUser = matrixUser, matrixClient = matrixClient, temporaryUriDeleter = FakeTemporaryUriDeleter( - deleteCallback = { assertThat(it).isEqualTo(userAvatarUri) } + deleteLambda = { assertThat(it).isEqualTo(userAvatarUri) } ), ) moleculeFlow(RecompositionMode.Immediate) { diff --git a/tests/testutils/src/main/kotlin/io/element/android/tests/testutils/fake/FakeTemporaryUriDeleter.kt b/tests/testutils/src/main/kotlin/io/element/android/tests/testutils/fake/FakeTemporaryUriDeleter.kt index e530fdbc0b..bb14d2783d 100644 --- a/tests/testutils/src/main/kotlin/io/element/android/tests/testutils/fake/FakeTemporaryUriDeleter.kt +++ b/tests/testutils/src/main/kotlin/io/element/android/tests/testutils/fake/FakeTemporaryUriDeleter.kt @@ -12,9 +12,9 @@ import io.element.android.libraries.androidutils.file.TemporaryUriDeleter import io.element.android.tests.testutils.lambda.lambdaError class FakeTemporaryUriDeleter( - val deleteCallback: (uri: Uri?) -> Unit = { lambdaError() } + val deleteLambda: (uri: Uri?) -> Unit = { lambdaError() } ) : TemporaryUriDeleter { override fun delete(uri: Uri?) { - deleteCallback(uri) + deleteLambda(uri) } } From 2206e940bc2c4ee164cf6566c4d48fc73fef7a12 Mon Sep 17 00:00:00 2001 From: Jorge Martin Espinosa Date: Fri, 8 Nov 2024 16:42:27 +0100 Subject: [PATCH 15/15] Fix verification failed issue, simplify verification logic (#3830) * Simplify session verification: - Reuse Rust `Client` instances created on the login process so we don't need to restore one right before the session verification. - Remove unnecessary sources of verification state updates. - Add an intermediate FTUE flow step which will display an indeterminate progress indicator instead of a blank screen. * Remove unnecessary workaround: the SDK should already handle this * Add regression tests for noop analytics service usage. * Add `services.analytics.noop` module to the test dependencies --------- Co-authored-by: Benoit Marty --- .../android/appnav/di/MatrixClientsHolder.kt | 10 ++- .../appnav/di/MatrixClientsHolderTest.kt | 13 +++ features/ftue/impl/build.gradle.kts | 1 + .../features/ftue/impl/FtueFlowNode.kt | 36 ++++++-- .../ftue/impl/state/DefaultFtueService.kt | 58 ++++++------- .../ftue/impl/DefaultFtueServiceTest.kt | 22 +++++ .../api/auth/MatrixAuthenticationService.kt | 3 + .../matrix/impl/RustMatrixClientFactory.kt | 16 ++-- .../auth/RustMatrixAuthenticationService.kt | 27 +++--- .../RustSessionVerificationService.kt | 85 +++++++------------ .../auth/FakeMatrixAuthenticationService.kt | 18 +++- 11 files changed, 176 insertions(+), 113 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 168cbe8314..8cff242cb2 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 @@ -27,10 +27,18 @@ private const val SAVE_INSTANCE_KEY = "io.element.android.x.di.MatrixClientsHold @SingleIn(AppScope::class) @ContributesBinding(AppScope::class) -class MatrixClientsHolder @Inject constructor(private val authenticationService: MatrixAuthenticationService) : MatrixClientProvider { +class MatrixClientsHolder @Inject constructor( + private val authenticationService: MatrixAuthenticationService, +) : MatrixClientProvider { private val sessionIdsToMatrixClient = ConcurrentHashMap() private val restoreMutex = Mutex() + init { + authenticationService.listenToNewMatrixClients { matrixClient -> + sessionIdsToMatrixClient[matrixClient.sessionId] = matrixClient + } + } + fun removeAll() { sessionIdsToMatrixClient.clear() } diff --git a/appnav/src/test/kotlin/io/element/android/appnav/di/MatrixClientsHolderTest.kt b/appnav/src/test/kotlin/io/element/android/appnav/di/MatrixClientsHolderTest.kt index 0058485aa5..390b095c40 100644 --- a/appnav/src/test/kotlin/io/element/android/appnav/di/MatrixClientsHolderTest.kt +++ b/appnav/src/test/kotlin/io/element/android/appnav/di/MatrixClientsHolderTest.kt @@ -81,4 +81,17 @@ class MatrixClientsHolderTest { matrixClientsHolder.restoreWithSavedState(savedStateMap) assertThat(matrixClientsHolder.getOrNull(A_SESSION_ID)).isEqualTo(fakeMatrixClient) } + + @Test + fun `test AuthenticationService listenToNewMatrixClients emits a Client value and we save it`() = runTest { + val fakeAuthenticationService = FakeMatrixAuthenticationService() + val matrixClientsHolder = MatrixClientsHolder(fakeAuthenticationService) + assertThat(matrixClientsHolder.getOrNull(A_SESSION_ID)).isNull() + + fakeAuthenticationService.givenMatrixClient(FakeMatrixClient(sessionId = A_SESSION_ID)) + val loginSucceeded = fakeAuthenticationService.login("user", "pass") + + assertThat(loginSucceeded.isSuccess).isTrue() + assertThat(matrixClientsHolder.getOrNull(A_SESSION_ID)).isNotNull() + } } diff --git a/features/ftue/impl/build.gradle.kts b/features/ftue/impl/build.gradle.kts index a4a0ef8f71..1937a4b0fc 100644 --- a/features/ftue/impl/build.gradle.kts +++ b/features/ftue/impl/build.gradle.kts @@ -45,6 +45,7 @@ dependencies { testImplementation(libs.test.turbine) testImplementation(projects.libraries.matrix.test) testImplementation(projects.services.analytics.test) + testImplementation(projects.services.analytics.noop) testImplementation(projects.libraries.permissions.impl) testImplementation(projects.libraries.permissions.test) testImplementation(projects.libraries.preferences.test) diff --git a/features/ftue/impl/src/main/kotlin/io/element/android/features/ftue/impl/FtueFlowNode.kt b/features/ftue/impl/src/main/kotlin/io/element/android/features/ftue/impl/FtueFlowNode.kt index 16473e62f5..f84a0d2b87 100644 --- a/features/ftue/impl/src/main/kotlin/io/element/android/features/ftue/impl/FtueFlowNode.kt +++ b/features/ftue/impl/src/main/kotlin/io/element/android/features/ftue/impl/FtueFlowNode.kt @@ -8,7 +8,10 @@ package io.element.android.features.ftue.impl import android.os.Parcelable +import androidx.compose.foundation.layout.Box +import androidx.compose.foundation.layout.fillMaxSize import androidx.compose.runtime.Composable +import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier import androidx.lifecycle.lifecycleScope import com.bumble.appyx.core.lifecycle.subscribe @@ -31,12 +34,14 @@ import io.element.android.features.lockscreen.api.LockScreenEntryPoint import io.element.android.libraries.architecture.BackstackView import io.element.android.libraries.architecture.BaseFlowNode import io.element.android.libraries.architecture.createNode +import io.element.android.libraries.designsystem.theme.components.CircularProgressIndicator import io.element.android.libraries.di.AppScope import io.element.android.libraries.di.SessionScope import io.element.android.services.analytics.api.AnalyticsService import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.distinctUntilChanged +import kotlinx.coroutines.flow.filter import kotlinx.coroutines.flow.launchIn import kotlinx.coroutines.flow.onEach import kotlinx.coroutines.launch @@ -80,14 +85,17 @@ class FtueFlowNode @AssistedInject constructor( super.onBuilt() lifecycle.subscribe(onCreate = { - lifecycleScope.launch { moveToNextStep() } + moveToNextStepIfNeeded() }) analyticsService.didAskUserConsent() .distinctUntilChanged() - .onEach { - lifecycleScope.launch { moveToNextStep() } - } + .onEach { moveToNextStepIfNeeded() } + .launchIn(lifecycleScope) + + ftueState.isVerificationStatusKnown + .filter { it } + .onEach { moveToNextStepIfNeeded() } .launchIn(lifecycleScope) } @@ -99,7 +107,7 @@ class FtueFlowNode @AssistedInject constructor( NavTarget.SessionVerification -> { val callback = object : FtueSessionVerificationFlowNode.Callback { override fun onDone() { - lifecycleScope.launch { moveToNextStep() } + moveToNextStepIfNeeded() } } createNode(buildContext, listOf(callback)) @@ -107,7 +115,7 @@ class FtueFlowNode @AssistedInject constructor( NavTarget.NotificationsOptIn -> { val callback = object : NotificationsOptInNode.Callback { override fun onNotificationsOptInFinished() { - lifecycleScope.launch { moveToNextStep() } + moveToNextStepIfNeeded() } } createNode(buildContext, listOf(callback)) @@ -118,7 +126,7 @@ class FtueFlowNode @AssistedInject constructor( NavTarget.LockScreenSetup -> { val callback = object : LockScreenEntryPoint.Callback { override fun onSetupDone() { - lifecycleScope.launch { moveToNextStep() } + moveToNextStepIfNeeded() } } lockScreenEntryPoint.nodeBuilder(this, buildContext, LockScreenEntryPoint.Target.Setup) @@ -128,8 +136,11 @@ class FtueFlowNode @AssistedInject constructor( } } - private fun moveToNextStep() = lifecycleScope.launch { + private fun moveToNextStepIfNeeded() = lifecycleScope.launch { when (ftueState.getNextStep()) { + FtueStep.WaitingForInitialState -> { + backstack.newRoot(NavTarget.Placeholder) + } FtueStep.SessionVerification -> { backstack.newRoot(NavTarget.SessionVerification) } @@ -155,7 +166,14 @@ class FtueFlowNode @AssistedInject constructor( class PlaceholderNode @AssistedInject constructor( @Assisted buildContext: BuildContext, @Assisted plugins: List, - ) : Node(buildContext, plugins = plugins) + ) : Node(buildContext, plugins = plugins) { + @Composable + override fun View(modifier: Modifier) { + Box(modifier = modifier.fillMaxSize(), contentAlignment = Alignment.Center) { + CircularProgressIndicator() + } + } + } } private class NoOpBackstackHandlerStrategy : BaseBackPressHandlerStrategy() { diff --git a/features/ftue/impl/src/main/kotlin/io/element/android/features/ftue/impl/state/DefaultFtueService.kt b/features/ftue/impl/src/main/kotlin/io/element/android/features/ftue/impl/state/DefaultFtueService.kt index b530769abf..924259b9d2 100644 --- a/features/ftue/impl/src/main/kotlin/io/element/android/features/ftue/impl/state/DefaultFtueService.kt +++ b/features/ftue/impl/src/main/kotlin/io/element/android/features/ftue/impl/state/DefaultFtueService.kt @@ -24,18 +24,14 @@ import io.element.android.libraries.preferences.api.store.SessionPreferencesStor import io.element.android.services.analytics.api.AnalyticsService import io.element.android.services.toolbox.api.sdk.BuildVersionSdkIntProvider import kotlinx.coroutines.CoroutineScope -import kotlinx.coroutines.FlowPreview import kotlinx.coroutines.flow.MutableStateFlow -import kotlinx.coroutines.flow.catch import kotlinx.coroutines.flow.distinctUntilChanged import kotlinx.coroutines.flow.filter import kotlinx.coroutines.flow.first import kotlinx.coroutines.flow.launchIn +import kotlinx.coroutines.flow.map import kotlinx.coroutines.flow.onEach -import kotlinx.coroutines.flow.timeout -import timber.log.Timber import javax.inject.Inject -import kotlin.time.Duration.Companion.seconds @ContributesBinding(SessionScope::class) @SingleIn(SessionScope::class) @@ -50,6 +46,14 @@ class DefaultFtueService @Inject constructor( ) : FtueService { override val state = MutableStateFlow(FtueState.Unknown) + /** + * This flow emits true when the FTUE flow is ready to be displayed. + * In this case, the FTUE flow is ready when the session verification status is known. + */ + val isVerificationStatusKnown = sessionVerificationService.sessionVerifiedStatus + .map { it != SessionVerifiedStatus.Unknown } + .distinctUntilChanged() + override suspend fun reset() { analyticsService.reset() if (sdkVersionProvider.isAtLeast(Build.VERSION_CODES.TIRAMISU)) { @@ -70,7 +74,12 @@ class DefaultFtueService @Inject constructor( suspend fun getNextStep(currentStep: FtueStep? = null): FtueStep? = when (currentStep) { - null -> if (isSessionNotVerified()) { + null -> if (!isSessionVerificationStateReady()) { + FtueStep.WaitingForInitialState + } else { + getNextStep(FtueStep.WaitingForInitialState) + } + FtueStep.WaitingForInitialState -> if (isSessionNotVerified()) { FtueStep.SessionVerification } else { getNextStep(FtueStep.SessionVerification) @@ -90,34 +99,18 @@ class DefaultFtueService @Inject constructor( } else { getNextStep(FtueStep.AnalyticsOptIn) } - FtueStep.AnalyticsOptIn -> { - updateState() - null - } + FtueStep.AnalyticsOptIn -> null } - private suspend fun isAnyStepIncomplete(): Boolean { - return listOf Boolean>( - { isSessionNotVerified() }, - { shouldAskNotificationPermissions() }, - { needsAnalyticsOptIn() }, - { shouldDisplayLockscreenSetup() }, - ).any { it() } + private fun isSessionVerificationStateReady(): Boolean { + return sessionVerificationService.sessionVerifiedStatus.value != SessionVerifiedStatus.Unknown } - @OptIn(FlowPreview::class) private suspend fun isSessionNotVerified(): Boolean { - // Wait for the first known (or ready) verification status - val readyVerifiedSessionStatus = sessionVerificationService.sessionVerifiedStatus - .filter { it != SessionVerifiedStatus.Unknown } - // This is not ideal, but there are some very rare cases when reading the flow seems to get stuck - .timeout(5.seconds) - .catch { - Timber.e(it, "Failed to get session verification status, assume it's not verified") - emit(SessionVerifiedStatus.NotVerified) - } - .first() - return readyVerifiedSessionStatus == SessionVerifiedStatus.NotVerified && !canSkipVerification() + // Wait until the session verification status is known + isVerificationStatusKnown.filter { it }.first() + + return sessionVerificationService.sessionVerifiedStatus.value == SessionVerifiedStatus.NotVerified && !canSkipVerification() } private suspend fun canSkipVerification(): Boolean { @@ -145,14 +138,17 @@ class DefaultFtueService @Inject constructor( @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) internal suspend fun updateState() { + val nextStep = getNextStep() state.value = when { - isAnyStepIncomplete() -> FtueState.Incomplete - else -> FtueState.Complete + // Final state, there aren't any more next steps + nextStep == null -> FtueState.Complete + else -> FtueState.Incomplete } } } sealed interface FtueStep { + data object WaitingForInitialState : FtueStep data object SessionVerification : FtueStep data object NotificationsOptIn : FtueStep data object AnalyticsOptIn : FtueStep diff --git a/features/ftue/impl/src/test/kotlin/io/element/android/features/ftue/impl/DefaultFtueServiceTest.kt b/features/ftue/impl/src/test/kotlin/io/element/android/features/ftue/impl/DefaultFtueServiceTest.kt index 4788857e21..ca0222398c 100644 --- a/features/ftue/impl/src/test/kotlin/io/element/android/features/ftue/impl/DefaultFtueServiceTest.kt +++ b/features/ftue/impl/src/test/kotlin/io/element/android/features/ftue/impl/DefaultFtueServiceTest.kt @@ -23,6 +23,7 @@ import io.element.android.libraries.permissions.impl.FakePermissionStateProvider import io.element.android.libraries.preferences.api.store.SessionPreferencesStore import io.element.android.libraries.preferences.test.InMemorySessionPreferencesStore import io.element.android.services.analytics.api.AnalyticsService +import io.element.android.services.analytics.noop.NoopAnalyticsService import io.element.android.services.analytics.test.FakeAnalyticsService import io.element.android.services.toolbox.test.sdk.FakeBuildVersionSdkIntProvider import io.element.android.tests.testutils.lambda.lambdaRecorder @@ -73,6 +74,27 @@ class DefaultFtueServiceTest { assertThat(service.state.value).isEqualTo(FtueState.Complete) } + @Test + fun `given all checks being true with no analytics, FtueState is Complete`() = runTest { + val analyticsService = NoopAnalyticsService() + val sessionVerificationService = FakeSessionVerificationService() + val permissionStateProvider = FakePermissionStateProvider(permissionGranted = true) + val lockScreenService = FakeLockScreenService() + val service = createDefaultFtueService( + sessionVerificationService = sessionVerificationService, + analyticsService = analyticsService, + permissionStateProvider = permissionStateProvider, + lockScreenService = lockScreenService, + ) + + sessionVerificationService.emitVerifiedStatus(SessionVerifiedStatus.Verified) + permissionStateProvider.setPermissionGranted() + lockScreenService.setIsPinSetup(true) + service.updateState() + + assertThat(service.state.value).isEqualTo(FtueState.Complete) + } + @Test fun `traverse flow`() = runTest { val sessionVerificationService = FakeSessionVerificationService().apply { diff --git a/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/auth/MatrixAuthenticationService.kt b/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/auth/MatrixAuthenticationService.kt index b7b44ba45e..019bd0d8f0 100644 --- a/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/auth/MatrixAuthenticationService.kt +++ b/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/auth/MatrixAuthenticationService.kt @@ -56,4 +56,7 @@ interface MatrixAuthenticationService { suspend fun loginWithOidc(callbackUrl: String): Result suspend fun loginWithQrCode(qrCodeData: MatrixQrCodeLoginData, progress: (QrCodeLoginStep) -> Unit): Result + + /** Listen to new Matrix clients being created on authentication. */ + fun listenToNewMatrixClients(lambda: (MatrixClient) -> Unit) } diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/RustMatrixClientFactory.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/RustMatrixClientFactory.kt index c22cb84c4d..e22eda03f2 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/RustMatrixClientFactory.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/RustMatrixClientFactory.kt @@ -25,6 +25,7 @@ import io.element.android.services.analytics.api.AnalyticsService import io.element.android.services.toolbox.api.systemclock.SystemClock import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.withContext +import org.matrix.rustcomponents.sdk.Client import org.matrix.rustcomponents.sdk.ClientBuilder import org.matrix.rustcomponents.sdk.Session import org.matrix.rustcomponents.sdk.SlidingSyncVersion @@ -51,8 +52,9 @@ class RustMatrixClientFactory @Inject constructor( private val timelineEventTypeFilterFactory: TimelineEventTypeFilterFactory, private val clientBuilderProvider: ClientBuilderProvider, ) { + private val sessionDelegate = RustClientSessionDelegate(sessionStore, appCoroutineScope, coroutineDispatchers) + suspend fun create(sessionData: SessionData): RustMatrixClient = withContext(coroutineDispatchers.io) { - val sessionDelegate = RustClientSessionDelegate(sessionStore, appCoroutineScope, coroutineDispatchers) val client = getBaseClientBuilder( sessionPaths = sessionData.getSessionPaths(), passphrase = sessionData.passphrase, @@ -60,18 +62,21 @@ class RustMatrixClientFactory @Inject constructor( ) .homeserverUrl(sessionData.homeserverUrl) .username(sessionData.userId) - .setSessionDelegate(sessionDelegate) .use { it.build() } client.restoreSession(sessionData.toSession()) + create(client) + } + + suspend fun create(client: Client): RustMatrixClient { + val (anonymizedAccessToken, anonymizedRefreshToken) = client.session().anonymizedTokens() + val syncService = client.syncService() .withUtdHook(UtdTracker(analyticsService)) .finish() - val (anonymizedAccessToken, anonymizedRefreshToken) = sessionData.anonymizedTokens() - - RustMatrixClient( + return RustMatrixClient( client = client, baseDirectory = baseDirectory, sessionStore = sessionStore, @@ -98,6 +103,7 @@ class RustMatrixClientFactory @Inject constructor( dataPath = sessionPaths.fileDirectory.absolutePath, cachePath = sessionPaths.cacheDirectory.absolutePath, ) + .setSessionDelegate(sessionDelegate) .passphrase(passphrase) .userAgent(userAgentProvider.provide()) .addRootCertificates(userCertificatesProvider.provides()) diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/auth/RustMatrixAuthenticationService.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/auth/RustMatrixAuthenticationService.kt index 434b0b2aef..5ea9ce8550 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/auth/RustMatrixAuthenticationService.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/auth/RustMatrixAuthenticationService.kt @@ -51,7 +51,6 @@ import org.matrix.rustcomponents.sdk.QrCodeData import org.matrix.rustcomponents.sdk.QrCodeDecodeException import org.matrix.rustcomponents.sdk.QrLoginProgress import org.matrix.rustcomponents.sdk.QrLoginProgressListener -import org.matrix.rustcomponents.sdk.use import timber.log.Timber import uniffi.matrix_sdk.OidcAuthorizationData import javax.inject.Inject @@ -77,6 +76,11 @@ class RustMatrixAuthenticationService @Inject constructor( private var currentClient: Client? = null private var currentHomeserver = MutableStateFlow(null) + private var newMatrixClientObserver: ((MatrixClient) -> Unit)? = null + override fun listenToNewMatrixClients(lambda: (MatrixClient) -> Unit) { + newMatrixClientObserver = lambda + } + private fun rotateSessionPath(): SessionPaths { sessionPaths?.deleteRecursively() return sessionPathsFactory.create() @@ -155,7 +159,7 @@ class RustMatrixAuthenticationService @Inject constructor( passphrase = pendingPassphrase, sessionPaths = currentSessionPaths, ) - clear() + newMatrixClientObserver?.invoke(rustMatrixClientFactory.create(client)) sessionStore.storeData(sessionData) SessionId(sessionData.userId) }.mapFailure { failure -> @@ -226,9 +230,9 @@ class RustMatrixAuthenticationService @Inject constructor( passphrase = pendingPassphrase, sessionPaths = currentSessionPaths, ) - clear() pendingOidcAuthorizationData?.close() pendingOidcAuthorizationData = null + newMatrixClientObserver?.invoke(rustMatrixClientFactory.create(client)) sessionStore.storeData(sessionData) SessionId(sessionData.userId) }.mapFailure { failure -> @@ -256,15 +260,14 @@ class RustMatrixAuthenticationService @Inject constructor( oidcConfiguration = oidcConfiguration, progressListener = progressListener, ) - val sessionData = client.use { rustClient -> - rustClient.session() - .toSessionData( - isTokenValid = true, - loginType = LoginType.QR, - passphrase = pendingPassphrase, - sessionPaths = emptySessionPaths, - ) - } + val sessionData = client.session() + .toSessionData( + isTokenValid = true, + loginType = LoginType.QR, + passphrase = pendingPassphrase, + sessionPaths = emptySessionPaths, + ) + newMatrixClientObserver?.invoke(rustMatrixClientFactory.create(client)) sessionStore.storeData(sessionData) SessionId(sessionData.userId) }.mapFailure { diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/verification/RustSessionVerificationService.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/verification/RustSessionVerificationService.kt index 76fb3d9b17..ed3ddec98b 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/verification/RustSessionVerificationService.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/verification/RustSessionVerificationService.kt @@ -18,15 +18,13 @@ import io.element.android.libraries.matrix.api.verification.VerificationFlowStat import io.element.android.libraries.matrix.impl.util.cancelAndDestroy import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.NonCancellable -import kotlinx.coroutines.delay import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.SharingStarted import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.asStateFlow -import kotlinx.coroutines.flow.launchIn +import kotlinx.coroutines.flow.first import kotlinx.coroutines.flow.map -import kotlinx.coroutines.flow.onEach import kotlinx.coroutines.flow.stateIn import kotlinx.coroutines.launch import kotlinx.coroutines.sync.Mutex @@ -61,11 +59,13 @@ class RustSessionVerificationService( private val _sessionVerifiedStatus = MutableStateFlow(SessionVerifiedStatus.Unknown) override val sessionVerifiedStatus: StateFlow = _sessionVerifiedStatus.asStateFlow() + private val recoveryState = MutableStateFlow(RecoveryState.UNKNOWN) + // Listen for changes in verification status and update accordingly private val verificationStateListenerTaskHandle = encryptionService.verificationStateListener(object : VerificationStateListener { override fun onUpdate(status: VerificationState) { Timber.d("New verification state: $status") - sessionCoroutineScope.launch { updateVerificationStatus() } + _sessionVerifiedStatus.value = status.map() } }) @@ -74,7 +74,7 @@ class RustSessionVerificationService( override fun onUpdate(status: RecoveryState) { Timber.d("New recovery state: $status") // We could check the `RecoveryState`, but it's easier to just use the verification state directly - sessionCoroutineScope.launch { updateVerificationStatus() } + recoveryState.value = status } }) @@ -88,22 +88,7 @@ class RustSessionVerificationService( verificationStatus == SessionVerifiedStatus.NotVerified } - init { - // Update initial state in case sliding sync isn't ready - sessionCoroutineScope.launch { updateVerificationStatus() } - - isReady.onEach { isReady -> - if (isReady) { - Timber.d("Starting verification service") - // Immediate status update - updateVerificationStatus() - } else { - Timber.d("Stopping verification service") - updateVerificationStatus() - } - } - .launchIn(sessionCoroutineScope) - } + private var isOwnVerification = true override fun didReceiveVerificationRequest(details: RustSessionVerificationRequestDetails) { listener?.onIncomingSessionRequest(details.map()) @@ -135,6 +120,8 @@ class RustSessionVerificationService( } override suspend fun acknowledgeVerificationRequest(details: SessionVerificationRequestDetails) = tryOrFail { + isOwnVerification = false + initVerificationControllerIfNeeded() verificationController.acknowledgeVerificationRequest( senderId = details.senderId.value, flowId = details.flowId.value, @@ -179,14 +166,22 @@ class RustSessionVerificationService( // Ideally this should be `verificationController?.isVerified().orFalse()` but for some reason it returns false if run immediately // It also sometimes unexpectedly fails to report the session as verified, so we have to handle that possibility and fail if needed runCatching { - withTimeout(30.seconds) { - while (encryptionService.verificationState() != VerificationState.VERIFIED) { - delay(100) - } + withTimeout(20.seconds) { + // Wait until the SDK reports the state as verified + sessionVerifiedStatus.first { it == SessionVerifiedStatus.Verified } } } .onSuccess { - // Order here is important, first set the flow state as finished, then update the verification status + if (isOwnVerification) { + // Try waiting for the final recovery state for better UX, but don't block the verification state on it + tryOrNull { + withTimeout(10.seconds) { + // Wait until the recovery state is either fully loaded or we check it's explicitly disabled + recoveryState.first { it == RecoveryState.ENABLED || it == RecoveryState.DISABLED } + } + } + } + _verificationFlowState.value = VerificationFlowState.DidFinish updateVerificationStatus() } @@ -209,6 +204,7 @@ class RustSessionVerificationService( // end-region override suspend fun reset(cancelAnyPendingVerificationAttempt: Boolean) { + isOwnVerification = true if (isReady.value && cancelAnyPendingVerificationAttempt) { // Cancel any pending verification attempt tryOrNull { verificationController.cancelVerification() } @@ -237,37 +233,20 @@ class RustSessionVerificationService( } } - private suspend fun updateVerificationStatus() { - if (verificationFlowState.value == VerificationFlowState.DidFinish) { - // Calling `encryptionService.verificationState()` performs a network call and it will deadlock if there is no network - // So we need to check that *only* if we know there is network connection, which is the case when the verification flow just finished - Timber.d("Updating verification status: flow just finished") - runCatching { - encryptionService.waitForE2eeInitializationTasks() - }.onSuccess { - _sessionVerifiedStatus.value = when (encryptionService.verificationState()) { - VerificationState.UNKNOWN -> SessionVerifiedStatus.Unknown - VerificationState.VERIFIED -> SessionVerifiedStatus.Verified - VerificationState.UNVERIFIED -> SessionVerifiedStatus.NotVerified - } - Timber.d("New verification status: ${_sessionVerifiedStatus.value}") - } - } else { - // Otherwise, just check the current verification status from the session verification controller instead - Timber.d("Updating verification status: flow is pending or was finished some time ago") - runCatching { - initVerificationControllerIfNeeded() - _sessionVerifiedStatus.value = if (encryptionService.verificationState() == VerificationState.VERIFIED) { - SessionVerifiedStatus.Verified - } else { - SessionVerifiedStatus.NotVerified - } - Timber.d("New verification status: ${_sessionVerifiedStatus.value}") - } + private fun updateVerificationStatus() { + runCatching { + _sessionVerifiedStatus.value = encryptionService.verificationState().map() + Timber.d("New verification status: ${_sessionVerifiedStatus.value}") } } } +private fun VerificationState.map() = when (this) { + VerificationState.UNKNOWN -> SessionVerifiedStatus.Unknown + VerificationState.VERIFIED -> SessionVerifiedStatus.Verified + VerificationState.UNVERIFIED -> SessionVerifiedStatus.NotVerified +} + private fun RustSessionVerificationData.map(): SessionVerificationData { return use { sessionVerificationData -> when (sessionVerificationData) { diff --git a/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/auth/FakeMatrixAuthenticationService.kt b/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/auth/FakeMatrixAuthenticationService.kt index 2846542890..c8c8273275 100644 --- a/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/auth/FakeMatrixAuthenticationService.kt +++ b/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/auth/FakeMatrixAuthenticationService.kt @@ -18,6 +18,7 @@ import io.element.android.libraries.matrix.api.auth.qrlogin.QrCodeLoginStep import io.element.android.libraries.matrix.api.core.SessionId import io.element.android.libraries.matrix.test.A_SESSION_ID import io.element.android.libraries.matrix.test.A_USER_ID +import io.element.android.libraries.matrix.test.FakeMatrixClient import io.element.android.libraries.sessionstorage.api.LoggedInState import io.element.android.tests.testutils.lambda.lambdaError import io.element.android.tests.testutils.lambda.lambdaRecorder @@ -41,6 +42,7 @@ class FakeMatrixAuthenticationService( private var loginError: Throwable? = null private var changeServerError: Throwable? = null private var matrixClient: MatrixClient? = null + private var onAuthenticationListener: ((MatrixClient) -> Unit)? = null var getLatestSessionIdLambda: (() -> SessionId?) = { null } @@ -55,6 +57,7 @@ class FakeMatrixAuthenticationService( return it.invoke(sessionId) } return if (matrixClient != null) { + onAuthenticationListener?.invoke(matrixClient!!) Result.success(matrixClient!!) } else { Result.failure(IllegalStateException()) @@ -74,7 +77,10 @@ class FakeMatrixAuthenticationService( } override suspend fun login(username: String, password: String): Result = simulateLongTask { - loginError?.let { Result.failure(it) } ?: Result.success(A_USER_ID) + loginError?.let { Result.failure(it) } ?: run { + onAuthenticationListener?.invoke(matrixClient ?: FakeMatrixClient()) + Result.success(A_USER_ID) + } } override suspend fun importCreatedSession(externalSession: ExternalSession): Result = simulateLongTask { @@ -90,13 +96,21 @@ class FakeMatrixAuthenticationService( } override suspend fun loginWithOidc(callbackUrl: String): Result = simulateLongTask { - loginError?.let { Result.failure(it) } ?: Result.success(A_USER_ID) + loginError?.let { Result.failure(it) } ?: run { + onAuthenticationListener?.invoke(matrixClient ?: FakeMatrixClient()) + Result.success(A_USER_ID) + } } override suspend fun loginWithQrCode(qrCodeData: MatrixQrCodeLoginData, progress: (QrCodeLoginStep) -> Unit): Result = simulateLongTask { + onAuthenticationListener?.invoke(matrixClient ?: FakeMatrixClient()) loginWithQrCodeResult(qrCodeData, progress) } + override fun listenToNewMatrixClients(lambda: (MatrixClient) -> Unit) { + onAuthenticationListener = lambda + } + fun givenOidcError(throwable: Throwable?) { oidcError = throwable }