fix: improve KOSync bidirectional position matching accuracy (#1897)
## Summary **Goal:** Fix bidirectional KOSync position matching between CrossPoint and KOReader so that syncing in either direction lands on the correct page with character-level accuracy. **Changes included:** **Download — `toCrossPoint` (server XPath → CrossPoint page)** - **XPath ancestry mode for structured elements**: The previous `ParagraphStreamer` only tracked `<p>` elements. Replaced with a full ancestor-walking mode that correctly resolves XPaths pointing into `<li>`, `<ul>`, and other structured elements. Char offset within the target element is bounded to the matched element's content only. - **Slash-in-attribute-value corrupts depth tracking**: `processByteInTag()` treated every `/` byte as a self-closing tag marker, including `/` inside quoted attribute values (e.g. `xmlns="http://..."`, `src="Links/image.jpg"`). This drove `htmlDepth` to 0 prematurely, causing the ancestry search to exit far short of the target paragraph. Fixed with `inAttrQuote` tracking. - **Off-by-one in page formula**: `intra * totalPages` rounds up incorrectly for last-page positions. Changed to `intra * (totalPages - 1)` to map the `[0, 1]` intra fraction correctly onto the `[0, totalPages-1]` page range. Example: page 14 of 17 was returned as 15. **Upload — `toKOReader` (CrossPoint page → server XPath)** - **Off-by-one in page-to-intra formula**: Symmetric fix — `pageNumber / totalPages` changed to `pageNumber / (totalPages - 1)`, with the guard updated from `> 0` to `> 1` to avoid division by zero. - **`<li>`-based XPath generation**: When the current page starts on a list item, `findXPathForProgress` now generates `ul[N]/li[M]` XPaths rather than falling back to the preceding `<p>`. Requires the new `listItemIndex` field in `PageLutEntry` (section cache version bumped to 23). - **Text-node precision with correct `text()[N].M` format**: KOReader expects `text()[N].M` where `N` is the 1-based index of the specific text node within the element. The previous attempt generated `text().M` (no brackets), which caused KOReader to jump to the front of the book. Implements a per-element text-node index stack in `XPathProgressResolver` — parallel to the existing element path stack — that correctly tracks text node indices relative to each element. Empty text nodes from bare anchor elements (`<a id="anchor"/>`) are intentionally skipped, matching KOReader's own text node counting behavior. **Reviewer-caught bugs** - **Double `onCloseTag()` on malformed `</br/>`**: Both the `tagIsClose` path and the self-closing `/` check were firing, double-decrementing `htmlDepth`. Fixed with a `!tagIsClose` guard. - **Dangling pointer in `LOG_DBG`**: `std::to_string(*nextParagraphPage).c_str()` passed a pointer to a temporary destroyed before the variadic call. Fixed with `snprintf` into a stack `char[8]` buffer. ## Additional Context - Section cache version bumped from 22 → 23 due to the new `listItemIndex` field in `PageLutEntry`. Users upgrading will see a one-time re-render of all cached sections on first load — no data loss. - The `textNodeIndexStack` in `XPathProgressResolver` is a `std::vector<int>` that mirrors the existing `path` and `parentStates` stacks — same depth, same lifetime. No additional heap pressure beyond what was already present. - All fixes verified on device with *Gentle and Lowly* by Dane C. Ortlund (spine 21, 17 pages). Download syncs land on the correct page; upload syncs land at the correct paragraph with character-level offset. ## Test plan - [ ] Download: sync from KOReader → CrossPoint lands on correct page for `text()[N].M` XPaths - [ ] Download: ancestry correctly resolves `<li>` positions inbound from KOReader - [ ] Upload: sync from CrossPoint → KOReader lands within one page for mid-paragraph positions - [ ] Upload: sync from CrossPoint → KOReader correctly targets `<li>` elements when page starts on a list item - [ ] Upload: `text()[N].M` format XPaths do not cause KOReader to jump to front of book - [ ] Section cache version 23: delete `.crosspoint/` and verify clean re-parse with no crashes --- ### AI Usage Did you use AI tools to help write this code? **YES** — developed with Claude Code (Anthropic). --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Sonnet 4.6
parent
628794f8b8
commit
bf894fd343
+68
-11
@@ -10,14 +10,16 @@
|
||||
#include "parsers/ChapterHtmlSlimParser.h"
|
||||
|
||||
namespace {
|
||||
constexpr uint8_t SECTION_FILE_VERSION = 22;
|
||||
constexpr uint8_t SECTION_FILE_VERSION = 23;
|
||||
constexpr uint32_t HEADER_SIZE = sizeof(uint8_t) + sizeof(int) + sizeof(float) + sizeof(bool) + sizeof(uint8_t) +
|
||||
sizeof(uint16_t) + sizeof(uint16_t) + sizeof(uint16_t) + sizeof(bool) + sizeof(bool) +
|
||||
sizeof(uint8_t) + sizeof(uint32_t) + sizeof(uint32_t) + sizeof(uint32_t);
|
||||
sizeof(uint8_t) + sizeof(uint32_t) + sizeof(uint32_t) + sizeof(uint32_t) +
|
||||
sizeof(uint32_t);
|
||||
|
||||
struct PageLutEntry {
|
||||
uint32_t fileOffset;
|
||||
uint16_t paragraphIndex;
|
||||
uint16_t listItemIndex;
|
||||
};
|
||||
} // namespace
|
||||
|
||||
@@ -50,7 +52,7 @@ void Section::writeSectionFileHeader(const int fontId, const float lineCompressi
|
||||
sizeof(extraParagraphSpacing) + sizeof(paragraphAlignment) + sizeof(viewportWidth) +
|
||||
sizeof(viewportHeight) + sizeof(pageCount) + sizeof(hyphenationEnabled) +
|
||||
sizeof(embeddedStyle) + sizeof(imageRendering) + sizeof(uint32_t) +
|
||||
sizeof(uint32_t) + sizeof(uint32_t),
|
||||
sizeof(uint32_t) + sizeof(uint32_t) + sizeof(uint32_t),
|
||||
"Header size mismatch");
|
||||
serialization::writePod(file, SECTION_FILE_VERSION);
|
||||
serialization::writePod(file, fontId);
|
||||
@@ -66,6 +68,7 @@ void Section::writeSectionFileHeader(const int fontId, const float lineCompressi
|
||||
serialization::writePod(file, static_cast<uint32_t>(0)); // Placeholder for LUT offset (patched later)
|
||||
serialization::writePod(file, static_cast<uint32_t>(0)); // Placeholder for anchor map offset (patched later)
|
||||
serialization::writePod(file, static_cast<uint32_t>(0)); // Placeholder for paragraph LUT offset (patched later)
|
||||
serialization::writePod(file, static_cast<uint32_t>(0)); // Placeholder for li LUT offset (patched later)
|
||||
}
|
||||
|
||||
bool Section::loadSectionFile(const int fontId, const float lineCompression, const bool extraParagraphSpacing,
|
||||
@@ -217,8 +220,8 @@ bool Section::createSectionFile(const int fontId, const float lineCompression, c
|
||||
ChapterHtmlSlimParser visitor(
|
||||
epub, tmpHtmlPath, renderer, fontId, lineCompression, extraParagraphSpacing, paragraphAlignment, viewportWidth,
|
||||
viewportHeight, hyphenationEnabled,
|
||||
[this, &lut](std::unique_ptr<Page> page, const uint16_t paragraphIndex) {
|
||||
lut.push_back({this->onPageComplete(std::move(page)), paragraphIndex});
|
||||
[this, &lut](std::unique_ptr<Page> page, const uint16_t paragraphIndex, const uint16_t listItemIndex) {
|
||||
lut.push_back({this->onPageComplete(std::move(page)), paragraphIndex, listItemIndex});
|
||||
},
|
||||
embeddedStyle, contentBase, imageBasePath, imageRendering, popupFn, cssParser);
|
||||
Hyphenator::setPreferredLanguage(epub->getLanguage());
|
||||
@@ -270,12 +273,18 @@ bool Section::createSectionFile(const int fontId, const float lineCompression, c
|
||||
serialization::writePod(file, entry.paragraphIndex);
|
||||
}
|
||||
|
||||
// Patch header with final pageCount, lutOffset, anchorMapOffset, and paragraphLutOffset
|
||||
file.seek(HEADER_SIZE - sizeof(uint32_t) * 3 - sizeof(pageCount));
|
||||
const uint32_t liLutFileOffset = static_cast<uint32_t>(file.position());
|
||||
for (const auto& entry : lut) {
|
||||
serialization::writePod(file, entry.listItemIndex);
|
||||
}
|
||||
|
||||
// Patch header with final pageCount, lutOffset, anchorMapOffset, paragraphLutOffset, and liLutOffset
|
||||
file.seek(HEADER_SIZE - sizeof(uint32_t) * 4 - sizeof(pageCount));
|
||||
serialization::writePod(file, pageCount);
|
||||
serialization::writePod(file, lutOffset);
|
||||
serialization::writePod(file, anchorMapOffset);
|
||||
serialization::writePod(file, paragraphLutOffset);
|
||||
serialization::writePod(file, liLutFileOffset);
|
||||
// Explicit close() required: member variable persists beyond function scope
|
||||
file.close();
|
||||
if (cssParser) {
|
||||
@@ -289,7 +298,7 @@ std::unique_ptr<Page> Section::loadPageFromSectionFile() {
|
||||
return nullptr;
|
||||
}
|
||||
|
||||
file.seek(HEADER_SIZE - sizeof(uint32_t) * 3);
|
||||
file.seek(HEADER_SIZE - sizeof(uint32_t) * 4);
|
||||
uint32_t lutOffset;
|
||||
serialization::readPod(file, lutOffset);
|
||||
file.seek(lutOffset + sizeof(uint32_t) * currentPage);
|
||||
@@ -310,7 +319,7 @@ std::optional<uint16_t> Section::getPageForAnchor(const std::string& anchor) con
|
||||
}
|
||||
|
||||
const uint32_t fileSize = f.size();
|
||||
f.seek(HEADER_SIZE - sizeof(uint32_t) * 2);
|
||||
f.seek(HEADER_SIZE - sizeof(uint32_t) * 3);
|
||||
uint32_t anchorMapOffset;
|
||||
serialization::readPod(f, anchorMapOffset);
|
||||
if (anchorMapOffset == 0 || anchorMapOffset >= fileSize) {
|
||||
@@ -340,7 +349,7 @@ std::optional<uint16_t> Section::getPageForParagraphIndex(const uint16_t pIndex)
|
||||
}
|
||||
|
||||
const uint32_t fileSize = f.size();
|
||||
f.seek(HEADER_SIZE - sizeof(uint32_t));
|
||||
f.seek(HEADER_SIZE - sizeof(uint32_t) * 2);
|
||||
uint32_t paragraphLutOffset;
|
||||
serialization::readPod(f, paragraphLutOffset);
|
||||
if (paragraphLutOffset == 0 || paragraphLutOffset >= fileSize) {
|
||||
@@ -379,7 +388,7 @@ std::optional<uint16_t> Section::getParagraphIndexForPage(const uint16_t page) c
|
||||
}
|
||||
|
||||
const uint32_t fileSize = f.size();
|
||||
f.seek(HEADER_SIZE - sizeof(uint32_t));
|
||||
f.seek(HEADER_SIZE - sizeof(uint32_t) * 2);
|
||||
uint32_t paragraphLutOffset;
|
||||
serialization::readPod(f, paragraphLutOffset);
|
||||
if (paragraphLutOffset == 0 || paragraphLutOffset >= fileSize) {
|
||||
@@ -403,3 +412,51 @@ std::optional<uint16_t> Section::getParagraphIndexForPage(const uint16_t page) c
|
||||
serialization::readPod(f, pIdx);
|
||||
return pIdx;
|
||||
}
|
||||
|
||||
std::optional<uint16_t> Section::getPageForListItemIndex(const uint16_t liIndex) const {
|
||||
FsFile f;
|
||||
if (!Storage.openFileForRead("SCT", filePath, f)) {
|
||||
return std::nullopt;
|
||||
}
|
||||
|
||||
const uint32_t fileSize = f.size();
|
||||
f.seek(HEADER_SIZE - sizeof(uint32_t));
|
||||
uint32_t liLutOffset;
|
||||
serialization::readPod(f, liLutOffset);
|
||||
if (liLutOffset == 0 || liLutOffset >= fileSize) {
|
||||
return std::nullopt;
|
||||
}
|
||||
|
||||
// The li LUT shares count with the paragraph LUT; read count from paragraphLutOffset
|
||||
f.seek(HEADER_SIZE - sizeof(uint32_t) * 2);
|
||||
uint32_t paragraphLutOffset;
|
||||
serialization::readPod(f, paragraphLutOffset);
|
||||
if (paragraphLutOffset == 0 || paragraphLutOffset >= fileSize) {
|
||||
return std::nullopt;
|
||||
}
|
||||
|
||||
f.seek(paragraphLutOffset);
|
||||
uint16_t count;
|
||||
serialization::readPod(f, count);
|
||||
if (count == 0) {
|
||||
return std::nullopt;
|
||||
}
|
||||
|
||||
const uint32_t lutEnd = liLutOffset + count * sizeof(uint16_t);
|
||||
if (lutEnd > fileSize) {
|
||||
return std::nullopt;
|
||||
}
|
||||
|
||||
f.seek(liLutOffset);
|
||||
uint16_t resultPage = count - 1;
|
||||
for (uint16_t i = 0; i < count; i++) {
|
||||
uint16_t pageLiIdx;
|
||||
serialization::readPod(f, pageLiIdx);
|
||||
if (pageLiIdx >= liIndex) {
|
||||
resultPage = i;
|
||||
break;
|
||||
}
|
||||
}
|
||||
|
||||
return resultPage;
|
||||
}
|
||||
|
||||
@@ -46,6 +46,9 @@ class Section {
|
||||
// Look up the page number for a synthetic paragraph index from XPath p[N].
|
||||
std::optional<uint16_t> getPageForParagraphIndex(uint16_t pIndex) const;
|
||||
|
||||
// Look up the page number for a running list-item index from the li LUT.
|
||||
std::optional<uint16_t> getPageForListItemIndex(uint16_t liIndex) const;
|
||||
|
||||
// Look up the synthetic paragraph index for the given rendered page.
|
||||
std::optional<uint16_t> getParagraphIndexForPage(uint16_t page) const;
|
||||
};
|
||||
|
||||
@@ -157,6 +157,9 @@ void XMLCALL ChapterHtmlSlimParser::startElement(void* userData, const XML_Char*
|
||||
if (strcmp(name, "p") == 0) {
|
||||
self->xpathParagraphIndex++;
|
||||
}
|
||||
if (strcmp(name, "li") == 0) {
|
||||
self->xpathListItemIndex++;
|
||||
}
|
||||
|
||||
// Extract class, style, and id attributes
|
||||
std::string classAttr;
|
||||
@@ -449,7 +452,8 @@ void XMLCALL ChapterHtmlSlimParser::startElement(void* userData, const XML_Char*
|
||||
if (self->currentPage && !self->currentPage->elements.empty() &&
|
||||
(self->currentPageNextY + imageMarginTop + displayHeight + imageMarginBottom >
|
||||
self->viewportHeight)) {
|
||||
self->completePageFn(std::move(self->currentPage), self->xpathParagraphIndex);
|
||||
self->completePageFn(std::move(self->currentPage), self->xpathParagraphIndex,
|
||||
self->xpathListItemIndex);
|
||||
self->completedPageCount++;
|
||||
self->currentPage.reset(new Page());
|
||||
if (!self->currentPage) {
|
||||
@@ -1115,7 +1119,7 @@ bool ChapterHtmlSlimParser::parseAndBuildPages() {
|
||||
anchorData.push_back({std::move(pendingAnchorId), static_cast<uint16_t>(completedPageCount)});
|
||||
pendingAnchorId.clear();
|
||||
}
|
||||
completePageFn(std::move(currentPage), xpathParagraphIndex);
|
||||
completePageFn(std::move(currentPage), xpathParagraphIndex, xpathListItemIndex);
|
||||
completedPageCount++;
|
||||
currentPage.reset();
|
||||
currentTextBlock.reset();
|
||||
@@ -1133,7 +1137,7 @@ void ChapterHtmlSlimParser::addLineToPage(std::shared_ptr<TextBlock> line) {
|
||||
}
|
||||
|
||||
if (currentPageNextY + lineHeight > viewportHeight) {
|
||||
completePageFn(std::move(currentPage), xpathParagraphIndex);
|
||||
completePageFn(std::move(currentPage), xpathParagraphIndex, xpathListItemIndex);
|
||||
completedPageCount++;
|
||||
currentPage.reset(new Page());
|
||||
currentPageNextY = 0;
|
||||
|
||||
@@ -25,7 +25,7 @@ class ChapterHtmlSlimParser {
|
||||
std::shared_ptr<Epub> epub;
|
||||
const std::string& filepath;
|
||||
GfxRenderer& renderer;
|
||||
std::function<void(std::unique_ptr<Page>, uint16_t)> completePageFn;
|
||||
std::function<void(std::unique_ptr<Page>, uint16_t, uint16_t)> completePageFn;
|
||||
std::function<void()> popupFn; // Popup callback
|
||||
int depth = 0;
|
||||
int skipUntilDepth = INT_MAX;
|
||||
@@ -76,6 +76,7 @@ class ChapterHtmlSlimParser {
|
||||
std::vector<std::pair<std::string, uint16_t>> anchorData;
|
||||
std::string pendingAnchorId; // deferred until after previous text block is flushed
|
||||
uint16_t xpathParagraphIndex = 0;
|
||||
uint16_t xpathListItemIndex = 0;
|
||||
|
||||
// Footnote link tracking
|
||||
bool insideFootnoteLink = false;
|
||||
@@ -100,7 +101,7 @@ class ChapterHtmlSlimParser {
|
||||
const int fontId, const float lineCompression, const bool extraParagraphSpacing,
|
||||
const uint8_t paragraphAlignment, const uint16_t viewportWidth,
|
||||
const uint16_t viewportHeight, const bool hyphenationEnabled,
|
||||
const std::function<void(std::unique_ptr<Page>, uint16_t)>& completePageFn,
|
||||
const std::function<void(std::unique_ptr<Page>, uint16_t, uint16_t)>& completePageFn,
|
||||
const bool embeddedStyle, const std::string& contentBase,
|
||||
const std::string& imageBasePath, const uint8_t imageRendering = 0,
|
||||
const std::function<void()>& popupFn = nullptr, const CssParser* cssParser = nullptr)
|
||||
|
||||
Reference in New Issue
Block a user