feat(api): sync reading progress bidirectionally between KOReader and Grimmory web reader#305
Conversation
… Grimmory web reader Add reverse sync path: when the web reader saves EPUB progress, convert CFI to XPointer via EpubCfiService.convertCfiToProgressXPointer() and write to koreader_* columns on UserBookProgressEntity. KOReader picks up the updated position on next sync. Rename the sync toggle endpoint from /sync-progress-with-booklore to /sync-progress-with-grimmory, fixing the PATCH 405 error (grimmory-tools#191). Remove the legacy endpoint and the frontend's catchError fallback. Rename the DTO field syncWithBookloreReader to syncWithGrimmoryReader with a MapStruct @mapping to bridge the entity's unchanged column name. Closes grimmory-tools#225 Fixes grimmory-tools#191
This comment was marked as resolved.
This comment was marked as resolved.
|
Did you test this? Besides automated testing, I mean? |
|
Seems suspiciously lean PR considering what it the PR message claims to have achieved. Are you sure we are not overgeneralising this? Intuitively, I feel like there is much more to this than what's proposed here. I'm 100% ready to be proven absolutely wrong. But this PR may need more consideration than "LGTM! -> Merge" |
zachyale
left a comment
There was a problem hiding this comment.
As progress conversion is a bit lossy, and can fall back to chapter-level accuracy instead of page-level accuracy, have you experienced a loss in position accuracy when using just the web reader with this approach?
For ex:
- Open web reader
- Progress is pulled from latest koreader progress
- Progress is completed during the course of the reading session
- Web reader is closed
- Some time later (without having read on another device), the user opens the web reader
- Progress is pulled from latest Koreader progress
In this case would progress not be lost/degraded by using the newly updated Koreader position instead of the more accurate Grimmory stored CFI?
| public ResponseEntity<Void> toggleSyncProgressWithBooklore( | ||
| public ResponseEntity<Void> toggleSyncProgressWithGrimmory( | ||
| @Parameter(description = "Enable or disable sync progress") @RequestParam boolean enabled) { | ||
| koreaderUserService.toggleSyncProgressWithBooklore(enabled); |
There was a problem hiding this comment.
If you're going to rename/complete the deprecation of the booklore related endpoint, since this endpoint is the only implementation of the toggleSyncProgressWithBooklore method we should either also rename it here, or create an issue to rename the implements within the KoreaderUserService
|
So sorry, had a bit of a fallout at work and I will test it more thoroughly and reply to all comments during the weekend |
Summary
KOReader XPointer format and written to the KOReader progress columns, so KOReader picks it up on next sync.
/sync-progress-with-bookloreto/sync-progress-with-grimmory, fixing the PATCH 405 error (Sync reading progress with Grimmory eBook Reader PATCH error #191) caused by the frontend calling the new namewhile the backend only had the old one.
/sync-progress-with-bookloreendpoint and the frontend'scatchErrorfallback to it.What changed
Backend (4 production files):
KoreaderUserController.java/sync-progress-with-grimmory, removed legacy/sync-progress-with-bookloreReadingProgressService.javasyncProgressToKoreader()— converts CFI → XPointer viaEpubCfiService.convertCfiToProgressXPointer()and writes tokoreader_*columns whenKoreaderUser.javasyncWithBookloreReader→syncWithGrimmoryReaderKoreaderUserMapper.java@Mappingto bridge entity field name (syncWithBookloreReader) to DTO field name (syncWithGrimmoryReader)Frontend (3 files):
koreader.service.tscatchErrorfallback to legacy endpoint, removedsyncWithBookloreReaderfrom interfacekoreader-settings-component.ts?? koreaderUser.syncWithBookloreReaderfallbackkoreader.service.spec.tsNo database migrations. All required columns already exist.
How it works
The forward sync (KOReader → web reader) already existed —
KoreaderService.saveProgress()converts XPointer → CFI when the sync toggle is enabled. This PR adds the reverse path:ReadingProgressService.updateReadProgress()saves CFI + percentagesyncWithBookloreReader = true:EpubCfiService.convertCfiToProgressXPointer()koreader_progress,koreader_progress_percent,koreader_device = "Grimmory",koreader_last_sync_time = nowGET /api/koreader/syncs/progress/{hash}returns the web reader's positionPosition conversion is chapter-approximate, not character-exact. This is documented in the existing UI description for the toggle.
Test output
Backend —
just api test(3496 tests, 0 failures):BUILD SUCCESSFUL in 2m 24s
6 actionable tasks: 4 executed, 2 up-to-date
Frontend —
just ui check(1421 tests, 0 failures):Test Files 271 passed | 106 skipped (377)
Tests 1421 passed | 173 skipped (1594)
New test coverage (6 tests added)
updateReadProgress_withKoreaderSyncEnabled_shouldWriteKoreaderColumnsupdateReadProgress_withKoreaderSyncDisabled_shouldNotWriteKoreaderColumnsupdateReadProgress_withNoKoreaderAccount_shouldNotAttemptSyncupdateReadProgress_conversionFailure_shouldNotBreakSavetoggleSyncProgressWithGrimmory_returnsNoContentkoreader.service.spec: patches the Grimmory sync-progress toggleKnown limitations
convertCfiToProgressXPointer()existed but was previously unused — validated by new tests.syncWithBookloreReader(DB column:sync_with_booklore_reader). A follow-up Flyway migration to rename it is tracked asTODO(grimmory-rename)inthe mapper.
epubProgress/epubProgressPercentwhich are@Deprecatedon the entity. Tracked asTODO(deprecated-fields)in the sync method.Summary by CodeRabbit
New Features
Changes