Bug 610 - cannot pull wire from group.#621
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds positionEdgeVariables plumbing (frontend/backend), makes RenderVariable hit-testing zoom-aware, reorders Group hit-testing and introduces layout/position helpers for edge I/O variables, and adds regression tests validating selection and zoomed wire-pull behavior. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as "UI Layer"
participant Group as "Group\n(positionEdgeVariables / select)"
participant Render as "RenderVariable\n(inImage)"
User->>UI: Click at (x,y)
UI->>Group: clickType(x,y)
Group->>Group: positionEdgeVariables()
Note over Group: compute world-space positions\nfor edge I/O variables (zoom-aware)
Group->>Render: inImage(x,y) (coords normalized by zoom)
alt hit detected
Render-->>Group: hit
Group-->>UI: selected variable / onPort
else no hit
Render-->>Group: miss
Group-->>UI: continue other hit-tests
end
UI-->>User: selection result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
This PR addresses a regression where users could not start wire dragging from a Group’s edge I/O variables (issue #610), particularly when the Group is zoomed and contents are displayed.
Changes:
- Reorders
Group::clickType()logic to test edge I/O variables before theinIORegionearly-exit. - Adds
Group::positionEdgeVariables()and invokes it fromGroup::select()to ensure edge-variable positions are valid for hit-testing even without a prior draw pass. - Fixes
RenderVariable::inImage()hit-testing under zoom by accounting forzoomFactor().
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/testCanvas.cc | Adds a regression test covering Group edge output-variable selection without a prior draw. |
| model/group.h | Declares positionEdgeVariables() to support non-render hit-testing. |
| model/group.cc | Implements edge-variable positioning for hit-testing; adjusts click-type evaluation order to fix issue #610. |
| model/cairoItems.cc | Updates variable hit-testing to account for zoom. |
| gui-js/libs/shared/src/lib/backend/minsky.ts | Exposes positionEdgeVariables() on Group/Selection in the generated TypeScript backend API wrapper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| // Ensure edge variable positions are up-to-date before hit-testing. | ||
| // (Positions are otherwise only set during draw1edge() in a render pass.) | ||
| positionEdgeVariables(); | ||
| for (auto& v: inVariables) |
There was a problem hiding this comment.
select() now unconditionally calls positionEdgeVariables(), which in turn calls margins() (updating each I/O var’s bounding box via Pango) and does moveTo() on every edge variable. Because Canvas::itemAt() hit-tests groups via clickType() (which now calls select()), this can add a lot of work even when (x,y) is nowhere near the group. Consider short-circuiting before positionEdgeVariables() (eg, return early when inVariables/outVariables are empty, and/or only update positions when (x,y) is within an expanded group bounds that could plausibly include edge variables).
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/testCanvas.cc (1)
111-128: Please also cover the zoomed wire-pull path.This regression proves
select()now positions edge variables before hit-testing, but it still probes the icon center at default zoom. That will not catch regressions inmodel/cairoItems.cc, Lines 90-97, ormodel/group.cc, Lines 910-927, which are the changes that actually unblock pulling a wire when zoomed. Please add a non-unit zoom case and assertgrp->clickType(...) == ClickType::onPort(or the equivalent wire-drag path).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/testCanvas.cc` around lines 111 - 128, Add a test variant that verifies the zoomed wire-pull path by calling grp->positionEdgeVariables() (or relying on grp->select() which now positions them) at a non-default zoom and then asserting the hit-test yields a port click; specifically, reuse the existing expectedX/expectedY computation but set grp->setZoomFactor(...) (or grp->zoomFactor setter) to a non-1 value, call grp->select(expectedX, expectedY) (or directly call grp->clickType(expectedX, expectedY)) and assert the result equals ClickType::onPort (or the existing enum/value used for wire-drag), ensuring the test covers the code paths in model/cairoItems.cc and model/group.cc that handle zoomed icon-center probing.model/group.cc (1)
1188-1221: Factor the edge-variable placement math into one helper.
positionEdgeVariables()now duplicates the layout code indraw1edge()atmodel/group.cc, Lines 1048-1078. Keeping both copies in sync is what makes rendering and hit-testing agree, so this is worth centralizing before the next spacing tweak.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/group.cc` around lines 1188 - 1221, The positioning math for edge variables is duplicated between Group::positionEdgeVariables() and draw1edge(); extract the shared layout into a single helper (e.g., a new method like Group::layoutEdgeVariables or a static/free helper used by both) that contains the loop and calculations for xEdge, yOff, top/bottom, Rotate usage, v->moveTo(), v->rotation(), and per-variable height/zoom adjustments; replace the duplicated blocks in positionEdgeVariables() and the draw1edge() code (the block at model/group.cc lines referenced in the comment) with calls into this helper, passing the variables vector, computed xEdge (using iWidth()/zoomFactor()/margins results), and any needed context (rotation(), this->x()/this->y(), and zoom factors) so rendering and hit-testing share the exact same placement logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@model/group.cc`:
- Around line 1188-1221: The positioning math for edge variables is duplicated
between Group::positionEdgeVariables() and draw1edge(); extract the shared
layout into a single helper (e.g., a new method like Group::layoutEdgeVariables
or a static/free helper used by both) that contains the loop and calculations
for xEdge, yOff, top/bottom, Rotate usage, v->moveTo(), v->rotation(), and
per-variable height/zoom adjustments; replace the duplicated blocks in
positionEdgeVariables() and the draw1edge() code (the block at model/group.cc
lines referenced in the comment) with calls into this helper, passing the
variables vector, computed xEdge (using iWidth()/zoomFactor()/margins results),
and any needed context (rotation(), this->x()/this->y(), and zoom factors) so
rendering and hit-testing share the exact same placement logic.
In `@test/testCanvas.cc`:
- Around line 111-128: Add a test variant that verifies the zoomed wire-pull
path by calling grp->positionEdgeVariables() (or relying on grp->select() which
now positions them) at a non-default zoom and then asserting the hit-test yields
a port click; specifically, reuse the existing expectedX/expectedY computation
but set grp->setZoomFactor(...) (or grp->zoomFactor setter) to a non-1 value,
call grp->select(expectedX, expectedY) (or directly call
grp->clickType(expectedX, expectedY)) and assert the result equals
ClickType::onPort (or the existing enum/value used for wire-drag), ensuring the
test covers the code paths in model/cairoItems.cc and model/group.cc that handle
zoomed icon-center probing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 909517e4-5444-4ec3-a9ac-0331315b89bd
📒 Files selected for processing (5)
gui-js/libs/shared/src/lib/backend/minsky.tsmodel/cairoItems.ccmodel/group.ccmodel/group.htest/testCanvas.cc
|
@highperformancecoder I've opened a new pull request, #622, to work on those changes. Once the pull request is ready, I'll request review from you. |
…eVariables() Co-authored-by: highperformancecoder <3075825+highperformancecoder@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@model/group.cc`:
- Around line 1080-1109: The draw pass must apply the same zoom used during
layout so rendered offsets match hit-test positions: in draw1edge()/Group::draw
where you compute t = v->bb.top(), b = v->bb.bottom() and update top/bottom,
scale those bounding-box values by the variable's zoom (e.g. t *=
v->zoomFactor(); b *= v->zoomFactor();) and stop dividing heights by zoom; use
v->height() when adjusting top and bottom (replace top -= v->height() /
v->zoomFactor() and bottom += v->height() / v->zoomFactor() with top -=
v->height() and bottom += v->height()). This keeps yOff/top/bottom in
world-space to match layoutEdgeVariables()/layout results.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aa634275-0a0f-457c-8651-477e06f0ba57
📒 Files selected for processing (2)
model/group.cctest/testCanvas.cc
🚧 Files skipped from review as they are similar to previous changes (1)
- test/testCanvas.cc
Optimize Group::select() with short-circuit checks before positionEdgeVariables()
highperformancecoder
changed the title
There was a problem hiding this comment.
♻️ Duplicate comments (1)
model/group.cc (1)
1080-1109:⚠️ Potential issue | 🟠 MajorKeep
draw1edge()in the same zoom space aslayoutEdgeVariables().
layoutEdgeVariables()now computesyOff/top/bottomin world space, but this block switches back to unscaledbb.top()/bottom()and dividesheight()by zoom again. At non-unit zoom, the rendered edge ports drift away from the positions used for hit-testing.Group::draw()also has not scaled the Cairo context beforedrawEdgeVariables(), so the new comment here is misleading.Suggested fix
- // Iterate again solely to perform drawing steps. - // Group::draw has ALREADY called cairo_scale(cairo, z, z), so the `xEdge` - // parameter is properly matched to the visual coordinates. - // However, translating `yOff` locally during rendering requires tracking - // the variable bounding boxes WITHOUT pre-multiplying `z` onto them here. const double angle=rotation() * M_PI / 180.0; float top=0, bottom=0; for (size_t i=0; i<vars.size(); ++i) { auto& v=vars[i]; + const float z=v->zoomFactor(); float yOff = 0; - auto t = v->bb.top(), b = v->bb.bottom(); + auto t = v->bb.top()*z, b = v->bb.bottom()*z; if (i > 0) yOff = i % 2 ? top - b : bottom - t; const cairo::CairoSave cs(cairo); cairo_translate(cairo,xEdge,yOff); // cairo context is already rotated, so antirotate cairo_rotate(cairo,-angle); v->draw(cairo); if (i == 0) { bottom = b; top = t; } else if (i % 2) - top -= v->height() / v->zoomFactor(); // Remove zoom as Group::draw supplies it + top -= v->height(); else - bottom += v->height() / v->zoomFactor(); + bottom += v->height(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/group.cc` around lines 1080 - 1109, The rendering loop is mixing zoom spaces: layoutEdgeVariables() computes yOff/top/bottom in world (zoomed) coordinates but this block reverts to unscaled values by calling v->bb.top()/bottom() and then subtracting v->height()/v->zoomFactor(); to fix, keep everything in the same (world/zoomed) space — stop undoing zoom by removing the /v->zoomFactor() adjustments and use the bb/top/bottom and v->height() as-is (or explicitly multiply by the current zoom if those members are in local space), and update the comment near Group::draw()/drawEdgeVariables()/rotation() to reflect that the Cairo context is not pre-scaled here so we must use world-space values for positioning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@model/group.cc`:
- Around line 1080-1109: The rendering loop is mixing zoom spaces:
layoutEdgeVariables() computes yOff/top/bottom in world (zoomed) coordinates but
this block reverts to unscaled values by calling v->bb.top()/bottom() and then
subtracting v->height()/v->zoomFactor(); to fix, keep everything in the same
(world/zoomed) space — stop undoing zoom by removing the /v->zoomFactor()
adjustments and use the bb/top/bottom and v->height() as-is (or explicitly
multiply by the current zoom if those members are in local space), and update
the comment near Group::draw()/drawEdgeVariables()/rotation() to reflect that
the Cairo context is not pre-scaled here so we must use world-space values for
positioning.
This change is
Summary by CodeRabbit
Bug Fixes
Tests