Extract node list display settings to dedicated screen#5580
Merged
Conversation
Fixes two blocking issues identified in code review: 1. **State Management Anti-Pattern (10→1 listener)** - Created NodeListSettingsState data class to aggregate all node list prefs - Combined 10 individual state flows using flow.combine() operator - Reduces recomposition listeners from 10 to 1, eliminating unnecessary re-renders - NodeListScreen now uses single collectAsStateWithLifecycle() call 2. **Material 3 Divider Misuse (8 removed)** - Removed 8 HorizontalDividers between toggle rows (violated M3 guidelines) - Dividers now only used for major section breaks, not every item - Card/ExpressiveSection container already provides visual grouping - Updated padding spacing from 4.dp to 8.dp to compensate Implementation: - Added combine import to SettingsViewModel - Created NodeListSettingsState data class with all 10 preference fields - Exposed as nodeListSettings StateFlow with stateInWhileSubscribed() - Updated NodeListScreen to use aggregated state - Wrapped compact toggles in Column for better structure - All Material 3 compliance maintained, visual experience improved Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Three improvements: 1. **State model parameter**: Replace 10 individual state parameters with single NodeListSettingsState data class. Reduces parameter count from 19 to 11 (1 state + 9 callbacks + modifier). Follows M3 best practice of passing dedicated UiState objects for cohesive state groups. 2. **Fix combine() overload bug**: kotlinx.coroutines only provides typed combine() overloads for up to 5 flows. The prior 10-arg call with mixed types (Flow<String> + 9×Flow<Boolean>) would not compile. Fixed with nested combines (5+5→2). 3. **Cleanup**: Remove unused imports (NodeListDensity in NodeListScreen, Arrangement in NodeLayoutSettings), remove no-op spacedBy(0.dp). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add proper import and replace 3 fully-qualified references with short names for consistency with project conventions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Wrap compact-mode field toggles in a Card with surfaceContainerLow background to visually group them as a cohesive settings section - Add missing List icon import to Android SettingsScreen - Add missing node_layout_section_title import to Desktop SettingsScreen - Update screenshot references to reflect Card grouping Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
📄 Docs staleness check — advisoryThis PR modifies user-facing UI source files but does not update any page under
Changed source files: What to check:
New page checklist (if adding a new doc page):
If this PR does not require a doc update (e.g., internal refactor, bug fix, test change), add the
|
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
- Apply spotless formatting to all feature:settings source files - Add modifier parameter to NodeListScreen (detekt composable rule) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
The node list density and field visibility toggles were previously inlined in both the Android and Desktop settings screens. This made them harder to find, cluttered the main settings surface, and duplicated logic across platform source sets.
Approach
Extracts node list settings into a dedicated
NodeListScreenthat follows the state-holder/UI split pattern used elsewhere in the app:NodeListScreen(state-holder): collects an aggregatedNodeListSettingsStatefrom the ViewModel via a singlecollectAsStateWithLifecycle()call, wires callbacksNodeLayoutSettings(stateless UI): density picker (segmented button), animated live preview of both Complete and Compact node cards, and field visibility toggles grouped inside a CardKey design decisions:
StateFlow<NodeListSettingsState>using nestedcombine()(kotlinx.coroutines typed overloads cap at 5 params)surfaceContainerLow) to visually group them as a cohesive settings sectionListItemwithRectangleShape-- the checked-state rounded shape is intentional Expressive designListIteminternal padding provides appropriate densityScreenshots
Files changed
NodeListScreen.kt,NodeLayoutSettings.kt,NodeLayoutSettingsPreviews.ktRoutes.kt(new route),SettingsNavigation.kt(new entry),SettingsViewModel.kt(state aggregation),SettingsScreen.ktandDesktopSettingsScreen.kt(navigation links replace inline toggles)