fix: refactor transform border #196
Conversation
WalkthroughRefactors asset handling by moving Asset and AssetSerialized unions to types.zig with bounds helpers, updates index.zig to use transform_ui and new asset APIs, adds transform_ui.getBorderDrawVertex, and restructures texts.zig line-wrapping to unify ENTER and width-triggered breaks with explicit markers and vertices. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as index.zig
participant A as types.Asset
participant TU as transform_ui
participant L as lines
participant G as triangles buffer
UI->>A: getBoundsPtr()
Note right of A: Returns *[4]PointUV
UI->>TU: getBorderDrawVertex(asset, color)
TU->>A: getBoundsPtr()
TU->>L: getDrawVertexData(points, thickness, color, render_scale)
L-->>TU: Draw instances
TU-->>UI: [8] DrawInstance
UI->>G: Append border draw instances
sequenceDiagram
autonumber
participant T as texts.zig
participant V as Vertex list
T->>T: For each code point
alt ENTER or exceeded max width
T->>V: Mark previous char last_in_line
T->>V: Append SOFT_BREAK_MARKER (when width exceeded)
T->>V: Append line-break vertex
T->>V: Append ENTER marker
T->>V: Append line-break vertex
T->>T: Reset next_pos, space_before=0
else
T->>V: Append regular glyph vertex
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate 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. Comment |
There was a problem hiding this comment.
Pull Request Overview
Refactors transform border rendering by centralizing Asset-related shapes and border drawing logic, and adjusts text wrapping behavior.
- Move Asset and AssetSerialized unions to types.zig and add helpers to access bounds.
- Extract border line vertex generation into transform_ui.getBorderDrawVertex and update call sites to use transform_ui alias.
- Improve text line-breaking: handle ENTER vs soft-wrap distinctly and set last_in_line earlier.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/logic/types.zig | Introduces Asset/AssetSerialized unions and helper methods getBounds/getBoundsPtr to unify access to per-asset bounds. |
| src/logic/transform_ui.zig | Adds getBorderDrawVertex to produce border draw instances for any Asset; imports Asset and AssetId. |
| src/logic/texts/texts.zig | Modifies line-breaking flow to break on ENTER or width, mark previous glyph as last_in_line, and only insert soft break markers for width overflow. |
| src/logic/index.zig | Switches to transform_ui alias, uses new Asset helpers, refactors drawBorder to call transform_ui.getBorderDrawVertex, and updates pick rendering constants. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/logic/texts/texts.zig (2)
180-195: Consistent break markers on width wrap: OK; also set an explicit zero-width marker on ENTER for parity.Width-wrap path emits a SOFT_BREAK then ENTER marker with null CharVertex. Do the same for explicit ENTER to keep model/render parity and selection math stable, without fetching glyphs.
if (exceeded_max_width) { try updated_content.append(SOFT_BREAK_MARKER); try self.text_vertex.append(CharVertex{ .relative_bounds = self.getDrawRelativeBounds(0, 0, 0, 0, next_pos), .origin = next_pos, .char = null, }); // add word joiner character before line break so that when user copies the text, they get the same line breaks try updated_content.append(ENTER_CHAR_CODE); try self.text_vertex.append(CharVertex{ .relative_bounds = self.getDrawRelativeBounds(0, 0, 0, 0, next_pos), .origin = next_pos, .char = null, }); } + if (cp == ENTER_CHAR_CODE) { + // mirror width-wrap behavior for explicit newlines + try updated_content.append(ENTER_CHAR_CODE); + try self.text_vertex.append(CharVertex{ + .relative_bounds = self.getDrawRelativeBounds(0, 0, 0, 0, next_pos), + .origin = next_pos, + .char = null, + }); + continue; // skip glyph emission + }
208-216: Skip CharVertex glyph emission for ENTER; remove last_in_line flag coupling to codepoint.After adopting the guards above, ensure glyph vertices are only emitted for drawable code points.
- // Encode the Unicode codepoint as UTF-8 bytes - try updated_content.append(cp); - try self.text_vertex.append(CharVertex{ - .relative_bounds = relative_bounds, - .char = cp, - .origin = next_pos, - .last_in_line = cp == ENTER_CHAR_CODE, - }); + // Encode the Unicode codepoint as UTF-8 bytes + try updated_content.append(cp); + if (cp != ENTER_CHAR_CODE) { + try self.text_vertex.append(CharVertex{ + .relative_bounds = relative_bounds, + .char = cp, + .origin = next_pos, + }); + }
🧹 Nitpick comments (1)
src/logic/transform_ui.zig (1)
11-12: Remove unused import or use it.AssetId is imported but not referenced.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/logic/index.zig(7 hunks)src/logic/texts/texts.zig(1 hunks)src/logic/transform_ui.zig(2 hunks)src/logic/types.zig(1 hunks)
🔇 Additional comments (11)
src/logic/types.zig (3)
2-4: Centralizing type imports looks good.Importing images/shapes/texts here simplifies cohesion of Asset types. No issues.
6-26: Asset union + bounds helpers LGTM.Switch-based accessors are clear; pointer version uses self.* correctly. Good ergonomics.
28-32: AssetSerialized union LGTM.Keeps serialization variants co-located with Asset.
src/logic/transform_ui.zig (1)
244-264: Remove incorrect type-mismatch suggestions
The call tolines.getDrawVertexDatain src/logic/lines.zig:33 is defined aspub fn getDrawVertexData( buffer: *[2]triangles.DrawInstance, start: anytype, end: anytype, width: f32, color: [4]u8, rounded: f32, ) voidso passing
color: [4]u8and yourpoint/next_pointvalues (anytype) already matches the signature. NotoPoint()conversion or switch to@Vector(4,u8)is required.Likely an incorrect or invalid review comment.
src/logic/index.zig (7)
6-6: LGTM! Improved naming and modularity.The rename from
TransformUItotransform_uifollows Zig conventions, and movingAsset/AssetSerializedtotypes.zigimproves code organization.Also applies to: 22-23
546-546: LGTM! Consistent with renamed import.The function call correctly uses the new
transform_uimodule name.
680-684: LGTM! Consistent with renamed import.The
transformPointscall correctly uses the newtransform_uimodule name.
664-664: LGTM! Cleaner abstraction.Using
asset.getBoundsPtr()eliminates switch-based bound extraction, improving maintainability and reducing code duplication.
743-748: LGTM! Simplified border rendering.Delegating border rendering to
transform_ui.getBorderDrawVertexeliminates manual border point calculations and improves code clarity. The color coding (red for hovered, green for selected) is intuitive.Also applies to: 753-758
1340-1342: LGTM! Consistent with renamed import.The
renderPickfunction correctly usestransform_ui.PICK_TRIANGLE_INSTANCESandtransform_ui.getPickVertexDatawith the new module name.
761-762: Asset methods verified
Bothpub fn getBounds(self: Asset) [4]PointUVandpub fn getBoundsPtr(self: *Asset) *[4]PointUVare implemented insrc/logic/types.zig, so the by-value call on line 762 is intentional.
|
🎉 This PR is included in version 0.1.0-next.22 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary by CodeRabbit