Improve Editor code quality, migrate editor state management to `EditorViewModel` by Ethran · Pull Request #220 · Ethran/notable

Ethran · 2026-03-09T20:39:29Z

Relocated editor state (mode, pen, eraser, pen settings, and selection) from EditorState to EditorViewModel.
Refactored EditorState into a thin wrapper for backward compatibility with canvas components.
Introduced CanvasCommand and EditorUiEvent channels in EditorViewModel to decouple UI actions from side effects.
Unified toolbar and editor UI state into a single ToolbarUiState flow.
Moved persistence of editor settings into EditorViewModel using LaunchedEffect in EditorView.
Updated EditorView, EditorSurface, and ScrollIndicator to consume state directly from EditorViewModel.
Moved Mode, PlacementMode, and Clipboard definitions to EditorViewModel.kt.

- Relocated editor state (mode, pen, eraser, pen settings, and selection) from `EditorState` to `EditorViewModel`. - Refactored `EditorState` into a thin wrapper for backward compatibility with canvas components. - Introduced `CanvasCommand` and `EditorUiEvent` channels in `EditorViewModel` to decouple UI actions from side effects. - Unified toolbar and editor UI state into a single `ToolbarUiState` flow. - Moved persistence of editor settings into `EditorViewModel` using `LaunchedEffect` in `EditorView`. - Updated `EditorView`, `EditorSurface`, and `ScrollIndicator` to consume state directly from `EditorViewModel`. - Moved `Mode`, `PlacementMode`, and `Clipboard` definitions to `EditorViewModel.kt`.

Copilot

Pull request overview

This PR migrates editor mode/tool/selection/settings ownership from EditorState into EditorViewModel, keeping EditorState as a thin adapter so legacy canvas components can continue to operate while Compose UI consumes ViewModel-owned state and events.

Changes:

Refactors EditorState into a stable wrapper delegating editor state mutations to EditorViewModel.
Introduces CanvasCommand and EditorUiEvent streams in EditorViewModel to separate UI side effects (navigation/snacks) from imperative canvas actions.
Updates editor UI (surface + scroll indicators) and utilities to reference relocated enums (Mode, PlacementMode) and ViewModel state.

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
app/src/main/java/com/ethran/notable/editor/utils/Select.kt	Updates `PlacementMode` import after relocating the enum.
app/src/main/java/com/ethran/notable/editor/utils/ImageHandler.kt	Updates `PlacementMode` import after relocating the enum.
app/src/main/java/com/ethran/notable/editor/ui/toolbar/ToolbarMenu.kt	Updates `Mode` import to new package location.
app/src/main/java/com/ethran/notable/editor/ui/toolbar/Toolbar.kt	Updates `Mode` import to new package location.
app/src/main/java/com/ethran/notable/editor/ui/ScrollIndicator.kt	Switches scroll indicators to consume `EditorViewModel` + `PageView` rather than `EditorState`.
app/src/main/java/com/ethran/notable/editor/ui/EditorSurface.kt	Changes surface to accept `EditorViewModel` and constructs an `EditorState` adapter for `DrawCanvas`.
app/src/main/java/com/ethran/notable/editor/state/SelectionState.kt	Replaces `!!` usage and updates `PlacementMode` import.
app/src/main/java/com/ethran/notable/editor/state/EditorState.kt	Replaces prior stateful implementation with a ViewModel-delegating adapter.
app/src/main/java/com/ethran/notable/editor/canvas/OnyxInputHandler.kt	Updates `Mode` import to new package location.
app/src/main/java/com/ethran/notable/editor/canvas/DrawCanvas.kt	Updates `Mode` import to new package location.
app/src/main/java/com/ethran/notable/editor/canvas/CanvasRefreshManager.kt	Updates `Mode` import to new package location.
app/src/main/java/com/ethran/notable/editor/canvas/CanvasObserverRegistry.kt	Writes drawing-enabled state into `EditorViewModel` via the adapter.
app/src/main/java/com/ethran/notable/editor/EditorViewModel.kt	Centralizes editor/tool state, adds UI event + canvas command channels, and hosts moved enums/singletons.
app/src/main/java/com/ethran/notable/editor/EditorView.kt	Wires ViewModel init/persistence, splits UI events vs canvas commands, and updates UI consumers.
app/src/main/java/com/ethran/notable/editor/EditorControlTower.kt	Routes page switching/tool toggles through ViewModel-owned state.
app/src/main/java/com/ethran/notable/data/datastore/EditorSettingCacheManager.kt	Updates `Mode` import to new package location for persisted settings.

Comments suppressed due to low confidence (1)

app/src/main/java/com/ethran/notable/editor/EditorControlTower.kt:101

switchPage() now calls page.changePage(id) (inside the IO block). registerObservers() also calls page.changePage(pageId) immediately after calling switchPage(pageId), so page changes triggered via CanvasEventBus.changePage will run PageView.changePage() twice. This can cause redundant DB work and hard-to-debug state glitches; consider removing one of the calls (typically keep it centralized in switchPage()).

    private suspend fun switchPage(id: String) {
        // Switch to Main thread for Compose state mutations
        withContext(Dispatchers.Main) {
            state.viewModel.changePage(id)
            history.cleanHistory()
        }

        // Switch to (or ensure we are on) IO thread for Database operations
        withContext(Dispatchers.IO) {
            page.changePage(id)
        }
    }

You can also share your feedback on Copilot code review. Take the survey.

Copilot · 2026-03-09T20:44:11Z

app/src/main/java/com/ethran/notable/editor/EditorViewModel.kt

+    private fun sendUiEvent(event: EditorUiEvent) {
+        viewModelScope.launch { uiEventChannel.send(event) }
+    }
+
+    private fun sendCanvasCommand(command: CanvasCommand) {
+        viewModelScope.launch { canvasCommandChannel.send(command) }
+    }


Channel.send() on a default (RENDEZVOUS) Channel will suspend until there is an active collector. If a UI event/canvas command is sent while the screen isn’t collecting yet (or after it stops), these viewModelScope.launch { ...send(...) } coroutines will suspend and accumulate. Consider using a buffered channel (e.g., Channel(Channel.BUFFERED)), trySend, or switching back to MutableSharedFlow with extraBufferCapacity for one-shot events.

Copilot · 2026-03-09T20:44:11Z

app/src/main/java/com/ethran/notable/editor/ui/EditorSurface.kt

 fun EditorSurface(
    appRepository: AppRepository,
-    state: EditorState, page: PageView, history: History
+    viewModel: EditorViewModel, page: PageView, history: History
 ) {
    val coroutineScope = rememberCoroutineScope()
    log.i("recompose surface")

+    // Create EditorState wrapper for backward compatibility with DrawCanvas
+    val editorState = EditorState(viewModel, page)
+


EditorState(viewModel, page) is allocated on every recomposition of EditorSurface, but AndroidView(factory=...) typically only uses the instance created during the first composition (and there is no update block here to swap it). This is both wasted work and can be confusing if EditorState gains any real state later. Consider remember(viewModel, page) { EditorState(viewModel, page) }, or better: create the wrapper once in EditorView and pass it down to EditorSurface so the same adapter instance is used everywhere.

Copilot · 2026-03-09T20:44:12Z

app/src/main/java/com/ethran/notable/editor/EditorView.kt

            // TODO: check if it is correct, and remove exeption throwing
-            throw Exception("Page does not exist")
+//            throw Exception("Page does not exist")


Typo in comment: "exeption" → "exception".

Copilot · 2026-03-09T20:44:12Z

app/src/main/java/com/ethran/notable/editor/EditorView.kt

+        // Create EditorState wrapper for backward compatibility
+        val editorState = remember(viewModel, page) {
+            EditorState(viewModel, page)
+        }
+
+        // Initialize ViewModel with persisted settings on first composition
+        LaunchedEffect(Unit) {
+            viewModel.initFromPersistedSettings(editorSettingCacheManager.getEditorSettings())
+        }


Page navigation no longer triggers the onPageChange callback (it’s not used anywhere in EditorView after the refactor). NotableNavHost passes this callback to keep the editor’s pageId in SavedStateHandle in sync, so switching pages via EditorControlTower/EditorViewModel.changePage() can leave navigation state stale. Consider emitting a one-time "page changed" UI event or observing viewModel.currentPageId in EditorView and calling onPageChange(newPageId) when it changes (while avoiding the initial value).

Copilot · 2026-03-09T20:44:12Z

app/src/main/java/com/ethran/notable/editor/EditorView.kt

+        // Initialize ViewModel with persisted settings on first composition
+        LaunchedEffect(Unit) {
+            viewModel.initFromPersistedSettings(editorSettingCacheManager.getEditorSettings())
+        }


initFromPersistedSettings() is called from LaunchedEffect(Unit), which will re-run whenever EditorView leaves and re-enters composition (while the same EditorViewModel instance may still be alive on the back stack). That can overwrite in-memory editor state with persisted defaults, surprising users. Consider making initialization idempotent (e.g., a didInitSettings flag in the ViewModel) or keying the effect to something that guarantees a single call per ViewModel lifetime.

…nav, type relocation (#221) * Initial plan * Refactor EditorViewModel architecture: remove mutableStateOf, add StateFlow single source of truth Co-authored-by: Ethran <77555700+Ethran@users.noreply.github.com> * Address code review: AtomicBoolean for didInitSettings, remove scroll from LaunchedEffect keys, clarify isDrawing mutability Co-authored-by: Ethran <77555700+Ethran@users.noreply.github.com> * Update app/src/main/java/com/ethran/notable/editor/state/EditorState.kt Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update app/src/main/java/com/ethran/notable/editor/state/EditorState.kt Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Ethran <77555700+Ethran@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

- Move page navigation responsibility from `EditorControlTower` to `EditorViewModel`. - Update `currentPageId` to be a computed property derived from `_toolbarState`. - Implement `goToNextPage` and `goToPreviousPage` in `EditorViewModel` using `viewModelScope`. - Ensure history is cleared when navigating between pages. - Refactor `changePage` to handle its own coroutine scope and reset selection state.

Copilot

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 7 comments.

You can also share your feedback on Copilot code review. Take the survey.

Copilot · 2026-03-10T15:29:11Z

app/src/main/java/com/ethran/notable/editor/EditorControlTower.kt

    fun goToPreviousPage() {
-        scope.launch(Dispatchers.IO) {
-            logEditorControlTower.i("Going to previous page")
-            val previous = state.getPreviousPage()
-            if (previous != null)
-                switchPage(previous)
-        }
+        logEditorControlTower.i("Going to previous page")
+        state.viewModel.goToPreviousPage()
+        history.cleanHistory()
    }


Same issue as next-page: goToPreviousPage() no longer switches the PageView but still clears history. This can leave the canvas on the old page while the ViewModel/book state changes. Consider switching the PageView via switchPage(...) and only clearing history after a successful page switch.

Copilot · 2026-03-10T15:29:11Z

app/src/main/java/com/ethran/notable/editor/EditorControlTower.kt

    private suspend fun switchPage(id: String) {
-        // TODO: Check if this is a problem:
-        //`switchPage()` now calls `page.changePage(id)` inside `withContext(Dispatchers.IO)`,
-        // but `PageView.changePage()` mutates in-memory state (e.g., `currentPageId`,
-        // `zoomLevel.value`) before it does its own IO work. Calling it from IO risks
-        // off-main state mutations and also makes it easier for callers to accidentally
-        // invoke `page.changePage()` twice (which currently happens in `registerObservers()`).
-        // Prefer calling `page.changePage(id)` from the main thread (or let
-        // `PageView.changePage()` manage its own dispatching) and ensure callers don’t call
-        // it again after `switchPage()`.
-
        // Switch to Main thread for Compose state mutations
        withContext(Dispatchers.Main) {
-            state.changePage(id)
+            state.viewModel.changePage(id)
            history.cleanHistory()
        }


In switchPage(), the later page.changePage(id) call (in the IO block) runs off the main thread, but PageView.changePage() mutates in-memory state immediately (e.g., currentPageId, zoomLevel.value). This can introduce thread-safety issues, and registerObservers() also calls page.changePage(pageId) after switchPage(pageId), which risks double-switching. Consider ensuring page.changePage(id) is invoked on the main thread (or that PageView.changePage() handles dispatching internally) and that it’s only called once per page change.

Copilot · 2026-03-10T15:29:12Z

app/src/main/java/com/ethran/notable/editor/ui/ScrollIndicator.kt

+fun ScrollIndicator(viewModel: EditorViewModel, page: PageView) {
+    val toolbarState by viewModel.toolbarState.collectAsStateWithLifecycle()
    BoxWithConstraints(


ScrollIndicator collects the entire toolbarState data class but only uses isToolbarOpen. Any toolbar state update (e.g., pen changes, page info, drawing state) will recompose the indicator unnecessarily. Consider collecting a derived flow of just isToolbarOpen (e.g., toolbarState.map { it.isToolbarOpen }.distinctUntilChanged()) to reduce recompositions.

Copilot · 2026-03-10T15:29:12Z

app/src/main/java/com/ethran/notable/editor/state/EditorTypes.kt

+package com.ethran.notable.editor.state
+
+/**
+ * Drawing mode for the editor.
+ */
+enum class Mode {


PR description mentions moving Mode/PlacementMode/Clipboard definitions into EditorViewModel.kt, but the types are introduced in the new editor/state/EditorTypes.kt file instead. Please update the PR description (or relocate the types) so the change matches what reviewers/readers will expect.

Copilot · 2026-03-10T15:29:12Z

app/src/main/java/com/ethran/notable/editor/EditorView.kt


        if (!exists) {
            // TODO: check if it is correct, and remove exeption throwing
-            throw Exception("Page does not exist")
+//            throw Exception("Page does not exist")
            if (bookId != null) {
                // clean the book
                log.i("Could not find page, Cleaning book")


When exists is false the code navigates away, but execution continues (and pageExists becomes false, which still allows the rest of the composable to build since only null returns early). This can briefly build PageView/canvas state for a missing page and potentially trigger follow-up work/crashes. Consider returning immediately after navController.navigate(...) (or setting a guard so the editor UI is not composed when exists == false).

Copilot · 2026-03-10T15:29:13Z

app/src/main/java/com/ethran/notable/editor/EditorViewModel.kt

+                pen = settings?.pen ?: Pen.BALLPEN,
+                eraser = settings?.eraser ?: Eraser.PEN,
+                isToolbarOpen = settings?.isToolbarOpen ?: false,
+                penSettings = settings?.penSettings ?: DEFAULT_PEN_SETTINGS


initFromPersistedSettings() restores penSettings directly from persisted settings. If the persisted map is empty (which the comment above notes can crash DrawCanvas), this still propagates an empty map and can reintroduce the crash. Consider normalizing here (e.g., treat null/empty as DEFAULT_PEN_SETTINGS) so the ViewModel never emits an empty penSettings.

Suggested change

penSettings = settings?.penSettings ?: DEFAULT_PEN_SETTINGS

penSettings = settings?.penSettings?.takeIf { it.isNotEmpty() } ?: DEFAULT_PEN_SETTINGS

Copilot · 2026-03-10T15:29:13Z

app/src/main/java/com/ethran/notable/editor/EditorControlTower.kt

    fun goToNextPage() {
-        scope.launch(Dispatchers.IO) {
-            logEditorControlTower.i("Going to next page")
-            val next = state.getNextPage()
-            if (next != null)
-                switchPage(next)
-        }
+        logEditorControlTower.i("Going to next page")
+        state.viewModel.goToNextPage()
+        history.cleanHistory()
    }


goToNextPage() now only calls viewModel.goToNextPage() and cleans history, but it no longer updates the PageView (i.e., does not call switchPage(...) / page.changePage(...)). Gestures that use this path will update toolbar/book state without actually switching the rendered page. Consider restoring the previous behavior by obtaining the next pageId and calling switchPage(pageId) (or emitting the target pageId from the ViewModel so the control tower can switch PageView), and only cleaning history when a page switch actually happens.

…ing state

Ethran requested a review from Copilot March 9, 2026 20:39

Copilot started reviewing on behalf of Ethran March 9, 2026 20:40 View session

Copilot AI reviewed Mar 9, 2026

View reviewed changes

Ethran and others added 3 commits March 9, 2026 21:44

cleanup

0817c81

fix for canvas init.

9afa2a5

Ethran force-pushed the dev branch from 0dca5b6 to 9afa2a5 Compare March 10, 2026 14:55

Ethran added 2 commits March 10, 2026 16:09

refactor page change observation to use snapshotFlow in EditorView

0a9ef64

Ethran requested a review from Copilot March 10, 2026 15:21

Copilot started reviewing on behalf of Ethran March 10, 2026 15:21 View session

Copilot AI reviewed Mar 10, 2026

View reviewed changes

Ethran added 4 commits March 10, 2026 20:16

update pen and stroke initialization logic

83a905e

add logging and improve selection state handling

4e8ef6b

add logging and improve selection state handling

5fc6226

sync EditorViewModel with quick navigation visibility and update draw…

52ce406

…ing state

Ethran merged commit 0a22a96 into main Mar 10, 2026
1 check passed

	penSettings = settings?.penSettings ?: DEFAULT_PEN_SETTINGS
	penSettings = settings?.penSettings?.takeIf { it.isNotEmpty() } ?: DEFAULT_PEN_SETTINGS

Uh oh!

Conversation

Ethran commented Mar 9, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants