Skip to content

Improve Editor code quality, migrate editor state management to EditorViewModel #220

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Ethran merged 10 commits into from
Mar 10, 2026
Merged

Improve Editor code quality, migrate editor state management to EditorViewModel #220

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
ed3016b
migrate editor state management to `EditorViewModel`
Ethran Mar 9, 2026
0817c81
cleanup
Ethran Mar 9, 2026
604c2c4
Refactor EditorViewModel: StateFlow single source of truth, fix page …
Copilot Mar 10, 2026
9afa2a5
fix for canvas init.
Ethran Mar 10, 2026
0a9ef64
refactor page change observation to use snapshotFlow in EditorView
Ethran Mar 10, 2026
639ee31
refactor page navigation logic and state management
Ethran Mar 10, 2026
83a905e
update pen and stroke initialization logic
Ethran Mar 10, 2026
4e8ef6b
add logging and improve selection state handling
Ethran Mar 10, 2026
5fc6226
add logging and improve selection state handling
Ethran Mar 10, 2026
52ce406
sync EditorViewModel with quick navigation visibility and update draw…
Ethran Mar 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 11 additions & 28 deletions app/src/main/java/com/ethran/notable/editor/EditorControlTower.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ import com.ethran.notable.editor.state.EditorState
import com.ethran.notable.editor.state.History
import com.ethran.notable.editor.state.HistoryBusActions
import com.ethran.notable.editor.state.Mode
import com.ethran.notable.editor.state.Operation
import com.ethran.notable.editor.state.PlacementMode
import com.ethran.notable.editor.state.Operation
import com.ethran.notable.editor.state.SelectionState
import com.ethran.notable.editor.state.UndoRedoType
import com.ethran.notable.editor.utils.divideStrokesFromCut
Expand Down Expand Up @@ -90,19 +90,9 @@ class EditorControlTower(
* @param id The unique identifier of the page to switch to.
*/
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()
}
Comment on lines 92 to 97
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.

Expand All @@ -121,11 +111,11 @@ class EditorControlTower(
}

fun toggleTool() {
state.mode = if (state.mode == Mode.Draw) Mode.Erase else Mode.Draw
state.viewModel.onToolbarAction(ToolbarAction.ChangeMode(if (state.mode == Mode.Draw) Mode.Erase else Mode.Draw))
}

fun toggleZen() {
state.isToolbarOpen = !state.isToolbarOpen
state.viewModel.onToolbarAction(ToolbarAction.ToggleToolbar)
}

fun getSnapshotOfSelectionState(): SelectionState {
Expand All @@ -137,21 +127,15 @@ class EditorControlTower(
}

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()
}
Comment on lines 129 to 133
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

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()
}
Comment on lines 135 to 139
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.

fun undo() {
Expand Down Expand Up @@ -325,5 +309,4 @@ class EditorControlTower(

showHint("Pasted content from clipboard", scope)
}
}

}
163 changes: 79 additions & 84 deletions app/src/main/java/com/ethran/notable/editor/EditorView.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,16 @@ import androidx.compose.foundation.layout.fillMaxWidth
import androidx.compose.runtime.Composable
import androidx.compose.runtime.DisposableEffect
import androidx.compose.runtime.LaunchedEffect
import androidx.compose.runtime.collectAsState
import androidx.compose.runtime.getValue
import androidx.compose.runtime.mutableStateOf
import androidx.compose.runtime.remember
import androidx.compose.runtime.rememberCoroutineScope
import androidx.compose.runtime.setValue
import androidx.compose.runtime.snapshotFlow
import androidx.compose.ui.Modifier
import androidx.compose.ui.platform.LocalContext
import androidx.hilt.lifecycle.viewmodel.compose.hiltViewModel
import androidx.lifecycle.compose.collectAsStateWithLifecycle
import androidx.navigation.NavController
import com.ethran.notable.data.AppRepository
import com.ethran.notable.data.datastore.EditorSettingCacheManager
Expand All @@ -42,6 +43,9 @@ import com.ethran.notable.ui.views.LibraryDestination
import com.ethran.notable.ui.views.PagesDestination
import io.shipbook.shipbooksdk.ShipBook
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.flow.distinctUntilChanged
import kotlinx.coroutines.flow.drop
import kotlinx.coroutines.flow.filterNotNull
import kotlinx.coroutines.launch
import kotlinx.coroutines.withContext

Expand Down Expand Up @@ -73,6 +77,7 @@ fun EditorView(
appRepository: AppRepository,
bookId: String?,
pageId: String,
isQuickNavOpen: Boolean,
onPageChange: (String) -> Unit,
viewModel: EditorViewModel = hiltViewModel()
) {
Expand Down Expand Up @@ -107,13 +112,17 @@ fun EditorView(
}
}

// Sync isQuickNavOpen to ViewModel
LaunchedEffect(isQuickNavOpen) {
viewModel.onToolbarAction(ToolbarAction.UpdateQuickNavOpen(isQuickNavOpen))
}

if (pageExists == null) return

BoxWithConstraints {
val height = convertDpToPixel(this.maxHeight, context).toInt()
val width = convertDpToPixel(this.maxWidth, context).toInt()


val page = remember {
PageView(
context = context,
Expand All @@ -126,37 +135,29 @@ fun EditorView(
)
}

val editorState = remember {
EditorState(
appRepository = appRepository,
bookId = bookId,
pageId = pageId,
pageView = page,
persistedEditorSettings = editorSettingCacheManager.getEditorSettings(),
onPageChange = onPageChange
)
}

val history = remember {
History(page)
}

// Create EditorState wrapper for backward compatibility
val editorState = remember(viewModel, page) {
EditorState(viewModel)
}

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

val editorControlTower = remember {
EditorControlTower(scope, page, history, editorState).apply { registerObservers() }
}

// Collect UI Events from ViewModel
// Collect UI Events from ViewModel (navigation and snackbars)
LaunchedEffect(Unit) {
viewModel.uiEvents.collect { event ->
when (event) {
is EditorUiEvent.Undo -> editorControlTower.undo()
is EditorUiEvent.Redo -> editorControlTower.redo()
is EditorUiEvent.Paste -> editorControlTower.pasteFromClipboard()
is EditorUiEvent.ResetView -> editorControlTower.resetZoomAndScroll()
is EditorUiEvent.ClearAllStrokes -> {
CanvasEventBus.clearPageSignal.emit(Unit)
snackManager.displaySnack(SnackConf(text = "Cleared all strokes"))
}

is EditorUiEvent.NavigateToLibrary -> {
navController.navigate(LibraryDestination.createRoute(event.folderId))
}
Expand All @@ -170,37 +171,31 @@ fun EditorView(
}

is EditorUiEvent.ShowSnackbar -> {
snackManager.displaySnack(SnackConf(text = event.message))
snackManager.displaySnack(SnackConf(text = event.message, duration = 2000))
}
}
}
}

is EditorUiEvent.CopyImageToCanvas -> {
CanvasEventBus.addImageByUri.value = event.uri
// Collect Canvas Commands from ViewModel
LaunchedEffect(Unit) {
viewModel.canvasCommands.collect { command ->
when (command) {
CanvasCommand.Undo -> editorControlTower.undo()
CanvasCommand.Redo -> editorControlTower.redo()
CanvasCommand.Paste -> editorControlTower.pasteFromClipboard()
CanvasCommand.ResetView -> editorControlTower.resetZoomAndScroll()
CanvasCommand.ClearAllStrokes -> {
CanvasEventBus.clearPageSignal.emit(Unit)
snackManager.displaySnack(SnackConf(text = "Cleared all strokes"))
}

EditorUiEvent.RefreshCanvas -> {
CanvasCommand.RefreshCanvas -> {
CanvasEventBus.reloadFromDb.emit(Unit)
}

is EditorUiEvent.ModeChanged -> {
editorState.mode = event.mode
}

is EditorUiEvent.PenChanged -> {
editorState.pen = event.pen
}

is EditorUiEvent.PenSettingChanged -> {
val newSettings = editorState.penSettings.toMutableMap()
newSettings[event.pen.penName] = event.setting
editorState.penSettings = newSettings
}

is EditorUiEvent.EraserChanged -> {
editorState.eraser = event.eraser
}

is EditorUiEvent.ToolbarVisibilityChanged -> {
editorState.isToolbarOpen = event.visible
is CanvasCommand.CopyImageToCanvas -> {
CanvasEventBus.addImageByUri.value = command.uri
}
}
}
Expand All @@ -222,38 +217,39 @@ fun EditorView(
}
}

// Sync legacy state to ViewModel for Toolbar rendering
val zoomLevel by page.zoomLevel.collectAsState()
val selectionActive = editorState.selectionState.isNonEmpty()
// Collect toolbar state and sync EditorState (keeps snapshotFlow observers in canvas alive)
val toolbarState by viewModel.toolbarState.collectAsStateWithLifecycle()
LaunchedEffect(toolbarState) {
editorState.syncFrom(toolbarState)
}

// Observe pageId changes from ViewModel state for navigation
LaunchedEffect(viewModel) {
snapshotFlow { toolbarState.pageId }
.filterNotNull()
.distinctUntilChanged()
.drop(1) // Skip initial emission from loadBookData
.collect { newPageId ->
onPageChange(newPageId)
}
}

// Sync PageView state to ViewModel for Toolbar rendering
val zoomLevel by page.zoomLevel.collectAsStateWithLifecycle()
val selectionActive = viewModel.selectionState.isNonEmpty()
LaunchedEffect(
zoomLevel,
page.scroll,
editorState.clipboard,
editorState.isToolbarOpen,
editorState.mode,
editorState.pen,
editorState.eraser,
editorState.penSettings,
selectionActive
) {
viewModel.setHasClipboard(editorState.clipboard != null)
viewModel.setShowResetView(zoomLevel != 1.0f) // page.scroll != Offset.Zero
log.v("EditorView: zoomLevel=$zoomLevel, selectionActive=$selectionActive")
viewModel.setShowResetView(zoomLevel != 1.0f)
viewModel.setSelectionActive(selectionActive)
viewModel.updateToolbarSettings(
ToolbarUiState(
isToolbarOpen = editorState.isToolbarOpen,
mode = editorState.mode,
pen = editorState.pen,
eraser = editorState.eraser,
penSettings = editorState.penSettings
)
)
}

DisposableEffect(Unit) {
onDispose {
// finish selection operation
editorState.selectionState.applySelectionDisplace(page)
viewModel.selectionState.applySelectionDisplace(page)
if (bookId != null) exportToLinkedFile(
exportEngine,
bookId,
Expand All @@ -263,28 +259,27 @@ fun EditorView(
}
}

// TODO put in editorSetting class
// Persist editor settings when they change
LaunchedEffect(
editorState.isToolbarOpen,
editorState.pen,
editorState.penSettings,
editorState.mode,
editorState.eraser
toolbarState.isToolbarOpen,
toolbarState.pen,
toolbarState.penSettings,
toolbarState.mode,
toolbarState.eraser
) {
log.i("EditorView: saving")
log.i("EditorView: saving editor settings")
editorSettingCacheManager.setEditorSettings(
EditorSettingCacheManager.EditorSettings(
isToolbarOpen = editorState.isToolbarOpen,
mode = editorState.mode,
pen = editorState.pen,
eraser = editorState.eraser,
penSettings = editorState.penSettings
isToolbarOpen = toolbarState.isToolbarOpen,
mode = toolbarState.mode,
pen = toolbarState.pen,
eraser = toolbarState.eraser,
penSettings = toolbarState.penSettings
)
)
}



InkaTheme {
EditorGestureReceiver(controlTower = editorControlTower)
EditorSurface(
Expand All @@ -299,11 +294,11 @@ fun EditorView(
.fillMaxHeight()
) {
Spacer(modifier = Modifier.weight(1f))
ScrollIndicator(state = editorState)
ScrollIndicator(viewModel = viewModel, page = page)
}
PositionedToolbar(
viewModel = viewModel, onDrawingStateCheck = { viewModel.updateDrawingState() })
HorizontalScrollIndicator(state = editorState)
HorizontalScrollIndicator(viewModel = viewModel, page = page)
}
}
}
Loading
Loading