Conversation
…en using observe pdf.
### Page Management & Backgrounds
- **PageDataManager**:
- Added a safety check in `setBackground` to prevent writing null bitmaps to the cache and emit a `GenericError` event if encountered.
- Enhanced `observeBackgroundFile` to handle file deletion and recreation more robustly. It now re-registers the `FileObserver` when a file is replaced.
- Optimized `invalidateFileFlow` emissions to trigger only on `CLOSE_WRITE` or `MOVED_TO` events, ensuring file stability before background refreshes.
- **renderFromFile**: Added bitmap state information to the "loaded background" performance log.
### File Operations
- **FileUtils**:
- Improved `waitForFileAvailable` with detailed logging for file existence and size updates during the polling period.
### Infrastructure
- Removed an unused `coroutineScope` import in `PageDataManager`.
There was a problem hiding this comment.
Pull request overview
Fixes cases where page backgrounds (notably PDFs) fail to refresh correctly when the underlying PDF/background file is replaced/rewritten, by hardening background caching and file observation behavior.
Changes:
- Add null-bitmap guard in
PageDataManager.setBackgroundand improve background file observation (delete/recreate handling + emit only when file is stable). - Add additional logging to
waitForFileAvailableto aid diagnosing file availability races. - Add bitmap info to the “loaded background” timing log.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| app/src/main/java/com/ethran/notable/io/renderFromFile.kt | Expands the performance timing log for background loading. |
| app/src/main/java/com/ethran/notable/io/FileUtils.kt | Adds debug logging around file availability polling. |
| app/src/main/java/com/ethran/notable/data/PageDataManager.kt | Prevents caching null backgrounds and improves FileObserver handling for background updates. |
| val msg = "setBackground: skipping cache write, bitmap is null (id=${background.id})" | ||
| log.w(msg) | ||
| appEventBus.tryEmit( | ||
| AppEvent.GenericError( | ||
| msg | ||
| ) | ||
| ) |
There was a problem hiding this comment.
setBackground() emits AppEvent.GenericError when background.bitmap is null. Since GenericError is user-facing (snackbar) and setBackground() can be hit repeatedly during rendering/zoom while a file is temporarily unavailable, this risks spamming users for transient conditions. Consider downgrading this to logging (or a throttled/once-per-file ActionHint) and only emitting a user-visible error after repeated/persistent failures.
| val msg = "setBackground: skipping cache write, bitmap is null (id=${background.id})" | |
| log.w(msg) | |
| appEventBus.tryEmit( | |
| AppEvent.GenericError( | |
| msg | |
| ) | |
| ) | |
| log.w("setBackground: skipping cache write, bitmap is null (id=${background.id})") |
| if (event == IN_IGNORED) | ||
| return@launch | ||
| val eventString = fileObserverEventNames(event) | ||
|
|
||
| log.d("Background file changed: $filePath [event=$eventString]") | ||
|
|
||
| // HANDLE DELETION / RE-REGISTRATION | ||
| if (event == DELETE || event == DELETE_SELF) { | ||
| log.d("Background file deleted.") | ||
| log.d("Background file deleted. Waiting for recreation...") | ||
| synchronized(fileObservers) { |
There was a problem hiding this comment.
FileObserver.onEvent's event is a bitmask; comparing with == can miss events when extra flags are present. The checks like event == IN_IGNORED and event == DELETE/DELETE_SELF should use bitwise tests (e.g., event and DELETE != 0) to be reliable.
| // Only emit when the file is ready to be read (CLOSE_WRITE or MOVED_TO) | ||
| if (event == CLOSE_WRITE || event == MOVED_TO) { | ||
| log.d("File is stable. Invalidating flow for: $filePath") | ||
| invalidateFileFlow.emit(filePath) | ||
| } |
There was a problem hiding this comment.
Same bitmask issue for the stability gate: event == CLOSE_WRITE || event == MOVED_TO should use bitwise checks, otherwise a combined event value may skip invalidation and the background won't refresh.
| else | ||
| renderPdfPageAndroid(file, pageNumber, targetWidth.toInt(), resolutionModifier = 1.2f) | ||
| timer.end("loaded background") | ||
| timer.end("loaded background, newBitmap: $newBitmap") |
There was a problem hiding this comment.
Timing.end()'s label is now interpolating the Bitmap object ($newBitmap). This will produce a different log line every time (and usually only an object id), which makes timing logs harder to aggregate/grep and adds avoidable string allocation. Keep the timing label stable (e.g., "loaded background") and log bitmap details (size/config/isRecycled/null) in a separate log statement if needed.
…en using observe pdf.
Page Management & Backgrounds
setBackgroundto prevent writing null bitmaps to the cache and emit aGenericErrorevent if encountered.observeBackgroundFileto handle file deletion and recreation more robustly. It now re-registers theFileObserverwhen a file is replaced.invalidateFileFlowemissions to trigger only onCLOSE_WRITEorMOVED_TOevents, ensuring file stability before background refreshes.File Operations
waitForFileAvailablewith detailed logging for file existence and size updates during the polling period.Infrastructure
coroutineScopeimport inPageDataManager.