Add underline, overline, and strikethrough text decorations to the Text node#4193
Add underline, overline, and strikethrough text decorations to the Text node#4193jsjgdh wants to merge 2 commits into
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. |
jsjgdh
force-pushed
the
text-decoration
branch
2 times, most recently
from
4e9c832 to
34c57c1
Compare
There was a problem hiding this comment.
1 issue found and verified against the latest diff
Confidence score: 4/5
- In
editor/src/messages/portfolio/document/overlays/utility_types_native.rs, the only reported risk is process-level: the PR title does not meet the imperative-format rule, which can cause CI/policy checks to fail and delay or block merge despite no concrete code defect being identified — rename the PR to the required imperative format before merging.
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/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
| 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>
There was a problem hiding this comment.
1 issue found across 16 files
Confidence score: 3/5
- In
editor/src/messages/portfolio/document/graph_operation/graph_operation_message_handler.rs, SVG text import appears to use only the firstusvg::Textchunk’s position, so multi-chunk text (such as<tspan>-based content) can be placed incorrectly after import; merging as-is risks visibly broken layouts in user SVGs — update transform handling to account for all text chunks and add a regression test with multi-chunk text before merging.
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:734">
P2: SVG text transform import only accounts for the first text chunk's position, so multi-chunk SVG text can be mispositioned. `usvg::Text` can contain multiple chunks (for example from `<tspan>` elements with different `x`/`y` attributes), but all chunks are merged into a single Graphite text node and only the first chunk's coordinates are used for the transform offset. Consider splitting chunks into separate text layers or encoding their individual offsets so that imported multi-chunk SVG text preserves its original positioning.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| typesetting | ||
| } | ||
|
|
||
| fn apply_usvg_text_transform(modify_inputs: &mut ModifyInputsContext, text: &usvg::Text) { |
There was a problem hiding this comment.
P2: SVG text transform import only accounts for the first text chunk's position, so multi-chunk SVG text can be mispositioned. usvg::Text can contain multiple chunks (for example from <tspan> elements with different x/y attributes), but all chunks are merged into a single Graphite text node and only the first chunk's coordinates are used for the transform offset. Consider splitting chunks into separate text layers or encoding their individual offsets so that imported multi-chunk SVG text preserves its original positioning.
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 734:
<comment>SVG text transform import only accounts for the first text chunk's position, so multi-chunk SVG text can be mispositioned. `usvg::Text` can contain multiple chunks (for example from `<tspan>` elements with different `x`/`y` attributes), but all chunks are merged into a single Graphite text node and only the first chunk's coordinates are used for the transform offset. Consider splitting chunks into separate text layers or encoding their individual offsets so that imported multi-chunk SVG text preserves its original positioning.</comment>
<file context>
@@ -684,13 +706,45 @@ fn import_usvg_node_inner(
+ typesetting
+}
+
+fn apply_usvg_text_transform(modify_inputs: &mut ModifyInputsContext, text: &usvg::Text) {
+ let elem_transform = usvg_transform(text.abs_transform());
+ let chunk_offset = text.chunks().first().map(|c| DVec2::new(c.x().unwrap_or(0.) as f64, c.y().unwrap_or(0.) as f64)).unwrap_or_default();
</file context>
1 participant
Text-decoration