feature/character-breakline-strategy#561
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds character-based line-wrapping and optional preservation of explicit newlines; introduces a CharacterStrategy constant and PreserveLineBreaks prop; extends provider API with GetStringWidth; refactors text wrapping to use a normalized, strategy-dispatched pipeline; adds a RichText component with layout/rendering, examples, fixtures, and tests; updates docs and benchmarks. ChangesText wrapping & provider implementation
RichText component and integration
Examples, docs, and serialization fixtures
Build / dependency updates
Sequence Diagram(s)sequenceDiagram
participant RichText as RichText Component
participant Core as Core (layout/row)
participant Provider as Provider (gofpdf)
participant FPDF as FPDF Engine
RichText->>Core: Request layout/render (cell, width)
Core->>Provider: Call GetStringWidth(text, font) for measurements
Provider->>FPDF: SetFont(...) / GetStringWidth(text)
FPDF-->>Provider: width
Provider-->>Core: width
Core-->>RichText: measured widths (lines)
RichText->>Provider: For each segment: AddText(x,y, text, props)
Provider->>FPDF: Text(x,y, text)
FPDF-->>Provider: rendered
Provider-->>RichText: ack
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/providers/gofpdf/text.go (1)
96-102:⚠️ Potential issue | 🟡 MinorKeep
GetLinesQuantityconsistent withAddfor fit-case inputs.At Line 101, counting only via
getLines(...)can return0for empty/whitespace-only strings, whileAddrenders a single line when the text width fits (Line 75). This can still desync auto-row height vs rendered output.💡 Proposed fix
func (s *Text) GetLinesQuantity(text string, textProp *props.Text, colWidth float64) int { s.font.SetFont(textProp.Family, textProp.Style, textProp.Size) textTranslated := s.textToUnicode(text, textProp) - return len(s.getLines(textTranslated, textProp.BreakLineStrategy, colWidth)) + if s.pdf.GetStringWidth(textTranslated) <= colWidth { + return 1 + } + + return len(s.getLines(textTranslated, textProp.BreakLineStrategy, colWidth)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/providers/gofpdf/text.go` around lines 96 - 102, GetLinesQuantity currently returns len(s.getLines(...)) which can be 0 for empty or whitespace-only inputs whereas Add will render a single line when the text fits; update GetLinesQuantity (after calling s.textToUnicode and s.getLines) to handle the fit-case: if the computed lines slice is empty but the (trimmed or original) text width fits within colWidth (use the same width-measure logic as Add after s.font.SetFont), return 1; otherwise return the length of the lines slice. Ensure you reuse s.textToUnicode, s.font.SetFont and s.getLines logic so behavior stays consistent with Add.
🧹 Nitpick comments (1)
internal/providers/gofpdf/text_test.go (1)
27-88: Add a regression subtest for empty/whitespace-only fit-case line counting.A small case asserting
GetLinesQuantitymatchesAddbehavior for fit-path empty/space-only text would lock in the render/height consistency goal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/providers/gofpdf/text_test.go` around lines 27 - 88, Add a regression subtest in TestGetLinesHeight that verifies GetLinesQuantity handles empty or whitespace-only strings the same way the Add rendering path does: create a props.Text (valid font), set up mocks for font.SetFont and pdf.UnicodeTranslatorFromDescriptor and any expected pdf.GetStringWidth calls for spaces, then call text.GetLinesQuantity with inputs like "" and " " and assert the returned line count equals what the Add/renderer would produce for those inputs (use the same NewText constructor: gofpdf.NewText(pdf, mocks.NewMath(t), font) and call GetLinesQuantity to compare). Ensure the test name explains it's for empty/whitespace-only fit-case regression and mark it t.Parallel().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/providers/gofpdf/text.go`:
- Around line 96-102: GetLinesQuantity currently returns len(s.getLines(...))
which can be 0 for empty or whitespace-only inputs whereas Add will render a
single line when the text fits; update GetLinesQuantity (after calling
s.textToUnicode and s.getLines) to handle the fit-case: if the computed lines
slice is empty but the (trimmed or original) text width fits within colWidth
(use the same width-measure logic as Add after s.font.SetFont), return 1;
otherwise return the length of the lines slice. Ensure you reuse
s.textToUnicode, s.font.SetFont and s.getLines logic so behavior stays
consistent with Add.
---
Nitpick comments:
In `@internal/providers/gofpdf/text_test.go`:
- Around line 27-88: Add a regression subtest in TestGetLinesHeight that
verifies GetLinesQuantity handles empty or whitespace-only strings the same way
the Add rendering path does: create a props.Text (valid font), set up mocks for
font.SetFont and pdf.UnicodeTranslatorFromDescriptor and any expected
pdf.GetStringWidth calls for spaces, then call text.GetLinesQuantity with inputs
like "" and " " and assert the returned line count equals what the
Add/renderer would produce for those inputs (use the same NewText constructor:
gofpdf.NewText(pdf, mocks.NewMath(t), font) and call GetLinesQuantity to
compare). Ensure the test name explains it's for empty/whitespace-only fit-case
regression and mark it t.Parallel().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e0aae567-68e2-4437-9af1-7933b34e8bdd
⛔ Files ignored due to path filters (1)
docs/assets/pdf/textgridv2.pdfis excluded by!**/*.pdf
📒 Files selected for processing (9)
README.mddocs/assets/examples/textgrid/v2/main.godocs/assets/text/textgridv2.txtdocs/v2/features/text.mdinternal/providers/gofpdf/text.gointernal/providers/gofpdf/text_test.gopkg/components/text/example_test.gopkg/consts/breakline/breakline.gotest/maroto/examples/textgrid.json
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/providers/gofpdf/text.go`:
- Around line 196-225: The loop in getLinesBreakingLineByCharacter repeatedly
calls s.pdf.GetStringWidth for each character; to improve performance for
long/repetitive input (e.g., CJK) memoize per-character widths: create a local
map[rune]float64 (or map[string]float64) named e.g. charWidthCache inside
getLinesBreakingLineByCharacter, look up the rune/character in the cache and
call s.pdf.GetStringWidth only on cache miss, store the result, then use the
cached width for sizing decisions (keep the rest of the logic using
currentlySize, content, and colWidth unchanged).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4aeb4f72-9b35-4b05-99a2-a0e8ce3fe727
⛔ Files ignored due to path filters (1)
docs/assets/pdf/textgridv2.pdfis excluded by!**/*.pdf
📒 Files selected for processing (7)
docs/assets/examples/textgrid/v2/main.godocs/assets/text/textgridv2.txtdocs/v2/features/text.mdinternal/providers/gofpdf/text.gointernal/providers/gofpdf/text_test.gopkg/props/text.gotest/maroto/examples/textgrid.json
| func (s *Text) getLinesBreakingLineByCharacter(words string, colWidth float64) []string { | ||
| currentlySize := 0.0 | ||
| lines := []string{} | ||
| var content string | ||
|
|
||
| for _, letter := range words { | ||
| letterString := fmt.Sprintf("%c", letter) | ||
| width := s.pdf.GetStringWidth(letterString) | ||
|
|
||
| if currentlySize+width > colWidth && content != "" { | ||
| lines = append(lines, content) | ||
| content = "" | ||
| currentlySize = 0 | ||
| } | ||
|
|
||
| // Skip spaces if they would be at the start of a new line. | ||
| if letterString == " " && content == "" { | ||
| continue | ||
| } | ||
|
|
||
| content += letterString | ||
| currentlySize += width | ||
| } | ||
|
|
||
| if content != "" { | ||
| lines = append(lines, content) | ||
| } | ||
|
|
||
| return lines | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider caching per-character widths for repeated characters.
The current implementation calls GetStringWidth for every character, which may be expensive for long texts with repeated characters (e.g., CJK content). The underlying gofpdf method performs font metric lookups.
For this PR, the implementation is functionally correct. If performance becomes a concern with large texts, consider memoizing character widths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/providers/gofpdf/text.go` around lines 196 - 225, The loop in
getLinesBreakingLineByCharacter repeatedly calls s.pdf.GetStringWidth for each
character; to improve performance for long/repetitive input (e.g., CJK) memoize
per-character widths: create a local map[rune]float64 (or map[string]float64)
named e.g. charWidthCache inside getLinesBreakingLineByCharacter, look up the
rune/character in the cache and call s.pdf.GetStringWidth only on cache miss,
store the result, then use the cached width for sizing decisions (keep the rest
of the logic using currentlySize, content, and colWidth unchanged).
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/v2/features/richtext.md`:
- Line 27: The doc currently links the external fixture at
'https://raw.githubusercontent.com/johnfercher/maroto/master/test/maroto/examples/richtextgrid.json',
which points to master and can drift; update this link to a pinned artifact by
replacing 'master' with a specific tag or commit SHA (or alternatively copy the
fixture into the repo and reference it as a local relative path) so the example
remains reproducible.
- Around line 23-24: Replace the awkward section header "## Time Execution" with
the clearer title "## Execution Time" in the markdown (look for the exact header
string "## Time Execution"); keep all following content and the included code
block reference unchanged so only the heading text is updated.
In `@internal/providers/gofpdf/provider.go`:
- Around line 64-66: The GetStringWidth method can panic when prop is nil;
update provider.GetStringWidth to first guard for a nil props.Font (prop) and
avoid calling g.font.SetFont when prop is nil—e.g., if prop == nil return 0.0
(or another agreed safe default) and optionally log the unexpected nil;
otherwise proceed to call g.font.SetFont(prop.Family, prop.Style, prop.Size) and
g.fpdf.GetStringWidth(text). Ensure you only reference provider.GetStringWidth
and props.Font in the change.
In `@pkg/components/richtext/richtext.go`:
- Around line 429-456: The cluster-handling code (case richTextClusterElement)
currently appends an entire cluster built from segments (using measureText,
clusterWidth, clusterHeight and appending richTextLinePart entries) without
handling the case where clusterWidth > colWidth; update this block to detect
when clusterWidth exceeds colWidth and perform character-level (or
grapheme-level) breaking: iterate the offending segment.text by runes/graphemes,
measure progressively (using measureText and provider.GetFontHeight via
fontPropFromText) to form smaller parts that fit within remaining space (calling
flush() when a part doesn't fit and starting a new current richTextLine), split
long segments across lines as needed, and update current.width/current.height
accordingly; alternatively, if you prefer not to implement breaking now, add an
explicit comment or exported constant documenting the limitation for oversized
clusters and ensure callers are aware (reference symbols:
richTextClusterElement, segment, measureText, fontPropFromText, clusterWidth,
colWidth, flush, current, richTextLinePart).
- Around line 234-236: Add a concise doc comment to RichText.layoutStyle
explaining that paragraph-level layout properties (Top, Bottom, Left, Right,
Align, VerticalPadding, PreserveLineBreaks) are taken from the first chunk only;
mention that subsequent chunks' Style fields do not affect overall paragraph
layout to avoid surprises. Place the comment immediately above the layoutStyle()
method and reference RichText.layoutStyle and r.chunks[0].Style in the comment
so maintainers can quickly find the behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 98f24d5d-5656-4919-807e-ab140c0ddad9
⛔ Files ignored due to path filters (1)
docs/assets/pdf/richtextgridv2.pdfis excluded by!**/*.pdf
📒 Files selected for processing (13)
docs/assets/examples/richtextgrid/v2/main.godocs/assets/examples/richtextgrid/v2/main_test.godocs/assets/text/richtextgridv2.txtdocs/v2/features/_sidebar.mddocs/v2/features/richtext.mdinternal/providers/gofpdf/provider.gointernal/providers/gofpdf/provider_test.gomocks/Provider.gopkg/components/richtext/example_test.gopkg/components/richtext/richtext.gopkg/components/richtext/richtext_test.gopkg/core/provider.gotest/maroto/examples/richtextgrid.json
| ## Time Execution | ||
| [filename](../../assets/text/richtextgridv2.txt ':include :type=code') |
There was a problem hiding this comment.
Use a clearer section title: Execution Time.
Time Execution reads awkwardly in user-facing docs.
✏️ Suggested doc tweak
-## Time Execution
+## Execution Time📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ## Time Execution | |
| [filename](../../assets/text/richtextgridv2.txt ':include :type=code') | |
| ## Execution Time | |
| [filename](../../assets/text/richtextgridv2.txt ':include :type=code') |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/v2/features/richtext.md` around lines 23 - 24, Replace the awkward
section header "## Time Execution" with the clearer title "## Execution Time" in
the markdown (look for the exact header string "## Time Execution"); keep all
following content and the included code block reference unchanged so only the
heading text is updated.
| [filename](../../assets/text/richtextgridv2.txt ':include :type=code') | ||
|
|
||
| ## Test File | ||
| [filename](https://raw.githubusercontent.com/johnfercher/maroto/master/test/maroto/examples/richtextgrid.json ':include :type=code') |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid linking fixtures from master in versioned docs.
Using master can drift as the branch evolves; pin to a tag/commit (or local artifact) to keep docs reproducible.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/v2/features/richtext.md` at line 27, The doc currently links the
external fixture at
'https://raw.githubusercontent.com/johnfercher/maroto/master/test/maroto/examples/richtextgrid.json',
which points to master and can drift; update this link to a pinned artifact by
replacing 'master' with a specific tag or commit SHA (or alternatively copy the
fixture into the repo and reference it as a local relative path) so the example
remains reproducible.
| func (g *provider) GetStringWidth(text string, prop *props.Font) float64 { | ||
| g.font.SetFont(prop.Family, prop.Style, prop.Size) | ||
| return g.fpdf.GetStringWidth(text) |
There was a problem hiding this comment.
Guard GetStringWidth against nil font props.
If prop is nil, Line 65 will panic. Add a defensive check to avoid runtime crashes on invalid input.
🛡️ Proposed fix
func (g *provider) GetStringWidth(text string, prop *props.Font) float64 {
+ if prop == nil {
+ return 0
+ }
g.font.SetFont(prop.Family, prop.Style, prop.Size)
return g.fpdf.GetStringWidth(text)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (g *provider) GetStringWidth(text string, prop *props.Font) float64 { | |
| g.font.SetFont(prop.Family, prop.Style, prop.Size) | |
| return g.fpdf.GetStringWidth(text) | |
| func (g *provider) GetStringWidth(text string, prop *props.Font) float64 { | |
| if prop == nil { | |
| return 0 | |
| } | |
| g.font.SetFont(prop.Family, prop.Style, prop.Size) | |
| return g.fpdf.GetStringWidth(text) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/providers/gofpdf/provider.go` around lines 64 - 66, The
GetStringWidth method can panic when prop is nil; update provider.GetStringWidth
to first guard for a nil props.Font (prop) and avoid calling g.font.SetFont when
prop is nil—e.g., if prop == nil return 0.0 (or another agreed safe default) and
optionally log the unexpected nil; otherwise proceed to call
g.font.SetFont(prop.Family, prop.Style, prop.Size) and
g.fpdf.GetStringWidth(text). Ensure you only reference provider.GetStringWidth
and props.Font in the change.
| func (r *RichText) layoutStyle() *props.Text { | ||
| return &r.chunks[0].Style | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider documenting that only the first chunk controls layout properties.
layoutStyle() returns the first chunk's style for paragraph-level properties (Top, Bottom, Left, Right, Align, VerticalPadding, PreserveLineBreaks). This is intentional for flowing paragraphs but could surprise users who set spacing on subsequent chunks. A brief doc comment would clarify this behavior.
📝 Suggested documentation
+// layoutStyle returns the first chunk's style, which controls paragraph-level
+// properties such as margins, alignment, and vertical padding. Properties set
+// on subsequent chunks are ignored for layout purposes.
func (r *RichText) layoutStyle() *props.Text {
return &r.chunks[0].Style
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (r *RichText) layoutStyle() *props.Text { | |
| return &r.chunks[0].Style | |
| } | |
| // layoutStyle returns the first chunk's style, which controls paragraph-level | |
| // properties such as margins, alignment, and vertical padding. Properties set | |
| // on subsequent chunks are ignored for layout purposes. | |
| func (r *RichText) layoutStyle() *props.Text { | |
| return &r.chunks[0].Style | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/components/richtext/richtext.go` around lines 234 - 236, Add a concise
doc comment to RichText.layoutStyle explaining that paragraph-level layout
properties (Top, Bottom, Left, Right, Align, VerticalPadding,
PreserveLineBreaks) are taken from the first chunk only; mention that subsequent
chunks' Style fields do not affect overall paragraph layout to avoid surprises.
Place the comment immediately above the layoutStyle() method and reference
RichText.layoutStyle and r.chunks[0].Style in the comment so maintainers can
quickly find the behavior.
| case richTextClusterElement: | ||
| clusterParts := make([]richTextLinePart, 0, len(element.segments)) | ||
| clusterWidth := 0.0 | ||
| clusterHeight := 0.0 | ||
|
|
||
| for _, segment := range element.segments { | ||
| width := measureText(provider, segment.text, segment.style) | ||
| height := provider.GetFontHeight(fontPropFromText(segment.style)) | ||
|
|
||
| clusterParts = append(clusterParts, richTextLinePart{ | ||
| kind: richTextTextPart, | ||
| text: segment.text, | ||
| style: segment.style, | ||
| width: width, | ||
| }) | ||
| clusterWidth += width | ||
| clusterHeight = max(clusterHeight, height) | ||
| } | ||
|
|
||
| if len(current.parts) > 0 && colWidth > 0 && current.width+clusterWidth > colWidth { | ||
| flush(false) | ||
| current = richTextLine{} | ||
| } | ||
|
|
||
| current.parts = append(current.parts, clusterParts...) | ||
| current.width += clusterWidth | ||
| current.height = max(current.height, clusterHeight) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
No word-breaking for oversized clusters.
If a single cluster (word) is wider than colWidth, it will overflow the column rather than being broken. This matches typical text rendering behavior but could cause layout issues with very long words in narrow columns. Consider documenting this limitation or adding character-level breaking as a future enhancement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/components/richtext/richtext.go` around lines 429 - 456, The
cluster-handling code (case richTextClusterElement) currently appends an entire
cluster built from segments (using measureText, clusterWidth, clusterHeight and
appending richTextLinePart entries) without handling the case where clusterWidth
> colWidth; update this block to detect when clusterWidth exceeds colWidth and
perform character-level (or grapheme-level) breaking: iterate the offending
segment.text by runes/graphemes, measure progressively (using measureText and
provider.GetFontHeight via fontPropFromText) to form smaller parts that fit
within remaining space (calling flush() when a part doesn't fit and starting a
new current richTextLine), split long segments across lines as needed, and
update current.width/current.height accordingly; alternatively, if you prefer
not to implement breaking now, add an explicit comment or exported constant
documenting the limitation for oversized clusters and ensure callers are aware
(reference symbols: richTextClusterElement, segment, measureText,
fontPropFromText, clusterWidth, colWidth, flush, current, richTextLinePart).
Description
This PR adds
breakline.CharacterStrategy, a new break line strategy that wraps text at character boundaries without appending a dash.This is useful for languages and content where breaking by spaces is not appropriate, but adding hyphens is also undesirable.
The implementation also makes line splitting consistent between rendering and height calculation by reusing the same strategy path in
AddandGetLinesQuantity, including Unicode-translated text. This avoids mismatches between rendered output and auto-row sizing.In addition, this PR updates the text documentation, README, Go examples, and the
textgridexample assets to show howCharacterStrategybehaves compared withDashStrategy.Related Issue
N/A
Checklist
func (<first letter of struct> *struct) method() {}name style.when,shouldnaming pattern.m := mocks.NewConstructor(t).m.EXPECT().MethodName()method to mock methods.example_test.go.make dodwith none issues pointed out bygolangci-lint