From 35768ff7e8171efbe2d8ad03bea2337ff33b663c Mon Sep 17 00:00:00 2001 From: Dominic Fischer Date: Fri, 6 Nov 2020 16:49:51 +0000 Subject: [PATCH 1/7] Convert `AccountService` to suspend functions Signed-off-by: Dominic Fischer --- .../android/sdk/account/ChangePasswordTest.kt | 6 ++-- .../sdk/account/DeactivateAccountTest.kt | 4 +-- .../android/sdk/common/CommonTestHelper.kt | 10 +++++++ .../sdk/api/session/account/AccountService.kt | 6 ++-- .../session/account/DefaultAccountService.kt | 23 ++++----------- .../settings/VectorSettingsGeneralFragment.kt | 28 +++++++++---------- .../DeactivateAccountViewModel.kt | 23 +++++++++------ 7 files changed, 51 insertions(+), 49 deletions(-) diff --git a/matrix-sdk-android/src/androidTest/java/org/matrix/android/sdk/account/ChangePasswordTest.kt b/matrix-sdk-android/src/androidTest/java/org/matrix/android/sdk/account/ChangePasswordTest.kt index ec5477f976..6ae72aeb3a 100644 --- a/matrix-sdk-android/src/androidTest/java/org/matrix/android/sdk/account/ChangePasswordTest.kt +++ b/matrix-sdk-android/src/androidTest/java/org/matrix/android/sdk/account/ChangePasswordTest.kt @@ -16,6 +16,8 @@ package org.matrix.android.sdk.account +import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.withTimeout import org.matrix.android.sdk.InstrumentedTest import org.matrix.android.sdk.api.failure.isInvalidPassword import org.matrix.android.sdk.common.CommonTestHelper @@ -43,8 +45,8 @@ class ChangePasswordTest : InstrumentedTest { val session = commonTestHelper.createAccount(TestConstants.USER_ALICE, SessionTestParams(withInitialSync = false)) // Change password - commonTestHelper.doSync { - session.changePassword(TestConstants.PASSWORD, NEW_PASSWORD, it) + commonTestHelper.runBlockingTest { + session.changePassword(TestConstants.PASSWORD, NEW_PASSWORD) } // Try to login with the previous password, it will fail diff --git a/matrix-sdk-android/src/androidTest/java/org/matrix/android/sdk/account/DeactivateAccountTest.kt b/matrix-sdk-android/src/androidTest/java/org/matrix/android/sdk/account/DeactivateAccountTest.kt index a6fbfd9b7a..9996eef0a8 100644 --- a/matrix-sdk-android/src/androidTest/java/org/matrix/android/sdk/account/DeactivateAccountTest.kt +++ b/matrix-sdk-android/src/androidTest/java/org/matrix/android/sdk/account/DeactivateAccountTest.kt @@ -43,8 +43,8 @@ class DeactivateAccountTest : InstrumentedTest { val session = commonTestHelper.createAccount(TestConstants.USER_ALICE, SessionTestParams(withInitialSync = false)) // Deactivate the account - commonTestHelper.doSync { - session.deactivateAccount(TestConstants.PASSWORD, false, it) + commonTestHelper.runBlockingTest { + session.deactivateAccount(TestConstants.PASSWORD, false) } // Try to login on the previous account, it will fail (M_USER_DEACTIVATED) diff --git a/matrix-sdk-android/src/androidTest/java/org/matrix/android/sdk/common/CommonTestHelper.kt b/matrix-sdk-android/src/androidTest/java/org/matrix/android/sdk/common/CommonTestHelper.kt index cbe4cca8a3..660a3cc5e3 100644 --- a/matrix-sdk-android/src/androidTest/java/org/matrix/android/sdk/common/CommonTestHelper.kt +++ b/matrix-sdk-android/src/androidTest/java/org/matrix/android/sdk/common/CommonTestHelper.kt @@ -40,6 +40,7 @@ import kotlinx.coroutines.GlobalScope import kotlinx.coroutines.delay import kotlinx.coroutines.launch import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.withTimeout import org.junit.Assert.assertEquals import org.junit.Assert.assertNotNull import org.junit.Assert.assertTrue @@ -343,6 +344,15 @@ class CommonTestHelper(context: Context) { await(latch, timeout) } + // Transform a method with a MatrixCallback to a synchronous method + fun runBlockingTest(timeout: Long = TestConstants.timeOutMillis, block: suspend () -> T): T { + return runBlocking { + withTimeout(timeout) { + block() + } + } + } + // Transform a method with a MatrixCallback to a synchronous method inline fun doSync(timeout: Long? = TestConstants.timeOutMillis, block: (MatrixCallback) -> Unit): T { val lock = CountDownLatch(1) diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/account/AccountService.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/account/AccountService.kt index b05f0036b2..1913da7c78 100644 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/account/AccountService.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/account/AccountService.kt @@ -16,8 +16,6 @@ package org.matrix.android.sdk.api.session.account -import org.matrix.android.sdk.api.MatrixCallback -import org.matrix.android.sdk.api.util.Cancelable /** * This interface defines methods to manage the account. It's implemented at the session level. @@ -28,7 +26,7 @@ interface AccountService { * @param password Current password. * @param newPassword New password */ - fun changePassword(password: String, newPassword: String, callback: MatrixCallback): Cancelable + suspend fun changePassword(password: String, newPassword: String) /** * Deactivate the account. @@ -46,5 +44,5 @@ interface AccountService { * @param eraseAllData set to true to forget all messages that have been sent. Warning: this will cause future users to see * an incomplete view of conversations */ - fun deactivateAccount(password: String, eraseAllData: Boolean, callback: MatrixCallback): Cancelable + suspend fun deactivateAccount(password: String, eraseAllData: Boolean) } diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/account/DefaultAccountService.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/account/DefaultAccountService.kt index 31a39d45e5..1165d2116b 100644 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/account/DefaultAccountService.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/account/DefaultAccountService.kt @@ -16,30 +16,17 @@ package org.matrix.android.sdk.internal.session.account -import org.matrix.android.sdk.api.MatrixCallback import org.matrix.android.sdk.api.session.account.AccountService -import org.matrix.android.sdk.api.util.Cancelable -import org.matrix.android.sdk.internal.task.TaskExecutor -import org.matrix.android.sdk.internal.task.configureWith import javax.inject.Inject internal class DefaultAccountService @Inject constructor(private val changePasswordTask: ChangePasswordTask, - private val deactivateAccountTask: DeactivateAccountTask, - private val taskExecutor: TaskExecutor) : AccountService { + private val deactivateAccountTask: DeactivateAccountTask) : AccountService { - override fun changePassword(password: String, newPassword: String, callback: MatrixCallback): Cancelable { - return changePasswordTask - .configureWith(ChangePasswordTask.Params(password, newPassword)) { - this.callback = callback - } - .executeBy(taskExecutor) + override suspend fun changePassword(password: String, newPassword: String) { + changePasswordTask.execute(ChangePasswordTask.Params(password, newPassword)) } - override fun deactivateAccount(password: String, eraseAllData: Boolean, callback: MatrixCallback): Cancelable { - return deactivateAccountTask - .configureWith(DeactivateAccountTask.Params(password, eraseAllData)) { - this.callback = callback - } - .executeBy(taskExecutor) + override suspend fun deactivateAccount(password: String, eraseAllData: Boolean) { + deactivateAccountTask.execute(DeactivateAccountTask.Params(password, eraseAllData)) } } diff --git a/vector/src/main/java/im/vector/app/features/settings/VectorSettingsGeneralFragment.kt b/vector/src/main/java/im/vector/app/features/settings/VectorSettingsGeneralFragment.kt index d8171bd30d..b1ccabfb76 100644 --- a/vector/src/main/java/im/vector/app/features/settings/VectorSettingsGeneralFragment.kt +++ b/vector/src/main/java/im/vector/app/features/settings/VectorSettingsGeneralFragment.kt @@ -27,6 +27,7 @@ import android.widget.ImageView import android.widget.Toast import androidx.appcompat.app.AlertDialog import androidx.core.view.isVisible +import androidx.lifecycle.lifecycleScope import androidx.preference.EditTextPreference import androidx.preference.Preference import androidx.preference.PreferenceCategory @@ -451,28 +452,25 @@ class VectorSettingsGeneralFragment @Inject constructor( val newPwd = newPasswordText.text.toString() showPasswordLoadingView(true) - session.changePassword(oldPwd, newPwd, object : MatrixCallback { - override fun onSuccess(data: Unit) { - if (!isAdded) { - return - } - showPasswordLoadingView(false) + lifecycleScope.launch { + val result = runCatching { + session.changePassword(oldPwd, newPwd) + } + if (!isAdded) { + return@launch + } + showPasswordLoadingView(false) + result.fold({ dialog.dismiss() activity.toast(R.string.settings_password_updated) - } - - override fun onFailure(failure: Throwable) { - if (!isAdded) { - return - } - showPasswordLoadingView(false) + }, { failure -> if (failure.isInvalidPassword()) { oldPasswordTil.error = getString(R.string.settings_fail_to_update_password_invalid_current_password) } else { oldPasswordTil.error = getString(R.string.settings_fail_to_update_password) } - } - }) + }) + } } } dialog.show() diff --git a/vector/src/main/java/im/vector/app/features/settings/account/deactivation/DeactivateAccountViewModel.kt b/vector/src/main/java/im/vector/app/features/settings/account/deactivation/DeactivateAccountViewModel.kt index e08147d54f..f264f430ad 100644 --- a/vector/src/main/java/im/vector/app/features/settings/account/deactivation/DeactivateAccountViewModel.kt +++ b/vector/src/main/java/im/vector/app/features/settings/account/deactivation/DeactivateAccountViewModel.kt @@ -15,6 +15,7 @@ */ package im.vector.app.features.settings.account.deactivation +import androidx.lifecycle.viewModelScope import com.airbnb.mvrx.FragmentViewModelContext import com.airbnb.mvrx.MvRxState import com.airbnb.mvrx.MvRxViewModelFactory @@ -24,9 +25,12 @@ import com.squareup.inject.assisted.AssistedInject import im.vector.app.core.extensions.exhaustive import im.vector.app.core.platform.VectorViewModel import im.vector.app.core.platform.VectorViewModelAction +import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.launch import org.matrix.android.sdk.api.MatrixCallback import org.matrix.android.sdk.api.failure.isInvalidPassword import org.matrix.android.sdk.api.session.Session +import java.lang.Exception data class DeactivateAccountViewState( val passwordShown: Boolean = false @@ -67,19 +71,22 @@ class DeactivateAccountViewModel @AssistedInject constructor(@Assisted private v _viewEvents.post(DeactivateAccountViewEvents.Loading()) - session.deactivateAccount(action.password, action.eraseAllData, object : MatrixCallback { - override fun onSuccess(data: Unit) { - _viewEvents.post(DeactivateAccountViewEvents.Done) - } + viewModelScope.launch { + val event = try { + session.deactivateAccount(action.password, action.eraseAllData) + DeactivateAccountViewEvents.Done + } catch (failure: Exception) { + if (failure is CancellationException) throw failure - override fun onFailure(failure: Throwable) { if (failure.isInvalidPassword()) { - _viewEvents.post(DeactivateAccountViewEvents.InvalidPassword) + DeactivateAccountViewEvents.InvalidPassword } else { - _viewEvents.post(DeactivateAccountViewEvents.OtherFailure(failure)) + DeactivateAccountViewEvents.OtherFailure(failure) } } - }) + + _viewEvents.post(event) + } } companion object : MvRxViewModelFactory { From 983e02888caf53415a2635221c9a8d67ca1047bd Mon Sep 17 00:00:00 2001 From: Dominic Fischer Date: Fri, 6 Nov 2020 19:17:45 +0000 Subject: [PATCH 2/7] Remove incorrect comment Signed-off-by: Dominic Fischer --- .../java/org/matrix/android/sdk/common/CommonTestHelper.kt | 1 - 1 file changed, 1 deletion(-) diff --git a/matrix-sdk-android/src/androidTest/java/org/matrix/android/sdk/common/CommonTestHelper.kt b/matrix-sdk-android/src/androidTest/java/org/matrix/android/sdk/common/CommonTestHelper.kt index 660a3cc5e3..0e7088a6a5 100644 --- a/matrix-sdk-android/src/androidTest/java/org/matrix/android/sdk/common/CommonTestHelper.kt +++ b/matrix-sdk-android/src/androidTest/java/org/matrix/android/sdk/common/CommonTestHelper.kt @@ -344,7 +344,6 @@ class CommonTestHelper(context: Context) { await(latch, timeout) } - // Transform a method with a MatrixCallback to a synchronous method fun runBlockingTest(timeout: Long = TestConstants.timeOutMillis, block: suspend () -> T): T { return runBlocking { withTimeout(timeout) { From 0da4ff7b029b895d6b0c04c5e99970ae0eee8da1 Mon Sep 17 00:00:00 2001 From: Dominic Fischer Date: Fri, 6 Nov 2020 19:20:23 +0000 Subject: [PATCH 3/7] Run ktlint --- .../java/org/matrix/android/sdk/account/ChangePasswordTest.kt | 2 -- .../matrix/android/sdk/api/session/account/AccountService.kt | 1 - .../settings/account/deactivation/DeactivateAccountViewModel.kt | 1 - 3 files changed, 4 deletions(-) diff --git a/matrix-sdk-android/src/androidTest/java/org/matrix/android/sdk/account/ChangePasswordTest.kt b/matrix-sdk-android/src/androidTest/java/org/matrix/android/sdk/account/ChangePasswordTest.kt index 6ae72aeb3a..103b638c39 100644 --- a/matrix-sdk-android/src/androidTest/java/org/matrix/android/sdk/account/ChangePasswordTest.kt +++ b/matrix-sdk-android/src/androidTest/java/org/matrix/android/sdk/account/ChangePasswordTest.kt @@ -16,8 +16,6 @@ package org.matrix.android.sdk.account -import kotlinx.coroutines.runBlocking -import kotlinx.coroutines.withTimeout import org.matrix.android.sdk.InstrumentedTest import org.matrix.android.sdk.api.failure.isInvalidPassword import org.matrix.android.sdk.common.CommonTestHelper diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/account/AccountService.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/account/AccountService.kt index 1913da7c78..8915202f35 100644 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/account/AccountService.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/account/AccountService.kt @@ -16,7 +16,6 @@ package org.matrix.android.sdk.api.session.account - /** * This interface defines methods to manage the account. It's implemented at the session level. */ diff --git a/vector/src/main/java/im/vector/app/features/settings/account/deactivation/DeactivateAccountViewModel.kt b/vector/src/main/java/im/vector/app/features/settings/account/deactivation/DeactivateAccountViewModel.kt index f264f430ad..672caca556 100644 --- a/vector/src/main/java/im/vector/app/features/settings/account/deactivation/DeactivateAccountViewModel.kt +++ b/vector/src/main/java/im/vector/app/features/settings/account/deactivation/DeactivateAccountViewModel.kt @@ -27,7 +27,6 @@ import im.vector.app.core.platform.VectorViewModel import im.vector.app.core.platform.VectorViewModelAction import kotlinx.coroutines.CancellationException import kotlinx.coroutines.launch -import org.matrix.android.sdk.api.MatrixCallback import org.matrix.android.sdk.api.failure.isInvalidPassword import org.matrix.android.sdk.api.session.Session import java.lang.Exception From 6207aab19dcd0a73d84c2850e77423eb0221523e Mon Sep 17 00:00:00 2001 From: Dominic Fischer Date: Mon, 9 Nov 2020 17:20:23 +0000 Subject: [PATCH 4/7] Remove cancellation handling Signed-off-by: Dominic Fischer --- .../account/deactivation/DeactivateAccountViewModel.kt | 3 --- 1 file changed, 3 deletions(-) diff --git a/vector/src/main/java/im/vector/app/features/settings/account/deactivation/DeactivateAccountViewModel.kt b/vector/src/main/java/im/vector/app/features/settings/account/deactivation/DeactivateAccountViewModel.kt index 672caca556..211559c657 100644 --- a/vector/src/main/java/im/vector/app/features/settings/account/deactivation/DeactivateAccountViewModel.kt +++ b/vector/src/main/java/im/vector/app/features/settings/account/deactivation/DeactivateAccountViewModel.kt @@ -25,7 +25,6 @@ import com.squareup.inject.assisted.AssistedInject import im.vector.app.core.extensions.exhaustive import im.vector.app.core.platform.VectorViewModel import im.vector.app.core.platform.VectorViewModelAction -import kotlinx.coroutines.CancellationException import kotlinx.coroutines.launch import org.matrix.android.sdk.api.failure.isInvalidPassword import org.matrix.android.sdk.api.session.Session @@ -75,8 +74,6 @@ class DeactivateAccountViewModel @AssistedInject constructor(@Assisted private v session.deactivateAccount(action.password, action.eraseAllData) DeactivateAccountViewEvents.Done } catch (failure: Exception) { - if (failure is CancellationException) throw failure - if (failure.isInvalidPassword()) { DeactivateAccountViewEvents.InvalidPassword } else { From cedeea13e63e9f8e8891054c6704098219c39b50 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Mon, 9 Nov 2020 23:00:19 +0100 Subject: [PATCH 5/7] Fix discard change dialog displayed by mistake when avatar has been updated --- CHANGES.md | 3 ++- .../features/roomprofile/settings/RoomSettingsViewModel.kt | 5 ++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index fa30acabd5..71a677beb8 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -8,7 +8,8 @@ Improvements 🙌: - Open an existing DM instead of creating a new one (#2319) Bugfix 🐛: - - Fix issue when updating the avatar of a room + - Fix issue when updating the avatar of a room (new avatar vanishing) + - Discard change dialog displayed by mistake when avatar has been updated Translations 🗣: - diff --git a/vector/src/main/java/im/vector/app/features/roomprofile/settings/RoomSettingsViewModel.kt b/vector/src/main/java/im/vector/app/features/roomprofile/settings/RoomSettingsViewModel.kt index 6bf88e755e..4e540f867e 100644 --- a/vector/src/main/java/im/vector/app/features/roomprofile/settings/RoomSettingsViewModel.kt +++ b/vector/src/main/java/im/vector/app/features/roomprofile/settings/RoomSettingsViewModel.kt @@ -211,7 +211,10 @@ class RoomSettingsViewModel @AssistedInject constructor(@Assisted initialState: postLoading(false) setState { deletePendingAvatar(this) - copy(newHistoryVisibility = null) + copy( + avatarAction = RoomSettingsViewState.AvatarAction.None, + newHistoryVisibility = null + ) } _viewEvents.post(RoomSettingsViewEvents.Success) }, From 13ddc28d059fd9a5e7d4562281f6c84cc8ab5342 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Mon, 9 Nov 2020 23:07:53 +0100 Subject: [PATCH 6/7] Fix issue with callback.onSuccess() called multiple times --- .../session/room/state/DefaultStateService.kt | 29 ++++++++++--------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/state/DefaultStateService.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/state/DefaultStateService.kt index 65d375e176..3463b26c8a 100644 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/state/DefaultStateService.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/state/DefaultStateService.kt @@ -35,6 +35,7 @@ import org.matrix.android.sdk.internal.task.TaskExecutor import org.matrix.android.sdk.internal.task.configureWith import org.matrix.android.sdk.internal.task.launchToCallback import org.matrix.android.sdk.internal.util.MatrixCoroutineDispatchers +import org.matrix.android.sdk.internal.util.awaitCallback internal class DefaultStateService @AssistedInject constructor(@Assisted private val roomId: String, private val stateEventDataSource: StateEventDataSource, @@ -132,23 +133,23 @@ internal class DefaultStateService @AssistedInject constructor(@Assisted private override fun updateAvatar(avatarUri: Uri, fileName: String, callback: MatrixCallback): Cancelable { return taskExecutor.executorScope.launchToCallback(coroutineDispatchers.main, callback) { val response = fileUploader.uploadFromUri(avatarUri, fileName, "image/jpeg") - sendStateEvent( - eventType = EventType.STATE_ROOM_AVATAR, - body = mapOf("url" to response.contentUri), - callback = callback, - stateKey = null - ) + awaitCallback { + sendStateEvent( + eventType = EventType.STATE_ROOM_AVATAR, + body = mapOf("url" to response.contentUri), + callback = it, + stateKey = null + ) + } } } override fun deleteAvatar(callback: MatrixCallback): Cancelable { - return taskExecutor.executorScope.launchToCallback(coroutineDispatchers.main, callback) { - sendStateEvent( - eventType = EventType.STATE_ROOM_AVATAR, - body = emptyMap(), - callback = callback, - stateKey = null - ) - } + return sendStateEvent( + eventType = EventType.STATE_ROOM_AVATAR, + body = emptyMap(), + callback = callback, + stateKey = null + ) } } From dc17e5c3fa215f7350498a134a694a4232ed1e60 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Tue, 10 Nov 2020 10:39:34 +0100 Subject: [PATCH 7/7] Update CHANGES.md after merge of #2354 --- CHANGES.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGES.md b/CHANGES.md index 71a677beb8..26418c75f1 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -15,7 +15,7 @@ Translations 🗣: - SDK API changes ⚠️: - - + - AccountService now exposes suspendable function instead of using MatrixCallback (#2354). Note: We will incrementally migrate all the SDK API in a near future. Build 🧱: -