feat(publication): resolve locators whose href left the reading order - #210
Merged
ddfreiling merged 7 commits intoAug 27, 2026
Merged
Conversation
Publication.resolveLocator(Locator) recovers a position for a stored locator when its href is no longer in the reading order (e.g. a republish changes resource granularity). Prefers the segmentation-independent totalProgression mapped onto Link.duration, falls back to a bounds-safe position lookup, and returns null rather than guessing or throwing. Drops the stale cssSelector/fragments and updates type to match the resolved link.
Link compares by value, so a reading order containing two equal entries resolved every one of them to the first entry's index and reported a position the locator was never at. Both call sites already knew the real index; _relocate now takes it directly instead of recovering it with readingOrder.indexOf. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…aying one getTimeOffsetForLocatorWithProgression shadowed its `locator` parameter with `audioLocator`, so the parameter was never read. From resolveLocator that meant progression was multiplied by the duration of the track already playing rather than the track being navigated to. Reproduced end to end on a simulator with the 3-track audiobook fixture: jumping to track 3 (80s) at progression 0.5 while track 1 (10s) was playing landed at 5.0s instead of 40.0s. The cold-open path is worse. `_initialLocator = resolveLocator(initialLocator)` runs before any track is playing, so `audioLocator` is nil, the helper returns nil, and copyWithOffset falls back to `t=0` -- a start-of-track restore. Today a `t=` fragment usually saves it, but Publication.resolveLocator in this branch drops fragments and keeps progression, so adopting it downstream without this fix would have made restores worse rather than better. resolveLocator now also passes `resolvedLocator`, so the duration lookup uses the href after the reading-order repair rather than the original one. seek(toProgression:) is unaffected: it already passes audioLocator explicitly, which is the current track it is meant to scale by. Android was checked and is correct on both paths: goToLocator resolves the duration from the target item, and seekToProgression uses the current track by design. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
NikolajHarderNota
approved these changes
Aug 27, 2026
Conflicts were CHANGELOG-only, from `chore(release): v0.4.1` and `chore: reduce CHANGELOGs` landing after the last merge. Both of this branch's entries moved from the (now released) 0.4.1 section up into Unreleased, and were shortened to match main's reduced changelog style. Dropped the "reader-position event that failed to serialize" entry rather than readding it: Notalib#211 is merged and its Swift code is in main, but main's changelog reduction deliberately removed that entry as internal mechanism. Same for the standalone `ReadiumTimebasedState.fromJsonString` bullet, which 0.4.1 now covers inside its Fixed entry. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Member
Author
|
CI integration test timeout are flukes on GH hosted runners. |
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.
Adds
Publication.resolveLocator, for when a storedLocatorpoints at an href that is no longer in the reading order because the publication was re-issued with different resource granularity.Nota hit this switching audiobooks from WebPubAudio (reading order =
NNNNN-00XX-generic.xhtmlspine items with media overlays) to WebPubAudioOnly (reading order =.mp3files), so some stored positions address resources that no longer exist. The API is deliberately generic — no Nota-specific naming — because re-segmentation is a general Readium problem.Scope, corrected
An earlier revision of this description claimed every previously stored position was stale. That is wrong. Plenty of stored lastmarks still match their publication exactly, and for those
resolveLocatorreturns them unchanged at step 1 below.The evidence for the ones that don't is Sentry, where
readingOrder[position - 1]is still throwing daily:LYT4-WNRangeError (length): Not in inclusive range 0..24: 56LocatorMiddleware._bulkLoadLastmarkLYT4-HRRangeError (length): Not in inclusive range 0..13: 23Both are position-based remaps against a reading order that has since shrunk — exactly what step 2 is for.
Resolution order
linkWithHrefhits → return unchanged. Cheap, so the call is safe to make unconditionally.totalProgressionpresent and > 0 → resolve against accumulatedLink.duration. The only segmentation-independent field, so preferred.position→ bounds-safeelementAtOrNull(position - 1). Exact only when the new segmentation lines up with the old.null, so the caller decides rather than this guessing.On any href change the stale intra-resource locations are dropped (
cssSelector,fragments),progressionis recomputed or dropped,typebecomes the resolved link's, andpositionis recomputed. Locator-leveladditionalProperties(x-type,x-timestamp, …) are preserved.Also fixes an iOS receiver bug that this feature would otherwise expose
Review caught that
FlutterAudioNavigator.getTimeOffsetForLocatorWithProgressionshadowed its ownlocatorparameter withaudioLocator, so the parameter was never read. Called fromresolveLocator, that scaled progression by the duration of the track already playing instead of the target track.Reproduced on a simulator with the 3-track audiobook fixture — jumping to track 3 (80s) at progression 0.5 while track 1 (10s) played landed at 5.0s instead of 40.0s:
The cold-open path is worse, and it is why this belongs in this PR rather than a separate one.
_initialLocator = resolveLocator(initialLocator)runs before any track plays, soaudioLocatoris nil, the helper returns nil, andcopyWithOffsetfalls back tot=0. Today at=fragment usually rescues that. ButPublication.resolveLocatordeliberately drops fragments and keeps progression, so shipping this feature without the Swift fix would have turned "restore lands at a slightly wrong offset" into "restore lands at the start of the track" — worse than before.Scope checked rather than assumed:
seek(toProgression:)is unaffected — it already passesaudioLocatorexplicitly, which is the current track it is meant to scale by.resolveLocatornow passesresolvedLocator, so the duration lookup uses the href after the reading-order repair.goToLocatorresolves the duration from the target item, andseekToProgressionuses the current track by design.Verification
flutter testinflutter_readium_platform_interface: 208/208bin/format: 0 changed ·bin/analyze: no issues across all 3 packagesNotes for review
_relocatetakes an index, not aLink.Linkcompares by value, soreadingOrder.indexOf(link)reports the first of any duplicate entries — a position the locator was never at. There is a regression test; it fails withExpected: <3> Actual: <1>against the value-based version. NotelocatorFromLinkstill has that pattern; not touched here.33238, 59 mp3 tracks, all withduration) and step 2's arithmetic reproduces its storedtotalProgressionof0.6206to within0.00004.33238lastmark resolves cleanly at step 1 — its href is in the reading order and its position is in range — yet the book still restored to the start on device. That is a separate rendition-selection problem in the host app, not somethingresolveLocatorcan see. Do not close 0% reports on the strength of this PR alone.durationon every reading-orderLink. Without durations it falls through toposition, which is the case that is already failing.resolveLocatorinternally (goToLocator/audioEnable(fromLocator:)tolerating an unresolvable locator) is a deliberate follow-up.🤖 Generated with Claude Code