Move loading from persistent layer from PageView to PageDataManager & fix problems with quick nav #224
Conversation
…odel - Refactor `loadBookData` to be a suspend function that returns a success boolean. - Update `EditorView` to handle missing pages by triggering a notebook fix and navigating to the library. - Implement centralized error logging and snackbar notifications via `SnackState.logAndShowError`. - Add debug logging to `CanvasObserverRegistry` and `QuickNavViewModel` for canvas restoration and scrubbing. - Simplify `EditorView` logic by using a single entry point for book data loading based on `pageId`.
…- still not perfect, but better. --AI-- - Implement `DisposableEffect` in `EditorView` to properly register and unregister `EditorControlTower` observers. - Add lifecycle-aware observer management in `EditorControlTower` to prevent redundant job creation and ensure cleanup. - Refactor page transition logic in `EditorViewModel` and `EditorControlTower` to use `_toolbarState` updates instead of direct database calls. - Prevent duplicate page change emissions in `QuickNavViewModel` by tracking the last target page ID during scrubber gestures. - Add debug logging for page loading in `PageView` and navigation events in `NotableNavHost`.
…- still not perfect, but better. --AI-- - Implement `DisposableEffect` in `EditorView` to properly register and unregister `EditorControlTower` observers. - Add lifecycle-aware observer management in `EditorControlTower` to prevent redundant job creation and ensure cleanup. - Refactor page transition logic in `EditorViewModel` and `EditorControlTower` to use `_toolbarState` updates instead of direct database calls. - Prevent duplicate page change emissions in `QuickNavViewModel` by tracking the last target page ID during scrubber gestures. - Add debug logging for page loading in `PageView` and navigation events in `NotableNavHost`.
- Move `EditorSettingCacheManager` dependency and settings persistence logic from `EditorView` to `EditorViewModel`. - Update `EditorViewModel` to handle its own initialization and saving of editor settings. - Improve `CanvasObserverRegistry` performance by moving pen change observations to `Dispatchers.Default` and removing redundant UI refreshes. - Fix a potential crash in `DrawCanvas` by removing an unnecessary `glRenderer.release()` call during initialization. - Add enhanced debug logging for failed bitmap decoding in `persistBitmap.kt`. - Clean up unused `EditorSettingCacheManager` injections and parameters across `MainActivity`, `NotableApp`, and `NotableNavHost`.
- Call `saveToolbarState()` when toggling toolbar visibility or changing modes. - Update `handlePenChange`, `handleEraserChange`, and `handlePenSettingChange` to persist state updates to the cache. - Remove obsolete TODO regarding toolbar state persistence.
…cted with db from PageView.kt to PageDataManager.kt. Now PageDataManager.kt is responsible for connecting to db. - Refactor `PageDataManager` from an object to a Hilt-injectable `@Singleton` class. - Move business logic for database persistence (strokes, images, scroll) from `PageView` and `CanvasObserverRegistry` into `PageDataManager`. - Update `EditorViewModel` to manage `PageDataManager` and handle cleanup via a new `onDispose` method. - Simplify `PageView` by delegating data loading, background management, and coordinate state to `PageDataManager`. - Remove manual repository and export engine passing in `EditorView` and `NotableNavHost`, favoring ViewModel injection. - Improve lifecycle management by registering component callbacks for memory management in `MainActivity` via Hilt. - Fix various logging and nullability issues in `einkHelper` and `backgrounds.kt`.
Ethran
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors editor page data loading/caching responsibilities by moving persistence-layer interactions out of PageView into an injected PageDataManager, and adjusts QuickNav + navigation/editor wiring to reduce page-switch glitches.
Changes:
- Convert
PageDataManagerfrom a globalobjectto an injected@Singletonservice and migrate page loading/background/DB update responsibilities into it. - Rework editor navigation and page switching flow (
NotableNavHost→EditorView→EditorViewModel/PageView) and add guards against duplicate QuickNav scrub commits. - Update bug-report generation to include
PageDataManagerstate (memory usage) and fix package placement forBugReportGenerator.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| app/src/main/java/com/ethran/notable/ui/viewmodels/QuickNavViewModel.kt | Adds duplicate scrub-end guard and logging for QuickNav scrub commit. |
| app/src/main/java/com/ethran/notable/ui/viewmodels/LibraryViewModel.kt | Uses injected PageDataManager to cancel background page loading when switching folders. |
| app/src/main/java/com/ethran/notable/ui/viewmodels/BugReportViewModel.kt | Migrates to @HiltViewModel and injects PageDataManager into bug report generation. |
| app/src/main/java/com/ethran/notable/ui/viewmodels/BugReportGenerator.kt | Moves package + adds PageDataManager to include cache/memory info in reports. |
| app/src/main/java/com/ethran/notable/ui/components/NotableApp.kt | Removes EditorSettingCacheManager plumbing from app root and simplifies NotableNavHost call. |
| app/src/main/java/com/ethran/notable/navigation/NotableNavigator.kt | Adds navigation helpers for Bug Report and Pages destinations. |
| app/src/main/java/com/ethran/notable/navigation/NotableNavHost.kt | Updates EditorView API to use navigation callbacks + initialPageId. |
| app/src/main/java/com/ethran/notable/editor/utils/persistBitmap.kt | Adds extra diagnostic logging on preview decode failure. |
| app/src/main/java/com/ethran/notable/editor/utils/einkHelper.kt | Adds logging/branching changes in resetScreenFreeze. |
| app/src/main/java/com/ethran/notable/editor/utils/Select.kt | Routes rectangle selection queries through page.pageDataManager. |
| app/src/main/java/com/ethran/notable/editor/ui/EditorSurface.kt | Removes unused appRepository plumbing from surface/canvas creation. |
| app/src/main/java/com/ethran/notable/editor/drawing/pageDrawing.kt | Uses PageDataManager for background type/name during drawing. |
| app/src/main/java/com/ethran/notable/editor/drawing/backgrounds.kt | Logger renaming and adds a background-load log call in drawBg. |
| app/src/main/java/com/ethran/notable/editor/canvas/DrawCanvas.kt | Removes repository dependency from canvas observer registry and reworks init. |
| app/src/main/java/com/ethran/notable/editor/canvas/CanvasRefreshManager.kt | Adds refresh logging and adjusts formatting. |
| app/src/main/java/com/ethran/notable/editor/canvas/CanvasObserverRegistry.kt | Moves QuickNav preview page-number resolution into PageDataManager; tweaks observers/dispatchers. |
| app/src/main/java/com/ethran/notable/editor/PageView.kt | Central refactor to delegate persistence and loading logic to injected PageDataManager. |
| app/src/main/java/com/ethran/notable/editor/EditorViewModel.kt | Adds injected PageDataManager + EditorSettingCacheManager usage; reworks toolbar loading, export/snack handling. |
| app/src/main/java/com/ethran/notable/editor/EditorView.kt | Updates API to navigation callbacks; reworks observer lifecycle and page switching pipeline. |
| app/src/main/java/com/ethran/notable/editor/EditorControlTower.kt | Adds observer job tracking + explicit unregister to avoid duplicate collectors. |
| app/src/main/java/com/ethran/notable/data/PageDataManager.kt | Converts to injected singleton class and absorbs DB + caching responsibilities previously in PageView. |
| app/src/main/java/com/ethran/notable/MainActivity.kt | Injects PageDataManager and registers callbacks after permission gating; removes editor settings init plumbing. |
Comment on lines
+440
to
+453
| suspend fun loadToolbarState(bookId: String?, pageId: String) { | ||
| log.v("loadBookData: bookId=$bookId, pageId=$pageId") | ||
| this.bookId = bookId | ||
|
|
||
| viewModelScope.launch(Dispatchers.IO) { | ||
| val page = appRepository.pageRepository.getById(pageId) | ||
| val book = bookId?.let { appRepository.bookRepository.getById(it) } | ||
|
|
||
| val pageIndex = book?.getPageIndex(pageId) ?: 0 | ||
| val totalPages = book?.pageIds?.size ?: 1 | ||
| val page = appRepository.pageRepository.getById(pageId) | ||
| if (page == null) { | ||
| logAndShowError( | ||
| reason = "EditorViewModel", | ||
| message = "Could not find page", | ||
| ) | ||
| fixNotebook(bookId, pageId) | ||
| return | ||
| } | ||
| val book = bookId?.let { appRepository.bookRepository.getById(it) } |
There was a problem hiding this comment.
loadToolbarState performs Room repository reads directly in the caller context. Since it’s called from LaunchedEffect (Main dispatcher), this risks doing DB work on the UI thread. Move the repository calls into withContext(Dispatchers.IO) (or have the caller launch on IO) and only update _toolbarState on the main thread if needed.
app/src/main/java/com/ethran/notable/editor/utils/persistBitmap.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/ethran/notable/editor/canvas/CanvasObserverRegistry.kt
Show resolved
Hide resolved
app/src/main/java/com/ethran/notable/editor/canvas/CanvasObserverRegistry.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/ethran/notable/ui/viewmodels/QuickNavViewModel.kt
Outdated
Show resolved
Hide resolved
Comment on lines
+694
to
+697
| suspend fun getPageNumberInCurrentNotebook(pageId: String): Int { | ||
| val pageNumber = | ||
| appRepository.getPageNumber(pageFromDb?.notebookId!!, pageId) | ||
| log.d("Page number for page($pageNumber): $pageId") |
There was a problem hiding this comment.
getPageNumberInCurrentNotebook() force-unwraps pageFromDb?.notebookId!!. If setPage() hasn’t populated pageFromDb yet (or the current page failed to load), this will crash during QuickNav preview. Avoid !! here; either requireNotNull(pageFromDb?.notebookId) with a clear message, or accept/pass the notebookId explicitly.
- Update `PageView` to use the new `setCurrentBackground` method in `PageDataManager`. - Implement `setCurrentBackground` in `PageDataManager` to simplify setting the background for the current page. - Comment out the implementation of `fixNotebook` in `EditorViewModel` and add a TODO for further review. - Refine background file observation logic in `PageDataManager`.
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
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.
No description provided.