Rewrite dialog bindings#98
Conversation
a4e42e0 to
84103f0
Compare
84103f0 to
70edb6f
Compare
70edb6f to
d7b79f6
Compare
There was a problem hiding this comment.
Pull request overview
Migrates legacy dialog implementations from XML/ViewBinding-based Material dialogs to Jetpack Compose dialogs as part of the broader Compose UI rewrite (refs #65).
Changes:
- Replaced ViewBinding-based input/progress dialogs with Compose
ModelTextInputDialogandModelProgressBarDialog. - Updated
PropertiesDesign,LogcatDesign, andFilesDesignto drive dialogs via Compose state instead of suspend dialog helpers. - Removed ViewBinding support and deleted unused dialog XML layouts and binding-backed dialog helpers.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| design/src/main/res/layout/dialog_text_field.xml | Removed obsolete XML dialog layout (migrated to Compose). |
| design/src/main/res/layout/dialog_fetch_status.xml | Removed obsolete XML progress dialog layout (migrated to Compose). |
| design/src/main/java/com/github/kr328/clash/design/dialog/Progress.kt | Removed legacy suspend-based progress dialog helper. |
| design/src/main/java/com/github/kr328/clash/design/dialog/Input.kt | Removed legacy suspend-based text input dialog helper. |
| design/src/main/java/com/github/kr328/clash/design/component/ModelTextInputDialog.kt | Added Compose text input dialog component. |
| design/src/main/java/com/github/kr328/clash/design/component/ModelProgressBarDialog.kt | Added Compose progress dialog + state holder. |
| design/src/main/java/com/github/kr328/clash/design/PropertiesDesign.kt | Converted property-edit dialogs and progress UI to Compose state-driven dialogs. |
| design/src/main/java/com/github/kr328/clash/design/LogcatDesign.kt | Added Compose progress state for export flow and renders progress dialog. |
| design/src/main/java/com/github/kr328/clash/design/FilesDesign.kt | Converted rename dialog to Compose and moved new-name into request payload. |
| design/build.gradle.kts | Disabled ViewBinding in the design module. |
| app/src/main/java/com/github/kr328/clash/LogcatActivity.kt | Reworked export flow to update progress via LogcatDesign state. |
| app/src/main/java/com/github/kr328/clash/FilesActivity.kt | Uses new rename request payload; import no longer prompts for/validates filename. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This reverts commit b25ad09.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } finally { | ||
| withContext(Dispatchers.Main) { | ||
| progressBarState.visible = false | ||
| progressBarState.text = null | ||
| processingState = false | ||
| } |
There was a problem hiding this comment.
withProcessing does UI cleanup in a finally block via withContext(Dispatchers.Main). If the parent coroutine is cancelled (e.g., activity finishing / scope cancellation), this withContext can throw CancellationException before resetting processingState/progressBarState.visible, leaving the progress dialog stuck visible. Consider wrapping the finally UI reset in withContext(NonCancellable + Dispatchers.Main) (or Dispatchers.Main.immediate) so cleanup always runs.
| suspend fun finishExportProgress() = | ||
| withContext(Dispatchers.Main) { | ||
| exportProgressState.visible = false | ||
| exportProgressState.text = null | ||
| } |
There was a problem hiding this comment.
finishExportProgress() uses withContext(Dispatchers.Main) to hide the progress dialog. When called from a finally block, this can be skipped if the coroutine is already cancelled (since withContext is cancellable), leaving exportProgressState.visible stuck true. Consider using withContext(NonCancellable + Dispatchers.Main) for this cleanup path.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Refs #65.