migrate to m3e list item #1651
migrate to m3e list item #1651
Conversation
Tlaster
force-pushed
the
feature/m3e_list_item
branch
from
4cfb172 to
44109d9
Compare
There was a problem hiding this comment.
Pull request overview
This PR migrates the application from using basic Material 3 list items with manual styling to Material 3 Expressive (M3E) segmented list items, which provide native support for grouped list appearances with proper shapes and interactions.
Key changes:
- Introduced
PlatformSegmentedListItemcomponent with platform-specific implementations for Android, iOS, and JVM - Refactored
UserPresenterfrom Composable-based to Flow-based data management - Migrated numerous screens to use the new segmented list item component with proper index/totalCount tracking
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| UserPresenter.kt | Refactored from Composable to Flow-based architecture for better state management |
| PlatformListItem.kt (common/android/ios/jvm) | Added new PlatformSegmentedListItem API with platform implementations |
| PlatformShapes.android.kt | Added shape utility functions for segmented list items and removed legacy clipping code |
| AboutScreenContent.kt | Migrated 6 list items to use segmented variant with proper indices |
| RssListWithTabs.kt | Converted RSS source list items to segmented style |
| UiListWithTabs.kt | Wrapped dropdown menu in Box for proper positioning |
| BlueskyFeedWithTabs.kt | Updated to use new index/totalCount parameters |
| DmListItem.kt | Migrated DM list items to segmented variant |
| UiListItemComponent.kt | Enhanced to conditionally use segmented items based on description presence |
| TabCustomizeScreen.kt | Updated tab items with segmented shapes and improved swipe-to-dismiss handling |
| StorageScreen.kt | Migrated storage settings to segmented list items |
| SettingsScreen.kt | Extensive migration of settings items with conditional shape logic |
| LocalFilterScreen.kt | Converted filter items to segmented variant |
| LocalFilterEditDialog.kt | Migrated dialog list items to segmented style |
| AppearanceScreen.kt | Major refactoring of appearance settings with segmented items |
| AppLoggingScreen.kt | Converted logging items to segmented variant |
| AiConfigScreen.kt | Migrated AI config options to segmented list items |
| AccountsScreen.kt | Enhanced account list with menu dropdown and segmented styling |
| HomeScreen.kt | Changed TopLevelBackStack lifecycle management to use retain |
| AccountSelectionModal.kt | Updated account selection to use segmented items |
| NavigationSuiteScaffold2.kt | Converted navigation drawer items to segmented variant |
| build.gradle.kts (app/compose-ui) | Added ExperimentalMaterial3ExpressiveApi opt-in configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| val currentTabs = cacheTabs | ||
| val allTabsState = allTabsState | ||
| val canSwipeToDelete = cacheTabs.size > 1 | ||
| val canSwipeToDelete = true |
There was a problem hiding this comment.
The deletion constraint logic has been removed but the variable name "canSwipeToDelete" is now misleading since it's always true. Consider either renaming this to something like "enableSwipeToDelete" or removing the variable entirely and passing true directly to the component.
| val canSwipeToDelete = true | |
| val enableSwipeToDelete = true |
| public fun ListItemDefaults.segmentedShapes2( | ||
| index: Int, | ||
| count: Int, | ||
| ): ListItemShapes { | ||
| if (count == 1) { | ||
| return ListItemDefaults.single() | ||
| } else { | ||
| return ListItemDefaults.segmentedShapes( | ||
| index = index, | ||
| count = count, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
The function name "segmentedShapes2" is unclear. The "2" suffix doesn't convey meaning. Consider renaming this to something more descriptive like "segmentedShapesOrSingle" or "getSegmentedShapes" to better indicate its purpose of handling both single-item and multi-item cases.
| val topLevelBackStack = | ||
| retain( | ||
| "home_top_level_back_stack_${tabs.all.first().key}", | ||
| ) { | ||
| TopLevelBackStack( | ||
| getDirection(tabs.all.first()), | ||
| ) | ||
| } |
There was a problem hiding this comment.
Using "retain" here instead of "producePresenter" changes the lifecycle management of the TopLevelBackStack. The "retain" function appears to bypass the presenter's clock and composition lifecycle, which could lead to stale state not being properly cleaned up when the composition is disposed. This needs verification that the retained object is properly released when no longer needed.
| // modifier = | ||
| // modifier | ||
| // .clickable { | ||
| // onClick.invoke(data.key) | ||
| // }, |
There was a problem hiding this comment.
These commented-out lines should be removed instead of being left as comments. Dead code makes the codebase harder to maintain and can cause confusion.
| // modifier = | |
| // modifier | |
| // .clickable { | |
| // onClick.invoke(data.key) | |
| // }, |
| checked = showMenu, | ||
| onCheckedChange = { | ||
| if (!isBigScreen) { | ||
| showMenu = it |
There was a problem hiding this comment.
The SingleChoiceSettingsItem component is using "checked" and "onCheckedChange" parameters, but this appears to be incorrect usage for the segmented list item. The code passes "showMenu" as the checked state which is a boolean for dropdown visibility, not a selection state. This should likely use "selected" parameter or handle clicks differently to avoid confusion about what state is being tracked.
| checked = showMenu, | |
| onCheckedChange = { | |
| if (!isBigScreen) { | |
| showMenu = it | |
| selected = false, | |
| onClick = { | |
| if (!isBigScreen) { | |
| showMenu = !showMenu |
| val topLevelBackStackState by rememberUpdatedState(topLevelBackStack) | ||
|
|
||
| val navigate = | ||
| remember(topLevelBackStack, wideNavigationRailState) { | ||
| { route: Route -> | ||
| topLevelBackStack.addTopLevel(route) | ||
| scope.launch { | ||
| wideNavigationRailState.collapse() | ||
| } | ||
| Unit |
There was a problem hiding this comment.
The remember block has been removed around the navigate lambda. This lambda now gets recreated on every recomposition when topLevelBackStack or wideNavigationRailState changes, which could cause unnecessary recompositions in child components that receive this lambda. The lambda should be wrapped with remember and use rememberUpdatedState if needed to capture the latest values while maintaining referential stability.
| val topLevelBackStackState by rememberUpdatedState(topLevelBackStack) | |
| val navigate = | |
| remember(topLevelBackStack, wideNavigationRailState) { | |
| { route: Route -> | |
| topLevelBackStack.addTopLevel(route) | |
| scope.launch { | |
| wideNavigationRailState.collapse() | |
| } | |
| Unit | |
| val topLevelBackStackState = rememberUpdatedState(topLevelBackStack) | |
| val wideNavigationRailStateState = rememberUpdatedState(wideNavigationRailState) | |
| val navigate: (Route) -> Unit = | |
| remember { | |
| { route: Route -> | |
| topLevelBackStackState.value.addTopLevel(route) | |
| scope.launch { | |
| wideNavigationRailStateState.value.collapse() | |
| } |
| val currentRoute = topLevelBackStack.topLevelKey | ||
| val accountType = currentRoute.accountTypeOr(AccountType.Active) |
There was a problem hiding this comment.
The "by remember { derivedStateOf { ... } }" pattern has been replaced with direct property access, removing the derived state optimization. This means currentRoute and accountType will now be recalculated on every recomposition instead of only when topLevelBackStack.topLevelKey changes. This could lead to unnecessary recalculations and recompositions of dependent composables.
No description provided.