Text-decoration#4193
Conversation
|
@jsjgdh I have started the AI code review. It will take a few minutes to complete. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for text decorations (underline, overline, and strikethrough) in the text rendering pipeline. It updates TypesettingConfig and the text node to support these properties, implements rendering logic in PathBuilder, and updates the USVG importer to extract decorations and text transforms. Additionally, document migration is added to upgrade existing text nodes. The reviewer suggested a performance optimization in render_decoration_run to return early if no decorations are enabled, avoiding unnecessary font metric lookups.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 13 files
Confidence score: 4/5
- This PR looks safe to merge from a functionality standpoint; the reported issue is process-related (PR title wording) rather than a code behavior defect.
- The most severe finding targets title style compliance, not runtime logic in
editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs, so user-facing regression risk appears low. - Because the issue has high confidence but non-functional impact, it introduces mild merge uncertainty for workflow/policy compliance rather than product stability.
- Pay close attention to
editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs- ensure no actual graph operation behavior changes were missed while addressing the title-enforcement note.
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/portfolio/document/graph_operation/graph_operation_message_handler.rs">
<violation number="1" location="editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs:17">
P2: Custom agent: **PR title enforcement**
PR title is not written in imperative mood with a leading action verb. "Text-decoration" is a noun phrase and does not begin with an action verb as required by the PR title enforcement rule. Suggested: "Add text decoration support for SVG import" or similar imperative form.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| @@ -14,14 +14,15 @@ use graphene_std::Color; | |||
| use graphene_std::renderer::Quad; | |||
There was a problem hiding this comment.
P2: Custom agent: PR title enforcement
PR title is not written in imperative mood with a leading action verb. "Text-decoration" is a noun phrase and does not begin with an action verb as required by the PR title enforcement rule. Suggested: "Add text decoration support for SVG import" or similar imperative form.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs, line 17:
<comment>PR title is not written in imperative mood with a leading action verb. "Text-decoration" is a noun phrase and does not begin with an action verb as required by the PR title enforcement rule. Suggested: "Add text decoration support for SVG import" or similar imperative form.</comment>
<file context>
@@ -14,14 +14,15 @@ use graphene_std::Color;
use graphene_std::renderer::convert_usvg_path::convert_usvg_path;
use graphene_std::table::Table;
-use graphene_std::text::{Font, TypesettingConfig};
+use graphene_std::text::{Font, FontCache, TypesettingConfig};
use graphene_std::vector::style::{Fill, Gradient, GradientSpreadMethod, GradientStop, GradientStops, GradientType, PaintOrder, Stroke, StrokeAlign, StrokeCap, StrokeJoin};
</file context>
There was a problem hiding this comment.
Advice title and description.
|
@jsjgdh I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
3 issues found and verified against the latest diff
Confidence score: 3/5
- In
node-graph/nodes/text/src/path_builder.rs,render_decoration_runis not applying alignment-derivedx_offset/space_extra, so underlines/strikethroughs can render in the wrong position for shifted or justified text; merging as-is risks visibly incorrect text decoration layout—apply the same offset/spacing adjustments used for glyph placement before merging. - In
editor/src/messages/portfolio/document_migration.rs, the text-decoration migration checks the wrong input count, which can skip migration for both legacy and already-split text nodes; merging as-is risks opening documents with stale decoration wiring—fix the input-count condition and verify migration on representative old/new documents before merge. - In
editor/src/messages/portfolio/document/overlays/utility_types_native.rs, the PR title rule is currently failing (Text-decorationformat), which is a process gate rather than a runtime defect; update the PR title to the required imperative format to avoid avoidable CI/policy friction at merge time.
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="node-graph/nodes/text/src/path_builder.rs">
<violation number="1" location="node-graph/nodes/text/src/path_builder.rs:166">
P1: `render_decoration_run` ignores `x_offset` and `space_extra` from alignment, causing decorations to misalign with shifted/justified glyphs.</violation>
</file>
<file name="editor/src/messages/portfolio/document_migration.rs">
<violation number="1" location="editor/src/messages/portfolio/document_migration.rs:1719">
P1: Text-decoration migration targets the wrong input count for current text nodes, leaving both legacy and already-split text nodes unmigrated.</violation>
</file>
<file name="editor/src/messages/portfolio/document/overlays/utility_types_native.rs">
<violation number="1" location="editor/src/messages/portfolio/document/overlays/utility_types_native.rs:1117">
P1: Custom agent: **PR title enforcement**
PR title 'Text-decoration' violates the PR title enforcement rule by not using an imperative verb as the first word and not following the required format.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| } | ||
| } | ||
|
|
||
| pub fn render_decoration_run(&mut self, glyph_run: &GlyphRun<'_, ()>, underline: bool, overline: bool, strikethrough: bool, per_glyph_items: bool) { |
There was a problem hiding this comment.
P1: render_decoration_run ignores x_offset and space_extra from alignment, causing decorations to misalign with shifted/justified glyphs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/nodes/text/src/path_builder.rs, line 166:
<comment>`render_decoration_run` ignores `x_offset` and `space_extra` from alignment, causing decorations to misalign with shifted/justified glyphs.</comment>
<file context>
@@ -163,6 +163,37 @@ impl PathBuilder {
}
}
+ pub fn render_decoration_run(&mut self, glyph_run: &GlyphRun<'_, ()>, underline: bool, overline: bool, strikethrough: bool, per_glyph_items: bool) {
+ let run = glyph_run.run();
+ let baseline = glyph_run.baseline() as f64;
</file context>
| } | ||
|
|
||
| // Insert text decoration parameters: underline, overline, and strikethrough. | ||
| if reference == DefinitionIdentifier::ProtoNode(graphene_std::text::text::IDENTIFIER) && inputs_count == 13 { |
There was a problem hiding this comment.
P1: Text-decoration migration targets the wrong input count for current text nodes, leaving both legacy and already-split text nodes unmigrated.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/portfolio/document_migration.rs, line 1719:
<comment>Text-decoration migration targets the wrong input count for current text nodes, leaving both legacy and already-split text nodes unmigrated.</comment>
<file context>
@@ -1715,6 +1715,35 @@ fn migrate_node(node_id: &NodeId, node: &DocumentNode, network_path: &[NodeId],
}
+ // Insert text decoration parameters: underline, overline, and strikethrough.
+ if reference == DefinitionIdentifier::ProtoNode(graphene_std::text::text::IDENTIFIER) && inputs_count == 13 {
+ let mut template: NodeTemplate = resolve_document_node_type(&reference)?.default_node_template();
+ document.network_interface.replace_implementation(node_id, network_path, &mut template);
</file context>
| max_width: None, | ||
| max_height: None, | ||
| align: TextAlign::AlignLeft, | ||
| underline: false, |
There was a problem hiding this comment.
P1: Custom agent: PR title enforcement
PR title 'Text-decoration' violates the PR title enforcement rule by not using an imperative verb as the first word and not following the required format.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/portfolio/document/overlays/utility_types_native.rs, line 1117:
<comment>PR title 'Text-decoration' violates the PR title enforcement rule by not using an imperative verb as the first word and not following the required format.</comment>
<file context>
@@ -1114,6 +1114,9 @@ impl OverlayContextInternal {
max_width: None,
max_height: None,
align: TextAlign::AlignLeft,
+ underline: false,
+ overline: false,
+ strikethrough: false,
</file context>
1 participant
Text-decoration