Improve toolbar code quality by Ethran · Pull Request #219 · Ethran/notable

Ethran · 2026-03-07T21:05:28Z

No description provided.

…d of direct dependency injection of `NavController`, `AppRepository`, and `ExportEngine`. Key changes: - Moved logic for navigation, image picking, and exporting from `Toolbar` components to `EditorView`. - Introduced explicit callback parameters (`onNavigateToLibrary`, `onNavigateToPages`, `onExport`, etc.) to the `Toolbar` composable. - Optimized page number calculation by moving the logic to `EditorView`. - Cleaned up unused imports and simplified the `PositionedToolbar` internal structure. - Improved separation of concerns between UI components and data repositories.

Copilot

Pull request overview

This PR refactors the toolbar in the Notable e-ink note-taking app to decouple the UI from business logic. It introduces a ToolbarUiState data class and splits the Toolbar composable into a stateful bridge (Toolbar) and a stateless, previewable UI layer (ToolbarContent). Navigation, image picking, export, and settings logic are lifted out of the toolbar components into callback parameters.

Changes:

Introduced ToolbarUiState data class and ToolbarContent stateless composable, moving state management and side effects (navigation, image handling, exports) up to the PositionedToolbar in EditorView.kt.
Replaced ExportEngine dependency in ToolbarMenu with a suspend (ExportTarget, ExportFormat) -> String lambda, and removed NavController/AppRepository dependencies from Toolbar.
Cleaned up dead code (commented-out blocks, empty preview, unused functions like isSelected), and added a working @Preview for the toolbar.

Reviewed changes

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

File	Description
`Toolbar.kt`	Introduced `ToolbarUiState`, split `Toolbar` into stateful bridge + stateless `ToolbarContent`, removed direct state/navigation dependencies, added `@Preview`.
`ToolbarMenu.kt`	Replaced `ExportEngine` parameter with an `onExport` lambda, removed unused imports and dead code (commented blocks, empty preview).
`EditorView.kt`	Moved navigation, image picking, export, and page-number logic into `PositionedToolbar`, passing them as callbacks to `Toolbar`.

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

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

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

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

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

…ern approach. Key changes: - **Decoupled UI from Logic**: Moved toolbar state management, navigation handling, and side-effect execution (export, image picking, etc.) from the view layer into `EditorViewModel`. - **Introduced Intent-based Actions**: Added `ToolbarAction` to represent user interactions and `EditorUiEvent` for one-time effects like navigation and snackbars. - **Improved State Management**: Replaced legacy state synchronization with a reactive `ToolbarUiState` flow. - **Simplified Components**: Refactored `Toolbar`, `ToolbarMenu`, and `PositionedToolbar` to be stateless, delegating all actions back to the ViewModel. - **Centralized Data Loading**: Moved book/page metadata loading and page numbering logic into the ViewModel.

Copilot

Pull request overview

Copilot reviewed 9 out of 9 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-08T16:03:28Z

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

+    private fun handlePenChange(pen: Pen) {
+        _toolbarState.update { state ->
+            if (state.mode == Mode.Draw && state.pen == pen) {
+                state.copy(isStrokeSelectionOpen = true)
+            } else {
+                sendUiEvent(EditorUiEvent.PenChanged(pen))
+                if (state.mode != Mode.Draw) sendUiEvent(EditorUiEvent.ModeChanged(Mode.Draw))
+                state.copy(mode = Mode.Draw, pen = pen)
+            }
+        }
+        sendUiEvent(EditorUiEvent.CheckDrawingState)
+    }


sendUiEvent() is called inside the _toolbarState.update { } lambda. The update function on MutableStateFlow can retry its lambda if there's a concurrent modification (it uses compare-and-set internally). This means sendUiEvent could be called multiple times if there's contention. Side effects should be moved outside the update block to avoid this. Extract the old state first, compute the new state inside update, and emit events afterward.

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

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

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

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

…nitSignal

- Migrated menu and drawing state logic from `EditorState` to `EditorViewModel` to consolidate UI control. - Introduced `CanvasEventBus.closeMenusSignal` to trigger menu closures from the canvas layer. - Added `updateDrawingState` to re-evaluate drawing availability based on active menus and selections. - Improved synchronization between `EditorState` and `EditorViewModel` using `LaunchedEffect` observers for zoom, scroll, and selection status. - Added a snackbar notification when clearing all strokes. - Switched to `collectAsStateWithLifecycle` for toolbar state observation.

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 8 comments.

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

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

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

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

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

Copilot · 2026-03-08T21:52:19Z

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

+        _toolbarState.update { state ->
+            val newSettings = state.penSettings.toMutableMap()
+            newSettings[pen.penName] = setting
+            sendUiEvent(EditorUiEvent.PenSettingChanged(pen, setting))
+            state.copy(penSettings = newSettings)
+        }


Same issue as handlePenChange: sendUiEvent() is called inside the _toolbarState.update {} lambda. Since MutableStateFlow.update uses compare-and-set and may retry the lambda on contention, this side effect could fire multiple times. Move the sendUiEvent call outside the update {} block.

Suggested change

_toolbarState.update { state ->

val newSettings = state.penSettings.toMutableMap()

newSettings[pen.penName] = setting

sendUiEvent(EditorUiEvent.PenSettingChanged(pen, setting))

state.copy(penSettings = newSettings)

}

val event = EditorUiEvent.PenSettingChanged(pen, setting)

_toolbarState.update { state ->

val newSettings = state.penSettings.toMutableMap()

newSettings[pen.penName] = setting

state.copy(penSettings = newSettings)

}

sendUiEvent(event)

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

app/src/main/java/com/ethran/notable/editor/canvas/CanvasObserverRegistry.kt

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

… logic

Copilot

Pull request overview

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

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

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

Copilot · 2026-03-09T10:21:22Z

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

+        }
+
+        // Sync legacy state to ViewModel for Toolbar rendering
+        val zoomLevel by page.zoomLevel.collectAsState()


This uses collectAsState() instead of collectAsStateWithLifecycle(). The rest of the codebase consistently uses collectAsStateWithLifecycle() for collecting Flows in Composables (see HomeView.kt:99, PagesView.kt:88, QuickNav.kt:61, and even the new PositionedToolbar.kt:27). Using collectAsState() means the collection won't be lifecycle-aware and will continue collecting even when the app is in the background, which can waste resources.

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

Copilot · 2026-03-09T10:21:23Z

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

+            selectionActive
+        ) {
+            viewModel.setHasClipboard(editorState.clipboard != null)
+            viewModel.setShowResetView(zoomLevel != 1.0f) // page.scroll != Offset.Zero


The old code checked both scroll and zoom: state.pageView.scroll.x != 0f || zoomLevel != 1.0f. The new code only checks zoomLevel != 1.0f and the scroll check is commented out. This means the "Reset view" button will not appear when the user scrolls horizontally without zooming, which is a behavioral regression. If the scroll check was intentionally dropped, the comment should be removed; otherwise, page.scroll should be included in the condition.

Copilot · 2026-03-09T10:21:23Z

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

+    private fun handlePenSettingChange(pen: Pen, setting: PenSetting) {
+        _toolbarState.update { state ->
+            val newSettings = state.penSettings.toMutableMap()
+            newSettings[pen.penName] = setting
+            sendUiEvent(EditorUiEvent.PenSettingChanged(pen, setting))
+            state.copy(penSettings = newSettings)
+        }


Same issue as in handlePenChange: sendUiEvent() is called inside the _toolbarState.update { } lambda. Since update may retry the lambda on CAS conflicts, the PenSettingChanged event could be emitted multiple times. Move the sendUiEvent call outside the update block.

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

Copilot · 2026-03-09T10:21:23Z

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

                PenToolbarButton(
-                    onStrokeMenuOpenChange = { state.isDrawing = !it },
                    pen = Pen.BALLPEN,
                    icon = R.drawable.ballpen,
-                    isSelected = isSelected(state, Pen.BALLPEN),
-                    onSelect = { handleChangePen(Pen.BALLPEN) },
+                    isSelected = isSelected(uiState, Pen.BALLPEN),
+                    onSelect = { onAction(ToolbarAction.ChangePen(Pen.BALLPEN)) },
                    sizes = SIZES_STROKES_DEFAULT,
-                    penSetting = state.penSettings[Pen.BALLPEN.penName] ?: return,
-                    onChangeSetting = { onChangeStrokeSetting(Pen.BALLPEN.penName, it) })
+                    penSetting = uiState.penSettings[Pen.BALLPEN.penName] ?: PenSetting(
+                        5f,
+                        android.graphics.Color.BLACK
+                    ),
+                    onChangeSetting = { onAction(ToolbarAction.ChangePenSetting(Pen.BALLPEN, it)) })


The onStrokeMenuOpenChange callback is no longer passed to PenToolbarButton (it defaults to null). In the old code, this callback was used to toggle state.isDrawing when pen stroke menus opened/closed. Without it, PenToolbarButton's internal stroke menu open/close state is no longer communicated to the ViewModel, so updateDrawingState() won't know a stroke menu popup is open. This means drawing input won't be disabled while a pen's stroke selection popup is visible, potentially allowing the user to draw through the popup.

The EraserToolbarButton was properly refactored to hoist its menu state (via isMenuOpen/onMenuOpenChange parameters), but PenToolbarButton was not given the same treatment. Consider either hoisting PenToolbarButton's menu state the same way, or at minimum passing the onStrokeMenuOpenChange callback to update the ViewModel's isStrokeSelectionOpen.

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

…backgrounds

…or in pageDrawing.kt

Ethran added 2 commits March 7, 2026 19:48

refactor Toolbar to use a decoupled UI state and content function

48f46a5

Ethran requested a review from Copilot March 7, 2026 21:05

Copilot started reviewing on behalf of Ethran March 7, 2026 21:05 View session

Copilot AI reviewed Mar 7, 2026

View reviewed changes

Ethran and others added 7 commits March 7, 2026 22:37

Apply suggestions from code review

3bfcf27

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Toolbar.kt refactored pen selection logic into isSelected function

a124a13

ToolbarMenu simplified export parameters and added preview

e897157

move Topbar.kt to ui package

0e1b967

move PositionedToolbar to its own file

72ef977

create the EditorViewModel.kt

0c21f98

Ethran requested a review from Copilot March 8, 2026 15:57

Copilot started reviewing on behalf of Ethran March 8, 2026 15:57 View session

Copilot AI reviewed Mar 8, 2026

View reviewed changes

Ethran added 2 commits March 8, 2026 18:19

implement reloadFromDb event and rename restartAfterConfChange to rei…

89e998d

…nitSignal

Ethran requested a review from Copilot March 8, 2026 19:22

Copilot AI reviewed Mar 8, 2026

View reviewed changes

Ethran requested a review from Copilot March 8, 2026 21:45

Copilot started reviewing on behalf of Ethran March 8, 2026 21:45 View session

Copilot AI reviewed Mar 8, 2026

View reviewed changes

Ethran added 5 commits March 9, 2026 10:04

code formating

3d15d79

EditorViewModel: refactor handleEraserChange and add stroke selection…

096848f

… logic

change from log.e to log.d

4c1d81a

change from log.e to log.d

b24670b

add comments

df893fb

Ethran requested a review from Copilot March 9, 2026 10:13

Copilot started reviewing on behalf of Ethran March 9, 2026 10:13 View session

Copilot AI reviewed Mar 9, 2026

View reviewed changes

Ethran added 5 commits March 9, 2026 11:42

EditorViewModel: calculate and update background page number for PDF …

9d152ac

…backgrounds

PageView.kt updated background caching and logging; changed debug col…

67aec53

…or in pageDrawing.kt

change log level

c1ffe2d

correct typo

d219b89

move side-effects out of _toolbarState.update in EditorViewModel

5321daa

Ethran merged commit e31c488 into main Mar 9, 2026
1 check passed

Uh oh!

Conversation

Ethran commented Mar 7, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 8, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Mar 8, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants