lvdocview: fix source page numbers with a page-list not in reading order - #711
Open
albertmichaelj wants to merge 3 commits into
Open
lvdocview: fix source page numbers with a page-list not in reading order#711albertmichaelj wants to merge 3 commits into
albertmichaelj wants to merge 3 commits into
Conversation
A publisher page list is not required to be in reading order, so the page map items are not necessarily ordered by their position in the document. Add a helper to order them by their resolved _doc_y, to be used by LVDocView::updatePageMapInfo() once it has resolved these positions. Items sharing the same position are tie-broken on their _index, so that they keep their relative order. Insertion sort: page lists are small (a few hundred items at most), and are usually already ordered or nearly so, which insertion sort handles in O(n) passes. It also avoids any new dependency: LVPtrVector::sort() uses qsort(), which is not stable, and std::stable_sort() would pull in <algorithm>. Kept private (with LVDocView made a friend, as it already is of LVPageMapItem): _doc_y is only known to be up-to-date inside updatePageMapInfo(), so this is not something to expose more widely. Co-authored-by: Tachibana Shin <tachibshin@duck.com>
The page map is built in page list order (epubfmt.cpp: ReadEpubNavPageMap(), ReadEpubNcxPageList(), ReadEpubAdobePageMap()), and updatePageMapInfo() was ensuring page numbers never go backward *in that list order*: any item whose target is earlier in the document than its predecessor's had its resolved position and page number replaced by the predecessor's. But a page list is not required to be in reading order. Conversions commonly move front matter (a copyright page) to the end of the spine while the page list keeps referencing it among the front matter - and the page list is meant to reference the equivalent location, not to be renumbered (see https://kb.daisy.org/publishing/docs/navigation/pagelist.html). epubcheck also dropped its reading order requirement (w3c/epubcheck commit d0f12a9). With such a page list, a single out-of-order item raises the clamping values to a near-the-end position, and every following item is clamped up to it. The displayed source page number then freezes on the last label before it, and the page numbers in the margin all pile up at that one position. So, resolve each item's position from its XPointer with no clamping, order the items by that resolved position, and only then ensure page numbers don't go backward - which, positions being ordered, now only means having items with no page number of their own get their predecessor's. With the sample EPUBs from the report (a spine of title(i), ch1(1), ch2(2), ch3(3), copyright(ii), with the page list in print order i, ii, 1, 2, 3): rendered page 0 1 2 3 4 before i i i i ii after i 1 2 3 ii Ordering the items also makes true what consumers already expect: KOReader's cre.cpp binary searches the page map by doc_y to find the current or a visible page label ("We expect items to be ordered by doc_y"), and takes the first and last items as the first and last page labels. The sorting is skipped when the resolved positions are already in ascending order, which is the case for a page list in reading order and for synthetic page maps (ldomDocument::buildSyntheticPageMap() walks the document in order). Co-authored-by: Tachibana Shin <tachibshin@duck.com>
_index is set by addPage() to the index of the item in the page map, so renumber the items after having reordered them, so it keeps that meaning (and stays a valid tie-break if we are called again). Nothing reads LVPageMapItem::getIndex() currently, but the original page list order is of no use once we have ordered the items by their position in the document.
poire-z
approved these changes
Jul 27, 2026
poire-z
left a comment
Contributor
There was a problem hiding this comment.
Thanks. Reads well.
@tachibana-shin : any comment ?
tachibana-shin
approved these changes
Jul 27, 2026
Contributor
|
I think it's good |
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.
Fixes #688.
Problem
The page map is built in page list order, and
LVDocView::updatePageMapInfo()made_page/_doc_ynon-decreasing in that list order. But a page list is not required to be in reading order: conversions commonly move front matter (a copyright page) to the end of the spine while the page list keeps referencing it among the front matter, which is what the DAISY page-list guidance asks for, and epubcheck dropped its reading order requirement in w3c/epubcheck@d0f12a9.With such a page list, one out-of-order entry raises the clamping values to a near-the-end position, and every entry after it is clamped up to that. With the two sample EPUBs from #688 (identical spine
title(i) ch1(1) ch2(2) ch3(3) copyright(ii), one with the page list in print orderi, ii, 1, 2, 3):So the source page number freezes on the last label before the out-of-order entry, and the page numbers in the margin all pile up at that one position.
Fix
updatePageMapInfo()now resolves each item's position with no clamping, orders the items by that resolved position, and only then ensures page numbers don't go backward. Three commits:lvtinydom: add LVPageMap::sortByDocY()— private helper (LVDocViewmade a friend, as it already is ofLVPageMapItem): insertion sort on_doc_y, tie-broken on_index.lvdocview: updatePageMapInfo(): order page map by resolved position— the fix itself.lvtinydom: LVPageMap::sortByDocY(): keep _index in sync— the extra pass you asked for, kept separate so it is easy to review or drop.Notes for review
std::sort→ insertion sort follow-up, used with their permission and credited viaCo-authored-by:. Two changes to it: the_indexpass above, and the handling of unresolvable entries below.buildSyntheticPageMap()walks the document in order)._doc_y = -1and filling them in after the sort would instead move them all to the start of the page map.)cre.cppbinary searches it bydoc_yto find the current or a visible page label ("We expect items to be ordered by doc_y"), and reads the first and last items as the first and last page labels. The "go to page" list follows reading order too, so jumping to a page lands where the label is._childrenorder, so a book already in cache keeps its old page map until its next re-render (any setting change triggering one, sincerender()callsinvalidatePageInfo()). I did not bumpCACHE_FILE_FORMAT_VERSIONfor this — your call if you'd rather have it fixed on first reopen.Testing
i 1 2 3 ii), a reading-order page list left untouched with no sort, stability and_indexrenumbering on entries sharing a position, unresolvable entries, an entry past the last rendered page, empty/single-item page maps, idempotency on a second run, and 2000 randomized page maps (ordered, stable,_index= 0..n-1, positions preserved).This change is