Skip to content

feature/character-breakline-strategy#561

Open
joeyave wants to merge 6 commits into
johnfercher:masterfrom
joeyave:master
Open

feature/character-breakline-strategy#561
joeyave wants to merge 6 commits into
johnfercher:masterfrom
joeyave:master

Conversation

@joeyave
Copy link
Copy Markdown

@joeyave joeyave commented Mar 23, 2026

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 Add and GetLinesQuantity, 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 textgrid example assets to show how CharacterStrategy behaves compared with DashStrategy.

Related Issue
N/A

Checklist

  • All methods associated with structs has func (<first letter of struct> *struct) method() {} name style.
  • Wrote unit tests for new/changed features.
  • Followed the unit test when,should naming pattern.
  • All mocks created with m := mocks.NewConstructor(t).
  • All mocks using m.EXPECT().MethodName() method to mock methods.
  • Updated docs/*
  • Updated example_test.go.
  • Updated README.md
  • New public methods/structs/interfaces has comments upside them explaining they responsibilities
  • Executed make dod with none issues pointed out by golangci-lint

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Mar 24, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 8798184b-6f5c-4eba-8f28-558bab070133

📥 Commits

Reviewing files that changed from the base of the PR and between 680cc12 and 0e07553.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (1)
  • go.mod

📝 Walkthrough

Summary by CodeRabbit

  • New Features

    • Character-based text wrapping for long words without hyphens.
    • PreserveLineBreaks option to treat explicit \n as hard breaks.
    • Rich Text component for styled inline chunks with wrapping, alignment, and justification.
  • Documentation

    • Updated text docs with guidance and examples for new wrap/line-break options and rich text.
    • Added example artifacts demonstrating character wrapping, preserved breaks, and rich-text layouts.
  • Tests & Examples

    • Added/updated example programs and tests covering new behaviors and rendering.

Walkthrough

Adds 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.

Changes

Text wrapping & provider implementation

Layer / File(s) Summary
Data Shape / Props
pkg/consts/breakline/breakline.go, pkg/props/text.go
Adds CharacterStrategy constant and PreserveLineBreaks bool to props.Text, and serializes prop_preserve_line_breaks when true.
Core Implementation
internal/providers/gofpdf/text.go
Refactors text flow to getTextLines() with normalization; adds normalizeLineBreaks, paragraph splitting, getLines dispatcher, and getLinesBreakingLineByCharacter implementing character-bound wrapping; updates Text.Add and GetLinesQuantity to use computed lines with widths.
Provider API / Wiring
pkg/core/provider.go, internal/providers/gofpdf/provider.go
Adds GetStringWidth(text string, prop *props.Font) float64 to Provider interface and implements it in the gofpdf provider by setting font then calling fpdf.GetStringWidth.
Mocks / Tests
mocks/Provider.go, internal/providers/gofpdf/text_test.go, internal/providers/gofpdf/provider_test.go
Extends mocks with GetStringWidth helpers; updates/extends text tests to cover CharacterStrategy and PreserveLineBreaks and to mock measured substring widths; adds provider unit test for GetStringWidth.

RichText component and integration

Layer / File(s) Summary
API / Types
pkg/components/richtext/richtext.go
Introduces Chunk type and exported RichText component with constructors (New, NewCol, NewRow, NewAutoRow) and SetConfig.
Layout / Rendering
pkg/components/richtext/richtext.go
Implements rich-text tokenization across chunks, line layout constrained by column width, alignment (left/center/right/justify), justify-space distribution, newline handling (with optional PreserveLineBreaks), and per-segment rendering via provider AddText.
Height / Structure
pkg/components/richtext/richtext.go
Implements GetHeight based on font metrics, per-line heights, paddings, and line count. GetStructure serializes chunked content and styles.
Tests / Examples
pkg/components/richtext/richtext_test.go, pkg/components/richtext/example_test.go
Adds tests exercising GetStructure, GetHeight, and Render behaviors and example functions demonstrating RichText usage.
Docs / Examples
docs/assets/examples/richtextgrid/v2/main.go, docs/assets/examples/richtextgrid/v2/main_test.go
Adds example generator and test that emits richtext PDF/text artifacts and asserts structure mapping.

Examples, docs, and serialization fixtures

Layer / File(s) Summary
Example code changes
docs/assets/examples/textgrid/v2/main.go, pkg/components/text/example_test.go
Adds example strings and rows demonstrating CharacterStrategy vs DashStrategy and PreserveLineBreaks; adds example test for character strategy in text column.
Documentation
docs/v2/features/text.md, docs/v2/features/richtext.md, docs/v2/features/_sidebar.md, README.md
Documents CharacterStrategy and PreserveLineBreaks in text docs; adds Rich Text feature page and sidebar entry; updates README sentence about multiple line-break strategies.
Text artifacts / Benchmarks
docs/assets/text/textgridv2.txt, docs/assets/text/richtextgridv2.txt
Updates recorded benchmark/text artifacts for textgridv2 and adds richtextgridv2 metrics.
JSON fixtures
test/maroto/examples/textgrid.json, test/maroto/examples/richtextgrid.json
Replaces/expands textgrid fixture rows to cover character-breaking and explicit-line examples; adds richtextgrid example fixture describing chunked rich text layout.

Build / dependency updates

Layer / File(s) Summary
Go modules
go.mod
Bumps go directive to 1.26.1, updates github.com/pdfcpu/pdfcpu and several indirect dependencies.

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
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 Hopping through lines with care,
Characters split with room to spare.
Newlines kept, paragraphs bright —
I nibble spaces through the night.
Hop, wrap, and render — joyous light! ✨

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 29.03% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title 'feature/character-breakline-strategy' follows the project's naming pattern (feature/name) but is primarily a branch name rather than a descriptive summary of the main change. Consider using a more descriptive title that clearly summarizes the main feature, e.g., 'Add character-based line breaking strategy for text wrapping' or 'Implement CharacterStrategy for wrapping text at character boundaries'.
✅ Passed checks (3 passed)
Check name Status Explanation
Description check ✅ Passed The PR description is well-structured, addresses the template requirements, provides clear context about the feature, explains the technical implementation, and includes a comprehensive checklist with most items marked as completed.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share
Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.

Comment @coderabbitai help to get the list of available commands and usage tips.

@johnfercher
Copy link
Copy Markdown
Owner

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Mar 24, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 | 🟡 Minor

Keep GetLinesQuantity consistent with Add for fit-case inputs.

At Line 101, counting only via getLines(...) can return 0 for empty/whitespace-only strings, while Add renders 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 GetLinesQuantity matches Add behavior 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

📥 Commits

Reviewing files that changed from the base of the PR and between 4f0b54a and c7fc9ac.

⛔ Files ignored due to path filters (1)
  • docs/assets/pdf/textgridv2.pdf is excluded by !**/*.pdf
📒 Files selected for processing (9)
  • README.md
  • docs/assets/examples/textgrid/v2/main.go
  • docs/assets/text/textgridv2.txt
  • docs/v2/features/text.md
  • internal/providers/gofpdf/text.go
  • internal/providers/gofpdf/text_test.go
  • pkg/components/text/example_test.go
  • pkg/consts/breakline/breakline.go
  • test/maroto/examples/textgrid.json

@johnfercher
Copy link
Copy Markdown
Owner

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Mar 24, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between c7fc9ac and 382c7db.

⛔ Files ignored due to path filters (1)
  • docs/assets/pdf/textgridv2.pdf is excluded by !**/*.pdf
📒 Files selected for processing (7)
  • docs/assets/examples/textgrid/v2/main.go
  • docs/assets/text/textgridv2.txt
  • docs/v2/features/text.md
  • internal/providers/gofpdf/text.go
  • internal/providers/gofpdf/text_test.go
  • pkg/props/text.go
  • test/maroto/examples/textgrid.json

Comment on lines +196 to +225
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
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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).

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 382c7db and 680cc12.

⛔ Files ignored due to path filters (1)
  • docs/assets/pdf/richtextgridv2.pdf is excluded by !**/*.pdf
📒 Files selected for processing (13)
  • docs/assets/examples/richtextgrid/v2/main.go
  • docs/assets/examples/richtextgrid/v2/main_test.go
  • docs/assets/text/richtextgridv2.txt
  • docs/v2/features/_sidebar.md
  • docs/v2/features/richtext.md
  • internal/providers/gofpdf/provider.go
  • internal/providers/gofpdf/provider_test.go
  • mocks/Provider.go
  • pkg/components/richtext/example_test.go
  • pkg/components/richtext/richtext.go
  • pkg/components/richtext/richtext_test.go
  • pkg/core/provider.go
  • test/maroto/examples/richtextgrid.json

Comment on lines +23 to +24
## Time Execution
[filename](../../assets/text/richtextgridv2.txt ':include :type=code')
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
## 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')
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

Comment on lines +64 to +66
func (g *provider) GetStringWidth(text string, prop *props.Font) float64 {
g.font.SetFont(prop.Family, prop.Style, prop.Size)
return g.fpdf.GetStringWidth(text)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
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.

Comment on lines +234 to +236
func (r *RichText) layoutStyle() *props.Text {
return &r.chunks[0].Style
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

Suggested change
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.

Comment on lines +429 to +456
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)
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants