Unwrap LogcatDesign#137
Conversation
27b3c34 to
463885b
Compare
463885b to
1acc651
Compare
There was a problem hiding this comment.
Pull request overview
Refactors the logcat feature to align with issue #119 by moving UI logic into a @Composable screen and extracting side effects/state management into a ViewModel, replacing the prior Design-based implementation.
Changes:
- Introduces
LogcatViewModelto own logcat streaming, file loading, export, and one-shot UI events. - Reworks
LogcatScreeninto a composable that renders state, handles events, and uses activity-result APIs for export. - Simplifies
LogcatActivityto aBaseActivityhosting Compose content.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| app/src/main/kotlin/com/github/kr328/clash/log/vm/LogcatViewModel.kt | New ViewModel encapsulating logcat streaming, file operations, export progress, and event signaling |
| app/src/main/kotlin/com/github/kr328/clash/log/util/LogcatReader.kt | Makes log reading suspendable and moves execution onto Dispatchers.IO |
| app/src/main/kotlin/com/github/kr328/clash/log/ui/LogcatScreen.kt | Converts the old design-driven UI into a composable screen driven by the ViewModel’s flows |
| app/src/main/kotlin/com/github/kr328/clash/log/LogcatActivity.kt | Replaces coroutine-driven DesignActivity logic with a simple Compose host activity |
Comments suppressed due to low confidence (1)
app/src/main/kotlin/com/github/kr328/clash/log/ui/LogcatScreen.kt:73
- Local name
evenStatelooks like a typo foreventState, which makes the event-handling code harder to read/grep. Rename it toeventState(or similar) for clarity.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…del.kt Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
app/src/main/kotlin/com/github/kr328/clash/log/ui/LogcatScreen.kt:73
evenStatelooks like a typo foreventState. Since this is the main event flow used by the screen, the misspelling makes the code harder to read and increases the chance of mistakes when editing the event handling logic later.
💡 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 4 out of 4 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
app/src/main/kotlin/com/github/kr328/clash/log/ui/LogcatScreen.kt:73
- Typo in local state name:
evenStateshould beeventStateto match the ViewModel and avoid confusion/incorrect future usage.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…er.kt Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…del.kt Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
app/src/main/kotlin/com/github/kr328/clash/log/ui/LogcatScreen.kt:73
- Typo/unclear naming:
evenStateis collectingviewModel.eventState, and laterwhen (val event = evenState)uses it as an event. Rename this local toeventState(or similar) to avoid confusion and improve readability.
💡 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 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
app/src/main/kotlin/com/github/kr328/clash/log/ui/LogcatScreen.kt:35
- This file imports
androidx.compose.runtime.setValue, but there is no delegated mutable state usingbyhere anymore. Unused imports are compilation errors in Kotlin; remove this import (and any other unused ones) to fix the build.
app/src/main/kotlin/com/github/kr328/clash/log/ui/LogcatScreen.kt:73 - Local variable
evenStatelooks like a typo foreventState(it actually holdsviewModel.eventState). Renaming it will reduce confusion and make theLaunchedEffect(...)block easier to read.
app/src/main/kotlin/com/github/kr328/clash/log/ui/LogcatScreen.kt:128 - Auto-follow while streaming likely won't work reliably:
listState.isBottomis evaluated afteruiState.messages.sizeincreases, so the list is no longer considered "bottom" once a new item is appended, and the scroll won't trigger. Track whether the user was at bottom before the list grows (e.g., keep awasAtBottomstate updated fromsnapshotFlow { listState.isBottom }, or computeshouldAutoFollowusing the previous size) and use that to decide whether to scroll.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Refs #119.