diff --git a/app/src/main/kotlin/com/chmouel/liseur/data/liseursync/LiseurSyncPositionSync.kt b/app/src/main/kotlin/com/chmouel/liseur/data/liseursync/LiseurSyncPositionSync.kt index ac988e1e..4adbde81 100644 --- a/app/src/main/kotlin/com/chmouel/liseur/data/liseursync/LiseurSyncPositionSync.kt +++ b/app/src/main/kotlin/com/chmouel/liseur/data/liseursync/LiseurSyncPositionSync.kt @@ -45,6 +45,7 @@ import com.chmouel.liseur.domain.needsReconciling import com.chmouel.liseur.domain.readingStatusFor import com.chmouel.liseur.domain.reconcileReadingState import com.chmouel.liseur.reader.progress.ExactLocatorAnchor +import com.chmouel.liseur.reader.progress.ResourceAnchor import java.io.IOException import org.json.JSONArray import org.json.JSONObject @@ -192,10 +193,13 @@ class LiseurSyncPositionSync( val stored = progressDao.get(bookUrl) val exact = head.locatorJson.takeIf { - head.editionSha != null && - head.editionSha == alias.editionSha && - ExactLocatorAnchor.isExactJson(it) + sameEdition(head.editionSha, alias) && ExactLocatorAnchor.isExactJson(it) } + // What the reader is *told* comes from the exact anchor; where + // they are actually put comes from the ladder. A peer that named + // the chapter but not the passage is an approximate answer, and + // it is still a far better one than a percentage. + val restorable = restorableLocator(head.locatorJson, head.editionSha, alias) return PreviewOutcome.Ready( SyncPreview( local = stored?.totalProgression, @@ -214,7 +218,7 @@ class LiseurSyncPositionSync( accountKey = account.peerId, remoteStatus = head.progression .let { ReadingStatus.forProgression(it).wireName }, - remoteLocatorJson = exact, + remoteLocatorJson = restorable, localRevision = stored?.localRevision, resolvable = !ours, ), @@ -248,7 +252,7 @@ class LiseurSyncPositionSync( ), accountKey = account.peerId, remoteStatus = state.pendingStatus, - remoteLocatorJson = exact, + remoteLocatorJson = state.restorableLocatorFor(alias), localRevision = progressDao.get(bookUrl)?.localRevision, ).takeIf { !it.agrees } } @@ -287,7 +291,7 @@ class LiseurSyncPositionSync( val alias = bookDao.getByUrl(bookUrl) ?.let { works.cached(it, account.peerId) } - val locator = state.exactLocatorFor(alias) + val locator = state.restorableLocatorFor(alias) var applied = false var accountMatches = false @@ -1156,7 +1160,19 @@ class LiseurSyncPositionSync( exactRemote, ), ) - return act(account, book, alias, stored, state, exactRemote, decision) + // The merge is decided on the exact anchor, which is the only + // thing that can say two places are the same passage. What a + // pull then *stores* is the ladder's answer, so a peer that + // named only the chapter is still restored to it. + return act( + account, + book, + alias, + stored, + state, + state?.restorableLocatorFor(alias), + decision, + ) } private suspend fun act( @@ -1165,7 +1181,7 @@ class LiseurSyncPositionSync( alias: WorkAlias, stored: ReadingProgress?, state: SyncPeerState?, - exactRemote: String?, + restorableRemote: String?, decision: SyncDecision, ): Reconciled { val at = now() @@ -1198,7 +1214,7 @@ class LiseurSyncPositionSync( progression = progression, status = decision.state.status.wireName, now = at, - locatorJson = exactRemote, + locatorJson = restorableRemote, remoteUpdatedAt = state?.pendingUpdatedAt, ) if (applied) { @@ -2010,11 +2026,32 @@ class LiseurSyncPositionSync( /** Exact placement is safe only for byte-identical editions. */ private fun SyncPeerState.exactLocatorFor(alias: WorkAlias?): String? = pendingLocatorJson.takeIf { - pendingEditionSha != null && - pendingEditionSha == alias?.editionSha && - ExactLocatorAnchor.isExactJson(it) + sameEdition(pendingEditionSha, alias) && ExactLocatorAnchor.isExactJson(it) } + /** + * The best place in the peer's locator this device can actually put + * the reader — the exact passage where there is one, and the + * resource with its own progression where there is not. + * + * Kept apart from [exactLocatorFor] because the two answer different + * questions. That one asks "may the reader be told this is the exact + * spot", and its answer drives the excerpt, the confidence and the + * agreement test. This one asks "what is the best place to open at", + * and its answer is what gets *stored*. Conflating them is what made + * a percentage the only fallback: a peer that names the chapter and + * how far through it the reader was had that thrown away, and the + * position was rebuilt from a whole-book fraction the two clients + * do not even compute the same way. + * + * Still bounded by the edition. A locator recorded against other + * bytes names a resource this copy may not have, at an offset that + * means nothing here, so it is no more restorable than an exact + * anchor would be. See ADR-0028. + */ + private fun SyncPeerState.restorableLocatorFor(alias: WorkAlias?): String? = + restorableLocator(pendingLocatorJson, pendingEditionSha, alias) + private companion object { /** * The answers to a session batch that say something about one @@ -2100,3 +2137,37 @@ class LiseurSyncPositionSync( const val CONFLICT = "conflict" } } + +/** + * Whether a locator recorded against [editionSha] describes this copy. + * + * An unknown edition is not a matching one. A peer that did not say + * which bytes it was reading may be reading a different translation + * under the same work, and a resource path or an offset from that book + * is not a place in this one. + */ +private fun sameEdition(editionSha: String?, alias: WorkAlias?): Boolean = + editionSha != null && editionSha == alias?.editionSha + +/** + * The best place [locatorJson] can put the reader in this copy. + * + * The ladder, in one place so every caller descends it identically: the + * exact passage, else the resource and how far through it the reader + * was, else nothing — and "nothing" leaves the whole-book progression, + * which travels beside the locator and is what the reader gets when + * this copy and the peer's are not the same bytes. + */ +private fun restorableLocator( + locatorJson: String?, + editionSha: String?, + alias: WorkAlias?, +): String? { + if (!sameEdition(editionSha, alias)) return null + if (ExactLocatorAnchor.isExactJson(locatorJson)) return locatorJson + // Rebuilt rather than passed through: what arrives may carry a + // quote that no longer resolves and a page number from another + // client's pagination, and both would be read here as things they + // are not. + return ResourceAnchor.sanitize(locatorJson)?.toJSON()?.toString() +} diff --git a/app/src/main/kotlin/com/chmouel/liseur/reader/ReaderActivity.kt b/app/src/main/kotlin/com/chmouel/liseur/reader/ReaderActivity.kt index e2903e85..a22a8050 100644 --- a/app/src/main/kotlin/com/chmouel/liseur/reader/ReaderActivity.kt +++ b/app/src/main/kotlin/com/chmouel/liseur/reader/ReaderActivity.kt @@ -426,6 +426,7 @@ class ReaderActivity : FragmentActivity() { resolvePercent = viewModel::resolvePercent, locatorAtOrBeforeProgression = viewModel::locatorAtOrBeforeProgression, + resourceTargetFor = viewModel::resourceTargetFor, prepareLocator = viewModel::prepareLocator, onApproximateResume = viewModel::onApproximateResume, ) diff --git a/app/src/main/kotlin/com/chmouel/liseur/reader/ReaderScreen.kt b/app/src/main/kotlin/com/chmouel/liseur/reader/ReaderScreen.kt index 376f2847..fb09fd13 100644 --- a/app/src/main/kotlin/com/chmouel/liseur/reader/ReaderScreen.kt +++ b/app/src/main/kotlin/com/chmouel/liseur/reader/ReaderScreen.kt @@ -125,6 +125,7 @@ import com.chmouel.liseur.reader.footnotes.FootnoteLayout import com.chmouel.liseur.data.settings.FooterMode import com.chmouel.liseur.data.settings.ColumnMode import com.chmouel.liseur.data.settings.PageTurnStyle +import com.chmouel.liseur.reader.progress.ResourceAnchor import com.chmouel.liseur.data.settings.ReadingFont import com.chmouel.liseur.data.settings.ReaderPrefs import com.chmouel.liseur.data.settings.readingCssFor @@ -729,8 +730,16 @@ fun ReaderScreen( settleLayout() if (ExactLocatorAnchor.verify(nav, locator)) return } - val progression = locator.locations.totalProgression ?: return - val fallback = onProgressAction.locatorAtOrBeforeProgression(progression) ?: return + // The quote did not land. The chapter it named is still the best + // thing known about where the reader was: falling straight to a + // whole-book fraction can move them to a different chapter + // entirely, because that fraction may have been computed by the + // other client, which does not compute it the same way. + val fallback = onProgressAction.resourceTargetFor(locator) + ?: locator.locations.totalProgression + ?.takeIf(ResourceAnchor::isFraction) + ?.let(onProgressAction.locatorAtOrBeforeProgression) + ?: return retargetGate(fallback) pendingPositionEvent = event val fallbackToken = moves.issue( @@ -1086,8 +1095,13 @@ fun ReaderScreen( val requested = restoreTarget ?: return@LaunchedEffect if (!ExactLocatorAnchor.isExact(requested)) return@LaunchedEffect if (openingExactAnchorArrived(nav, requested)) return@LaunchedEffect - val progression = requested.locations.totalProgression ?: return@LaunchedEffect - val fallback = onProgressAction.locatorAtOrBeforeProgression(progression) + // The chapter before the percentage, for the reason given in + // navigate(): the whole-book fraction is the only rung that can + // land in the wrong chapter. + val fallback = onProgressAction.resourceTargetFor(requested) + ?: requested.locations.totalProgression + ?.takeIf(ResourceAnchor::isFraction) + ?.let(onProgressAction.locatorAtOrBeforeProgression) ?: return@LaunchedEffect pendingPositionEvent = NavigatorPositionEvent.FRAGMENT_RECREATION // Retarget rather than release. This navigation is asynchronous @@ -3273,6 +3287,7 @@ class ReaderProgressActions( val currentPercent: () -> Int, val resolvePercent: (String) -> GoToDestination?, val locatorAtOrBeforeProgression: (Double) -> Locator?, + val resourceTargetFor: (Locator) -> Locator?, val prepareLocator: (Locator) -> Locator, val onApproximateResume: () -> Unit, ) { diff --git a/app/src/main/kotlin/com/chmouel/liseur/reader/ReaderViewModel.kt b/app/src/main/kotlin/com/chmouel/liseur/reader/ReaderViewModel.kt index c4a78a26..c9398a16 100644 --- a/app/src/main/kotlin/com/chmouel/liseur/reader/ReaderViewModel.kt +++ b/app/src/main/kotlin/com/chmouel/liseur/reader/ReaderViewModel.kt @@ -78,6 +78,7 @@ import com.chmouel.liseur.reader.footnotes.FootnoteResolver import com.chmouel.liseur.reader.progress.ReaderProgress import com.chmouel.liseur.reader.progress.ReadingPace import com.chmouel.liseur.reader.progress.ReadingSpeedEstimator +import com.chmouel.liseur.reader.progress.ResourceAnchor import com.chmouel.liseur.reader.progress.StableBookProgress import kotlinx.coroutines.CancellationException import kotlinx.coroutines.CoroutineScope @@ -681,6 +682,7 @@ class ReaderViewModel( // settled on the next open; the book still opens now. if (outcome != ResolveOutcome.Done || !takeRemote) return positionsFor(publication) + this@ReaderViewModel.publication = publication offerWayBack(localLocator, here, preview) } @@ -696,12 +698,16 @@ class ReaderViewModel( preview: SyncPreview, ) { val positions = bookPositions?.takeIf { it.isUsable } ?: return - val locator = exactLocal?.takeIf(ExactLocatorAnchor::isExact) - ?: positions.locatorAtOrBeforeProgression(progression) - ?: return - val position = positions.resolve(locator)?.position + val locator = ResourceAnchor.resumeTarget( + saved = exactLocal, + totalProgression = progression, + readingOrder = readingOrderPaths(), + byProgression = positions::locatorAtOrBeforeProgression, + ) ?: return + val prepared = prepareLocator(locator) + val position = positions.resolve(prepared)?.position _jumpBack.value = JumpBack( - locator = prepareLocator(locator), + locator = prepared, position = position, fromSync = true, excerpt = preview.excerpt, @@ -723,11 +729,16 @@ class ReaderViewModel( /** Navigates to the position that was offered, not a later database snapshot. */ private suspend fun goToRemotePosition(preview: SyncPreview) { val positions = positionsFor(publication ?: return) - val exact = preview.remoteLocatorJson + val remoteLocator = preview.remoteLocatorJson ?.let { runCatching { Locator.fromJSON(JSONObject(it)) }.getOrNull() } - ?.takeIf(ExactLocatorAnchor::isExact) + val exact = remoteLocator?.takeIf(ExactLocatorAnchor::isExact) val target = exact - ?: preview.remote?.let(positions::locatorAtOrBeforeProgression) + ?: ResourceAnchor.approximateTarget( + saved = remoteLocator, + totalProgression = preview.remote, + readingOrder = readingOrderPaths(), + byProgression = positions::locatorAtOrBeforeProgression, + ) ?: return onJump() _jumpBack.value = _jumpBack.value?.copy( @@ -749,6 +760,7 @@ class ReaderViewModel( private var publication: Publication? = null private var bookPositions: BookPositions? = null + private var readingOrderPaths: List? = null private var goToPageResolver: GoToPageResolver? = null private var speed = ReadingSpeedEstimator() private var jumpBackTimer: Job? = null @@ -973,6 +985,7 @@ class ReaderViewModel( ?: ReadingPace.Unknown, ) val positions = positionsFor(publication) + this@ReaderViewModel.publication = publication if (pulledAutomatically != null) { val localLocator = beforeSync?.locatorJson ?.let { runCatching { Locator.fromJSON(JSONObject(it)) }.getOrNull() } @@ -985,17 +998,19 @@ class ReaderViewModel( val savedLocator = stored?.locatorJson ?.let { runCatching { Locator.fromJSON(JSONObject(it)) }.getOrNull() } // Unmarked Readium text may describe the following synthetic - // position. It is approximate and deliberately resumes at or - // before the stable progression instead. - val initialLocator = if (ExactLocatorAnchor.isExact(savedLocator)) { - savedLocator - } else { - stored?.totalProgression?.let(positions::locatorAtOrBeforeProgression) - ?: savedLocator - }?.let(::prepareLocator) + // position, so it is not treated as exact. What is left of + // such a locator is still the chapter and the place in it, + // which is tried before the whole-book progression: that + // last rung can land in a different chapter altogether when + // the fraction was written down by the other client. + val initialLocator = ResourceAnchor.resumeTarget( + saved = savedLocator, + totalProgression = stored?.totalProgression, + readingOrder = readingOrderPaths(), + byProgression = positions::locatorAtOrBeforeProgression, + )?.let(::prepareLocator) lastLocator = initialLocator library.markOpened(bookId) - this@ReaderViewModel.publication = publication _state.value = UiState.Ready( publication = publication, navigatorFactory = EpubNavigatorFactory(publication), @@ -1254,6 +1269,31 @@ class ReaderViewModel( fun locatorAtOrBeforeProgression(progression: Double): Locator? = bookPositions?.locatorAtOrBeforeProgression(progression) + /** + * The chapter a peer named, if this book has it. + * + * Offered to the reading chrome so that a quote which will not + * verify falls back to the right chapter rather than straight to a + * whole-book percentage, which can land in a different one. + */ + fun resourceTargetFor(locator: Locator): Locator? { + val target = ResourceAnchor.targetIn(locator, readingOrderPaths()) ?: return null + val stable = bookPositions?.resolve(target) ?: return target + return target.copy( + locations = target.locations.copy(totalProgression = stable.progression), + ) + } + + /** The opened publication's spine paths, computed once per book. */ + private fun readingOrderPaths(): List { + readingOrderPaths?.let { return it } + val paths = publication?.readingOrder + ?.map { ResourceAnchor.path(it.url().toString()) } + ?: return emptyList() + readingOrderPaths = paths + return paths + } + fun onApproximateResume() { _jumpBack.value = _jumpBack.value?.copy(confidence = ResumeConfidence.APPROXIMATE) } diff --git a/app/src/main/kotlin/com/chmouel/liseur/reader/ResourceAddress.kt b/app/src/main/kotlin/com/chmouel/liseur/reader/ResourceAddress.kt index 163fed5c..357575f3 100644 --- a/app/src/main/kotlin/com/chmouel/liseur/reader/ResourceAddress.kt +++ b/app/src/main/kotlin/com/chmouel/liseur/reader/ResourceAddress.kt @@ -42,6 +42,10 @@ internal object ResourceAddress { */ fun href(webUrl: String?): String? = relative(webUrl) + /** A publication-relative path suitable for comparing two href spellings. */ + fun canonicalPath(raw: String?): String? = + path(raw) + private fun relative(raw: String?): String? { val address = raw?.substringBefore('#')?.substringBefore('?')?.trim() ?: return null if (address.isEmpty()) return null diff --git a/app/src/main/kotlin/com/chmouel/liseur/reader/progress/ResourceAnchor.kt b/app/src/main/kotlin/com/chmouel/liseur/reader/progress/ResourceAnchor.kt new file mode 100644 index 00000000..dc89cf3f --- /dev/null +++ b/app/src/main/kotlin/com/chmouel/liseur/reader/progress/ResourceAnchor.kt @@ -0,0 +1,165 @@ +package com.chmouel.liseur.reader.progress + +import com.chmouel.liseur.reader.ResourceAddress +import org.json.JSONObject +import org.readium.r2.shared.publication.Locator +import org.readium.r2.shared.util.Url + +/** + * The place in a book a peer named, kept when the exact word cannot be. + * + * An exact anchor ([ExactLocatorAnchor]) names a passage and survives a + * different font, a different column count and a different screen. When + * it cannot be resolved — the other client never captured one, the quote + * has moved, the selector no longer matches — what is left is still far + * better than a percentage: the resource the reader was in, and how far + * through it they were. + * + * That matters most in the books this exists for. A whole-book + * progression is a fraction of *the whole book*, and the two clients + * compute it differently: this one interpolates Readium's synthetic + * positions, the web reader interpolates the server's position list by + * byte weight. The two disagree by a little on every book, and by more + * on one whose resources are unevenly sized — which is the ordinary + * shape of an EPUB 2 with few, large spine items. Turning that fraction + * back into a place lands in roughly the right region of the book. The + * resource and its own progression land in the right chapter, at the + * right point of it, whoever wrote them down. + * + * So this is deliberately *not* a smaller exact anchor. It is the + * coarser rung below one, and everything that reads it must go on + * calling the result approximate. + */ +object ResourceAnchor { + + /** + * The resource-relative place in [locatorJson], with nothing else. + * + * Everything an anchor could be mistaken for is dropped rather than + * carried: the text quote and its selector (which no longer + * describe a passage this device can verify), the CFI fragments + * (which this client cannot resolve at all), and the synthetic + * `position` (which is the *writing* client's pagination — Readium's + * count and the web reader's are two different numbers for the same + * spot, so a foreign one indexes into the wrong place here). + * + * Null when there is no usable resource-relative place, which is not + * a failure: it simply means the whole-book progression is the best + * that can be done. + */ + fun sanitize(locatorJson: String?): Locator? = + parse(locatorJson)?.let(::sanitize) + + fun sanitize(locator: Locator?): Locator? { + val locator = locator ?: return null + if (locator.href.toString().isBlank()) return null + // The within-resource fraction is the whole point: without it + // there is no "how far into the chapter" to restore, and the + // resource alone would send the reader to its first paragraph + // when they were most of the way through it. + val progression = fraction(locator.locations.progression) ?: return null + // copy() keeps the href and the media type and replaces the rest + // outright, which is what makes this a whitelist: a locations + // field invented after this was written is dropped by default + // rather than carried by an omission here. + return locator.copy( + locations = Locator.Locations( + progression = progression, + totalProgression = fraction(locator.locations.totalProgression), + ), + text = Locator.Text(), + ) + } + + /** + * Whether [locatorJson] names a resource-relative place at all. + * + * Cheaper than sanitising when the answer is all that is wanted, and + * it asks exactly the same question, so the two cannot disagree. + */ + fun isResourceJson(locatorJson: String?): Boolean = sanitize(locatorJson) != null + + /** + * The sanitised place in [locator], if this publication has that + * resource. + * + * [readingOrder] is the opened publication's spine paths. A locator + * that names a resource this book does not have is not a place in + * it — it came from another edition, or from a client that spells + * the path differently — and following it would leave the navigator + * nowhere. Checked here rather than at the sync boundary because + * only the open publication can answer it. + */ + fun targetIn(locator: Locator?, readingOrder: Collection): Locator? { + val resource = sanitize(locator) ?: return null + val target = ResourceAddress.canonicalPath(resource.href.toString()) ?: return null + val matches = readingOrder.filter { + ResourceAddress.canonicalPath(it) == target + } + if (matches.size != 1) return null + val localHref = matches.single().let { Url(it) ?: Url("/$it") } ?: return null + return resource.copy(href = localHref) + } + + /** + * Where to reopen: the exact passage, else the chapter, else the + * percentage. + * + * The order is the point, and it lives here so that opening a book, + * accepting a catch-up offer and recovering from a quote that would + * not verify cannot drift apart. The percentage is last because it + * is the only rung that can land in the wrong chapter. + */ + fun resumeTarget( + saved: Locator?, + totalProgression: Double?, + readingOrder: Collection, + byProgression: (Double) -> Locator?, + ): Locator? = saved?.takeIf(ExactLocatorAnchor::isExact) + ?: approximateTarget(saved, totalProgression, readingOrder, byProgression) + ?: saved + + /** + * The rungs below the exact one, for a caller whose exact attempt + * has already been made and failed. + */ + fun approximateTarget( + saved: Locator?, + totalProgression: Double?, + readingOrder: Collection, + byProgression: (Double) -> Locator?, + ): Locator? = targetIn(saved, readingOrder) + ?: (totalProgression ?: saved?.locations?.totalProgression) + ?.takeIf(::isFraction) + ?.let(byProgression) + + /** A spine path, without the fragment or query an anchor carries. */ + fun path(href: String): String = href.substringBefore('#').substringBefore('?') + + /** Whether a value can be used as a whole-book or resource fraction. */ + fun isFraction(value: Double?): Boolean = + value?.let(::fraction) != null + + /** + * A number that was really written down, told apart from one that + * was not. + * + * Out of range is refused rather than clamped. A peer that sends + * `1.7` has not told this device the reader is at the end of the + * chapter; it has told it something impossible, and treating that as + * the end would move the reader on the strength of a bug. Zero is + * legitimate and must survive: it is the top of the chapter. + */ + private fun fraction(value: Double?): Double? { + val value = value ?: return null + if (!value.isFinite()) return null + // Written this way round so a NaN is refused, as in SyncOps. + if (!(value >= 0.0 && value <= 1.0)) return null + return value + } + + private fun parse(json: String?): Locator? { + if (json.isNullOrEmpty() || json == "{}") return null + return runCatching { Locator.fromJSON(JSONObject(json)) }.getOrNull() + } +} diff --git a/app/src/test/kotlin/com/chmouel/liseur/data/liseursync/LiseurSyncPositionSyncTest.kt b/app/src/test/kotlin/com/chmouel/liseur/data/liseursync/LiseurSyncPositionSyncTest.kt index bfffe8ee..91b4b29a 100644 --- a/app/src/test/kotlin/com/chmouel/liseur/data/liseursync/LiseurSyncPositionSyncTest.kt +++ b/app/src/test/kotlin/com/chmouel/liseur/data/liseursync/LiseurSyncPositionSyncTest.kt @@ -19,6 +19,7 @@ import com.chmouel.liseur.data.remote.SyncFailure import com.chmouel.liseur.data.remote.SyncOutcome import com.chmouel.liseur.data.remote.LiveIdentity import com.chmouel.liseur.data.remote.LiveTopic +import com.chmouel.liseur.reader.progress.ExactLocatorAnchor import java.io.File import java.net.InetAddress import javax.crypto.KeyGenerator @@ -606,6 +607,128 @@ class LiseurSyncPositionSyncTest { assertEquals(ResumeConfidence.EXACT, preview.confidence) } + @Test + fun `a peer that names the chapter has it kept, not reduced to a percentage`() = runTest { + // Issue #184. The web reader records the resource it is in and + // how far through it the reader is, but not the app's exact + // passage anchor. That locator used to be discarded outright, + // leaving the whole-book percentage as the only thing to reopen + // on — and the two clients do not compute that percentage the + // same way, so the book reopened somewhere near the place + // instead of at it. The chapter is kept now. + connect() + db.bookDao().upsert(local()) + alias(editionSha = "sha-local") + db.syncPeerStateDao().persistPending( + bookUrl = LOCAL, + peerId = peer(), + progression = 0.31, + status = "reading", + remoteUpdatedAt = NOW, + locatorJson = resourceLocator( + href = "/chapter-9.xhtml", + progression = 0.74, + totalProgression = 0.31, + ), + editionSha = "sha-local", + ) + db.readingProgressDao().recordLocal(LOCAL, LOCATOR, 0.2, null, "reading", NOW) + + assertEquals(ResolveOutcome.Done, sync().takeRemotePosition(LOCAL, 1)) + + val stored = JSONObject(db.readingProgressDao().get(LOCAL)!!.locatorJson!!) + assertEquals("/chapter-9.xhtml", stored.getString("href")) + assertEquals(0.74, stored.getJSONObject("locations").getDouble("progression"), 0.0) + } + + @Test + fun `a kept chapter is not dressed up as the exact passage`() = runTest { + // The chapter is worth restoring and worth saying so about. It + // is not the reader's word, so it must not raise the exact + // confidence or offer an excerpt of a passage nobody captured. + connect() + db.bookDao().upsert(local()) + alias(editionSha = "sha-local") + db.syncPeerStateDao().persistPending( + bookUrl = LOCAL, + peerId = peer(), + progression = 0.31, + status = "reading", + remoteUpdatedAt = NOW, + locatorJson = resourceLocator(), + editionSha = "sha-local", + ) + db.readingProgressDao().recordLocal(LOCAL, LOCATOR, 0.2, null, "reading", NOW) + + val preview = sync().preservedConflict(LOCAL, null)!! + + assertEquals(ResumeConfidence.APPROXIMATE, preview.confidence) + assertNull(preview.excerpt) + // Nothing exact on either side, so the percentages decide, as + // they always did. + assertNull(preview.exactPositionAgreement) + assertEquals("/chapter-9.xhtml", JSONObject(preview.remoteLocatorJson!!).getString("href")) + } + + @Test + fun `a chapter from another edition is still refused`() = runTest { + // A resource path and an offset in someone else's bytes are not + // a place in this book. Keeping the ladder inside the edition + // check is what stops this being a regression of ADR-0028. + connect() + db.bookDao().upsert(local()) + alias(editionSha = "sha-local") + db.syncPeerStateDao().persistPending( + bookUrl = LOCAL, + peerId = peer(), + progression = 0.31, + status = "reading", + remoteUpdatedAt = NOW, + locatorJson = resourceLocator(), + editionSha = "sha-remote", + ) + db.readingProgressDao().recordLocal(LOCAL, LOCATOR, 0.2, null, "reading", NOW) + + assertEquals(ResolveOutcome.Done, sync().takeRemotePosition(LOCAL, 1)) + assertEquals("{}", db.readingProgressDao().get(LOCAL)?.locatorJson) + } + + @Test + fun `a foreign page number and a stale quote do not ride along`() = runTest { + // The peer's `position` is its own pagination and its `text` is + // a quote this device never verified. Stored as they arrive, + // the first indexes into the wrong place and the second reads + // as an exact anchor on the next run. + connect() + db.bookDao().upsert(local()) + alias(editionSha = "sha-local") + db.syncPeerStateDao().persistPending( + bookUrl = LOCAL, + peerId = peer(), + progression = 0.31, + status = "reading", + remoteUpdatedAt = NOW, + locatorJson = """ + {"href":"/chapter-9.xhtml","type":"application/xhtml+xml", + "locations":{"progression":0.74,"totalProgression":0.31, + "position":812,"fragments":["epubcfi(/6/14!/4/2/2)"]}, + "text":{"highlight":"a quote from another engine"}} + """.trimIndent(), + editionSha = "sha-local", + ) + db.readingProgressDao().recordLocal(LOCAL, LOCATOR, 0.2, null, "reading", NOW) + + assertEquals(ResolveOutcome.Done, sync().takeRemotePosition(LOCAL, 1)) + + val stored = db.readingProgressDao().get(LOCAL)!!.locatorJson!! + val locations = JSONObject(stored).getJSONObject("locations") + assertFalse(stored, locations.has("position")) + assertFalse(stored, locations.has("fragments")) + assertFalse(stored, JSONObject(stored).has("text")) + assertFalse(stored, ExactLocatorAnchor.isExactJson(stored)) + assertEquals(0.74, locations.getDouble("progression"), 0.0) + } + @Test fun `different edition and legacy locators fall back to progression`() = runTest { connect() @@ -2411,6 +2534,22 @@ class LiseurSyncPositionSyncTest { .put("text", JSONObject().put("highlight", highlight)) .toString() + /** What the web reader can say: a resource and how far into it. */ + private fun resourceLocator( + href: String = "/chapter-9.xhtml", + progression: Double = 0.74, + totalProgression: Double = 0.31, + ): String = JSONObject() + .put("href", href) + .put("type", "application/xhtml+xml") + .put( + "locations", + JSONObject() + .put("progression", progression) + .put("totalProgression", totalProgression), + ) + .toString() + /** Every request the server has seen, including earlier runs'. */ private fun requests(): List { while (seen.size < server.requestCount) seen += server.takeRequest() diff --git a/app/src/test/kotlin/com/chmouel/liseur/reader/progress/ResourceAnchorTest.kt b/app/src/test/kotlin/com/chmouel/liseur/reader/progress/ResourceAnchorTest.kt new file mode 100644 index 00000000..3c0013c2 --- /dev/null +++ b/app/src/test/kotlin/com/chmouel/liseur/reader/progress/ResourceAnchorTest.kt @@ -0,0 +1,335 @@ +package com.chmouel.liseur.reader.progress + +import org.json.JSONObject +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNotNull +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Test +import org.junit.runner.RunWith +import org.readium.r2.shared.publication.Locator +import org.readium.r2.shared.util.Url +import com.chmouel.liseur.reader.ResourceAddress +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config + +@Config(sdk = [35], application = android.app.Application::class) +@RunWith(RobolectricTestRunner::class) +class ResourceAnchorTest { + + @Test + fun `the resource and its own progression survive`() { + val sanitized = ResourceAnchor.sanitize( + locator(progression = 0.62, totalProgression = 0.23).toJSON().toString(), + ) + + assertNotNull(sanitized) + assertEquals("/chapter-7.xhtml", sanitized?.href.toString()) + assertEquals(0.62, sanitized?.locations?.progression ?: 0.0, 0.0) + assertEquals(0.23, sanitized?.locations?.totalProgression ?: 0.0, 0.0) + } + + @Test + fun `a foreign pagination and a stale quote are dropped, not carried`() { + // Everything here describes the *writing* client: its synthetic + // page number, its engine's CFI, and a quote this device has no + // way to confirm. Restoring against any of them is how a + // fallback ends up more wrong than the percentage it replaced. + val foreign = locator().copy( + locations = Locator.Locations( + fragments = listOf("epubcfi(/6/14!/4/2/2[p9]/1:0)"), + progression = 0.62, + position = 812, + totalProgression = 0.23, + otherLocations = mapOf( + ExactLocatorAnchor.MARKER to 1, + ExactLocatorAnchor.CSS_SELECTOR to "body > p:nth-of-type(9)", + ), + ), + text = Locator.Text(before = "before", highlight = "moved", after = "after"), + ) + + val sanitized = requireNotNull(ResourceAnchor.sanitize(foreign.toJSON().toString())) + + assertEquals(emptyList(), sanitized.locations.fragments) + assertNull(sanitized.locations.position) + assertEquals(emptyMap(), sanitized.locations.otherLocations) + assertNull(sanitized.text.highlight) + // What it is *for* is still there. + assertEquals(0.62, sanitized.locations.progression ?: 0.0, 0.0) + // And it must not read as an exact anchor afterwards, or the + // reader would be told the passage was restored exactly. + assertFalse(ExactLocatorAnchor.isExact(sanitized)) + } + + @Test + fun `the top of a chapter is a place, not a missing one`() { + val top = ResourceAnchor.sanitize( + locator(progression = 0.0, totalProgression = 0.0).toJSON().toString(), + ) + + assertNotNull(top) + assertEquals(0.0, top?.locations?.progression ?: -1.0, 0.0) + } + + @Test + fun `a progression that is not a fraction is refused rather than clamped`() { + // Clamping would move the reader to the end of a chapter on the + // strength of another client's bug. There is no place here. + // + // Written as raw JSON because that is how these actually arrive. + // org.json refuses to *emit* a NaN, so a client whose fraction + // went wrong sends `null` (what JSON.stringify does with one) or + // the string "NaN" — which is just a string until something asks + // it for a double. SyncOps guards the same door for the same + // reason. + val broken = listOf("1.7", "-0.2", "\"NaN\"", "null", "\"\"", "\"later\"") + for (value in broken) { + val json = """ + {"href":"/chapter-7.xhtml","type":"application/xhtml+xml", + "locations":{"progression":$value,"totalProgression":0.23}} + """.trimIndent() + assertNull(value, ResourceAnchor.sanitize(json)) + } + } + + @Test + fun `a broken whole-book percentage does not cost the chapter`() { + // The resource progression is the part that matters here, and it + // is sound. Dropping the whole place because the *other* number + // is nonsense would be refusing the good half of the answer. + val json = """ + {"href":"/chapter-7.xhtml","type":"application/xhtml+xml", + "locations":{"progression":0.62,"totalProgression":4.2}} + """.trimIndent() + + val sanitized = requireNotNull(ResourceAnchor.sanitize(json)) + assertEquals(0.62, sanitized.locations.progression ?: 0.0, 0.0) + assertNull(sanitized.locations.totalProgression) + } + + @Test + fun `a locator with no resource progression has no resource place`() { + val percentageOnly = JSONObject() + .put("href", "/chapter-7.xhtml") + .put("type", "application/xhtml+xml") + .put("locations", JSONObject().put("totalProgression", 0.23)) + + assertNull(ResourceAnchor.sanitize(percentageOnly.toString())) + assertFalse(ResourceAnchor.isResourceJson(percentageOnly.toString())) + } + + @Test + fun `nothing at all is not a place`() { + assertNull(ResourceAnchor.sanitize(null as String?)) + assertNull(ResourceAnchor.sanitize("")) + assertNull(ResourceAnchor.sanitize("{}")) + assertNull(ResourceAnchor.sanitize("not json")) + assertFalse(ResourceAnchor.isResourceJson(null)) + } + + @Test + fun `an exact anchor is also a resource place`() { + // The ladder descends: an anchor whose quote will not resolve + // must still leave the chapter behind it, or the fallback is the + // percentage again. + val exact = ExactLocatorAnchor.mark( + locator(progression = 0.62), + ViewportTextAnchor("#chapter", "before ", "word", " after"), + ) + + assertTrue(ExactLocatorAnchor.isExact(exact)) + val sanitized = requireNotNull(ResourceAnchor.sanitize(exact.toJSON().toString())) + assertEquals("/chapter-7.xhtml", sanitized.href.toString()) + assertEquals(0.62, sanitized.locations.progression ?: 0.0, 0.0) + } + + @Test + fun `a chapter this book does not have is not a place in it`() { + val spine = setOf("/chapter-7.xhtml") + assertNotNull(ResourceAnchor.targetIn(locator(), spine)) + assertNull(ResourceAnchor.targetIn(locator(), setOf("/other.xhtml"))) + assertNull(ResourceAnchor.targetIn(locator(), emptySet())) + } + + @Test + fun `equivalent href spellings resolve to the opened spine spelling`() { + val foreign = locator().copy( + href = requireNotNull(Url("/OEBPS/ch2%20long.xhtml#part")), + ) + assertEquals("OEBPS/ch2 long.xhtml", ResourceAddress.canonicalPath(foreign.href.toString())) + assertEquals("OEBPS/ch2 long.xhtml", ResourceAddress.canonicalPath("OEBPS/ch2%20long.xhtml")) + val target = ResourceAnchor.targetIn(foreign, setOf("OEBPS/ch2%20long.xhtml")) + + assertEquals("OEBPS/ch2%20long.xhtml", target?.href.toString()) + assertEquals(0.62, target?.locations?.progression ?: 0.0, 0.0) + } + + @Test + fun `duplicate spine resources are too ambiguous for the resource rung`() { + assertNull( + ResourceAnchor.targetIn( + locator(), + listOf("/chapter-7.xhtml", "/chapter-7.xhtml"), + ), + ) + } + + @Test + fun `whole-book fractions must stay in range`() { + assertTrue(ResourceAnchor.isFraction(0.0)) + assertTrue(ResourceAnchor.isFraction(1.0)) + assertFalse(ResourceAnchor.isFraction(-0.1)) + assertFalse(ResourceAnchor.isFraction(1.1)) + assertFalse(ResourceAnchor.isFraction(Double.NaN)) + } + + @Test + fun `a fragment on the href still names its resource`() { + // A peer's anchor may carry an id. The spine is keyed by path. + val withFragment = requireNotNull( + Locator.fromJSON( + JSONObject() + .put("href", "/chapter-7.xhtml#p42") + .put("type", "application/xhtml+xml") + .put("locations", JSONObject().put("progression", 0.4)), + ), + ) + assertNotNull(ResourceAnchor.targetIn(withFragment, setOf("/chapter-7.xhtml"))) + } + + @Test + fun `the chapter is tried before the percentage`() { + // Issue #184: the percentage is the only rung that can land in + // the wrong chapter, so it must be the last one tried. + var percentageAsked = false + val target = ResourceAnchor.resumeTarget( + saved = locator(), + totalProgression = 0.23, + readingOrder = setOf("/chapter-7.xhtml"), + byProgression = { percentageAsked = true; null }, + ) + assertEquals("/chapter-7.xhtml", target?.href.toString()) + assertEquals(0.62, target?.locations?.progression ?: 0.0, 0.0) + assertFalse(percentageAsked) + } + + @Test + fun `a chapter from elsewhere gives way to the percentage`() { + val elsewhere = requireNotNull( + Locator.fromJSON( + JSONObject() + .put("href", "/from-another-book.xhtml") + .put("type", "application/xhtml+xml") + .put("locations", JSONObject().put("progression", 0.4)), + ), + ) + val fromPercentage = locator(progression = 0.1, totalProgression = 0.23) + val target = ResourceAnchor.resumeTarget( + saved = elsewhere, + totalProgression = 0.23, + readingOrder = setOf("/chapter-7.xhtml"), + byProgression = { fromPercentage }, + ) + assertEquals(fromPercentage, target) + } + + @Test + fun `an exact anchor is taken as it is, whole`() { + // The ladder must not sanitise the rung it is not on: an exact + // anchor needs its quote and selector to be verifiable. + val exact = requireNotNull( + Locator.fromJSON( + JSONObject() + .put("href", "/chapter-7.xhtml") + .put("type", "application/xhtml+xml") + .put( + "locations", + JSONObject() + .put("progression", 0.62) + .put("cssSelector", "#p42") + .put("liseurAnchor", 1), + ) + .put("text", JSONObject().put("highlight", "the word")), + ), + ) + val target = ResourceAnchor.resumeTarget( + saved = exact, + totalProgression = 0.23, + readingOrder = setOf("/chapter-7.xhtml"), + byProgression = { null }, + ) + assertEquals(exact, target) + assertTrue(ExactLocatorAnchor.isExact(target)) + } + + @Test + fun `nothing usable leaves the caller what it had`() { + // Preserving the old behaviour: a locator nobody can improve on + // is still passed through rather than dropped. + val bare = requireNotNull( + Locator.fromJSON( + JSONObject() + .put("href", "/chapter-7.xhtml") + .put("type", "application/xhtml+xml"), + ), + ) + assertEquals( + bare, + ResourceAnchor.resumeTarget(bare, null, setOf("/chapter-7.xhtml")) { null }, + ) + assertNull(ResourceAnchor.resumeTarget(null, 0.5, emptySet()) { null }) + } + + @Test + fun `a peer's own total progression is used when nothing else says`() { + var asked: Double? = null + ResourceAnchor.approximateTarget( + saved = locator(totalProgression = 0.44), + totalProgression = null, + readingOrder = emptySet(), + byProgression = { asked = it; null }, + ) + assertEquals(0.44, asked ?: -1.0, 0.0) + } + + @Test + fun `a locator written by the browser reader is read as exact here`() { + // The literal bytes liseur-sync's reader-anchor.js emits. The two + // readers agree on this shape or they cannot use each other's + // anchors at all, so it is worth pinning rather than describing. + val fromBrowser = """ + {"href":"OEBPS/ch1.xhtml","type":"application/xhtml+xml","title":"Moby-Dick", + "locations":{"fragments":[],"progression":0.25,"totalProgression":0.1, + "position":2,"liseurAnchor":1,"cssSelector":"body > p:nth-of-type(3)"}, + "text":{"before":"and then ","highlight":"Ishmael","after":" said nothing"}} + """.trimIndent() + assertTrue(ExactLocatorAnchor.isExactJson(fromBrowser)) + + // And when its quote will not resolve, the chapter underneath is + // still there to fall back to, unpolluted by the browser's own + // spine index. + val target = ResourceAnchor.sanitize(fromBrowser) + assertEquals("OEBPS/ch1.xhtml", target?.href.toString()) + assertEquals(0.25, target?.locations?.progression ?: -1.0, 0.0) + assertNull(target?.locations?.position) + assertNull(target?.text?.highlight) + } + + private fun locator( + progression: Double = 0.62, + totalProgression: Double = 0.23, + ): Locator = requireNotNull( Locator.fromJSON( + JSONObject() + .put("href", "/chapter-7.xhtml") + .put("type", "application/xhtml+xml") + .put( + "locations", + JSONObject() + .put("progression", progression) + .put("totalProgression", totalProgression), + ), + ), + ) +} diff --git a/docs/SERVER_CAPABILITIES.md b/docs/SERVER_CAPABILITIES.md index 958ec3c6..e01d96fc 100644 --- a/docs/SERVER_CAPABILITIES.md +++ b/docs/SERVER_CAPABILITIES.md @@ -239,13 +239,33 @@ per-server capability detection. |--------|-----------|--------| | Komga | Exact position in chapter | Full Readium locator, with position snapping | | calibre-web | Page-level at best | Percentage (`totalProgression`) via the Kobo protocol | -| liseur-sync | Exact position in chapter | Full Readium locator via the op log | +| liseur-sync | Exact position in chapter, in the same file | Full Readium locator via the op log | | Grimmory | Page-level at best | Percentage via KOReader's kosync, paired alongside the catalog | | Any kosync server | Page-level at best | Same partner: it speaks the generic protocol, so a stock kosync server pairs the same way | Every sync goes through the shared `reconcileReadingState` merge logic in `domain/ReadingStateMerge.kt`. +A locator is only followed into the file it was written against. Ops +carry the edition's SHA-256 (`edition_sha`), and a position from an +edition this device cannot confirm it holds is used as a percentage and +nothing more — the same paragraph is not at the same offset in another +translation or another conversion of the book. + +Within a matching edition the restoration descends three rungs, in +`reader/progress/ResourceAnchor.kt`: + +1. **The passage.** A locator this app marked as exact + (`liseurAnchor`): a block selector and a bounded quote, verified to + be on screen after the chapter has laid out. +2. **The chapter.** The resource and the fraction *within* it. Kept + whenever the exact rung is absent or will not verify, including + positions from clients that do not provide a usable exact anchor. +3. **The percentage.** The whole-book fraction, and the only rung that + can land in the wrong chapter: two engines compute that fraction + from different things, and on a book with a few large chapters they + disagree by pages. + The kosync partner is not a kind of server: it is paired *alongside* a connected one, covers that server's downloaded books (matched by KOReader's partial MD5 of the file), and has its own lifecycle.