Skip to content

feat(publication): resolve locators whose href left the reading order - #210

Merged
ddfreiling merged 7 commits into
Notalib:mainfrom
ddfreiling:feat/resolve-locator-reading-order-change
Aug 27, 2026
Merged

feat(publication): resolve locators whose href left the reading order#210
ddfreiling merged 7 commits into
Notalib:mainfrom
ddfreiling:feat/resolve-locator-reading-order-change

Conversation

@ddfreiling

@ddfreiling ddfreiling commented Aug 24, 2026

Copy link
Copy Markdown
Member

Adds Publication.resolveLocator, for when a stored Locator points 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.xhtml spine items with media overlays) to WebPubAudioOnly (reading order = .mp3 files), 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 resolveLocator returns them unchanged at step 1 below.

The evidence for the ones that don't is Sentry, where readingOrder[position - 1] is still throwing daily:

Issue Error Culprit Users
LYT4-WN RangeError (length): Not in inclusive range 0..24: 56 LocatorMiddleware._bulkLoadLastmark 289
LYT4-HR RangeError (length): Not in inclusive range 0..13: 23 bookshelf selector build 252

Both are position-based remaps against a reading order that has since shrunk — exactly what step 2 is for.

Resolution order

  1. linkWithHref hits → return unchanged. Cheap, so the call is safe to make unconditionally.
  2. totalProgression present and > 0 → resolve against accumulated Link.duration. The only segmentation-independent field, so preferred.
  3. position → bounds-safe elementAtOrNull(position - 1). Exact only when the new segmentation lines up with the old.
  4. Otherwise null, so the caller decides rather than this guessing.

On any href change the stale intra-resource locations are dropped (cssSelector, fragments), progression is recomputed or dropped, type becomes the resolved link's, and position is recomputed. Locator-level additionalProperties (x-type, x-timestamp, …) are preserved.

Also fixes an iOS receiver bug that this feature would otherwise expose

Review caught that FlutterAudioNavigator.getTimeOffsetForLocatorWithProgression shadowed its own locator parameter with audioLocator, so the parameter was never read. Called from resolveLocator, 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:

Expected: a value less than <0.0>
  Actual: <35.0>
Landed at 5.0s: closer to the current-track answer (5.0s)
than to the target-track answer (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, so audioLocator is nil, the helper returns nil, and copyWithOffset falls back to t=0. Today a t= fragment usually rescues that. But Publication.resolveLocator deliberately 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 passes audioLocator explicitly, which is the current track it is meant to scale by.
  • resolveLocator now passes resolvedLocator, so the duration lookup uses the href after the reading-order repair.
  • Android is correct on both paths and needs no change: goToLocator resolves the duration from the target item, and seekToProgression uses the current track by design.

Verification

  • flutter test in flutter_readium_platform_interface: 208/208
  • iOS integration suite on a simulator: 42/42 (iPhone 17 Pro, iOS 26.5), including the new cross-track regression test. Confirmed red before the Swift fix, green after.
  • bin/format: 0 changed · bin/analyze: no issues across all 3 packages
  • Dart tests written first (red → green) and built on three real production lastmark records

Notes for review

  • _relocate takes an index, not a Link. Link compares by value, so readingOrder.indexOf(link) reports the first of any duplicate entries — a position the locator was never at. There is a regression test; it fails with Expected: <3> Actual: <1> against the value-based version. Note locatorFromLink still has that pattern; not touched here.
  • Dart tests use synthetic reading orders built to match the fixtures' arithmetic, not a real WebPubAudioOnly manifest. One real manifest has since been checked by hand (Nota 33238, 59 mp3 tracks, all with duration) and step 2's arithmetic reproduces its stored totalProgression of 0.6206 to within 0.00004.
  • This does not fix every "book shows 0%" report. That same 33238 lastmark 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 something resolveLocator can see. Do not close 0% reports on the strength of this PR alone.
  • Step 2 needs duration on every reading-order Link. Without durations it falls through to position, which is the case that is already failing.
  • Making the native navigators call resolveLocator internally (goToLocator / audioEnable(fromLocator:) tolerating an unresolvable locator) is a deliberate follow-up.

🤖 Generated with Claude Code

ddfreiling and others added 5 commits August 24, 2026 21:29
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>
@ddfreiling
ddfreiling requested a review from m-abs August 25, 2026 01:46
ddfreiling and others added 2 commits August 27, 2026 20:47
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>
@ddfreiling

Copy link
Copy Markdown
Member Author

CI integration test timeout are flukes on GH hosted runners.
Confirmed pass with local simulators.

@ddfreiling
ddfreiling merged commit 6b21b31 into Notalib:main Aug 27, 2026
12 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants