Repository navigation
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: YangtseSu The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @YangtseSu. Thanks for your PR. I'm waiting for a linuxdeepin member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Reviewer's GuideThe PR fixes semantic text geometry drift by allowing core to consume optional per-scalar font advances, implementing that provider through the renderer’s exact glyph-positioning path, and wiring the default provider into the FFI so layout, search, selection, and hit-testing align with drawn text while retaining conservative font-size behavior when metrics are unavailable. Sequence diagram for font-resolved semantic text layoutsequenceDiagram
participant FFI as rofd-ffi
participant Document as rofd-core Document
participant Page as rofd-core Page
participant Provider as FontTextAdvances
participant Positioner as position_glyph_runs
participant Layout as PageText
participant Renderer as Renderer
FFI->>Document: open_with_advances(path, options, provider)
FFI->>Renderer: render text
Page->>Document: text()
Document->>Provider: advances_mm(page, text, size_mm)
Provider->>Positioner: position_glyph_runs(resolver, resource, text, limits)
Positioner-->>Provider: PositionedGlyph.advance()
Provider-->>Document: per-scalar advances
Document->>Layout: build_page_text(page, advances)
Layout-->>FFI: geometry for search, selection, and hit-testing
Renderer-->>FFI: drawn glyph positions
Flow diagram for fallback text advance selectionflowchart TD
Start[Text scalar in a run] --> Explicit{Explicit DeltaX?}
Explicit -->|Yes| Delta[Use explicit delta]
Explicit -->|No| Provider{Advance provider available?}
Provider -->|Yes| Resolved{Valid per-scalar advance?}
Resolved -->|Yes| Font[Use font advance]
Resolved -->|No| Size[Use font-size footprint]
Provider -->|No| Size
Delta --> Geometry[Update semantic text geometry]
Font --> Geometry
Size --> Geometry
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="crates/rofd-render/src/fonts.rs" line_range="1269-1270" />
<code_context>
+impl TextAdvanceProvider for FontTextAdvances<'_> {
+ fn advances_mm(&self, page: &Page, text: &TextObject, size_mm: f64) -> Option<Vec<f64>> {
+ let resource = page.font_resource(text.font_id()).ok()?;
+ let runs =
+ position_glyph_runs(self.resolver, &resource, text, page.resource_limits()).ok()?;
+ let total = text.runs().iter().try_fold(0usize, |total, run| {
+ total.checked_add(run.text().chars().count())
</code_context>
<issue_to_address>
**Font errors become silent fallback**
When font lookup or glyph positioning fails for a reason other than an unavailable font, `position_glyph_runs` errors are converted to `None`, which makes the core builder use font-size steps without exposing why resolution failed. Invalid embedded fonts, resource-limit failures, or cache errors thus produce silently inaccurate geometry instead of a diagnosable failure.
Preserve or report provider failures separately from the ordinary unresolved-font fallback so callers can distinguish degraded geometry from successful font resolution.
</issue_to_address>
### Comment 2
<location path="crates/rofd-core/src/semantic.rs" line_range="560" />
<code_context>
+ // authoritative and are never replaced.
+ let resolved = self
+ .advances
+ .and_then(|advances| advances.advances_mm(self.page, object, size));
+ let mut scalar = 0usize;
for run in object.runs() {
</code_context>
<issue_to_address>
**Explicit advances still load fonts**
When semantic text is requested for objects whose runs all declare explicit `DeltaX`, `text_object` calls the provider before checking whether any run needs inferred advances, so the provider resolves fonts and positions glyphs even though explicit `DeltaX` values are authoritative. Semantic text construction does unnecessary font-loading and per-glyph work.
Check whether any run needs inferred advances before calling the provider.
</issue_to_address>Carry the font-advance fixes from YangtseSu/rofd feat/font-text-advances (upstream linuxdeepin/rofd#13), on top of the page-size query of #11: - rofd-core resolves the advance of a scalar whose run declares no DeltaX through an injected provider, so page geometry stops drifting from the rendered glyphs (a Ctrl+F highlight used to land right of its match); - a resolved glyph box is sized by its advance, so boxes tile a run and a rectangle hugs the matched text; - rofd-render implements the provider over the positioner's own font resolution, and rofd-ffi loads documents with the default font stack. Both patches apply to the pinned upstream commit (140b936) and the package builds fully offline. Verified: the packaged library exports rofd_document_get_page_size_mm and gives a two-scalar "on" match a 3.65 mm rectangle where the size-based footprint produced 4.82 mm.
YangtseSu
changed the title
7f6a185 to
a364014
Compare
|
I have read the CLA Document and I hereby sign the CLA. |
An OFD text run that declares no `DeltaX` for a scalar is positioned by that scalar's font advance, which is what the renderer draws. The semantic text index, however, advanced such scalars by the font size, so page geometry drifted away from the glyphs: on a 22-character Latin line at size 3 mm the character boxes spread over 63 mm while the drawn text was 28 mm wide, and a search rectangle for a match late in the line fell to the right of the text it belonged to. Consumers that draw from this geometry - search highlights, selection, caret and hit-testing - therefore looked misplaced on any document whose producers rely on font metrics instead of explicit deltas. The core crate still does not touch fonts. `TextAdvanceProvider` lets a caller supply the per-scalar advances, `Document::from_bytes_with_advances` / `open_with_advances` carry a provider into the page text index, and without one the previous font-size footprint stays in place (and stays flagged conservative). rofd-render implements the provider over exactly the resolution and advance code the positioner uses, exposing the advance each positioned glyph contributed so the two layers cannot drift apart. rofd-ffi loads documents with the default font stack, which is what a renderer created with NULL options uses. The provider is asked only when a run actually declares no `DeltaX`: an object whose runs all carry their own deltas never loads a font for its geometry. Verified: cargo fmt --all -- --check; cargo clippy -p rofd-core -p rofd-render -p rofd-ffi --all-targets -- -D warnings; cargo test for the three crates; crates/rofd-ffi/tests/run_c_tests.sh (symbol list, C/C++ headers, smoke). New tests cover the provider seam in the core builder (including explicit deltas staying authoritative and a provider that cannot resolve a font), the provider/positioner agreement on an embedded subset font, and the ABI's layout and search rectangles following font metrics.
The conservative footprint (at least one font size per scalar) exists so a font-free build still produces usable hit boxes. With a provider that footprint is no longer needed: it made every box wider than its glyph, so the boxes of a run overlapped and a match rectangle overhung its text by a fraction of a glyph - visible as a search highlight that extends past the match into the next letter. Measured on a 3 mm Latin line: the "on" match box was 4.82 mm wide against a 3.65 mm advance span, and the single-glyph boxes overlapped by 1.25 mm. A scalar whose advance the provider resolved now gets exactly that advance as its box width, so the boxes tile the run and the rectangles hug the matched text. Scalars the provider cannot resolve, and builds without a provider, keep the font-size footprint (an explicit zero advance still gets a nonempty box). Verified: cargo fmt --all -- --check; cargo clippy -p rofd-core -p rofd-render -p rofd-ffi --all-targets -- -D warnings; cargo test for the three crates (the core and FFI tests now assert the advance width and that adjacent boxes meet); crates/rofd-ffi/tests/run_c_tests.sh.
a364014 to
f0572f2
Compare
YangtseSu
changed the title
YangtseSu
changed the title
|
Status as of 2026-10-19 — what this PR is now, and what is deliberately not in it. The branch is recut onto
The vertical half moved out, to a stacked branch. Spanning a box with the face's Review comments, answered in their threads.
Also: the four other changes this fork carried are in Verified on the branch tip: Still waiting on |
|
我把#13重构了下，目前的代码只修两个水平问题——缺 DeltaX 的标量按字体 advance 定位（高亮不再右移、落到文字右侧）与已解析字符框的宽度取该 advance（匹配框不再比文字宽、相邻框不再重叠）。我vibe的 papers-ofd，需要这个来修复选中的高亮向右偏移的问题。 而「下面没覆盖全、上下各露多少」的那部分（按字面 ascent/descent 的行带 + 选择区 1/8 margin）已拆到叠在它之上的 feat/font-text-band，等 #13 落地后另开 PR。之所以这样操作，是看到别的PR都合并了，这个没有合并。虽然没有收到回复，但是我自己在我电脑上用deepin-reader（https://aur.archlinux.org/packages/deepin-reader）测试了，deepin-reader对于pdf和ofd的行为基本一致，都没有在文字上下多高亮一点区域。而GNOME Papers对于pdf的处理，是在文字上下多高亮一点区域，所以我目前的feat/font-text-band和原先的#13 PR，都是这样的。 |
2 participants
Edited 2026-10-19. The title and this description were rewritten, and the branch
recut from three commits to two. The original description is preserved verbatim at the
end of this body, under "Original description".
Why: upstream
mainhas since taken four of the five requests this fork carried (#11,#12, #14, #15), so this branch was rebased onto it; and the third commit of the old
branch mixed a bug fix (a highlight cut the bottom off the text it covered) with a
model change (one band for a whole face, plus the selection region's margin), which is
the part a reviewer is least likely to want riding along. The vertical half now lives
on a stacked branch,
feat/font-text-band, to be opened as its own PR once this onelands. What remains here is the horizontal half alone: the layer that reports
rectangles now follows the font the layer that paints already uses.
Text geometry has to describe the glyphs the renderer paints. A scalar of a run that
declares no
DeltaXis positioned by that scalar's font advance — that is whatrofd-render's positioner does — butrofd-coreadvanced it by the font size andbounded its box by the font size as well. The two layers therefore disagreed on every
document whose producers rely on font metrics instead of explicit deltas:
character boxes spread over 63.0 mm while the drawn ink was 28.2 mm wide, and the
42match rectangle sat at 60.0–66.0 mm while the line ends at 43.4 mm — a searchhighlight in blank space, 4.5 mm right of the text it belongs to. CJK hides it: a
full-width glyph's advance is the font size.
right = left + max(|dx|, size)made every box atleast one font size wide, so a match rectangle extended past its text: on a 3 mm line
the
onmatch box was 4.82 mm wide against a 3.65 mm advance span, and thesingle-glyph boxes overlapped by 1.25 mm.
Both are fixed here, in two commits:
TextAdvanceProviderlets a caller supply the per-scalar advances,Document::from_bytes_with_advances/open_with_advancescarry one into the pagetext index, and without one the previous font-size footprint stays in place (still
flagged conservative). Explicit deltas stay authoritative, and the provider is asked
only when a run actually declares no
DeltaX.width, so the boxes of a run tile it and a match rectangle hugs its text.
rofd-renderimplements the provider over exactly the resolution and advance code thepositioner uses (
position_glyph_runs, exposing the advance each positioned glyphcontributed), so the layer that draws and the layer that reports rectangles cannot
drift apart.
rofd-ffiloads documents with the default font stack, i.e. the one arenderer created with NULL options uses; the header documents the caveat for renderers
configured with other fallback families.
The vertical extents are deliberately out of scope here and proposed separately, in a
branch stacked on this one (the fork's
feat/font-text-band): a box still stops at thebaseline, which cuts CJK overshoot and Latin descenders off a highlight, and the
follow-up spans the box with the face's ascent and descent. I will open it as its own
PR once this one lands, so this PR's diff stays the horizontal geometry alone.
Verified:
cargo fmt --all -- --check;cargo clippy -p rofd-core -p rofd-render -p rofd-ffi --all-targets -- -D warnings;cargo testfor the three crates;crates/rofd-ffi/tests/run_c_tests.sh(the export set is unchanged, C/C++ headers,smoke). New tests cover the provider seam in the core builder (including
explicit deltas staying authoritative and a provider that cannot resolve a font), the
provider/positioner agreement on an embedded subset font, and the ABI's layout and
search rectangles following font metrics.
Original description
An OFD text run that declares no
DeltaXfor a scalar is positioned by that scalar'sfont advance, which is what the renderer draws, and the glyph is painted at that font's
own extent around the baseline. The semantic text index advanced such scalars by the
font size and sized every character box by it too, so page geometry drifted away from
the glyphs: on a 22-character Latin line at 3 mm the character boxes spread over 63 mm
while the drawn text was 28 mm wide, a search rectangle for a late match fell to the
right of the text it belonged to, and a selection highlight cut the bottom off the text
it covered — on a real document, 86% of 2464 glyphs reach below their baseline (CJK
overshoot 0.10–0.13 em, Latin descenders up to 0.19 em, mean 0.50 mm and up to 1.46 mm).
Consumers that draw from this geometry — search highlights, selection, caret,
hit-testing — looked misplaced or clipped on any document whose producers rely on font
metrics.
The fix follows the model a PDF reader uses, so highlights also line up with the way
Papers' PDF backend behaves:
TextMetricsProviderlets a caller supply the per-scalar geometry asTextScalarMetrics { advance_mm, ascent_mm, descent_mm };Document::from_bytes_with_metrics/open_with_metricscarry one into the page textindex. Without a provider nothing changes, and a scalar whose glyph no face owns keeps
the font-size footprint.
match rectangle hugs its text) and spans it with the face's typographic ascent and
descent, which every glyph of that face shares — the same pair poppler's
TextWord::setInitialBoundsuses (yMin = y - ascent,yMax = y - descent). Boxes ofLatin, CJK and punctuation therefore keep one band instead of hugging their own ink.
TextSelection::regionsgrows that band by an eighth of its height above and below,the margin poppler's
TextSelectionSizer::visitLinegives a PDF selection region, so aselected line reads as one band that clears the glyphs. Character boxes and match
rectangles keep the plain band, as poppler keeps them.
rofd-renderimplements the provider over exactly the resolution and metrics code thepositioner uses: the face's ascender/descender are read once while the face is
validated, each glyph's advance is cached beside them, and
PositionedGlyph::metricsexposes the advance and the face extents so the two layers cannot drift apart.
rofd-ffiloads documents with the default font stack, i.e. the one a renderer createdwith NULL options uses, and the provider owns that shared resolver instead of leaking
it. A renderer configured with other fallback families may still draw different glyphs
for runs without deltas; the header documents this.
Verified:
cargo fmt --all -- --check;cargo clippy -p rofd-core -p rofd-render -p rofd-ffi --all-targets -- -D warnings;cargo testfor the three crates;crates/rofd-ffi/tests/run_c_tests.sh(symbol list unchanged, 77 entries, C/C++headers, smoke). New tests cover the provider seam in the core builder (explicit deltas
stay authoritative; a provider that cannot resolve a font leaves the footprint; the
box spans the resolved extents), the selection region's eighth-height margin on an
aligned band, the provider/positioner agreement on an embedded subset font (advance plus
the face's 1160/288 metrics for every glyph), and the ABI's layout and search
rectangles. Two pixel-level tests fail against the previous model and pass with this
one:
crates/rofd-render/tests/cairo_text.rs::semantic_character_boxes_cover_the_rendered_ink(the two scalars of one run share top and bottom, and the band is strictly taller than
the ink) and
crates/rofd-ffi/tests/semantic.rs::text_layout_rectangles_cover_the_rendered_ink_below_the_baseline(same band for A and 中, the region equals the band plus the margin).
Note:
crates/rofd-render/tests/images.rs::decoded_limits_have_exact_and_one_over_boundariesfails on
mainhere as well; it is unrelated to this change.