chore: code tidiness and structure#1
Open
jessejamesjohnston wants to merge 10 commits into
Open
Conversation
test.kt and test_api36.kt were one-off probes for the API 36 Notification.ProgressStyle / setShortCriticalText APIs. They live outside any source set, so they never compiled into the app; delete them to keep the module root clean.
FlashScreen.kt in particular had ~38 fully-qualified references (androidx.compose.ui.platform.LocalLifecycleOwner, android.app.PendingIntent, and friends) inline in the code. Promote them all to proper imports in FlashScreen, RecoveryApp, SelectDriveScreen, SelectModelScreen, UsbFlasher, and KeepAliveService so the code reads like idiomatic Kotlin. No behavior change; the compiled output is identical.
The navigation graph, the top-bar step indicator, and every navigate() call previously agreed on eight string literals by convention alone. A typo in any one of them would compile fine and fail at runtime. Add a sealed Route hierarchy in ui/Route.kt as the single source of truth: each destination exposes its route pattern, and SelectChannel (the only parameterized destination) exposes forModel() so the URL-encoding of the model-name argument lives in exactly one place. No behavior change; the route strings are byte-for-byte the same.
FlashScreen was a 353-line composable that managed the Foreground Service, built API-36 Live Update notifications, ran the erase simulation, drove UsbFlasher, and rendered the UI, all inline. Business logic in the view layer meant no state survival story of its own and no way to unit test the flow. Split it three ways: - FlashViewModel owns the state machine (erasing -> flashing -> success/error, plus the cancel-and-reset path) and exposes a single FlashUiState via StateFlow. Scoped to the flash NavBackStackEntry, so the running flash now survives configuration changes by design rather than relying on the activity's configChanges opt-out. - FlashNotificationController owns the notification channel, the KeepAliveService start/stop, and the Live Update / NotificationCompat builders. The ViewModel decides when to notify, never how. - FlashScreen renders FlashUiState. The only view concerns left are the POST_NOTIFICATIONS permission prompt (needs an Activity) and lifecycle/window-focus tracking (needs a View), both reported to the ViewModel. Also names the flasher's 'Cancelled' sentinel (UsbFlasher.RESULT_CANCELLED) instead of comparing a bare string literal across files. Behavior is unchanged: same state transitions, same notification timing and content, same service lifetime.
Roughly half the user-facing copy lived in strings.xml (the flasher's step and error messages); the other half was hardcoded in composables, the notification builders, and the flash state machine. Move all of it to resources so the copy is localizable and reviewable in one place: - Screen copy for Welcome, Identify, SelectModel, SelectChannel, SelectDrive, Erase, and Flash. - Notification titles, channel name, and the rotating fun-words/status-chip pairs (now index-aligned <string-array>s). - Shared actions (Continue, OK, Back to Home) deduplicated. Two deliberate non-changes, noted for review: the copy is byte-for-byte identical (including the existing mix of ASCII '...' and typographic ellipsis, worth a separate copy pass), and the notification percentage keeps its explicit Locale.US formatting rather than picking up the resource locale, to avoid a behavior change in this PR.
- Drop the Log.d progress line that fired every 500ms during the entire write, and the Log.d in SelectDriveScreen's permission-denied path (the denial already surfaces to the user via the error dialog). Log.i/Log.e diagnostics stay. - Fix the remaining Kotlin compiler warnings: deprecated LinearProgressIndicator overload in EraseScreen, a redundant displayName initializer, and an unnecessary non-null assertion. The build is now warning-free except for EraseScreen's unused 'device' parameter, which is kept deliberately: the erase flow is currently simulated and a follow-up will make it operate on the real device.
gradle.properties already declares kotlin.code.style=official; this makes editors and IDEs enforce the same conventions (4-space indent, UTF-8, LF, final newline) without depending on per-machine IDE settings.
Runs assembleDebug and Android lint on every PR and on pushes to main, uploading the lint HTML report as an artifact when the build fails. Both tasks pass clean on the current tree. No secrets required.
select_channel was never included in the top bar's step mapping, so the channel screen fell through to the else branch and displayed 'Step 1 of 4'. Model identification and channel choice are both part of picking the image, so it joins identify/select_model as step 2.
Author
📋 Contribution series overviewEight small, sequential PRs that take Book Recovery from AI-generated draft to production-grade Android craft, plus one signature playful feature. Each is independently reviewable; they're stacked, so please take them in order — each PR's diff against Recommended review / merge order:
Testing: the full flow is hardware-validated end to end over USB-OTG on a Pixel 10 Pro XL and a Pixel Tablet (real Cr-48 flash with checksum verification, real erase, adaptive two-pane layout, the game, and the status copy). Pure-Kotlin logic (digests, game engine) is covered by 19 JVM unit tests. A note on approach: #2 (toolchain) stops at the current stable Material 3; #4 pins the Expressive |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
chore: code tidiness and structure
Summary
Ground-preparation PR: makes the codebase easier to review and extend without changing behavior. Eight commits, each independently revertible:
test.kt,test_api36.kt) from the module root — one-off API 36 notification probes outside any source set.FlashScreenwas the main offender; alsoRecoveryApp,SelectDriveScreen,SelectModelScreen,UsbFlasher,KeepAliveService).Routehierarchy replaces the eight string literals that the nav graph, top-bar step logic, andnavigate()calls previously agreed on by convention.SelectChannel.forModel()centralizes the URL-encoding of the model-name argument.FlashViewModel— the 353-lineFlashScreencomposable managed the foreground service, notification building, erase simulation, and flasher lifecycle inline. Now:FlashViewModelowns the state machine (erasing → flashing → success/error, plus cancel-and-reset) and exposes oneFlashUiState;FlashNotificationControllerowns the channel, KeepAliveService start/stop, and the API 36 Live Update / NotificationCompat builders;FlashScreen(172 lines) renders state. Only the POST_NOTIFICATIONS prompt and lifecycle/window-focus tracking stay in the composable — they need the view layer.<string-array>s) join the flasher messages that were already instrings.xml. Copy is byte-for-byte identical.Log.dprogress line in the write loop, aLog.din the permission-denied path) and fix the compiler warnings (deprecatedLinearProgressIndicatoroverload, redundant initializer, unnecessary!!)..editorconfigmatching the Kotlin official style already declared ingradle.properties.assembleDebug+linton PRs and pushes to main; lint HTML report uploaded on failure. No secrets required.Motivation
Subsequent PRs (adaptive layouts, M3 Expressive polish, post-flash verification, and a couple of surprises) all touch the flash flow and navigation. This PR gets the structure ready so those diffs stay small and focused: feature PRs shouldn't have to carve business logic out of composables to add a pane layout, and route changes shouldn't require hunting string literals.
A concrete payoff already visible: because the flash state now lives in a ViewModel scoped to the flash destination, a configuration change (rotation, fold, resize) no longer depends solely on the activity's
configChangesopt-out to keep the flash alive. The adaptive-layout PR will build on exactly this.Behavior
Intentionally identical, with one deliberate exception: a one-line
fix:commit makes the channel screen read "Step 2 of 4" instead of "Step 1 of 4" (select_channelwas never in the step mapping). Things deliberately not fixed here:UsbFlasher's cancellation story..../ typographic…,Locale.USpercentage formatting in notifications) preserved byte-for-byte; a copy pass can follow separately.Testing
./gradlew assembleDebug lintpasses clean; the only remaining compiler warning (unuseddeviceparameter inEraseScreen) is deliberate — the erase flow is currently simulated, and making it real is planned follow-up work.Deferred follow-ups
EraseScreencurrently simulates).Verification
./gradlew assembleDebug lintclean.