From 0b5b1a738a23fdfe16423ed8993ce0566da8fb26 Mon Sep 17 00:00:00 2001 From: Grisha Kruglov Date: Thu, 14 Oct 2021 00:50:24 -0700 Subject: [PATCH] Do less work while navigating Logins views Fetching a set of logins from the store is quite expensive. This commit avoids doing that while navigating back and forth between the list and detail views: - retain processes logins state when navigating into detail view - use the `get` storage api to obtain specific login, instead of `list().filter {...}` - avoid re-sorting retained logins when navigating back into the list view --- .../settings/logins/LoginsFragmentStore.kt | 8 ++-- .../SavedLoginsStorageController.kt | 42 +++++++++---------- .../logins/LoginsFragmentStoreTest.kt | 4 +- .../SavedLoginsStorageControllerTest.kt | 4 +- 4 files changed, 28 insertions(+), 30 deletions(-) diff --git a/app/src/main/java/org/mozilla/fenix/settings/logins/LoginsFragmentStore.kt b/app/src/main/java/org/mozilla/fenix/settings/logins/LoginsFragmentStore.kt index beff187873..fd92277df6 100644 --- a/app/src/main/java/org/mozilla/fenix/settings/logins/LoginsFragmentStore.kt +++ b/app/src/main/java/org/mozilla/fenix/settings/logins/LoginsFragmentStore.kt @@ -53,6 +53,7 @@ class LoginsFragmentStore(initialState: LoginsListState) : sealed class LoginsAction : Action { data class FilterLogins(val newText: String?) : LoginsAction() data class UpdateLoginsList(val list: List) : LoginsAction() + object LoginsListUpToDate : LoginsAction() data class UpdateCurrentLogin(val item: SavedLogin) : LoginsAction() data class SortLogins(val sortingStrategy: SortingStrategy) : LoginsAction() data class DuplicateLogin(val dupe: SavedLogin?) : LoginsAction() @@ -97,6 +98,9 @@ private fun savedLoginsStateReducer( action: LoginsAction ): LoginsListState { return when (action) { + is LoginsAction.LoginsListUpToDate -> { + state.copy(isLoading = false) + } is LoginsAction.UpdateLoginsList -> { state.copy( isLoading = false, @@ -125,9 +129,7 @@ private fun savedLoginsStateReducer( } is LoginsAction.LoginSelected -> { state.copy( - isLoading = true, - loginList = emptyList(), - filteredItems = emptyList() + isLoading = true ) } is LoginsAction.DuplicateLogin -> { diff --git a/app/src/main/java/org/mozilla/fenix/settings/logins/controller/SavedLoginsStorageController.kt b/app/src/main/java/org/mozilla/fenix/settings/logins/controller/SavedLoginsStorageController.kt index d4bbbf0c88..7d61b7f2b5 100644 --- a/app/src/main/java/org/mozilla/fenix/settings/logins/controller/SavedLoginsStorageController.kt +++ b/app/src/main/java/org/mozilla/fenix/settings/logins/controller/SavedLoginsStorageController.kt @@ -203,35 +203,31 @@ open class SavedLoginsStorageController( loginsFragmentStore.dispatch(LoginsAction.DuplicateLogin(dupe)) } - fun fetchLoginDetails(loginId: String) { - var deferredLogin: Deferred>? = null - val fetchLoginJob = lifecycleScope.launch(ioDispatcher) { - deferredLogin = async { - passwordsStorage.list() - } - val fetchedLoginList = deferredLogin?.await() - - fetchedLoginList?.let { - withContext(Dispatchers.Main) { - val login = fetchedLoginList.filter { - it.guid == loginId - }.first() - loginsFragmentStore.dispatch( - LoginsAction.UpdateCurrentLogin( - login.mapToSavedLogin() - ) + fun fetchLoginDetails(loginId: String) = lifecycleScope.launch(ioDispatcher) { + val fetchedLogin = passwordsStorage.get(loginId) + withContext(Dispatchers.Main) { + if (fetchedLogin != null) { + loginsFragmentStore.dispatch( + LoginsAction.UpdateCurrentLogin( + fetchedLogin.mapToSavedLogin() ) - } - } - } - fetchLoginJob.invokeOnCompletion { - if (it is CancellationException) { - deferredLogin?.cancel() + ) + } else { + navController.popBackStack() } } } fun handleLoadAndMapLogins() { + // Don't touch the store if we already have the logins loaded. + // This has a slight downside of possibly being out of date with the storage if, say, Sync + // ran in the meantime, but that's fairly unlikely and the speedy UI is worth it. + if (loginsFragmentStore.state.loginList.isNotEmpty()) { + lifecycleScope.launch(Dispatchers.Main) { + loginsFragmentStore.dispatch(LoginsAction.LoginsListUpToDate) + } + return + } var deferredLogins: Deferred>? = null val fetchLoginsJob = lifecycleScope.launch(ioDispatcher) { deferredLogins = async { diff --git a/app/src/test/java/org/mozilla/fenix/settings/logins/LoginsFragmentStoreTest.kt b/app/src/test/java/org/mozilla/fenix/settings/logins/LoginsFragmentStoreTest.kt index 4b64a44336..c1a8ef40c9 100644 --- a/app/src/test/java/org/mozilla/fenix/settings/logins/LoginsFragmentStoreTest.kt +++ b/app/src/test/java/org/mozilla/fenix/settings/logins/LoginsFragmentStoreTest.kt @@ -167,7 +167,7 @@ class LoginsFragmentStoreTest { store.dispatch(LoginsAction.LoginSelected(mockk())).joinBlocking() assertTrue(store.state.isLoading) - assertTrue(store.state.loginList.isEmpty()) - assertTrue(store.state.filteredItems.isEmpty()) + assertTrue(store.state.loginList.isNotEmpty()) + assertTrue(store.state.filteredItems.isNotEmpty()) } } diff --git a/app/src/test/java/org/mozilla/fenix/settings/logins/SavedLoginsStorageControllerTest.kt b/app/src/test/java/org/mozilla/fenix/settings/logins/SavedLoginsStorageControllerTest.kt index 0468b6f9d7..55933bd335 100644 --- a/app/src/test/java/org/mozilla/fenix/settings/logins/SavedLoginsStorageControllerTest.kt +++ b/app/src/test/java/org/mozilla/fenix/settings/logins/SavedLoginsStorageControllerTest.kt @@ -92,14 +92,14 @@ class SavedLoginsStorageControllerTest { httpRealm = "httpRealm", formActionOrigin = "" ) - coEvery { passwordsStorage.list() } returns listOf(login) + coEvery { passwordsStorage.get("id") } returns login controller.fetchLoginDetails(login.guid) val expectedLogin = login.mapToSavedLogin() coVerify { - passwordsStorage.list() + passwordsStorage.get("id") loginsFragmentStore.dispatch( LoginsAction.UpdateCurrentLogin( expectedLogin