From d8e4ada781ecab7b08eff9c999428ff6dc44c7b0 Mon Sep 17 00:00:00 2001 From: davotoula Date: Wed, 3 Jun 2026 00:21:11 +0200 Subject: [PATCH] Code reviewP - Collapse filterSettings' two identical lookup lambdas into one stringLookup - Rebuild via SettingsCategory.copy() so new fields aren't silently dropped - Memoize buildSettingsCatalog with remember(hasPrivateKey, nav, uriHandler); onResetMarmot reads isResettingMarmot via rememberUpdatedState to avoid a stale-capture, eliminating ~60 allocations per keystroke --- .../loggedIn/settings/AllSettingsScreen.kt | 22 ++++++++++++------- .../loggedIn/settings/SettingsCatalog.kt | 17 +++++--------- .../settings/SettingsCatalogFilterTest.kt | 8 ++----- 3 files changed, 22 insertions(+), 25 deletions(-) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/settings/AllSettingsScreen.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/settings/AllSettingsScreen.kt index 8b29bcdd8d..a402702772 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/settings/AllSettingsScreen.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/settings/AllSettingsScreen.kt @@ -46,6 +46,7 @@ import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.remember import androidx.compose.runtime.rememberCoroutineScope +import androidx.compose.runtime.rememberUpdatedState import androidx.compose.runtime.setValue import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier @@ -97,20 +98,25 @@ fun AllSettingsScreen( val searchState = rememberTextFieldState() val query = searchState.text.toString() + // The catalog is structurally stable for the screen's lifetime, so it is rebuilt only when + // an input actually changes — not on every keystroke. `onResetMarmot` reads the volatile + // `isResettingMarmot` through `rememberUpdatedState` so the memoized closure never goes stale. + val onResetMarmot by rememberUpdatedState(newValue = { if (!isResettingMarmot) showResetMarmotDialog = true }) val catalog = - buildSettingsCatalog( - nav = nav, - uriHandler = uriHandler, - hasPrivateKey = hasPrivateKey, - onResetMarmot = { if (!isResettingMarmot) showResetMarmotDialog = true }, - ) + remember(hasPrivateKey, nav, uriHandler) { + buildSettingsCatalog( + nav = nav, + uriHandler = uriHandler, + hasPrivateKey = hasPrivateKey, + onResetMarmot = { onResetMarmot() }, + ) + } val filtered = filterSettings( catalog = catalog, query = query, - titleLookup = { stringRes(context, it) }, - keywordsLookup = { stringRes(context, it) }, + stringLookup = { stringRes(context, it) }, ) Scaffold( diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/settings/SettingsCatalog.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/settings/SettingsCatalog.kt index 624851bf72..bdbe765bd8 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/settings/SettingsCatalog.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/settings/SettingsCatalog.kt @@ -56,29 +56,24 @@ data class SettingsCategory( * Filters [catalog] by [query] (case-insensitive substring over category title + entry title + keywords). * Blank/whitespace query returns [catalog] unchanged. Categories left with no * matching entries are dropped. Pure — no Compose/Android — so it is unit-testable; - * string resolution is injected via [titleLookup] / [keywordsLookup]. + * string-resource resolution is injected via [stringLookup]. */ fun filterSettings( catalog: List, query: String, - titleLookup: (Int) -> String, - keywordsLookup: (Int) -> String, + stringLookup: (Int) -> String, ): List { val needle = query.trim().lowercase() if (needle.isEmpty()) return catalog return catalog.mapNotNull { category -> - val categoryTitle = titleLookup(category.titleRes) + val categoryTitle = stringLookup(category.titleRes) val matched = category.entries.filter { entry -> - val keywords = entry.keywordsRes?.let { keywordsLookup(it) } ?: "" - val haystack = (categoryTitle + " " + titleLookup(entry.titleRes) + " " + keywords).lowercase() + val keywords = entry.keywordsRes?.let { stringLookup(it) } ?: "" + val haystack = (categoryTitle + " " + stringLookup(entry.titleRes) + " " + keywords).lowercase() haystack.contains(needle) } - if (matched.isEmpty()) { - null - } else { - SettingsCategory(category.titleRes, category.isDanger, matched) - } + if (matched.isEmpty()) null else category.copy(entries = matched) } } diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/settings/SettingsCatalogFilterTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/settings/SettingsCatalogFilterTest.kt index 5bbf16f34a..6430af319a 100644 --- a/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/settings/SettingsCatalogFilterTest.kt +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/settings/SettingsCatalogFilterTest.kt @@ -25,16 +25,13 @@ import org.junit.Assert.assertTrue import org.junit.Test class SettingsCatalogFilterTest { - private val titles = + private val strings = mapOf( 100 to "Account Settings", 200 to "Danger Zone", 1 to "Relay Setup", 2 to "UI Preferences", 3 to "Backup Keys", - ) - private val keywords = - mapOf( 20 to "dark mode, theme, font size", ) @@ -71,8 +68,7 @@ class SettingsCatalogFilterTest { filterSettings( catalog = catalog, query = query, - titleLookup = { titles.getValue(it) }, - keywordsLookup = { keywords.getValue(it) }, + stringLookup = { strings.getValue(it) }, ) @Test