diff --git a/internal/handlers/koreader.go b/internal/handlers/koreader.go index 075736b..1b5fd12 100644 --- a/internal/handlers/koreader.go +++ b/internal/handlers/koreader.go @@ -81,28 +81,39 @@ func (h *KOReaderHandler) loadAnnotationEpub(ctx context.Context, mediaItemID pg // convertHighlightPositions resolves a device annotation's pos0/pos1 // locators to canonical CFIs through the shared facade. contextText is the -// selection's own text — the ideal anchor for the converter's verification -// and text-search rungs. percentage anchors the last-resort fallback so a -// failed conversion degrades to the neighborhood of the true position -// rather than the document start. +// selection's own text — a quote of the document, so the converter can +// verify structural landings against it and, when it must search, anchor a +// range end that spans block boundaries. percentage anchors the +// last-resort fallback. Only structural/exact landings are returned: a +// low-confidence conversion yields "" so an echo preserves the row's +// existing web CFIs (applyLWW coalesces empty) instead of clobbering them +// with a guess, and a new device highlight paints nowhere rather than in +// the wrong place. func (h *KOReaderHandler) convertHighlightPositions(ec annotationEpub, pos0, pos1, contextText string, percentage float64) (string, string) { if pos0 == "" || !ec.convertible() { return "", "" } startLoc := wsync.ConvertToCanonical(wsync.LocatorSourceKOReader, pos0, percentage, contextText, ec.mediaItem.FormatGroup, ec.epubPath, "") - endLoc := wsync.ConvertToCanonical(wsync.LocatorSourceKOReader, pos1, percentage, "", ec.mediaItem.FormatGroup, ec.epubPath, "") - endCFI := endLoc.CFI - // The end conversion carries no context text, so unless it resolved - // exactly it degenerates to a percentage fallback anchored at the - // document start — useless as a range end. When the START resolved - // structurally/exactly, derive the end from it: same node, character - // offset advanced by the selection's UTF-16 length (the CFI offset - // unit). - if endLoc.Precision != "exact" && endLoc.Precision != "structural" && - (startLoc.Precision == "exact" || startLoc.Precision == "structural") && contextText != "" { - endCFI = extendCFIByLength(startLoc.CFI, contextText) + startCFI := webUsableCFI(startLoc) + endCFI := "" + // A text-search start matched the selection text itself: its extent + // is the selection's true end, even across blocks. + if startCFI != "" && startLoc.EndCFI != "" { + endCFI = startLoc.EndCFI } - return startLoc.CFI, endCFI + if endCFI == "" && pos1 != "" { + endLoc := wsync.ConvertToCanonical(wsync.LocatorSourceKOReader, pos1, percentage, "", ec.mediaItem.FormatGroup, ec.epubPath, "") + endCFI = webUsableCFI(endLoc) + } + // The end conversion carries no context text; when neither resolved + // confidently, derive the end from the start advanced by the + // selection's UTF-16 length (the CFI offset unit). Multi-node + // selections produce an out-of-range offset — harmless: resolution + // clamps or fails, and consumers fall back to the start. + if endCFI == "" && startCFI != "" && contextText != "" { + endCFI = extendCFIByLength(startCFI, contextText) + } + return startCFI, endCFI } // convertBookmarkPosition resolves a device bookmark's locator to the @@ -866,10 +877,26 @@ func (h *KOReaderHandler) updateProgressForBook(c *echo.Context, deviceID pgtype contextText = *book.ContextText } loc := wsync.ConvertToCanonical(wsync.LocatorSourceKOReader, *epubcfi, pct, contextText, ec.mediaItem.FormatGroup, ec.epubPath, "") - if loc.CFI != "" && loc.CFI != *epubcfi { + switch { + case strings.HasPrefix(loc.CFI, "epubcfi(") && + (loc.Precision == "structural" || loc.Precision == "exact"): converted := loc.CFI epubcfi = &converted log.Printf("Bookhoard: CRE→CFI converted progress (%s) to %s", loc.Precision, converted) + case loc.CFI != "" && loc.CFI != *epubcfi && + (loc.Precision == "element" || loc.Precision == "section"): + // Fragment-ID positions resolve to a section href: keep + // serving it (legacy behavior). + converted := loc.CFI + epubcfi = &converted + log.Printf("Bookhoard: CRE→CFI converted progress (%s) to href %s", loc.Precision, converted) + default: + // Low-confidence (percentage/fallback): store no + // canonical locator — the row's percentage restores + // approximately instead of a confidently-wrong CFI, + // and the device keeps its own native position. + log.Printf("Bookhoard: CRE→CFI conversion low-confidence (%s) for %s; storing percentage only", loc.Precision, *epubcfi) + epubcfi = nil } } } diff --git a/internal/sync/cfi_converter.go b/internal/sync/cfi_converter.go index 8a3803b..a23fdfe 100644 --- a/internal/sync/cfi_converter.go +++ b/internal/sync/cfi_converter.go @@ -12,6 +12,7 @@ import ( "strconv" "strings" "sync" + "unicode" "unicode/utf8" "golang.org/x/net/html" @@ -245,7 +246,12 @@ func parseElementPart(part string) (string, int) { } type ConversionResult struct { - EPUBCFI string + EPUBCFI string + // EndEPUBCFI is set when the conversion matched a context that is a + // quote of the document (text search): the range end anchor of the + // quoted text, valid across block boundaries. Selections use it as + // the highlight end; point positions ignore it. + EndEPUBCFI string Href string Percentage float64 Precision string @@ -510,23 +516,22 @@ func (c *CFIConverter) convertByStructuralPath(body *html.Node, xp *CREXPointer, return nil } - // Text is the final verification: when the device sent usable words and - // they disagree with this structural landing, reject it and let text - // search / percentage decide rather than storing a confident-but-wrong CFI. + // Text is the final verification, in quote form: a usable context must + // be a prefix of the document as read forward from this landing point. + // Device captures are exactly that shape — a position's context is the + // text from the position onward (the plugin's walk-up may concatenate + // the heading with the paragraphs below), and a selection is the + // document text between its two anchors. The single-block containment + // checks stay as secondary acceptance for reverse shapes. if usable, normalized := usableContextText(contextText); usable { + doc := documentTextFrom(body, textNode, localOffset, utf8.RuneCountInString(normalized)+64) flat := blockFlattenedText(textNode) - if flat != "" && !strings.Contains(flat, normalized) && !strings.Contains(normalized, flat) { - // Compare a prefix too: device sends ~100 chars from the reader - // position while the block may be longer. - prefix := normalized - if utf8.RuneCountInString(prefix) > 40 { - runes := []rune(prefix) - prefix = string(runes[:40]) - } - if !strings.Contains(flat, prefix) { - log.Printf("Bookhoard: structural landing disagrees with context in %s (block %q vs ctx %q)", href, truncateForLog(flat, 80), truncateForLog(normalized, 80)) - return nil - } + if !(strings.HasPrefix(doc, normalized) || + strings.Contains(flat, normalized) || + strings.Contains(normalized, flat)) { + log.Printf("Bookhoard: structural landing disagrees with context in %s (reads %q vs ctx %q)", + href, truncateForLog(doc, 80), truncateForLog(normalized, 80)) + return nil } } @@ -592,28 +597,149 @@ func (c *CFIConverter) convertFragmentID(s string, storedPercentage float64) (*C }, nil } +// docRunePos records which text node (and rune offset within it) produced +// each rune of the flattened document text. +type docRunePos struct { + node *html.Node + off int +} + +// flattenDocumentForSearch renders the document's text as a +// whitespace-normalized rune stream in document order, crossing block +// boundaries: a single space separates blocks, while inline spans join +// directly (drop-cap splits like Convergence read as one +// word). Every emitted rune carries its source (node, rune offset) so +// matches map back to CFIs. +func flattenDocumentForSearch(body *html.Node) ([]rune, []docRunePos) { + var runes []rune + var poss []docRunePos + pendingSpace := false + var lastBlock *html.Node + + appendNode := func(n *html.Node) { + block := findBlockParent(n) + if lastBlock != nil && block != lastBlock { + pendingSpace = true + } + lastBlock = block + off := 0 + for _, r := range n.Data { + if unicode.IsSpace(r) { + pendingSpace = true + } else { + if pendingSpace && len(runes) > 0 { + runes = append(runes, ' ') + poss = append(poss, docRunePos{node: n, off: off}) + } + pendingSpace = false + runes = append(runes, r) + poss = append(poss, docRunePos{node: n, off: off}) + } + off++ + } + } + + var walk func(n *html.Node) + walk = func(n *html.Node) { + if n.Type == html.TextNode { + if strings.TrimSpace(n.Data) != "" { + appendNode(n) + } + return + } + for c := n.FirstChild; c != nil; c = c.NextSibling { + walk(c) + } + } + walk(body) + return runes, poss +} + +// documentTextFrom reads the whitespace-normalized document text starting +// at rune `offset` in `start` (typically a structural landing point), +// crossing block boundaries — the document "as read" from that position. +// Capped at maxRunes. +func documentTextFrom(body *html.Node, start *html.Node, offset, maxRunes int) string { + runes, poss := flattenDocumentForSearch(body) + begin := -1 + for i := range poss { + if poss[i].node == start && poss[i].off >= offset { + begin = i + break + } + } + if begin < 0 { + return "" + } + // The separator space before a block's first content rune shares its + // (node, offset) — skip it so the read starts on real text. + if runes[begin] == ' ' && begin+1 < len(runes) { + begin++ + } + end := len(runes) + if begin+maxRunes < end { + end = begin + maxRunes + } + return string(runes[begin:end]) +} + func (c *CFIConverter) convertByTextSearch(body *html.Node, xp *CREXPointer, spine *spineCache, href string, storedPercentage float64, contextText string) *ConversionResult { normalizedCtx := normalizeWhitespace(contextText) if normalizedCtx == "" { return nil } - match, matchOffset := findTextInNode(body, normalizedCtx) - if match == nil { - log.Printf("Bookhoard: text search no match for %q in %s", normalizedCtx, href) + // Match against the whole document flattened in reading order — the + // context may span block boundaries (a selection covering several + // paragraphs, a walk-up capture joining a heading with what follows). + runes, poss := flattenDocumentForSearch(body) + text := string(runes) + + words := strings.Fields(normalizedCtx) + quoted := make([]string, len(words)) + for i, w := range words { + quoted[i] = regexp.QuoteMeta(w) + } + pattern := strings.Join(quoted, `\s+`) + re, err := regexp.Compile(pattern) + if err != nil { + return nil + } + loc := re.FindStringIndex(text) + if loc == nil { + log.Printf("Bookhoard: text search no match for %q in %s", truncateForLog(normalizedCtx, 60), href) return nil } + startRune := utf8.RuneCountInString(text[:loc[0]]) + endRune := utf8.RuneCountInString(text[:loc[1]]) // exclusive spineIndex := xp.FragmentIndex - 1 - cfi, err := buildCFI(spineIndex, match, matchOffset) - if err != nil || cfi == "" { + + startPos := poss[startRune] + startCFI, err := buildCFI(spineIndex, startPos.node, startPos.off) + if err != nil || startCFI == "" { log.Printf("Bookhoard: text search found match but buildCFI failed: %v", err) return nil } - log.Printf("Bookhoard: text search matched %q → %s (precision: exact)", normalizedCtx, cfi) + // The matched context is a quote of the document: its extent gives + // selections a true range end, across block boundaries. + endCFI := "" + if endRune > startRune && endRune <= len(poss) { + endPos := poss[endRune-1] + endOff := endPos.off + 1 + if nRunes := utf8.RuneCountInString(endPos.node.Data); endOff > nRunes { + endOff = nRunes + } + if ec, err := buildCFI(spineIndex, endPos.node, endOff); err == nil && ec != "" { + endCFI = ec + } + } + + log.Printf("Bookhoard: text search matched %q → %s (precision: exact)", truncateForLog(normalizedCtx, 60), startCFI) return &ConversionResult{ - EPUBCFI: cfi, + EPUBCFI: startCFI, + EndEPUBCFI: endCFI, Href: href, Percentage: storedPercentage, Precision: "exact", @@ -780,7 +906,10 @@ func (c *CFIConverter) convertByPercentageOffset(body *html.Node, xp *CREXPointe EPUBCFI: cfi, Href: href, Percentage: storedPercentage, - Precision: "exact", + // This is a char-count estimate, not an exact landing: say + // so. Callers gate storage on precision, and a guess must + // never masquerade as an exact anchor. + Precision: "percentage", }, nil } } diff --git a/internal/sync/cfi_converter_test.go b/internal/sync/cfi_converter_test.go index 4b50b79..58efcbd 100644 --- a/internal/sync/cfi_converter_test.go +++ b/internal/sync/cfi_converter_test.go @@ -667,6 +667,96 @@ func TestReverseIgnoresSingleCharContext(t *testing.T) { } } +// A position at a chapter heading sends walk-up context: the heading text +// concatenated with the paragraphs below (block walk-up in the plugin). +// The structural landing at the heading is correct and must verify — the +// context is a prefix of the document as read from the landing. +func TestWalkUpContextVerifiesAtHeading(t *testing.T) { + c := NewCFIConverter(writeDropCapEPUB(t)) + + // ch10: h1[1] "Chapter 10", h1[2] "The Three C's of the New Covenant", + // then paragraphs. Position at h1[2]'s text start; the device captured + // the heading plus the following paragraph. + xp := "/body/DocFragment[2]/body/h1[2]/text().0" + ctx := "The Three C's of the New Covenant The Cleansing Life of Christ" + + result, err := c.ConvertCREToStandard(xp, 0.52, ctx) + if err != nil { + t.Fatalf("ConvertCREToStandard error: %v", err) + } + t.Logf("walk-up heading → %s (%s)", result.EPUBCFI, result.Precision) + if result.Precision != "structural" { + t.Fatalf("expected structural precision (quote verification), got %s (%s)", result.Precision, result.EPUBCFI) + } + if strings.HasSuffix(result.EPUBCFI, "/4/2/1:0)") { + t.Errorf("collapsed to doc start: %s", result.EPUBCFI) + } + // The landing must be the heading, not a paragraph below it. + reverse, rerr := c.ConvertStandardToCRE(result.EPUBCFI, result.Percentage, "") + if rerr != nil { + t.Fatalf("reverse conversion error: %v", rerr) + } + if !strings.Contains(reverse.XPointer, "h1[2]") { + t.Errorf("expected landing in h1[2], got %s", reverse.XPointer) + } +} + +// A selection spanning blocks (heading tail into the next paragraph) must +// be findable by text search across block boundaries, and the matched +// quote's extent gives a true range end. +func TestCrossBlockSearchSpansBlocks(t *testing.T) { + c := NewCFIConverter(writeDropCapEPUB(t)) + + // No element path → structural rung skipped, text search runs. + xp := "/body/DocFragment[2]/body" + ctx := "New Covenant The Cleansing Life of Christ" + + result, err := c.ConvertCREToStandard(xp, 0.52, ctx) + if err != nil { + t.Fatalf("ConvertCREToStandard error: %v", err) + } + t.Logf("cross-block search → %s … %s (%s)", result.EPUBCFI, result.EndEPUBCFI, result.Precision) + if result.Precision != "exact" { + t.Fatalf("expected exact text-search precision, got %s", result.Precision) + } + if result.EPUBCFI == "" || result.EndEPUBCFI == "" { + t.Fatalf("expected range anchors, got %q…%q", result.EPUBCFI, result.EndEPUBCFI) + } + if result.EPUBCFI == result.EndEPUBCFI { + t.Fatalf("range collapsed: %s", result.EPUBCFI) + } + + // The start lands in the heading, the end in the following paragraph. + startRev, err1 := c.ConvertStandardToCRE(result.EPUBCFI, result.Percentage, "") + endRev, err2 := c.ConvertStandardToCRE(result.EndEPUBCFI, result.Percentage, "") + if err1 != nil || err2 != nil { + t.Fatalf("reverse conversions failed: %v %v", err1, err2) + } + if !strings.Contains(startRev.XPointer, "h1[2]") { + t.Errorf("expected start in h1[2], got %s", startRev.XPointer) + } + if !strings.Contains(endRev.XPointer, "p[1]") { + t.Errorf("expected end in p[1] (following paragraph), got %s", endRev.XPointer) + } +} + +// The percentage rung is a char-count estimate: it must never label its +// landing "exact". Feed it an unmatchable context so the ladder falls all +// the way through. +func TestPercentageFallbackIsHonestlyLabeled(t *testing.T) { + c := NewCFIConverter(writeDropCapEPUB(t)) + + xp := "/body/DocFragment[2]/body/p[3]/span[1]/text().0" + result, err := c.ConvertCREToStandard(xp, 0.52, "zzz qqq vvv uuu www") + if err != nil { + t.Fatalf("ConvertCREToStandard error: %v", err) + } + t.Logf("unmatchable context → %s (%s)", result.EPUBCFI, result.Precision) + if result.Precision != "percentage" { + t.Errorf("percentage rung must not claim exact, got %s", result.Precision) + } +} + // The bookmark route supplies no context (bookmark text is a display // label, never book text), so the facade must still resolve the drop-cap // xpointer structurally instead of collapsing to the document start. diff --git a/internal/sync/locators.go b/internal/sync/locators.go index c371226..9f81e2d 100644 --- a/internal/sync/locators.go +++ b/internal/sync/locators.go @@ -14,7 +14,11 @@ const ( ) type CanonicalLocator struct { - CFI string + CFI string + // EndCFI carries the matched context's range end (text-search + // conversions of selections) so callers can anchor a true highlight + // range across block boundaries. + EndCFI string Precision string Percentage float64 } @@ -94,7 +98,7 @@ func ConvertToCanonical( return CanonicalLocator{CFI: devicePos, Precision: "fallback", Percentage: percentage} } if result.EPUBCFI != "" { - return CanonicalLocator{CFI: result.EPUBCFI, Precision: result.Precision, Percentage: result.Percentage} + return CanonicalLocator{CFI: result.EPUBCFI, EndCFI: result.EndEPUBCFI, Precision: result.Precision, Percentage: result.Percentage} } if result.Href != "" { return CanonicalLocator{CFI: result.Href, Precision: result.Precision, Percentage: result.Percentage}