Skip to content

Improve toolbar code quality #219

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 21 commits into from
Mar 9, 2026
Merged

Improve toolbar code quality #219

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
6a0a0d7
Refactor `Toolbar` and `ToolbarMenu` to use callback functions instea…
Ethran Mar 7, 2026
48f46a5
refactor Toolbar to use a decoupled UI state and content function
Ethran Mar 7, 2026
3bfcf27
Apply suggestions from code review
Ethran Mar 7, 2026
a124a13
Toolbar.kt refactored pen selection logic into isSelected function
Ethran Mar 7, 2026
e897157
ToolbarMenu simplified export parameters and added preview
Ethran Mar 7, 2026
0e1b967
move Topbar.kt to ui package
Ethran Mar 8, 2026
72ef977
move PositionedToolbar to its own file
Ethran Mar 8, 2026
0c21f98
create the EditorViewModel.kt
Ethran Mar 8, 2026
c388c05
Refactor toolbar logic into `EditorViewModel` using a modern MVI-patt…
Ethran Mar 8, 2026
89e998d
implement reloadFromDb event and rename restartAfterConfChange to rei…
Ethran Mar 8, 2026
834e4f4
Refactor menu and drawing state management to EditorViewModel
Ethran Mar 8, 2026
3d15d79
code formating
Ethran Mar 9, 2026
096848f
EditorViewModel: refactor handleEraserChange and add stroke selection…
Ethran Mar 9, 2026
4c1d81a
change from log.e to log.d
Ethran Mar 9, 2026
b24670b
change from log.e to log.d
Ethran Mar 9, 2026
df893fb
add comments
Ethran Mar 9, 2026
9d152ac
EditorViewModel: calculate and update background page number for PDF …
Ethran Mar 9, 2026
67aec53
PageView.kt updated background caching and logging; changed debug col…
Ethran Mar 9, 2026
c1ffe2d
change log level
Ethran Mar 9, 2026
d219b89
correct typo
Ethran Mar 9, 2026
5321daa
move side-effects out of _toolbarState.update in EditorViewModel
Ethran Mar 9, 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
2 changes: 1 addition & 1 deletion app/src/main/java/com/ethran/notable/MainActivity.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ class MainActivity : ComponentActivity() {
super.onRestart()
// redraw after device sleep
this.lifecycleScope.launch {
CanvasEventBus.restartAfterConfChange.emit(Unit)
CanvasEventBus.reinitSignal.emit(Unit)
}
}

Expand Down
195 changes: 134 additions & 61 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
@@ -1,33 +1,33 @@
package com.ethran.notable.editor

import androidx.compose.foundation.layout.BoxWithConstraints
import androidx.compose.foundation.layout.Column
import androidx.compose.foundation.layout.Row
import androidx.compose.foundation.layout.Spacer
import androidx.compose.foundation.layout.fillMaxHeight
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.ui.Modifier
import androidx.compose.ui.platform.LocalContext
import androidx.hilt.lifecycle.viewmodel.compose.hiltViewModel
import androidx.navigation.NavController
import com.ethran.notable.data.AppRepository
import com.ethran.notable.data.datastore.AppSettings
import com.ethran.notable.data.datastore.EditorSettingCacheManager
import com.ethran.notable.data.datastore.GlobalAppSettings
import com.ethran.notable.editor.canvas.CanvasEventBus
import com.ethran.notable.editor.state.EditorState
import com.ethran.notable.editor.state.History
import com.ethran.notable.editor.ui.EditorSurface
import com.ethran.notable.editor.ui.HorizontalScrollIndicator
import com.ethran.notable.editor.ui.ScrollIndicator
import com.ethran.notable.editor.ui.SelectedBitmap
import com.ethran.notable.editor.ui.toolbar.Toolbar
import com.ethran.notable.editor.ui.toolbar.PositionedToolbar
import com.ethran.notable.gestures.EditorGestureReceiver
import com.ethran.notable.io.ExportEngine
import com.ethran.notable.io.exportToLinkedFile
Expand All @@ -37,7 +37,9 @@ import com.ethran.notable.ui.SnackConf
import com.ethran.notable.ui.SnackState
import com.ethran.notable.ui.convertDpToPixel
import com.ethran.notable.ui.theme.InkaTheme
import com.ethran.notable.ui.views.BugReportDestination
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.launch
Expand Down Expand Up @@ -71,14 +73,16 @@ fun EditorView(
appRepository: AppRepository,
bookId: String?,
pageId: String,
onPageChange: (String) -> Unit
onPageChange: (String) -> Unit,
viewModel: EditorViewModel = hiltViewModel()
) {
val context = LocalContext.current
val snackManager = LocalSnackContext.current
val scope = rememberCoroutineScope()

var pageExists by remember(pageId) { mutableStateOf<Boolean?>(null) }
LaunchedEffect(pageId) {
viewModel.loadBookData(bookId, pageId)
val exists = withContext(Dispatchers.IO) {
appRepository.pageRepository.getById(pageId) != null
}
Expand All @@ -92,8 +96,7 @@ fun EditorView(
log.i("Could not find page, Cleaning book")
SnackState.globalSnackFlow.tryEmit(
SnackConf(
text = "Could not find page, cleaning book",
duration = 4000
text = "Could not find page, cleaning book", duration = 4000
)
)
scope.launch(Dispatchers.IO) {
Expand All @@ -104,7 +107,6 @@ fun EditorView(
}
}


if (pageExists == null) return

BoxWithConstraints {
Expand All @@ -124,17 +126,16 @@ fun EditorView(
)
}

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

val history = remember {
History(page)
Expand All @@ -143,13 +144,121 @@ fun EditorView(
EditorControlTower(scope, page, history, editorState).apply { registerObservers() }
}

// Collect UI Events from ViewModel
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))
}

is EditorUiEvent.NavigateToPages -> {
navController.navigate(PagesDestination.createRoute(event.bookId))
}

EditorUiEvent.NavigateToBugReport -> {
navController.navigate(BugReportDestination.route)
}

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

is EditorUiEvent.CopyImageToCanvas -> {
CanvasEventBus.addImageByUri.value = event.uri
}

EditorUiEvent.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
}
}
}
}

// Handle Canvas signals in UI
LaunchedEffect(Unit) {
CanvasEventBus.closeMenusSignal.collect {
log.d("Closing all menus")
viewModel.onToolbarAction(ToolbarAction.CloseAllMenus)
}
}

// Handle focus changes from Canvas
LaunchedEffect(Unit) {
CanvasEventBus.onFocusChange.collect { hasFocus ->
log.d("Canvas has focus: $hasFocus")
viewModel.onFocusChanged(hasFocus)
}
}

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

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
val selectionActive = editorState.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
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
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)
if (bookId != null)
exportToLinkedFile(exportEngine, bookId, appRepository.bookRepository)
if (bookId != null) exportToLinkedFile(
exportEngine,
bookId,
appRepository.bookRepository
)
page.disposeOldPage()
}
}
Expand All @@ -160,7 +269,6 @@ fun EditorView(
editorState.pen,
editorState.penSettings,
editorState.mode,
editorState.isToolbarOpen,
editorState.eraser
) {
log.i("EditorView: saving")
Expand All @@ -180,14 +288,10 @@ fun EditorView(
InkaTheme {
EditorGestureReceiver(controlTower = editorControlTower)
EditorSurface(
appRepository = appRepository,
state = editorState,
page = page,
history = history
appRepository = appRepository, state = editorState, page = page, history = history
)
SelectedBitmap(
context = context,
controlTower = editorControlTower
context = context, controlTower = editorControlTower
)
Row(
modifier = Modifier
Expand All @@ -197,40 +301,9 @@ fun EditorView(
Spacer(modifier = Modifier.weight(1f))
ScrollIndicator(state = editorState)
}
PositionedToolbar(exportEngine,navController, appRepository, editorState, editorControlTower)
PositionedToolbar(
viewModel = viewModel, onDrawingStateCheck = { viewModel.updateDrawingState() })
HorizontalScrollIndicator(state = editorState)
}
}
}


@Composable
fun PositionedToolbar(
exportEngine: ExportEngine,
navController: NavController,
appRepository: AppRepository,
editorState: EditorState,
editorControlTower: EditorControlTower
) {
val position = GlobalAppSettings.current.toolbarPosition

when (position) {
AppSettings.Position.Top -> {
Toolbar(
exportEngine,
navController, appRepository, editorState, editorControlTower
)
}

AppSettings.Position.Bottom -> {
Column(
Modifier
.fillMaxWidth()
.fillMaxHeight()
) {
Spacer(modifier = Modifier.weight(1f))
Toolbar(exportEngine, navController, appRepository, editorState, editorControlTower)
}
}
}
}
Loading
Loading