Add Lorem Ipsum placeholder to text tool - #4027
Annonnymmousss wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
2 issues found across 5 files
Confidence score: 2/5
- There is high merge risk because both findings are high-severity (7–8/10) with high confidence (9/10) and affect active text-editing flows.
- In
editor/src/messages/tool/tool_messages/text_tool.rs, lorem ipsum sizing divides byline_heightwithout a zero guard, which can produce extremeword_countvalues and trigger expensive allocations during drag updates (performance/stability impact). - In
editor/src/messages/tool/tool_messages/text_tool.rs, persistentis_lorem_ipsumstate when editing existing layers can carry stale behavior across sessions, suppress clipping warnings, and truncate unrelated text on commit (user-visible data correctness risk). - Pay close attention to
editor/src/messages/tool/tool_messages/text_tool.rs- guard zeroline_heightmath and reset lorem-ipsum state when switching to existing layer edits.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="editor/src/messages/tool/tool_messages/text_tool.rs">
<violation number="1" location="editor/src/messages/tool/tool_messages/text_tool.rs:383">
P1: `is_lorem_ipsum` persists across sessions and is not reset when editing existing layers, so stale state can suppress clipping warnings and incorrectly truncate unrelated text on commit.</violation>
<violation number="2" location="editor/src/messages/tool/tool_messages/text_tool.rs:1097">
P1: Lorem ipsum sizing divides by `line_height` without guarding zero, allowing infinite/huge `word_count` and expensive allocation during drag updates.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Code Review
This pull request implements a 'Lorem Ipsum' placeholder feature for the text tool, allowing users to automatically fill new text boxes with placeholder text. It includes a new preference setting, UI components, and logic within the text tool's state machine to estimate and truncate the placeholder text based on the drawn box dimensions. The review feedback highlights several improvement opportunities regarding code maintainability and performance, specifically recommending the use of named constants instead of magic numbers for text estimation and suggesting optimizations to reduce string allocations during text generation and truncation.
f07c79b to
76938eb
Compare
4b7a823 to
847b8e9
Compare
15fcaac to
d5f0140
Compare
985cadb to
06d997f
Compare
There was a problem hiding this comment.
3 issues found across 8 files
Confidence score: 3/5
- In
editor/src/messages/tool/tool_messages/text_tool.rs, the placement preview ignores the selected font, so glyphs and line wrapping can differ from the text box after placement; pass the loaded font bytes into the preview. - In
editor/src/messages/tool/tool_messages/text_tool.rs, changing typography options during placement can leave the cached average advance stale because the font hash is unchanged; include the relevant typesetting values in the cache key or recompute the advance. - In
fit_lorem_ipsum_to_heightwithineditor/src/messages/tool/tool_messages/text_tool.rs, the decrementing loop can mismeasure a box that fits only one word because the generated text clamps the input at its lower bound; verify the one-word boundary case and adjust the fitting logic.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="editor/src/messages/tool/tool_messages/text_tool.rs">
<violation number="1" location="editor/src/messages/tool/tool_messages/text_tool.rs:861">
P2: When typography options change during placement, this cache keeps the old average advance because the font hash is unchanged. Include the relevant typesetting values in the cache key or recompute the advance when they change.</violation>
<violation number="2" location="editor/src/messages/tool/tool_messages/text_tool.rs:876">
P2: The placement preview does not use the selected font. Pass the loaded font bytes here, otherwise its glyphs and wrapping can differ from the text box shown immediately after placement.</violation>
<violation number="3" location="editor/src/messages/tool/tool_messages/text_tool.rs:1313">
P3: In `fit_lorem_ipsum_to_height`, the decrementing loop measures `generate_lorem_ipsum(low)` against `max_height`. When the box fits only one word, `low` reaches 1 but `generate_lorem_ipsum` clamps its input to `DEFAULT_LOREM_IPSUM_WORD_COUNT` (2) and returns a two-word string, so the height test still fails, the loop clears `new_text`, and `delete_empty_layer` removes the freshly dragged text layer even though a single word would have fit. Measure the requested word count instead of the clamped regenerated output.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| let typesetting = new_text_typesetting(tool_options, constraint_size); | ||
| let font_resource = fonts.get_resource_or_queue_load(&tool_options.font, responses); | ||
| let font_hash = font_resource.hash(); | ||
| if tool_data.lorem_ipsum_preview_font_hash != Some(font_hash) { |
There was a problem hiding this comment.
P2: When typography options change during placement, this cache keeps the old average advance because the font hash is unchanged. Include the relevant typesetting values in the cache key or recompute the advance when they change.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/tool/tool_messages/text_tool.rs, line 861:
<comment>When typography options change during placement, this cache keeps the old average advance because the font hash is unchanged. Include the relevant typesetting values in the cache key or recompute the advance when they change.</comment>
<file context>
@@ -804,6 +847,42 @@ impl Fsm for TextToolFsmState {
+ let typesetting = new_text_typesetting(tool_options, constraint_size);
+ let font_resource = fonts.get_resource_or_queue_load(&tool_options.font, responses);
+ let font_hash = font_resource.hash();
+ if tool_data.lorem_ipsum_preview_font_hash != Some(font_hash) {
+ tool_data.lorem_ipsum_preview_font_hash = Some(font_hash);
+ tool_data.lorem_ipsum_preview_average_advance = average_glyph_advance(&font_resource, typesetting);
</file context>
| line_height_ratio: typesetting.line_height_ratio, | ||
| font_size: tool_options.font_size, | ||
| color: tool_options.fill.active_color().map_or(COLOR_OVERLAY_BLACK.to_string(), |color| SRGBA8::from(color).to_css_hex()), | ||
| font_data: Vec::new().into(), |
There was a problem hiding this comment.
P2: The placement preview does not use the selected font. Pass the loaded font bytes here, otherwise its glyphs and wrapping can differ from the text box shown immediately after placement.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/tool/tool_messages/text_tool.rs, line 876:
<comment>The placement preview does not use the selected font. Pass the loaded font bytes here, otherwise its glyphs and wrapping can differ from the text box shown immediately after placement.</comment>
<file context>
@@ -804,6 +847,42 @@ impl Fsm for TextToolFsmState {
+ line_height_ratio: typesetting.line_height_ratio,
+ font_size: tool_options.font_size,
+ color: tool_options.fill.active_color().map_or(COLOR_OVERLAY_BLACK.to_string(), |color| SRGBA8::from(color).to_css_hex()),
+ font_data: Vec::new().into(),
+ transform: window_aligned_transform(document, position, DVec2::ONE).to_cols_array(),
+ max_width: constraint_size.map(|size| size.x),
</file context>
| font_data: Vec::new().into(), | |
| font_data: font_resource.as_ref().to_vec().into(), |
| } | ||
|
|
||
| while low > 0 { | ||
| let text = generate_lorem_ipsum(low); |
There was a problem hiding this comment.
P3: In fit_lorem_ipsum_to_height, the decrementing loop measures generate_lorem_ipsum(low) against max_height. When the box fits only one word, low reaches 1 but generate_lorem_ipsum clamps its input to DEFAULT_LOREM_IPSUM_WORD_COUNT (2) and returns a two-word string, so the height test still fails, the loop clears new_text, and delete_empty_layer removes the freshly dragged text layer even though a single word would have fit. Measure the requested word count instead of the clamped regenerated output.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/tool/tool_messages/text_tool.rs, line 1313:
<comment>In `fit_lorem_ipsum_to_height`, the decrementing loop measures `generate_lorem_ipsum(low)` against `max_height`. When the box fits only one word, `low` reaches 1 but `generate_lorem_ipsum` clamps its input to `DEFAULT_LOREM_IPSUM_WORD_COUNT` (2) and returns a two-word string, so the height test still fails, the loop clears `new_text`, and `delete_empty_layer` removes the freshly dragged text layer even though a single word would have fit. Measure the requested word count instead of the clamped regenerated output.</comment>
<file context>
@@ -1113,3 +1208,115 @@ impl Fsm for TextToolFsmState {
+ }
+
+ while low > 0 {
+ let text = generate_lorem_ipsum(low);
+ if bounding_box(&text, &font_resource, editing_text.typesetting, true).y <= max_height {
+ tool_data.new_text = text;
</file context>
Part of #1105
Not mentioned in the issue but is a part of Text.
This feature is applicable by default but can be off from file>preference.
Screen.Recording.2026-04-14.at.5.39.50.AM.mov