feat: implement text truncation and font property handling for templates#38
feat: implement text truncation and font property handling for templates#38
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds font-aware text measurement and truncation: new Changes
Sequence Diagram(s)sequenceDiagram
participant Route as routes/index.rs
participant Gen as generator/svg.rs
participant TextMeas as generator::text_measurement
participant FontDB as fontdb
Route->>Gen: generate_svg(..., &fontdb)
Gen->>Gen: apply_color_overrides()
rect rgb(240,250,240)
Note over Gen: Load template fonts & widths
Gen->>Gen: get_template_fonts(template_name)
Gen->>Gen: get_width_constraints(template_name)
end
rect rgb(245,240,250)
Note over Gen,TextMeas: Truncate fields using shared FontSystem
loop per field (title/description/subtitle)
Gen->>TextMeas: truncate_text_to_width(text, max_width, font_props, font_system)
TextMeas->>FontDB: query fonts/glyph metrics via cosmic-text
TextMeas->>TextMeas: binary search for fitting prefix
TextMeas-->>Gen: truncated_text
end
end
Gen->>Gen: update replacement map with truncated text
Gen->>Route: return rendered SVG
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)src/templates.rs (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
|
hahahahaha code coverage whats that |
# Conflicts: # src/fonts.rs
There was a problem hiding this comment.
Pull Request Overview
This PR adds text truncation functionality with automatic width measurement to prevent text overflow in SVG templates. The implementation extracts font properties from SVG templates, parses width constraints from YAML configuration, and uses the cosmic-text library to accurately measure text width and truncate with ellipsis when needed.
Key changes:
- Introduces
cosmic-textdependency for accurate text width measurement with font fallback support - Extracts font properties (family, size, weight) from SVG templates at load time
- Adds configurable width constraints via YAML with default fallback values
- Implements binary search-based text truncation algorithm
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/templates.rs | Adds parsing for font properties from SVG and width constraints from YAML; extends template loading with new data structures |
| src/generator/text_measurement.rs | New module implementing text width measurement and truncation using cosmic-text library |
| src/generator/svg.rs | Integrates text truncation into SVG generation pipeline |
| src/generator/mod.rs | Exports new text_measurement module and FontProperties type |
| src/routes/index.rs | Passes fontdb to SVG generation function |
| Cargo.toml | Adds cosmic-text 0.15 dependency |
| Cargo.lock | Lock file updates for new dependencies |
Comments suppressed due to low confidence (1)
Cargo.toml:4
- The Rust edition '2024' does not exist. Valid Rust editions are 2015, 2018, and 2021. This should be changed to
edition = \"2021\"which is the latest stable Rust edition as of January 2025.
edition = "2024"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/generator/text_measurement.rs (2)
116-123: Remove or implement the placeholder test.The test module contains only a commented-out test. Either implement proper unit/integration tests or remove this placeholder to avoid clutter.
15-51: Review concern is technically valid but may be overstated without profiling data.The fontdb database is indeed cloned inside the binary search loop (lines 89-104 in
truncate_text_to_width). Each of up to O(log n) iterations callsmeasure_text_width, which clones fontdb at line 26. FontSystem::new_with_locale_and_db requires ownership of the fontdb::Database, making the clone necessary with the current API.For typical text lengths (50-500 characters), this results in 6-10 fontdb clones per truncation operation. The actual performance impact depends on the size of the font database, which cannot be assessed without profiling.
If cloning is problematic in practice, consider:
- Creating a single
FontSysteminstance before the binary search and reusing it (would require refactoring the API or adding interior mutability)- Profiling to confirm the clone is a bottleneck before optimizing
src/templates.rs (2)
24-45: Consider documenting the default width value rationale.Lines 35, 39, and 43 all use
900.0as the default width (described as 75% of 1200px canvas). Consider extracting this as a named constant to improve maintainability and make the relationship to canvas size explicit.+const DEFAULT_TEXT_WIDTH: f32 = 900.0; // 75% of 1200px canvas width + impl TextWidthConstraints { pub fn new() -> Self { // ... } pub fn get_title_width(&self) -> f32 { - self.title.unwrap_or(900.0) + self.title.unwrap_or(DEFAULT_TEXT_WIDTH) } pub fn get_description_width(&self) -> f32 { - self.description.unwrap_or(900.0) + self.description.unwrap_or(DEFAULT_TEXT_WIDTH) } pub fn get_subtitle_width(&self) -> f32 { - self.subtitle.unwrap_or(900.0) + self.subtitle.unwrap_or(DEFAULT_TEXT_WIDTH) } }
190-196: Font weight parsing could be more robust.Line 194 uses
parse().unwrap_or(400)which could silently fall back to 400 for invalid numeric weights. Consider logging a warning when falling back to the default value to aid debugging template configuration issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
Cargo.toml(1 hunks)src/generator/mod.rs(1 hunks)src/generator/svg.rs(3 hunks)src/generator/text_measurement.rs(1 hunks)src/routes/index.rs(1 hunks)src/templates.rs(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/generator/mod.rs (2)
src/generator/png.rs (1)
render_to_png(3-26)src/generator/svg.rs (1)
generate_svg(94-159)
src/generator/svg.rs (2)
src/generator/text_measurement.rs (1)
truncate_text_to_width(56-114)src/generator/events/state.rs (1)
new(28-38)
src/templates.rs (1)
src/yaml_loader.rs (1)
load_yaml(7-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Agent
🔇 Additional comments (28)
Cargo.toml (2)
11-11: Dependency addition looks appropriate.The
cosmic-textlibrary is well-suited for text measurement and shaping. The version 0.15 aligns with the text truncation functionality being introduced.
4-4: The review comment is incorrect and should be disregarded.The Rust 2024 Edition is now stable, having been released with Rust 1.85. The edition value
"2024"in Cargo.toml is valid and standard-compliant. The original comment claiming edition "2024" is not standard was based on outdated information—editions have been delivered on a three-year cadence, with the 2015, 2018, and 2021 editions, and 2024 continues this pattern.Likely an incorrect or invalid review comment.
src/generator/text_measurement.rs (3)
6-11: Font property structure looks correct.The
FontPropertiesstruct appropriately captures the essential font attributes needed for text measurement. The weight field usingu16aligns with CSS font-weight conventions.
56-104: Binary search implementation is sound.The binary search correctly finds the maximum character count that fits within the available width. The early break at line 92 (
if mid == 0) prevents potential infinite loops whenleftandrightconverge to 0.
77-79: Confirm the fallback behavior when ellipsis doesn't fit.Returning an empty string when even the ellipsis doesn't fit (line 78) may result in invisible text fields in the generated image. Consider whether this is the desired UX or if a different fallback (e.g., returning the ellipsis anyway, or logging a warning) would be better.
src/generator/mod.rs (2)
5-5: Module declaration is correct.The new
text_measurementmodule is properly declared.
10-10: Public re-export enables clean API.Re-exporting
FontPropertiesthrough the generator module provides a cleaner public interface for consumers.src/routes/index.rs (1)
175-175: fontdb parameter correctly passed to generate_svg.The addition of
&state.fontdbaligns with the updatedgenerate_svgsignature and enables font-aware text truncation.src/generator/svg.rs (8)
5-5: Import addition is appropriate.Adding
Arcimport for the fontdb parameter type.
10-12: Imports enable truncation workflow.The new imports support the text measurement and template constraint features.
36-43: Safe fallback for missing width constraints.Using
unwrap_or_elsewith a defaultTextWidthConstraints::new()is a good defensive programming practice.
45-72: Truncation logic is well-structured.The
apply_truncationfunction cleanly applies width constraints to all three text fields (title, description, subtitle) using the cached font properties.
108-112: Truncation applied before text replacement.The truncation workflow is correctly positioned before XML processing, ensuring truncated text is used in the final SVG.
119-124: Text replacements use truncated values.The replacement map correctly uses the truncated text values computed earlier.
103-103: All callers have been properly updated with the newfontdbparameter.The function signature change is confirmed:
fontdb: &Arc<usvg::fontdb::Database>was added as the 9th parameter. The single call site insrc/routes/index.rs:166passes&state.fontdbas the corresponding argument, matching the new signature exactly. The breaking change has been properly implemented across the codebase.
28-34: No changes needed to the code—the original review concern does not reflect actual risk.The
expect()at line 33 is safe. Bothtemplatesandfont_propertiesare populated atomically in the same conditional block during template loading: a template is inserted totemplatesHashMap only if its SVG file loads successfully, andfont_propertiesare parsed and inserted in the same flow. Additionally,get_template_fonts()is called only afterget_template()succeeds (line 106), which verifies the template exists in the HashMap first. Therefore, any template reachingget_template_fonts()is guaranteed to have a corresponding entry infont_properties.src/templates.rs (12)
1-1: Import added for FontProperties.Correctly imports
FontPropertiesfrom the generator module.
3-4: XML parsing imports added.The
quick_xmlimports support SVG font extraction functionality.
8-14: Width constraint structure is well-designed.Using
Option<f32>for each field allows templates to selectively override constraints.
16-22: Font properties structure is appropriate.The
TemplateFontsstruct organizes font properties for all text elements in a template.
51-52: TemplateMap fields added correctly.The new fields integrate well with the existing structure and support the truncation feature.
99-150: Width constraint parsing handles both integer and float values.The parsing logic correctly handles both integer (lines 117-121) and floating-point (lines 119-121) values for width constraints, providing flexibility in template configuration.
152-218: Font extraction logic handles nested elements.The implementation correctly handles the case where font properties may be on a parent
<text>element rather than the<tspan>with the target ID. The stateful tracking ofcurrent_font(lines 159-163, 203-205) allows inheriting properties from parent elements.
220-231: Font parsing function is clean and well-structured.The function extracts font properties for all three text elements (title, description, subtitle) and constructs the
TemplateFontsstruct.
251-263: Font property logging is helpful for debugging.The detailed logging at lines 253-262 provides visibility into parsed font properties, which is valuable for template debugging and verification.
280-282: Width constraints always inserted, even if empty.Line 282 inserts width constraints for every template, even if no constraints were specified (resulting in all
Nonevalues). This is consistent with the approach for font properties and ensures the template name exists in the map.
296-302: Template loading updated to populate all new fields.The
load_templatecalls correctly pass the newwidth_constraintsandfont_propertiesmaps.
321-322: TemplateMap construction includes new fields.The new fields are properly included in the
TemplateMapconstruction.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/generator/svg.rs (1)
67-67: Locale configurability is a reasonable refactor, but the fontdb clone is a necessary API requirement.FontSystem::new_with_locale_and_db requires owned Database, so
.clone()is not optional—it's mandated by the API. The performance optimization of creating FontSystem once (vs. 21–63 times previously) far exceeds the cost of a single clone operation.The hardcoded "en-US" locale is worth reconsidering if your application supports multiple locales. If English-only is intentional, this can remain as-is. If locale should be configurable, consider accepting it as a function parameter or deriving it from request context.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/generator/svg.rs(3 hunks)src/generator/text_measurement.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/generator/text_measurement.rs
🧰 Additional context used
🧬 Code graph analysis (1)
src/generator/svg.rs (2)
src/generator/text_measurement.rs (1)
truncate_text_to_width(48-105)src/generator/events/state.rs (1)
new(28-38)
🪛 GitHub Check: Format Check
src/generator/svg.rs
[warning] 64-64:
Diff in /home/runner/work/ogis/ogis/src/generator/svg.rs
[warning] 31-31:
Diff in /home/runner/work/ogis/ogis/src/generator/svg.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Code Coverage
- GitHub Check: Code Coverage
🔇 Additional comments (5)
src/generator/svg.rs (5)
1-13: LGTM: Imports are clean and necessary.All new imports are used appropriately throughout the file for the text truncation functionality.
29-44: LGTM: Excellent error handling with descriptive messages.The error message includes the template name and provides helpful context about potential causes.
113-132: LGTM: Text truncation is correctly integrated into the SVG generation flow.The truncation happens at the right point (after color overrides, before XML processing), and errors are properly propagated. The new
fontdbparameter enables the font-aware truncation functionality.
138-143: LGTM: Truncated text is correctly used in replacements.The text replacement map now uses the truncated versions, ensuring that text fits within the template constraints.
46-53: No issues found; default width constraints are appropriate.The fallback to
TextWidthConstraints::new()creates constraints withNonevalues for all fields. Each getter method (get_title_width(), etc.) unwraps to a sensible default of 900.0px (75% of the 1200px canvas width), as documented. Templates without explicit width configurations will use this reasonable default constraint, ensuring proper truncation behavior.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/generator/svg.rs (1)
178-210: Tests cover helper functions, butapply_truncationcould use coverage.The existing tests verify
get_template_fontserror handling andget_width_constraintsdefaults. Consider adding tests forapply_truncationto verify the truncation logic integrates correctly with font properties and constraints.src/generator/text_measurement.rs (2)
45-105: Binary search logic is correct; consider documenting empty string fallback.The binary search correctly uses
right = chars.len()with exclusive slicingchars[..mid]. The loop bounds and termination conditions are sound.Lines 67-70 return an empty string when even the ellipsis doesn't fit within
max_width. While this is a reasonable fallback, it might be surprising to callers. Consider adding a comment explaining this behavior, or evaluate whether returning an error would be more appropriate for your use case.
107-139: Test covers the happy path; consider edge cases.The test verifies basic truncation with ellipsis. For more robust coverage, consider adding tests for:
- Text that already fits (no truncation needed)
- Empty text input
- Cases where ellipsis doesn't fit within max_width
measure_text_widthwith empty strings and various fonts
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/generator/svg.rs(4 hunks)src/generator/text_measurement.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/generator/svg.rs (2)
src/generator/text_measurement.rs (1)
truncate_text_to_width(48-105)src/templates.rs (3)
templates(58-61)new(25-31)get_title_width(34-36)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Agent
- GitHub Check: Code Coverage
- GitHub Check: Code Coverage
🔇 Additional comments (8)
src/generator/svg.rs (6)
1-13: LGTM!The new imports support font-aware text truncation. All dependencies are utilized in the truncation workflow.
29-41: LGTM!Clear error handling with descriptive message. The function correctly retrieves cached font properties.
43-50: LGTM!Safe fallback to default constraints using
unwrap_or_else.
111-129: LGTM!The new
fontdbparameter and upfront truncation flow are well-structured. Truncation results are computed once before XML processing, avoiding repeated calculations.Note: This is a breaking change to the function signature.
136-141: LGTM!Text replacement map correctly uses truncated values. The updated comment clearly documents the change.
52-89: The Database clone concern is valid, but verification of alternatives is needed.Line 65 does clone the
fontdbDatabase (viafontdb.as_ref().clone()), which occurs once per request. While this is a significant improvement over the prior behavior (21-63 FontSystem creations per request), the clone itself could still be expensive with large font databases.cosmic-text 0.15 improved its architecture to cache
FontKey(lightweight) rather thanFontobjects, and removed self-referencing constraints. This suggests the library may support passingArc<Database>or references directly toFontSystem::new_with_locale_and_db, but the current code clones instead.Action needed: Test whether cosmic-text 0.15's
FontSystem::new_with_locale_and_dbcan acceptArc<usvg::fontdb::Database>or&Databasedirectly, rather than requiring an owned clone. If so, this would eliminate the per-request clone.src/generator/text_measurement.rs (2)
1-11: LGTM!
FontPropertiesis a clear, simple data structure for font metadata. Public fields and derived traits support the intended usage.
13-43: LGTM!The function correctly uses
cosmic_textwithShaping::Advancedfor per-character font fallback. Early return for empty text and safe fallback to 0.0 for missing layout runs are appropriate.
Summary by CodeRabbit
New Features
Chores