From 4dff3e9cce8d52246bd006cfdb9ac4486b5a67c5 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 12 Jun 2024 11:38:24 +0200 Subject: [PATCH 1/5] Ensure that the Loading Dialog and the toggles update at the same time. --- .../NotificationSettingsPresenter.kt | 29 +++++++++++-------- ...EditDefaultNotificationSettingPresenter.kt | 20 ++++++++----- .../libraries/architecture/AsyncAction.kt | 18 ++++++++++++ 3 files changed, 48 insertions(+), 19 deletions(-) diff --git a/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/notifications/NotificationSettingsPresenter.kt b/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/notifications/NotificationSettingsPresenter.kt index 5cfc14545e8..b14dc53b56a 100644 --- a/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/notifications/NotificationSettingsPresenter.kt +++ b/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/notifications/NotificationSettingsPresenter.kt @@ -29,7 +29,8 @@ import androidx.compose.runtime.setValue import io.element.android.libraries.architecture.AsyncAction import io.element.android.libraries.architecture.AsyncData import io.element.android.libraries.architecture.Presenter -import io.element.android.libraries.architecture.runCatchingUpdatingState +import io.element.android.libraries.architecture.runUpdatingState +import io.element.android.libraries.architecture.runUpdatingStateNoSuccess import io.element.android.libraries.matrix.api.MatrixClient import io.element.android.libraries.matrix.api.notificationsettings.NotificationSettingsService import io.element.android.libraries.matrix.api.room.RoomNotificationMode @@ -74,7 +75,7 @@ class NotificationSettingsPresenter @Inject constructor( LaunchedEffect(Unit) { fetchSettings(matrixSettings) - observeNotificationSettings(matrixSettings) + observeNotificationSettings(matrixSettings, changeNotificationSettingAction) } // List of PushProvider -> Distributor @@ -172,11 +173,15 @@ class NotificationSettingsPresenter @Inject constructor( } @OptIn(FlowPreview::class) - private fun CoroutineScope.observeNotificationSettings(target: MutableState) { + private fun CoroutineScope.observeNotificationSettings( + target: MutableState, + changeNotificationSettingAction: MutableState>, + ) { notificationSettingsService.notificationSettingsChangeFlow .debounce(0.5.seconds) .onEach { fetchSettings(target) + changeNotificationSettingAction.value = AsyncAction.Uninitialized } .launchIn(this) } @@ -238,21 +243,21 @@ class NotificationSettingsPresenter @Inject constructor( } private fun CoroutineScope.setAtRoomNotificationsEnabled(enabled: Boolean, action: MutableState>) = launch { - suspend { - notificationSettingsService.setRoomMentionEnabled(enabled).getOrThrow() - }.runCatchingUpdatingState(action) + action.runUpdatingStateNoSuccess { + notificationSettingsService.setRoomMentionEnabled(enabled) + } } private fun CoroutineScope.setCallNotificationsEnabled(enabled: Boolean, action: MutableState>) = launch { - suspend { - notificationSettingsService.setCallEnabled(enabled).getOrThrow() - }.runCatchingUpdatingState(action) + action.runUpdatingStateNoSuccess { + notificationSettingsService.setCallEnabled(enabled) + } } private fun CoroutineScope.setInviteForMeNotificationsEnabled(enabled: Boolean, action: MutableState>) = launch { - suspend { - notificationSettingsService.setInviteForMeEnabled(enabled).getOrThrow() - }.runCatchingUpdatingState(action) + action.runUpdatingStateNoSuccess { + notificationSettingsService.setInviteForMeEnabled(enabled) + } } private fun CoroutineScope.setNotificationsEnabled(userPushStore: UserPushStore, enabled: Boolean) = launch { diff --git a/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/notifications/edit/EditDefaultNotificationSettingPresenter.kt b/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/notifications/edit/EditDefaultNotificationSettingPresenter.kt index 04accef4eab..3f1f62aafc5 100644 --- a/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/notifications/edit/EditDefaultNotificationSettingPresenter.kt +++ b/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/notifications/edit/EditDefaultNotificationSettingPresenter.kt @@ -29,7 +29,7 @@ import dagger.assisted.AssistedFactory import dagger.assisted.AssistedInject import io.element.android.libraries.architecture.AsyncAction import io.element.android.libraries.architecture.Presenter -import io.element.android.libraries.architecture.runCatchingUpdatingState +import io.element.android.libraries.architecture.runUpdatingStateNoSuccess import io.element.android.libraries.matrix.api.MatrixClient import io.element.android.libraries.matrix.api.notificationsettings.NotificationSettingsService import io.element.android.libraries.matrix.api.room.RoomNotificationMode @@ -73,7 +73,7 @@ class EditDefaultNotificationSettingPresenter @AssistedInject constructor( val localCoroutineScope = rememberCoroutineScope() LaunchedEffect(Unit) { fetchSettings(mode) - observeNotificationSettings(mode) + observeNotificationSettings(mode, changeNotificationSettingAction) observeRoomSummaries(roomsWithUserDefinedMode) displayMentionsOnlyDisclaimer = !notificationSettingsService.canHomeServerPushEncryptedEventsToDevice().getOrDefault(true) } @@ -102,11 +102,15 @@ class EditDefaultNotificationSettingPresenter @AssistedInject constructor( } @OptIn(FlowPreview::class) - private fun CoroutineScope.observeNotificationSettings(mode: MutableState) { + private fun CoroutineScope.observeNotificationSettings( + mode: MutableState, + changeNotificationSettingAction: MutableState>, + ) { notificationSettingsService.notificationSettingsChangeFlow .debounce(0.5.seconds) .onEach { fetchSettings(mode) + changeNotificationSettingAction.value = AsyncAction.Uninitialized } .launchIn(this) } @@ -139,10 +143,12 @@ class EditDefaultNotificationSettingPresenter @AssistedInject constructor( } private fun CoroutineScope.setDefaultNotificationMode(mode: RoomNotificationMode, action: MutableState>) = launch { - suspend { + action.runUpdatingStateNoSuccess { // On modern clients, we don't have different settings for encrypted and non-encrypted rooms (Legacy clients did). - notificationSettingsService.setDefaultRoomNotificationMode(isEncrypted = true, mode = mode, isOneToOne = isOneToOne).getOrThrow() - notificationSettingsService.setDefaultRoomNotificationMode(isEncrypted = false, mode = mode, isOneToOne = isOneToOne).getOrThrow() - }.runCatchingUpdatingState(action) + notificationSettingsService.setDefaultRoomNotificationMode(isEncrypted = true, mode = mode, isOneToOne = isOneToOne) + .map { + notificationSettingsService.setDefaultRoomNotificationMode(isEncrypted = false, mode = mode, isOneToOne = isOneToOne) + } + } } } diff --git a/libraries/architecture/src/main/kotlin/io/element/android/libraries/architecture/AsyncAction.kt b/libraries/architecture/src/main/kotlin/io/element/android/libraries/architecture/AsyncAction.kt index 9545f1ab688..3f5f4ac8d0f 100644 --- a/libraries/architecture/src/main/kotlin/io/element/android/libraries/architecture/AsyncAction.kt +++ b/libraries/architecture/src/main/kotlin/io/element/android/libraries/architecture/AsyncAction.kt @@ -125,6 +125,24 @@ suspend inline fun MutableState>.runUpdatingState( resultBlock = resultBlock, ) +/** + * Run the given block and update the state accordingly, using only Loading and Failure states. + * It's up to the caller to manage the Success state. + */ +@OptIn(ExperimentalContracts::class) +inline fun MutableState>.runUpdatingStateNoSuccess( + resultBlock: () -> Result, +): Result { + contract { + callsInPlace(resultBlock, InvocationKind.EXACTLY_ONCE) + } + value = AsyncAction.Loading + return resultBlock() + .onFailure { failure -> + value = AsyncAction.Failure(failure) + } +} + /** * Calls the specified [Result]-returning function [resultBlock] * encapsulating its progress and return value into an [AsyncAction] while From aef6f14ebf4f146270c966b29abe50865d93c5d0 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 12 Jun 2024 12:12:28 +0200 Subject: [PATCH 2/5] Rename Event for clarity --- .../notificationsettings/RoomNotificationSettingsEvents.kt | 2 +- .../RoomNotificationSettingsPresenter.kt | 2 +- .../notificationsettings/RoomNotificationSettingsView.kt | 2 +- .../UserDefinedRoomNotificationSettingsView.kt | 2 +- .../RoomNotificationSettingsPresenterTest.kt | 6 +++--- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/notificationsettings/RoomNotificationSettingsEvents.kt b/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/notificationsettings/RoomNotificationSettingsEvents.kt index 0ce6e561763..e0370b58c8a 100644 --- a/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/notificationsettings/RoomNotificationSettingsEvents.kt +++ b/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/notificationsettings/RoomNotificationSettingsEvents.kt @@ -19,7 +19,7 @@ package io.element.android.features.roomdetails.impl.notificationsettings import io.element.android.libraries.matrix.api.room.RoomNotificationMode sealed interface RoomNotificationSettingsEvents { - data class RoomNotificationModeChanged(val mode: RoomNotificationMode) : RoomNotificationSettingsEvents + data class ChangeRoomNotificationMode(val mode: RoomNotificationMode) : RoomNotificationSettingsEvents data class SetNotificationMode(val isDefault: Boolean) : RoomNotificationSettingsEvents data object DeleteCustomNotification : RoomNotificationSettingsEvents data object ClearSetNotificationError : RoomNotificationSettingsEvents diff --git a/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/notificationsettings/RoomNotificationSettingsPresenter.kt b/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/notificationsettings/RoomNotificationSettingsPresenter.kt index 84831fe92b8..05283c76693 100644 --- a/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/notificationsettings/RoomNotificationSettingsPresenter.kt +++ b/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/notificationsettings/RoomNotificationSettingsPresenter.kt @@ -95,7 +95,7 @@ class RoomNotificationSettingsPresenter @AssistedInject constructor( fun handleEvents(event: RoomNotificationSettingsEvents) { when (event) { - is RoomNotificationSettingsEvents.RoomNotificationModeChanged -> { + is RoomNotificationSettingsEvents.ChangeRoomNotificationMode -> { localCoroutineScope.setRoomNotificationMode(event.mode, pendingRoomNotificationMode, pendingSetDefault, setNotificationSettingAction) } is RoomNotificationSettingsEvents.SetNotificationMode -> { diff --git a/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/notificationsettings/RoomNotificationSettingsView.kt b/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/notificationsettings/RoomNotificationSettingsView.kt index 2022c87de0f..4ba812a068a 100644 --- a/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/notificationsettings/RoomNotificationSettingsView.kt +++ b/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/notificationsettings/RoomNotificationSettingsView.kt @@ -149,7 +149,7 @@ private fun RoomSpecificNotificationSettingsView( enabled = !state.displayIsDefault.orTrue(), displayMentionsOnlyDisclaimer = state.displayMentionsOnlyDisclaimer, onSelectOption = { - state.eventSink(RoomNotificationSettingsEvents.RoomNotificationModeChanged(it.mode)) + state.eventSink(RoomNotificationSettingsEvents.ChangeRoomNotificationMode(it.mode)) }, ) } diff --git a/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/notificationsettings/UserDefinedRoomNotificationSettingsView.kt b/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/notificationsettings/UserDefinedRoomNotificationSettingsView.kt index 81f0c5f9d96..f9f07f13039 100644 --- a/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/notificationsettings/UserDefinedRoomNotificationSettingsView.kt +++ b/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/notificationsettings/UserDefinedRoomNotificationSettingsView.kt @@ -68,7 +68,7 @@ fun UserDefinedRoomNotificationSettingsView( enabled = !state.displayIsDefault.orTrue(), displayMentionsOnlyDisclaimer = state.displayMentionsOnlyDisclaimer, onSelectOption = { - state.eventSink(RoomNotificationSettingsEvents.RoomNotificationModeChanged(it.mode)) + state.eventSink(RoomNotificationSettingsEvents.ChangeRoomNotificationMode(it.mode)) }, ) } diff --git a/features/roomdetails/impl/src/test/kotlin/io/element/android/features/roomdetails/notificationsettings/RoomNotificationSettingsPresenterTest.kt b/features/roomdetails/impl/src/test/kotlin/io/element/android/features/roomdetails/notificationsettings/RoomNotificationSettingsPresenterTest.kt index 54e23a39952..e1fc3bddf9a 100644 --- a/features/roomdetails/impl/src/test/kotlin/io/element/android/features/roomdetails/notificationsettings/RoomNotificationSettingsPresenterTest.kt +++ b/features/roomdetails/impl/src/test/kotlin/io/element/android/features/roomdetails/notificationsettings/RoomNotificationSettingsPresenterTest.kt @@ -55,7 +55,7 @@ class RoomNotificationSettingsPresenterTest { moleculeFlow(RecompositionMode.Immediate) { presenter.present() }.test { - awaitItem().eventSink(RoomNotificationSettingsEvents.RoomNotificationModeChanged(RoomNotificationMode.MENTIONS_AND_KEYWORDS_ONLY)) + awaitItem().eventSink(RoomNotificationSettingsEvents.ChangeRoomNotificationMode(RoomNotificationMode.MENTIONS_AND_KEYWORDS_ONLY)) val updatedState = consumeItemsUntilPredicate { it.roomNotificationSettings.dataOrNull()?.mode == RoomNotificationMode.MENTIONS_AND_KEYWORDS_ONLY }.last() @@ -129,7 +129,7 @@ class RoomNotificationSettingsPresenterTest { presenter.present() }.test { val initialState = awaitItem() - initialState.eventSink(RoomNotificationSettingsEvents.RoomNotificationModeChanged(RoomNotificationMode.MENTIONS_AND_KEYWORDS_ONLY)) + initialState.eventSink(RoomNotificationSettingsEvents.ChangeRoomNotificationMode(RoomNotificationMode.MENTIONS_AND_KEYWORDS_ONLY)) initialState.eventSink(RoomNotificationSettingsEvents.SetNotificationMode(true)) val defaultState = consumeItemsUntilPredicate { it.roomNotificationSettings.dataOrNull()?.mode == RoomNotificationMode.MENTIONS_AND_KEYWORDS_ONLY @@ -148,7 +148,7 @@ class RoomNotificationSettingsPresenterTest { presenter.present() }.test { val initialState = awaitItem() - initialState.eventSink(RoomNotificationSettingsEvents.RoomNotificationModeChanged(RoomNotificationMode.MENTIONS_AND_KEYWORDS_ONLY)) + initialState.eventSink(RoomNotificationSettingsEvents.ChangeRoomNotificationMode(RoomNotificationMode.MENTIONS_AND_KEYWORDS_ONLY)) initialState.eventSink(RoomNotificationSettingsEvents.SetNotificationMode(true)) val failedState = consumeItemsUntilPredicate { it.restoreDefaultAction.isFailure() From c98863a70ecb4dbd414022a9fea2dde93dfb2cf1 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 12 Jun 2024 12:35:20 +0200 Subject: [PATCH 3/5] Ensure that setting change is taken at least 300ms to avoid dialog flickering (#1647) --- .../RoomNotificationSettingsPresenter.kt | 5 ++-- .../libraries/core/coroutine/Suspend.kt | 30 +++++++++++++++++++ 2 files changed, 33 insertions(+), 2 deletions(-) create mode 100644 libraries/core/src/main/kotlin/io/element/android/libraries/core/coroutine/Suspend.kt diff --git a/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/notificationsettings/RoomNotificationSettingsPresenter.kt b/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/notificationsettings/RoomNotificationSettingsPresenter.kt index 05283c76693..170c0fabbac 100644 --- a/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/notificationsettings/RoomNotificationSettingsPresenter.kt +++ b/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/notificationsettings/RoomNotificationSettingsPresenter.kt @@ -32,6 +32,7 @@ import io.element.android.libraries.architecture.AsyncAction import io.element.android.libraries.architecture.AsyncData import io.element.android.libraries.architecture.Presenter import io.element.android.libraries.architecture.runCatchingUpdatingState +import io.element.android.libraries.core.coroutine.suspendWithMinimumDuration import io.element.android.libraries.matrix.api.notificationsettings.NotificationSettingsService import io.element.android.libraries.matrix.api.room.MatrixRoom import io.element.android.libraries.matrix.api.room.RoomNotificationMode @@ -171,7 +172,7 @@ class RoomNotificationSettingsPresenter @AssistedInject constructor( pendingDefaultState: MutableState, action: MutableState> ) = launch { - suspend { + suspendWithMinimumDuration { pendingModeState.value = mode pendingDefaultState.value = false val result = notificationSettingsService.setRoomNotificationMode(room.roomId, mode) @@ -187,7 +188,7 @@ class RoomNotificationSettingsPresenter @AssistedInject constructor( action: MutableState>, pendingDefaultState: MutableState ) = launch { - suspend { + suspendWithMinimumDuration { pendingDefaultState.value = true val result = notificationSettingsService.restoreDefaultRoomNotificationMode(room.roomId) if (result.isFailure) { diff --git a/libraries/core/src/main/kotlin/io/element/android/libraries/core/coroutine/Suspend.kt b/libraries/core/src/main/kotlin/io/element/android/libraries/core/coroutine/Suspend.kt new file mode 100644 index 00000000000..46a52c84438 --- /dev/null +++ b/libraries/core/src/main/kotlin/io/element/android/libraries/core/coroutine/Suspend.kt @@ -0,0 +1,30 @@ +/* + * Copyright (c) 2024 New Vector Ltd + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.element.android.libraries.core.coroutine + +import kotlinx.coroutines.delay +import kotlin.system.measureTimeMillis + +fun suspendWithMinimumDuration( + minimumDurationMillis: Long = 500, + block: suspend () -> Unit +) = suspend { + val duration = measureTimeMillis { + block() + } + delay(minimumDurationMillis - duration) +} From 418b92212aacafc321fb9b14773a8f3544852293 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 12 Jun 2024 12:50:20 +0200 Subject: [PATCH 4/5] Changelog --- changelog.d/1647.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/1647.bugfix diff --git a/changelog.d/1647.bugfix b/changelog.d/1647.bugfix new file mode 100644 index 00000000000..31fdd2e9f9a --- /dev/null +++ b/changelog.d/1647.bugfix @@ -0,0 +1 @@ +Improve UX on notification setting changes. From 0233e867edc139dcf763c3940afb96f0f532bb47 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 12 Jun 2024 13:54:11 +0200 Subject: [PATCH 5/5] Remove unused import --- .../impl/notifications/NotificationSettingsPresenter.kt | 1 - 1 file changed, 1 deletion(-) diff --git a/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/notifications/NotificationSettingsPresenter.kt b/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/notifications/NotificationSettingsPresenter.kt index b14dc53b56a..d61a05a935c 100644 --- a/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/notifications/NotificationSettingsPresenter.kt +++ b/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/notifications/NotificationSettingsPresenter.kt @@ -29,7 +29,6 @@ import androidx.compose.runtime.setValue import io.element.android.libraries.architecture.AsyncAction import io.element.android.libraries.architecture.AsyncData import io.element.android.libraries.architecture.Presenter -import io.element.android.libraries.architecture.runUpdatingState import io.element.android.libraries.architecture.runUpdatingStateNoSuccess import io.element.android.libraries.matrix.api.MatrixClient import io.element.android.libraries.matrix.api.notificationsettings.NotificationSettingsService