Conversation
056282e to
bc3416a
Compare
bc3416a to
12a7953
Compare
There was a problem hiding this comment.
Pull request overview
This PR continues the “split Design to @Composable + ViewModel” refactor (Issue #119) by removing the ProfilesDesign abstraction and migrating the Profiles feature to a Compose screen driven by a dedicated ProfilesViewModel.
Changes:
- Introduce
ProfilesViewModelto own Profiles UI state, lifecycle-driven fetching, and one-shot UI events. - Refactor
ProfilesScreeninto a ViewModel-backed composable (and rename the pure UI portion toProfilesContent). - Simplify
ProfilesActivityfromDesignActivitycoroutine/event loop into a plainBaseActivityhosting Compose content.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| app/src/main/kotlin/com/github/kr328/clash/profile/vm/ProfilesViewModel.kt | New ViewModel managing profile list state, update actions, and snackbar/navigation events. |
| app/src/main/kotlin/com/github/kr328/clash/profile/ui/ProfilesScreen.kt | Replaces ProfilesDesign with ProfilesScreen + ProfilesContent, adds snackbar handling and lifecycle observer wiring. |
| app/src/main/kotlin/com/github/kr328/clash/profile/ProfilesActivity.kt | Converts to Compose setContent hosting ProfilesScreen, removing the old DesignActivity loop. |
Comments suppressed due to low confidence (2)
app/src/main/kotlin/com/github/kr328/clash/profile/ui/ProfilesScreen.kt:93
showSnackbar(...)is called directly insideLaunchedEffect(eventState)andconsumeEvent()is executed only after the snackbar suspends/finishes. If the activity is recreated (e.g., rotation) while the snackbar is showing, the ViewModel survives andeventStatemay still beShowMessage/ShowEditableMessage, causing the snackbar/navigation to be replayed. Consider consuming/resetting the event before awaitingshowSnackbar, or launchingshowSnackbarin a separate coroutine (as done inMainScreen) so the event is cleared immediately.
app/src/main/kotlin/com/github/kr328/clash/profile/ui/ProfilesScreen.kt:271- The profile row is made clickable while the
RadioButtonhasonClick = null, so TalkBack/keyboard users won’t get proper radio-button semantics (selected state + role) for the whole row. In this repo, radio/checkbox rows useModifier.selectable(..., role = Role.RadioButton)/toggleable(..., role = Role.Checkbox)(e.g.ProxyDesign.ktandAccessControlScreen.kt). Consider switching the row modifier toselectablewithselected = profile.activeandrole = Role.RadioButton, and wire the click through consistently.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ViewModel.kt Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (3)
app/src/main/kotlin/com/github/kr328/clash/profile/ui/ProfilesScreen.kt:92
SnackbarHostState.showSnackbar(...)is a suspending call; handling it directly insideLaunchedEffectmeansviewModel.consumeEvent()won’t run until the snackbar completes. This can leave the event “stuck” across recompositions/config changes and can delay processing subsequent events. Consider launching the snackbar in a separate coroutine (or consuming the event before awaiting the snackbar) similar to howMainScreenhandles long snackbars.
app/src/main/kotlin/com/github/kr328/clash/profile/ui/ProfilesScreen.kt:99- In the editable snackbar branch,
showSnackbar(...)suspends while waiting for user action/dismissal, so theconsumeEvent()call after thewhenis delayed. This can cause theShowEditableMessageevent to be re-handled after a configuration change and can block newer events from being handled promptly. Consider consuming the event immediately and running the snackbar interaction in a child coroutine (keepingevent.uuidcaptured locally).
app/src/main/kotlin/com/github/kr328/clash/profile/ui/ProfilesScreen.kt:124 ProfilesContentusesMihomoScaffold, which already supports asnackbarHostslot. PlacingSnackbarHostoutside the scaffold (aligned manually in a wrappingBox) can lead to inconsistent insets/positioning and duplicates the layout pattern used elsewhere (e.g.,LogcatContentpassessnackbarHostintoMihomoScaffold). Consider threading asnackbarHostlambda intoProfilesContentand passing it toMihomoScaffoldinstead of overlaying it here.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ViewModel.kt Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Zongle Wang <wangzongler@gmail.com>
Refs #119.