Repository navigation
feat: draw, reshape, remove and relabel grid lines (T01.2) - #417
Conversation
axisrow
left a comment
There was a problem hiding this comment.
T01.2: drawn grid lines and relabelling
Verdict: approved. No changes requested.
Reviewed the delta over #416 (13 files, +1264): the typed GridLineEditRequest model with versioned fingerprints, the pure engine edit decisions, the Change::Grid authoring/validation path, the four CLI commands, the initial drawn line at creation, and the E2E suite.
What checked out:
- Atomicity and guards. Each operation validates the complete replacement record; frame, grid name and family modes are verified unchanged by the change validator (mode conversion refused with GRID_FAMILY_MODE), identities and geometry survive relabel, the last line cannot be removed, and a line named by an incoming reference is DEPENDENCY_CONFLICT. The engine never mutates its input; the footprint (reads of target lifecycle/shape/placement plus the fresh axis absence, one shape-input write) matches the validator declared groups, and check_footprint ties them together.
- Geometry. World points convert through the pinned frame with the exact inverse rotation, then re-validate against the local 1e6 cap and 1 mm floor; degenerate and colliding identities refuse without writes. The rotated-frame E2E pins the conversion.
- Replay. Fingerprints are independent of source state, so an exact retry replays from operation evidence without resurrecting a later-removed line — covered end to end, including undo/redo units.
- Creation reachability.
create-grid --u-from/--u-to/--u-labelmakes a drawn family with one line reachable at creation without storing an invalid empty grid — this also unblocks the GR-P14 shape of test case against the amended contract proposed in #415; the contract text (section 8.1 drawn-empty) should pick that up there. - Documentation. The design note states the conservative reference blocking (grid-level until axis-specific stored references arrive with W01), the replay rule and the unclaimed parent gate.
Minor observations, no change needed: a non-grid target reports UnsupportedProperty where NotFound might read better; the conservative removal guard refuses any line removal while the grid itself is referenced (safe over-refusal, already documented as a W01 follow-up); the relabel-family CLI counts labels only to satisfy the parser, the real count check lives in the engine where it belongs.
Tests, fmt and clippy were not run in this review; CI covers them.
What this changes
Adds T01.2 on top of #416: drawn grid lines can be created from world XY points, reshaped, removed and relabelled through the engine and the real CLI, with one atomic revision per command.
draw-grid-line,reshape-grid-line,remove-grid-lineandrelabel-grid-line.create-grid, so a fully drawn grid is reachable while every stored grid still has at least one line.Change::Grid; the frame and family modes stay unchanged.Stack: depends on #416 and the contract proposal #415. The T01.2-only comparison is fork base → this branch. Rebase after the preceding PRs land. Current upstream master and open PRs were checked for duplicates.
How you verified it
Never-crash and provenance
Bounded line/label counts and finite world/local coordinates; stored arrays remain checked by T01-V01. No new production unwrap/expect/panic, input-derived indexing, unsafe, dependencies or assets. Contributor-original MPL-2.0.
Review points
Keep draft for the preceding dependencies. Incoming references conservatively block line removal until axis-specific stored view references enter with W01. The historical AxisId lifetime policy across deletion/history compaction remains the review point described in #416; current collision checks use the complete pinned live graph. This PR does not claim the T01 parent acceptance gate.