Bug 610 - cannot pull wire from group.

gui-js/libs/shared/src/lib/backend/minsky.ts

@@ -1108,6 +1108,7 @@ export class Group extends Item {
   async portY(a1: number): Promise<number> {return this.$callMethod('portY',a1);}
   async ports(a1: number): Promise<object> {return this.$callMethod('ports',a1);}
   async portsSize(): Promise<number> {return this.$callMethod('portsSize');}
+  async positionEdgeVariables(): Promise<void> {return this.$callMethod('positionEdgeVariables');}
   async px(...args: number[]): Promise<number> {return this.$callMethod('px',...args);}
   async py(...args: number[]): Promise<number> {return this.$callMethod('py',...args);}
   async pz(...args: number[]): Promise<number> {return this.$callMethod('pz',...args);}
@@ -2031,6 +2032,7 @@ export class Selection extends CppClass {
   async portY(a1: number): Promise<number> {return this.$callMethod('portY',a1);}
   async ports(a1: number): Promise<object> {return this.$callMethod('ports',a1);}
   async portsSize(): Promise<number> {return this.$callMethod('portsSize');}
+  async positionEdgeVariables(): Promise<void> {return this.$callMethod('positionEdgeVariables');}
   async randomLayout(): Promise<void> {return this.$callMethod('randomLayout');}
   async relZoom(...args: number[]): Promise<number> {return this.$callMethod('relZoom',...args);}
   async removeDisplayPlot(): Promise<void> {return this.$callMethod('removeDisplayPlot');}

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;

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())<w && abs(y-this->y())<h+topMargin*z)) // check also if (x,y) is within top and bottom margins of group. for feature 88
       return ClickType::onItem;
     return ClickType::outside;
@@ -1040,26 +1045,21 @@ namespace minsky
 
   }
 
-  void Group::draw1edge(const vector<VariablePtr>& vars, cairo_t* cairo, 
-                        float x) const
+  // Helper to compute edge variable layout (world-space positions) radially
+  static void layoutEdgeVariables(const vector<VariablePtr>& 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; i<vars.size(); ++i)
       {
-        const Rotate r(rotation(),0,0);
+        const Rotate r(groupRotation,0,0);
         auto& v=vars[i];
-        float y=0;
+        float yOff=0;
         auto z=v->zoomFactor();
         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<VariablePtr>& 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; i<vars.size(); ++i)
+      {
+        auto& v=vars[i];
+
+        float yOff = 0;
+        auto z = v->zoomFactor();
+        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;
   }

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);     

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<Group>();
+    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<Group>();
+    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";
+}
+