Repository navigation
Conversation
rofd's page text geometry built every character box from the font size - one size above the baseline, stopping at it - while a PDF text box is built from the font's own metrics (poppler's `TextWord::setInitialBounds`: `yMin = y - ascent`, `yMax = y - descent`, the ascent/descent of the face). The mismatch cut the bottom off CJK glyphs and Latin descenders, and it left the vertical extent of a box dependent on the glyph inside it, so punctuation and Latin did not line up with CJK in one line. Measured on a real government document (2464 glyphs over six embedded fonts, sizes 4.9-7.8 mm): 86% of the scalars reach below the baseline - CJK overshoot 0.098-0.129 em, Latin descenders up to 0.188 em - by a mean of 0.50 mm and up to 1.46 mm, while the box top sat 1.45 mm above the painted top on average. The provider seam that already resolves per-scalar advances now answers the whole scalar geometry: `TextScalarMetrics` carries the advance of the glyph the positioner selected plus the *face's* typographic ascent and descent, which are the same for every glyph of that face at one size, so the boxes of a run share one band and cover the glyphs. `ResolvedFont` reads that pair once from FreeType while validating the face, `GlyphMetrics` exposes it beside the advance, and a face that declares no usable metrics keeps the font-size footprint - as does a document loaded without a provider. The provider is asked for every text object, even one whose runs all declare their own deltas: the vertical extents come from the resolved font whatever a scalar declares. 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 (the export set is unchanged). The 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 now 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 中) - and `crates/rofd-core/tests/semantic_text.rs` pins the aligned band.
A PDF reader grows a selected line's band by an eighth of its height above and below (poppler's `TextSelectionSizer::visitLine`: `margin = (yMax - yMin) / 8`). That is what makes a selected line read as one band that clears the glyphs instead of ending on them. `TextSelection::regions` now grows each line union the same way, so a selection region has the shape a PDF selection has in the same viewer. Character boxes and match rectangles keep the plain band, the way poppler keeps them. 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 (core: the three selection tests' regions and `crates/rofd-core/tests/semantic_text.rs::selection_regions_carry_the_pdf_highlight_margin_on_an_aligned_band`; FFI: `crates/rofd-ffi/tests/semantic.rs::text_layout_rectangles_cover_the_rendered_ink_below_the_baseline` asserts the region equals the band plus the margin), and `crates/rofd-ffi/tests/run_c_tests.sh` (the export set is unchanged).
|
[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 changes semantic text layout from font-size/advance-based boxes to renderer-aligned bands using each face’s typographic ascent and descent, preserves conservative fallbacks and the unchanged C ABI, and expands selection regions with Poppler-style margins while adding cross-layer tests for rendered-ink coverage. Sequence diagram for renderer-aligned page text geometrysequenceDiagram
participant Caller
participant Document
participant Page
participant TextMetricsProvider
participant FontResolver
participant TextBuilder
Caller->>Document: from_bytes_with_metrics(bytes, options, metrics)
Caller->>Page: text()
Page->>TextBuilder: build_page_text(page, metrics)
TextBuilder->>TextMetricsProvider: metrics_mm(page, text, size_mm)
TextMetricsProvider->>FontResolver: position_glyph_runs(resource, text, limits)
FontResolver-->>TextMetricsProvider: GlyphMetrics
TextMetricsProvider-->>TextBuilder: TextScalarMetrics
TextBuilder->>TextBuilder: build boxes from ascent and descent
TextBuilder-->>Page: PageText
Page-->>Caller: cached text layout
Flow diagram for Poppler-style selection regionsflowchart LR
A[Selected character boxes] --> B[Union boxes per logical line]
B --> C[selection_region]
C --> D[Grow by height / 8 above and below]
D --> E[Selection regions]
F[Character boxes and match rectangles] --> G[Remain unchanged]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
Author
|
I have read the CLA Document and I hereby sign the CLA. |
![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 5 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="1381" />
<code_context>
- let (x, _) = glyph.advance();
- *slot = Some(slot.unwrap_or(0.0) + x);
+ let metrics = glyph.metrics();
+ let slot = resolved.get_mut(glyph.scalar_index())?;
+ let entry = slot.get_or_insert(TextScalarMetrics {
+ advance_mm: 0.0,
</code_context>
<issue_to_address>
**Mapped glyph boxes omit source characters**
When a `CGTransform` covers multiple source scalars and emits glyphs for more than its starting scalar, `FontTextMetrics::metrics_mm` assigns the glyph’s ascent and descent only to `glyph.scalar_index()`, so later source characters get the font-size fallback and their search or selection boxes can clip the mapped glyph.
Apply the mapped glyph’s extents to every source scalar it represents.
Also at `crates/rofd-render/src/fonts.rs:1388-1389`.
</issue_to_address>
### Comment 2
<location path="crates/rofd-render/src/fonts.rs" line_range="449" />
<code_context>
+impl GlyphMetrics {
+ /// Returns the advance this glyph contributes on each axis, in millimetres.
+ pub fn advance(&self) -> (f64, f64) {
+ self.advance
+ }
+ /// Returns the selected face's typographic ascent above the baseline, in
</code_context>
<issue_to_address>
**Public advance API stops compiling**
When a downstream crate calls `PositionedGlyph::advance()` after upgrading, removing the public `PositionedGlyph::advance()` method and providing only `metrics()` breaks downstream compilation.
Preserve `advance()` or provide a compatible replacement for existing callers.
</issue_to_address>
### Comment 3
<location path="crates/rofd-ffi/src/renderer.rs" line_range="144" />
<code_context>
+/// metrics, because the system font database scan is the expensive part. A renderer
+/// created with other fallback families still chooses its own faces for drawing.
+pub(crate) fn text_metrics_provider() -> rofd_render::FontTextMetrics {
+ static RESOLVER: LazyLock<Arc<SystemFontResolver>> = LazyLock::new(|| {
+ Arc::new(SystemFontResolver::with_system_fonts(
default_fallback_families(),
</code_context>
<issue_to_address>
**Font resolver stays poisoned after panic**
When the initial system-font scan or resolver construction panics and the process continues, `LazyLock` marks the process-wide resolver as poisoned, so a later `text_metrics_provider()` call panics again and FFI document opens continue failing even after the original failure has passed.
Use a retryable `OnceLock` initialization path, as before, or otherwise recover from a poisoned initializer.
</issue_to_address>
### Comment 4
<location path="crates/rofd-core/src/semantic.rs" line_range="604-606" />
<code_context>
+ // and are never replaced, but a scalar takes its vertical extents from the
+ // resolved font whatever it declares, so the provider is asked for every
+ // object.
+ let resolved = self
+ .metrics
+ .and_then(|metrics| metrics.metrics_mm(self.page, object, size));
let mut scalar = 0usize;
for run in object.runs() {
</code_context>
<issue_to_address>
**Negative-delta boxes miss glyphs**
When a run has an explicit negative `DeltaX` and its font resolves, `TextBuilder` now receives font advances even for runs with explicit `DeltaX`; for a negative delta it anchors the box at `min(x, next_x)` but gives it only the positive font advance as width, shifting the box left of the glyph painted at `x`. Search and selection geometry can therefore miss the rendered glyph.
For negative deltas, compute the horizontal bounds to include both the glyph's advance span from the current pen and the next-pen position.
</issue_to_address>
### Comment 5
<location path="crates/rofd-render/src/fonts.rs" line_range="1380-1389" />
<code_context>
- let slot = advances.get_mut(glyph.scalar_index())?;
- let (x, _) = glyph.advance();
- *slot = Some(slot.unwrap_or(0.0) + x);
+ let metrics = glyph.metrics();
+ let slot = resolved.get_mut(glyph.scalar_index())?;
+ let entry = slot.get_or_insert(TextScalarMetrics {
+ advance_mm: 0.0,
+ ascent_mm: 0.0,
+ descent_mm: 0.0,
+ });
+ entry.advance_mm += metrics.advance().0;
+ entry.ascent_mm = entry.ascent_mm.max(metrics.ascent());
+ entry.descent_mm = entry.descent_mm.max(metrics.descent());
}
}
</code_context>
<issue_to_address>
**Whitespace boxes extend below baseline**
When a whitespace scalar resolves to a glyph that paints no ink, `FontTextMetrics::metrics_mm` includes the resolved glyph’s face ascent and descent, and `TextBuilder::text_object` uses those extents instead of the documented font-size footprint. Whitespace boxes and selection regions therefore extend below the baseline.
Exclude nonpainting glyphs from the aggregated ascent and descent so whitespace uses the font-size-above-baseline, zero-descent fallback.
Also at `crates/rofd-core/src/semantic.rs:620-627`.
</issue_to_address>| let (x, _) = glyph.advance(); | ||
| *slot = Some(slot.unwrap_or(0.0) + x); | ||
| let metrics = glyph.metrics(); | ||
| let slot = resolved.get_mut(glyph.scalar_index())?; |
There was a problem hiding this comment.
🟡 Medium · Mapped glyph boxes omit source characters
When a CGTransform covers multiple source scalars and emits glyphs for more than its starting scalar, FontTextMetrics::metrics_mm assigns the glyph’s ascent and descent only to glyph.scalar_index(), so later source characters get the font-size fallback and their search or selection boxes can clip the mapped glyph.
Apply the mapped glyph’s extents to every source scalar it represents.
Also at crates/rofd-render/src/fonts.rs:1388-1389.
Prompt for AI agents
In `crates/rofd-render/src/fonts.rs` at line 1381:
**Mapped glyph boxes omit source characters**
When a `CGTransform` covers multiple source scalars and emits glyphs for more than its starting scalar, `FontTextMetrics::metrics_mm` assigns the glyph’s ascent and descent only to `glyph.scalar_index()`, so later source characters get the font-size fallback and their search or selection boxes can clip the mapped glyph.
Apply the mapped glyph’s extents to every source scalar it represents.
Also at `crates/rofd-render/src/fonts.rs:1388-1389`.| /// per-scalar geometry of a run without explicit deltas can sum it instead of | ||
| /// resolving fonts again. | ||
| pub fn advance(&self) -> (f64, f64) { | ||
| self.advance |
There was a problem hiding this comment.
🟠 High · Public advance API stops compiling
When a downstream crate calls PositionedGlyph::advance() after upgrading, removing the public PositionedGlyph::advance() method and providing only metrics() breaks downstream compilation.
Preserve advance() or provide a compatible replacement for existing callers.
Prompt for AI agents
In `crates/rofd-render/src/fonts.rs` at line 449:
**Public advance API stops compiling**
When a downstream crate calls `PositionedGlyph::advance()` after upgrading, removing the public `PositionedGlyph::advance()` method and providing only `metrics()` breaks downstream compilation.
Preserve `advance()` or provide a compatible replacement for existing callers.| /// metrics, because the system font database scan is the expensive part. A renderer | ||
| /// created with other fallback families still chooses its own faces for drawing. | ||
| pub(crate) fn text_metrics_provider() -> rofd_render::FontTextMetrics { | ||
| static RESOLVER: LazyLock<Arc<SystemFontResolver>> = LazyLock::new(|| { |
There was a problem hiding this comment.
🟡 Medium · Font resolver stays poisoned after panic
When the initial system-font scan or resolver construction panics and the process continues, LazyLock marks the process-wide resolver as poisoned, so a later text_metrics_provider() call panics again and FFI document opens continue failing even after the original failure has passed.
Use a retryable OnceLock initialization path, as before, or otherwise recover from a poisoned initializer.
Prompt for AI agents
In `crates/rofd-ffi/src/renderer.rs` at line 144:
**Font resolver stays poisoned after panic**
When the initial system-font scan or resolver construction panics and the process continues, `LazyLock` marks the process-wide resolver as poisoned, so a later `text_metrics_provider()` call panics again and FFI document opens continue failing even after the original failure has passed.
Use a retryable `OnceLock` initialization path, as before, or otherwise recover from a poisoned initializer.
Comment on lines
+604
to
+606
| let resolved = self | ||
| .metrics | ||
| .and_then(|metrics| metrics.metrics_mm(self.page, object, size)); |
There was a problem hiding this comment.
🟡 Medium · Negative-delta boxes miss glyphs
When a run has an explicit negative DeltaX and its font resolves, TextBuilder now receives font advances even for runs with explicit DeltaX; for a negative delta it anchors the box at min(x, next_x) but gives it only the positive font advance as width, shifting the box left of the glyph painted at x. Search and selection geometry can therefore miss the rendered glyph.
For negative deltas, compute the horizontal bounds to include both the glyph's advance span from the current pen and the next-pen position.
Prompt for AI agents
In `crates/rofd-core/src/semantic.rs` at lines 604-606:
**Negative-delta boxes miss glyphs**
When a run has an explicit negative `DeltaX` and its font resolves, `TextBuilder` now receives font advances even for runs with explicit `DeltaX`; for a negative delta it anchors the box at `min(x, next_x)` but gives it only the positive font advance as width, shifting the box left of the glyph painted at `x`. Search and selection geometry can therefore miss the rendered glyph.
For negative deltas, compute the horizontal bounds to include both the glyph's advance span from the current pen and the next-pen position.
Comment on lines
+1380
to
+1389
| let metrics = glyph.metrics(); | ||
| let slot = resolved.get_mut(glyph.scalar_index())?; | ||
| let entry = slot.get_or_insert(TextScalarMetrics { | ||
| advance_mm: 0.0, | ||
| ascent_mm: 0.0, | ||
| descent_mm: 0.0, | ||
| }); | ||
| entry.advance_mm += metrics.advance().0; | ||
| entry.ascent_mm = entry.ascent_mm.max(metrics.ascent()); | ||
| entry.descent_mm = entry.descent_mm.max(metrics.descent()); |
There was a problem hiding this comment.
🟡 Medium · Whitespace boxes extend below baseline
When a whitespace scalar resolves to a glyph that paints no ink, FontTextMetrics::metrics_mm includes the resolved glyph’s face ascent and descent, and TextBuilder::text_object uses those extents instead of the documented font-size footprint. Whitespace boxes and selection regions therefore extend below the baseline.
Exclude nonpainting glyphs from the aggregated ascent and descent so whitespace uses the font-size-above-baseline, zero-descent fallback.
Also at crates/rofd-core/src/semantic.rs:620-627.
Prompt for AI agents
In `crates/rofd-render/src/fonts.rs` at lines 1380-1389:
**Whitespace boxes extend below baseline**
When a whitespace scalar resolves to a glyph that paints no ink, `FontTextMetrics::metrics_mm` includes the resolved glyph’s face ascent and descent, and `TextBuilder::text_object` uses those extents instead of the documented font-size footprint. Whitespace boxes and selection regions therefore extend below the baseline.
Exclude nonpainting glyphs from the aggregated ascent and descent so whitespace uses the font-size-above-baseline, zero-descent fallback.
Also at `crates/rofd-core/src/semantic.rs:620-627`.
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
2 participants
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.
Follow-up to #13 (now on
main): this is the vertical half. #13 made the horizontalgeometry follow the font the renderer paints; this PR makes a box span the face's
extent around the baseline and gives a selection region the margin a PDF reader's
selection has. The C ABI is untouched:
librofd_ffi.so.0still exports the same 78symbols.
Why. rofd built every character box from the font size — one size above the
baseline, stopping at it — while a PDF text box is built from the face's metrics
(poppler's
TextWord::setInitialBounds:yMin = y - ascent,yMax = y - descent).The mismatch cut CJK overshoot and Latin descenders off every highlight, selection and
search rectangle, and it made a box's vertical extent depend on the glyph inside it, so
Latin and punctuation in one line did not align with CJK. Measured on a real government
document (2464 glyphs, six embedded faces, 4.9–7.8 mm): 86% of scalars reach below the
baseline (CJK 0.098–0.129 em, Latin descenders up to 0.188 em) by 0.50 mm on average and
1.46 mm at most, while the box top sat 1.45 mm above the painted top on average.
Commit 1 — the band. The provider seam from #13 now answers the whole scalar
geometry:
TextScalarMetrics { advance_mm, ascent_mm, descent_mm }.ResolvedFontreads the face's typographic ascender/descender once while validating the face,
GlyphMetricsexposes them beside the advance, and the boxes of a run share one band.A face that declares no usable metrics — or a document loaded without a provider —
keeps the font-size footprint. Because the seam is no longer advance-only,
TextAdvanceProviderbecomesTextMetricsProviderandDocument::{open,from_bytes}_with_advancesbecome_with_metrics; the trait has notbeen in a release (upstream's newest tag is
v0.4.0), so no released API moves.Commit 2 — the selection margin.
TextSelection::regionsgrows each line union byan eighth of its height above and below, poppler's
TextSelectionSizer::visitLinemargin, so a selected line reads as one band that clears the glyphs. Character boxes
and match rectangles keep the plain band, as poppler keeps them.
One deliberate reversal of #13's review: the provider is now asked for every text
object, even when every run declares its own
DeltaX. The horizontal advance staysauthoritative, but the vertical extents come from the face whatever a scalar declares,
so the object needs the font. The cost is one provider pass per text object while a
page's text index is built (built once and cached per page); the resolver itself is
process-wide. I found no cheaper seam that keeps one aligned band across scripts —
happy to change the shape if you prefer a different trade.
rofd-ffikeeps supplying the default font stack (the one a renderer created with NULLoptions uses); the header documents that a renderer configured with other fallback
families may draw different glyphs for runs without deltas.
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 set unchanged, C/C++ headers, smoke);papers-ofd's headless suite (13/13 against this tree). Two pixel-level tests fail
against the previous model and pass here:
crates/rofd-render/tests/cairo_text.rs::semantic_character_boxes_cover_the_rendered_ink(the two scalars of one run now 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 中, and the region equals the band plus the margin).
Summary by Sourcery
Use resolved font metrics to align semantic text geometry with rendered glyphs and make selection highlights match PDF-style regions without changing the C ABI.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: