Skip to content

Unwrap PropertiesDesign#127

Merged
Goooler merged 1 commit intotrunkfrom
unwrap-properties-design
Apr 26, 2026
Merged

Unwrap PropertiesDesign#127
Goooler merged 1 commit intotrunkfrom
unwrap-properties-design

Conversation

@Goooler
Copy link
Copy Markdown
Owner

@Goooler Goooler commented Apr 23, 2026

Refs #119.

@Goooler Goooler force-pushed the unwrap-properties-design branch 5 times, most recently from d3b8208 to 213d348 Compare April 26, 2026 11:30
@Goooler Goooler force-pushed the unwrap-properties-design branch from 213d348 to 49c436a Compare April 26, 2026 11:33
@Goooler Goooler merged commit 353f9fd into trunk Apr 26, 2026
2 checks passed
@Goooler Goooler deleted the unwrap-properties-design branch April 26, 2026 11:35
Goooler added a commit that referenced this pull request Apr 26, 2026
@Goooler Goooler requested a review from Copilot April 27, 2026 00:34
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors the profile “Properties” flow to split the previous Design-based implementation into a Compose @Composable screen backed by a dedicated ViewModel (refs #119).

Changes:

  • Added PropertiesViewModel to own UI state/events, validation, saving, and commit progress reporting.
  • Reworked PropertiesScreen into a composable route that binds to PropertiesViewModel via collectAsStateWithLifecycle.
  • Simplified PropertiesActivity to a Compose setContent host for PropertiesScreen and navigation callbacks.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File Description
app/src/main/kotlin/com/github/kr328/clash/profile/vm/PropertiesViewModel.kt New ViewModel providing state/events, autosave-on-stop, and commit/progress handling for the Properties UI.
app/src/main/kotlin/com/github/kr328/clash/profile/ui/PropertiesScreen.kt Moves from Design to pure Compose; binds UI to PropertiesViewModel and forwards navigation/events.
app/src/main/kotlin/com/github/kr328/clash/profile/PropertiesActivity.kt Converts to a lightweight Compose host activity wiring navigation callbacks and result handling.
Comments suppressed due to low confidence (1)

app/src/main/kotlin/com/github/kr328/clash/profile/ui/PropertiesScreen.kt:148

  • ModelProgressBarState is mutated directly during composition via with(progressBarState) { ... }. Since its fields are mutableStateOf, writing them during composition can cause unnecessary recompositions and is inconsistent with existing patterns (e.g. LogcatScreen updates the progress state inside a LaunchedEffect). Consider moving these assignments into LaunchedEffect(progressState).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

val profile = uiState.value.profile ?: return
viewModelScope.launch {
runCatching {
withProfile { patch(profile.uuid, profile.name, profile.source, profile.interval) }
Copy link

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onStop() auto-saves via patch(...) when there are unsaved changes, but the ViewModel never updates originalProfile / hasUnsavedChanges after a successful patch. This means the UI can continue to think there are unsaved changes and will re-patch on every subsequent stop. Consider updating originalProfile (and resetting hasUnsavedChanges) after the patch succeeds.

Suggested change
withProfile { patch(profile.uuid, profile.name, profile.source, profile.interval) }
withProfile { patch(profile.uuid, profile.name, profile.source, profile.interval) }
}.onSuccess {
uiState.update { state -> state.copy(originalProfile = profile.copy()) }

Copilot uses AI. Check for mistakes.
Comment on lines +62 to +71
@OptIn(DelicateCoroutinesApi::class)
override fun onCleared() {
rootUuid?.let { uuid ->
GlobalScope.launch(Dispatchers.IO) {
try {
withProfile { release(uuid) }
} catch (e: Exception) {
// ignore
}
}
Copy link

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GlobalScope.launch in onCleared() is the only GlobalScope usage in the app module and opts into DelicateCoroutinesApi. Prefer using the app-wide com.github.kr328.clash.common.Global scope (used elsewhere, e.g. Remote.kt) or another structured application scope so this work is still cancellable/traceable.

Copilot uses AI. Check for mistakes.
canceled = true
eventState.value = EventState.Finish(true)
} catch (e: Exception) {
eventState.value = EventState.ShowMessage(e.message ?: "Unknown error")
Copy link

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fallback error message uses a hard-coded string ("Unknown error"). The rest of the app uses the localized R.string.unknown (e.g. NewProfileViewModel.kt). Consider switching to that resource (or another localized string) for consistency and translation coverage.

Suggested change
eventState.value = EventState.ShowMessage(e.message ?: "Unknown error")
eventState.value =
EventState.ShowMessage(
e.message ?: getApplication<Application>().getString(R.string.unknown),
)

Copilot uses AI. Check for mistakes.
Copilot AI added a commit that referenced this pull request Apr 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants