From 4e18bcc098637699cccda57b7054c0d022a03bd2 Mon Sep 17 00:00:00 2001 From: PonceGL Date: Thu, 10 Sep 2026 06:05:29 -0600 Subject: [PATCH] =?UTF-8?q?fix(settings):=20P.1=20=E2=80=94=20self-describ?= =?UTF-8?q?ing=20item=20count=20in=20SettingsScreen?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit totalItems = mainCategories.size + 3 counted three trailing items (Device Capabilities, Accounts, About) rendered by hand right after the loop, with a comment naming them — two sources of truth for the same fact. Whoever adds a fourth trailing item and forgets the literal breaks the last item's rounded corner. The plan's original justification for this fix was wrong (verified against code): adding a SettingsCategory does NOT trigger the bug, mainCategories.size is already dynamic. The real fragility is the literal +3 itself, disconnected from the three ExpressiveXxxItem calls it's supposed to count. Fix: a private TrailingSettingsItem enum (DEVICE_CAPABILITIES, ACCOUNTS, ABOUT) rendered via forEach + an exhaustive when, same pattern already used for mainCategories. totalItems = mainCategories.size + trailingItems.size — a real collection's size, not a literal. Adding a fourth item now needs a new enum entry, and the compiler refuses to compile until the when covers it — the bug class becomes impossible, not just less likely. Self-review (code-review skill, medium effort) found the fix was only half done: the loop's own exclusion filter (it != ABOUT && it != DEVICE_CAPABILITIES) was a second, independent list of the same two categories, never derived from the new enum. Fixed by giving TrailingSettingsItem a nullable `category: SettingsCategory?` (null for Accounts, which isn't a category) and deriving the loop's filter from it — one enum now drives both the count and the exclusion, so a category can't end up rendered twice or dropped by only remembering to update one of the two lists. Also renamed the enum's entries to UPPER_SNAKE_CASE to match the sibling SettingsCategory enum's convention in this same file (was PascalCase). Zero behavior change: same onClick, same colors, same order, same shapeFor()/itemIndex bookkeeping. Testing: no automated test added. SettingsScreen needs the full Hilt graph (NavController, PlayerViewModel with ~30 dependencies, SettingsViewModel via hiltViewModel()) to instantiate at all, and there's no existing Compose test for this screen to build on — standing that up just for a compiler-enforced, non-behavior-changing refactor is disproportionate. Attempted visual verification on-device instead; blocked by this session's sandbox not delivering synthetic input events to the emulator (adb shell input — confirmed via raw getevent capture, zero events for both taps and keys after ruling out disk space as the cause) — documented, not silently skipped. Verified instead: assembleDebug succeeds, full JVM baseline unaffected (5 pre-existing failures, none new), and the diff traced by hand against the original for behavioral equivalence. graphify-out/ refreshed per this repo's CLAUDE.md. --- .../presentation/screens/SettingsScreen.kt | 86 +++++++++++-------- 1 file changed, 52 insertions(+), 34 deletions(-) diff --git a/app/src/main/java/com/theveloper/pixelplay/presentation/screens/SettingsScreen.kt b/app/src/main/java/com/theveloper/pixelplay/presentation/screens/SettingsScreen.kt index fa40ecb265..765a98315f 100644 --- a/app/src/main/java/com/theveloper/pixelplay/presentation/screens/SettingsScreen.kt +++ b/app/src/main/java/com/theveloper/pixelplay/presentation/screens/SettingsScreen.kt @@ -89,6 +89,25 @@ import com.theveloper.pixelplay.data.preferences.LaunchTab // SettingsTopBar removed, replaced by CollapsibleCommonTopBar +/** + * The three fixed items rendered after the [SettingsCategory] loop in [SettingsScreen]: they + * aren't part of the loop (Accounts isn't a category at all; Device Capabilities and About are + * categories but excluded from the loop to render in a fixed trailing position instead). + * + * This enum is the single source of truth for "which items are trailing" — both the item count + * used to size the rounded-corner list (previously a literal `+ 3`, disconnected from the code + * that actually rendered those three items) and the filter that excludes [DEVICE_CAPABILITIES] + * and [ABOUT] from the main loop (previously a second, independent list of the same two + * categories, spelled out again). Adding a fourth trailing item now means adding one entry here; + * the compiler forces a matching branch in the `when` that renders it, and the loop's filter picks + * it up automatically if it wraps a [SettingsCategory] — no second place to remember to update. + */ +private enum class TrailingSettingsItem(val category: SettingsCategory?) { + DEVICE_CAPABILITIES(SettingsCategory.DEVICE_CAPABILITIES), + ACCOUNTS(category = null), + ABOUT(SettingsCategory.ABOUT), +} + @androidx.annotation.OptIn(UnstableApi::class) @OptIn(ExperimentalMaterial3Api::class) @Composable @@ -213,12 +232,11 @@ fun SettingsScreen( item { val isDark = MaterialTheme.colorScheme.surface.luminance() < 0.5f ExpressiveSettingsGroup { - val mainCategories = SettingsCategory.entries.filter { - it != SettingsCategory.ABOUT && - it != SettingsCategory.DEVICE_CAPABILITIES - } + val trailingItems = TrailingSettingsItem.entries + val trailingCategories = trailingItems.mapNotNull { it.category }.toSet() + val mainCategories = SettingsCategory.entries.filter { it !in trailingCategories } - val totalItems = mainCategories.size + 3 // Device + Accounts + About + val totalItems = mainCategories.size + trailingItems.size fun shapeFor(index: Int) = when { totalItems == 1 -> RoundedCornerShape(24.dp) @@ -250,36 +268,36 @@ fun SettingsScreen( itemIndex++ } - ExpressiveCategoryItem( - category = SettingsCategory.DEVICE_CAPABILITIES, - customColors = getCategoryColors(SettingsCategory.DEVICE_CAPABILITIES, isDark), - onClick = { navController.navigateSafely(Screen.DeviceCapabilities.route) }, - shape = shapeFor(itemIndex) - ) - if (itemIndex < totalItems - 1) { - Spacer(modifier = Modifier.height(2.dp)) - } - itemIndex++ - - ExpressiveNavigationItem( - title = stringResource(R.string.settings_category_accounts_title), - subtitle = stringResource(R.string.settings_category_accounts_subtitle), - icon = Icons.Rounded.AccountCircle, - colors = getAccountsColors(isDark), - onClick = { navController.navigateSafely(Screen.Accounts.route) }, - shape = shapeFor(itemIndex) - ) - if (itemIndex < totalItems - 1) { - Spacer(modifier = Modifier.height(2.dp)) + trailingItems.forEach { trailing -> + when (trailing) { + TrailingSettingsItem.DEVICE_CAPABILITIES -> ExpressiveCategoryItem( + category = SettingsCategory.DEVICE_CAPABILITIES, + customColors = getCategoryColors(SettingsCategory.DEVICE_CAPABILITIES, isDark), + onClick = { navController.navigateSafely(Screen.DeviceCapabilities.route) }, + shape = shapeFor(itemIndex) + ) + + TrailingSettingsItem.ACCOUNTS -> ExpressiveNavigationItem( + title = stringResource(R.string.settings_category_accounts_title), + subtitle = stringResource(R.string.settings_category_accounts_subtitle), + icon = Icons.Rounded.AccountCircle, + colors = getAccountsColors(isDark), + onClick = { navController.navigateSafely(Screen.Accounts.route) }, + shape = shapeFor(itemIndex) + ) + + TrailingSettingsItem.ABOUT -> ExpressiveCategoryItem( + category = SettingsCategory.ABOUT, + customColors = getCategoryColors(SettingsCategory.ABOUT, isDark), + onClick = { navController.navigateSafely("about") }, + shape = shapeFor(itemIndex) + ) + } + if (itemIndex < totalItems - 1) { + Spacer(modifier = Modifier.height(2.dp)) + } + itemIndex++ } - itemIndex++ - - ExpressiveCategoryItem( - category = SettingsCategory.ABOUT, - customColors = getCategoryColors(SettingsCategory.ABOUT, isDark), - onClick = { navController.navigateSafely("about") }, - shape = shapeFor(itemIndex) - ) } // for player active: