diff --git a/gui-js/libs/shared/src/lib/backend/minsky.ts b/gui-js/libs/shared/src/lib/backend/minsky.ts index f11f8ede3..ab5229b49 100644 --- a/gui-js/libs/shared/src/lib/backend/minsky.ts +++ b/gui-js/libs/shared/src/lib/backend/minsky.ts @@ -1108,6 +1108,7 @@ export class Group extends Item { async portY(a1: number): Promise {return this.$callMethod('portY',a1);} async ports(a1: number): Promise {return this.$callMethod('ports',a1);} async portsSize(): Promise {return this.$callMethod('portsSize');} + async positionEdgeVariables(): Promise {return this.$callMethod('positionEdgeVariables');} async px(...args: number[]): Promise {return this.$callMethod('px',...args);} async py(...args: number[]): Promise {return this.$callMethod('py',...args);} async pz(...args: number[]): Promise {return this.$callMethod('pz',...args);} @@ -2031,6 +2032,7 @@ export class Selection extends CppClass { async portY(a1: number): Promise {return this.$callMethod('portY',a1);} async ports(a1: number): Promise {return this.$callMethod('ports',a1);} async portsSize(): Promise {return this.$callMethod('portsSize');} + async positionEdgeVariables(): Promise {return this.$callMethod('positionEdgeVariables');} async randomLayout(): Promise {return this.$callMethod('randomLayout');} async relZoom(...args: number[]): Promise {return this.$callMethod('relZoom',...args);} async removeDisplayPlot(): Promise {return this.$callMethod('removeDisplayPlot');} diff --git a/model/cairoItems.cc b/model/cairoItems.cc index 9dd0c6cb5..9482bd812 100644 --- a/model/cairoItems.cc +++ b/model/cairoItems.cc @@ -89,7 +89,8 @@ void RenderVariable::draw() bool RenderVariable::inImage(float x, float y) { - const float dx=x-var.x(), dy=y-var.y(); + const float z=var.zoomFactor(); + const float dx=(x-var.x())/z, dy=(y-var.y())/z; const float rx=dx*cos(var.rotation()*M_PI/180)-dy*sin(var.rotation()*M_PI/180); const float ry=dy*cos(var.rotation()*M_PI/180)+dx*sin(var.rotation()*M_PI/180); return rx>=-w && rx<=w && ry>=-h && ry <= h; diff --git a/model/group.cc b/model/group.cc index 5dbf77cda..39221d9ba 100644 --- a/model/group.cc +++ b/model/group.cc @@ -911,11 +911,16 @@ namespace minsky { auto z=zoomFactor(); const double w=0.5*iWidth()*z, h=0.5*iHeight()*z; - if (onResizeHandle(x,y)) return ClickType::onResize; - if (displayContents() && inIORegion(x,y)==IORegion::none) - return ClickType::outside; + if (onResizeHandle(x,y)) return ClickType::onResize; + // Check edge I/O variables BEFORE the inIORegion early exit. + // When a group is zoomed (displayContents()==true), edge variables may + // lie outside the I/O region strip. Without this ordering, clicking on + // them returns ClickType::outside and the canvas falls back to dragging + // the whole group instead of initiating wire dragging. Fixes issue #610. if (auto item=select(x,y)) return item->clickType(x,y); + if (displayContents() && inIORegion(x,y)==IORegion::none) + return ClickType::outside; if ((abs(x-this->x())y())& vars, cairo_t* cairo, - float x) const + // Helper to compute edge variable layout (world-space positions) radially + static void layoutEdgeVariables(const vector& vars, float xEdge, + const Item* groupItem, double groupRotation) { float top=0, bottom=0; - const double angle=rotation() * M_PI / 180.0; for (size_t i=0; izoomFactor(); auto t=v->bb.top()*z, b=v->bb.bottom()*z; - if (i>0) y = i%2? top-b: bottom-t; - v->moveTo(r.x(x,y)+this->x(), r.y(x,y)+this->y()); - const cairo::CairoSave cs(cairo); - cairo_translate(cairo,x,y); - // cairo context is already rotated, so antirotate - cairo_rotate(cairo,-angle); - v->rotation(rotation()); - v->draw(cairo); + if (i>0) yOff = i%2? top-b: bottom-t; + v->moveTo(r.x(xEdge,yOff)+groupItem->x(), r.y(xEdge,yOff)+groupItem->y()); + v->rotation(groupRotation); if (i==0) { bottom=b; @@ -1072,6 +1072,41 @@ namespace minsky } } + void Group::draw1edge(const vector& vars, cairo_t* cairo, + float xEdge) const + { + layoutEdgeVariables(vars, xEdge, this, rotation()); + + // Iterate again solely to perform drawing steps. + const double angle=rotation() * M_PI / 180.0; + float top=0, bottom=0; + for (size_t i=0; izoomFactor(); + 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(); + else + bottom += v->height(); + } + } + void Group::drawEdgeVariables(cairo_t* cairo) const { float left, right; margins(left,right); @@ -1180,13 +1215,45 @@ namespace minsky return rotFactor; } + void Group::positionEdgeVariables() const + { + // Replicates the positioning logic of draw1edge() without requiring a + // Cairo context. Edge I/O variable world-space positions are only set + // during a draw() call (via moveTo inside draw1edge()). Calling this + // before hit-testing ensures RenderVariable::inImage() sees the correct + // positions. Fixes issue #610 (unable to pull wire from group output). + float leftMargin, rightMargin; + margins(leftMargin, rightMargin); + const float z = zoomFactor(); + + layoutEdgeVariables(inVariables, -0.5f * (iWidth() * z - leftMargin), this, rotation()); + layoutEdgeVariables(outVariables, 0.5f * (iWidth() * z - rightMargin), this, rotation()); + } + ItemPtr Group::select(float x, float y) const { + // Short-circuit: no edge variables to hit-test. + if (inVariables.empty() && outVariables.empty()) + return nullptr; + + // Short-circuit: avoid the expensive positionEdgeVariables() call when + // (x,y) is clearly outside the expanded group bounding box. Using + // w+h as the conservative AABB half-extent covers all rotation angles. + const float z = zoomFactor(); + const float w = 0.5f * iWidth() * z; + const float h = 0.5f * iHeight() * z + topMargin * z; + const float maxExtent = w + h; + if (abs(x - this->x()) > maxExtent || abs(y - this->y()) > maxExtent) + return nullptr; + + // 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) - if (RenderVariable(*v).inImage(x,y)) + if (RenderVariable(*v).inImage(x,y) || v->clickType(x,y) == ClickType::onPort) return v; for (auto& v: outVariables) - if (RenderVariable(*v).inImage(x,y)) + if (RenderVariable(*v).inImage(x,y) || v->clickType(x,y) == ClickType::onPort) return v; return nullptr; } diff --git a/model/group.h b/model/group.h index d339efe3b..e9e043ce4 100644 --- a/model/group.h +++ b/model/group.h @@ -278,6 +278,9 @@ namespace minsky /// draw notches in the I/O region to indicate dockability of /// variables there void drawIORegion(cairo_t*) const; + /// position edge I/O variables at their correct rendered locations + /// without requiring a Cairo context (used by select() for hit-testing) + void positionEdgeVariables() const; /// move all items from source to this void moveContents(Group& source); diff --git a/test/testCanvas.cc b/test/testCanvas.cc index 1068c6ac8..b79462cb4 100644 --- a/test/testCanvas.cc +++ b/test/testCanvas.cc @@ -87,3 +87,84 @@ TEST_F(CanvasTest, defaultPlotOptions) EXPECT_EQ(originalPlot->palette.back().width, plotAfterDefaultsApplied->palette.back().width); EXPECT_EQ(originalPlot->palette.back().dashStyle, plotAfterDefaultsApplied->palette.back().dashStyle); } + +/// Regression test for issue #610: unable to pull wire from group output. +/// Verifies that Group::select() correctly finds edge I/O variables even +/// without a preceding draw() call, by using positionEdgeVariables(). +TEST(GroupIOTest, groupIOVarSelect) +{ + // Build a standalone group with an output variable. + auto grp = make_shared(); + grp->self = grp; + grp->moveTo(200, 200); + grp->iWidth(100); + grp->iHeight(100); + + // Add an output variable to the group (populates outVariables). + grp->addOutputVar(); + ASSERT_EQ(1u, grp->outVariables.size()); + + // Update bounding box for the output variable so margins() works. + auto& outVar = *grp->outVariables[0]; + outVar.bb.update(outVar); + + // Compute the expected output-variable x position (right edge midpoint): + // This replicates the draw1edge() math used by positionEdgeVariables(). + float leftMargin, rightMargin; + grp->margins(leftMargin, rightMargin); + const float z = grp->zoomFactor(); + const float xEdge = 0.5f * (grp->iWidth() * z - rightMargin); + // First variable (i==0): Rotate is identity for zero rotation, yOff == 0. + const float expectedX = grp->x() + xEdge; + const float expectedY = grp->y(); + + // Before the fix, select() used stale positions (0,0) so this would return + // nullptr. After the fix, positionEdgeVariables() is called first. + auto found = grp->select(expectedX, expectedY); + EXPECT_NE(nullptr, found) + << "select() should find the output variable without a prior draw() call (issue #610)"; + if (found) + { + EXPECT_EQ(grp->outVariables[0].get(), found.get()) + << "select() should return the output variable, not some other item"; + } +} + +/// Regression test to verify the zoomed wire-pull path works. +TEST(GroupIOTest, groupIOVarSelectZoomed) +{ + // Build a standalone group with an output variable. + auto grp = make_shared(); + grp->self = grp; + grp->moveTo(200, 200); + grp->iWidth(100); + grp->iHeight(100); + + // Set a non-default zoom + grp->setZoom(2.5f); + + grp->addOutputVar(); + ASSERT_EQ(1u, grp->outVariables.size()); + + // Update bounding box for the output variable so margins() works. + auto& outVar = *grp->outVariables[0]; + outVar.bb.update(outVar); + + float leftMargin, rightMargin; + grp->margins(leftMargin, rightMargin); + const float z = grp->zoomFactor(); + const float xEdge = 0.5f * (grp->iWidth() * z - rightMargin); + const float expectedX = grp->x() + xEdge; + const float expectedY = grp->y(); + + auto found = grp->select(expectedX, expectedY); + EXPECT_NE(nullptr, found) + << "select() should find the output variable at a non-default zoom"; + + // Since `draw()` hasn't positioned the ports dynamically, we check the exact + // underlying static port coordinates of the found variable after positions are set. + auto outPort = outVar.ports(0).lock(); + EXPECT_EQ(ClickType::onPort, grp->clickType(outPort->x(), outPort->y())) + << "clickType() on group should yield onPort at a non-default zoom for wire-pulling"; +} +