Improve terminal node naming within building blocks to ensure no duplicates by adding a name of the nested component as a prefix

diylc/diylc-core/src/main/java/org/diylc/components/composite/CompositeComponent.java

@@ -28,8 +28,12 @@ import java.awt.Stroke;
 import java.awt.geom.Point2D;
 import java.awt.geom.Rectangle2D;
 import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
 import java.util.List;
+import java.util.Map;
 import java.util.Objects;
+import java.util.Set;
 
 import org.diylc.components.AbstractComponent;
 import org.diylc.components.AbstractTransparentComponent;
@@ -83,6 +87,7 @@ public class CompositeComponent extends AbstractTransparentComponent<String> {
   // serialized (transient).
   private transient int[] childOf;
   private transient int[] pointOf;
+  private transient String[] nodeNameOf;
 
   /**
    * Required no-arg constructor, used reflectively by {@code ComponentProcessor} and XStream.
@@ -111,8 +116,77 @@ public class CompositeComponent extends AbstractTransparentComponent<String> {
         idx++;
       }
     }
-    childOf = newChildOf;
+    nodeNameOf = buildNodeNames(newChildOf, newPointOf);
     pointOf = newPointOf;
+    // assigned last, it is what marks the index as built
+    childOf = newChildOf;
+  }
+
+  /**
+   * Resolves the node names the composite exposes. A child's own node name is kept unchanged
+   * whenever it is unique, so the pads of a custom board read "VCC" rather than "Pad3.VCC" - the
+   * pad's component name is an arbitrary leftover from the project the block was saved in.
+   * Clashing names are resolved in two passes: pins of multi-point children are qualified with the
+   * child's name ("R1.1", "R2.1"), then whatever still clashes, typically several pads sharing a
+   * name, gets a numeric suffix in save order ("GND_1", "GND_2").
+   *
+   * <p>Only sticky, named points take part. Non-sticky points never become netlist nodes, and a
+   * board's corner points would otherwise force a rename on a pad that shares their default
+   * numeric name.
+   */
+  private String[] buildNodeNames(int[] newChildOf, int[] newPointOf) {
+    String[] names = new String[newChildOf.length];
+    boolean[] terminal = new boolean[newChildOf.length];
+    for (int i = 0; i < names.length; i++) {
+      IDIYComponent<?> child = childComponents.get(newChildOf[i]);
+      names[i] = child.getControlPointNodeName(newPointOf[i]);
+      terminal[i] = names[i] != null && child.isControlPointSticky(newPointOf[i]);
+    }
+
+    Set<String> clashing = findClashingNames(names, terminal);
+    for (int i = 0; i < names.length; i++) {
+      IDIYComponent<?> child = childComponents.get(newChildOf[i]);
+      String childName = child.getName();
+      if (terminal[i] && clashing.contains(names[i]) && child.getControlPointCount() > 1
+          && childName != null && !childName.trim().isEmpty()) {
+        names[i] = childName.trim() + "." + names[i];
+      }
+    }
+
+    clashing = findClashingNames(names, terminal);
+    if (clashing.isEmpty()) {
+      return names;
+    }
+    Set<String> taken = new HashSet<String>();
+    for (int i = 0; i < names.length; i++) {
+      if (terminal[i] && !clashing.contains(names[i])) {
+        taken.add(names[i]);
+      }
+    }
+    Map<String, Integer> nextSuffix = new HashMap<String, Integer>();
+    for (int i = 0; i < names.length; i++) {
+      if (terminal[i] && clashing.contains(names[i])) {
+        int suffix = nextSuffix.getOrDefault(names[i], 1);
+        while (taken.contains(names[i] + "_" + suffix)) {
+          suffix++;
+        }
+        nextSuffix.put(names[i], suffix + 1);
+        names[i] = names[i] + "_" + suffix;
+        taken.add(names[i]);
+      }
+    }
+    return names;
+  }
+
+  private static Set<String> findClashingNames(String[] names, boolean[] terminal) {
+    Set<String> seen = new HashSet<String>();
+    Set<String> clashing = new HashSet<String>();
+    for (int i = 0; i < names.length; i++) {
+      if (terminal[i] && !seen.add(names[i])) {
+        clashing.add(names[i]);
+      }
+    }
+    return clashing;
   }
 
   /**
@@ -179,7 +253,7 @@ public class CompositeComponent extends AbstractTransparentComponent<String> {
   @Override
   public String getControlPointNodeName(int index) {
     buildIndexIfNeeded();
-    return childComponents.get(childOf[index]).getControlPointNodeName(pointOf[index]);
+    return nodeNameOf[index];
   }
 
   /**

diylc/diylc-core/src/main/java/org/diylc/presenter/BuildingBlockManager.java

@@ -164,10 +164,10 @@ public class BuildingBlockManager {
       return clones;
     }
 
-    // COMPOSITE mode: clone components keeping their saved names. Those names are namespaced
-    // under the composite's own name in the netlist (e.g. "ARD1.PH1.3"), so running them through
-    // createUniqueName against the whole project would produce meaningless drift and break
-    // netlist labels between two instances of the same block.
+    // COMPOSITE mode: clone components keeping their saved names. Those names surface in the
+    // composite's terminal names when pins clash (e.g. "BLK1.R1.1"), so running them through
+    // createUniqueName against the whole project would produce meaningless drift and make two
+    // instances of the same block resolve different netlist labels.
     List<IDIYComponent<?>> childClones = new ArrayList<IDIYComponent<?>>();
     for (IDIYComponent<?> c : components) {
       try {

diylc/diylc-core/src/test/java/org/diylc/components/composite/CompositeComponentTest.java

@@ -24,17 +24,20 @@ package org.diylc.components.composite;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 
 import java.awt.Graphics2D;
 import java.awt.geom.Point2D;
 import java.awt.image.BufferedImage;
+import java.util.Arrays;
 import java.util.UUID;
 
 import org.diylc.common.Percentage;
 import org.diylc.core.IDIYComponent;
 import org.diylc.netlist.Node;
 import org.diylc.serialization.ProjectFileManager;
+import org.diylc.testcomponents.OnePointTestComponent;
 import org.diylc.testcomponents.TwoPointTestComponent;
 import org.junit.Test;
 
@@ -55,6 +58,32 @@ public class CompositeComponentTest {
     return composite;
   }
 
+  private CompositeComponent customBoard(IDIYComponent<?>... children) {
+    CompositeComponent composite = new CompositeComponent();
+    composite.setName("BLK1");
+    composite.initBlockName("Custom Board");
+    composite.getChildComponents().addAll(Arrays.asList(children));
+    return composite;
+  }
+
+  private OnePointTestComponent pad(String name, String nodeName) {
+    return new OnePointTestComponent(name, nodeName, new Point2D.Double(0, 0));
+  }
+
+  // Stands in for a blank board: its corner points keep the default "1" and "2" node names but
+  // are not sticky.
+  private TwoPointTestComponent blankBoard() {
+    TwoPointTestComponent board = new TwoPointTestComponent("Board1", new Point2D.Double(0, 0),
+        new Point2D.Double(100, 100));
+    board.setSticky(0, false);
+    board.setSticky(1, false);
+    return board;
+  }
+
+  private TwoPointTestComponent twoPoint(String name) {
+    return new TwoPointTestComponent(name, new Point2D.Double(0, 0), new Point2D.Double(10, 0));
+  }
+
   @Test
   public void controlPointCountIsSumOverChildren() {
     CompositeComponent composite = twoChildComposite();
@@ -103,18 +132,94 @@ public class CompositeComponentTest {
   }
 
   @Test
-  public void nodeNamingProducesHierarchicalNetlistLabels() {
-    // Design decision D3: Node.toString() should read "<composite>.<child>.<local point name>"
+  public void clashingPinsOfMultiPointChildrenAreQualifiedWithTheChildName() {
     CompositeComponent composite = twoChildComposite();
 
-    Node r1FirstPoint = new Node(composite, 0);
-    Node r2FirstPoint = new Node(composite, 2);
-    Node r2SecondPoint = new Node(composite, 3);
-
-    assertEquals("BLK1.1", r1FirstPoint.toString());
-    assertEquals("BLK1.1", r2FirstPoint.toString());
+    assertEquals("R1.1", composite.getControlPointNodeName(0));
+    assertEquals("R1.2", composite.getControlPointNodeName(1));
+    assertEquals("BLK1.R2.1", new Node(composite, 2).toString());
     // local point index must reset per child, not keep growing with the flat composite index
-    assertEquals("BLK1.2", r2SecondPoint.toString());
+    assertEquals("BLK1.R2.2", new Node(composite, 3).toString());
+  }
+
+  @Test
+  public void uniquePadNamesPassThroughWithoutThePadName() {
+    CompositeComponent composite =
+        customBoard(blankBoard(), pad("Pad3", "VCC"), pad("Pad7", "SDA"));
+
+    assertEquals("VCC", composite.getControlPointNodeName(2));
+    assertEquals("SDA", composite.getControlPointNodeName(3));
+    assertEquals("BLK1.VCC", new Node(composite, 2).toString());
+  }
+
+  @Test
+  public void nonStickyPointsDoNotForceARename() {
+    CompositeComponent composite = customBoard(blankBoard(), pad("Pad1", "1"));
+
+    assertEquals("1", composite.getControlPointNodeName(2));
+  }
+
+  @Test
+  public void padsSharingANameGetNumericSuffixesInSaveOrder() {
+    CompositeComponent composite = customBoard(blankBoard(), pad("Pad1", "GND"),
+        pad("Pad2", "VCC"), pad("Pad3", "GND"), pad("Pad4", "GND"));
+
+    assertEquals("GND_1", composite.getControlPointNodeName(2));
+    assertEquals("VCC", composite.getControlPointNodeName(3));
+    assertEquals("GND_2", composite.getControlPointNodeName(4));
+    assertEquals("GND_3", composite.getControlPointNodeName(5));
+  }
+
+  @Test
+  public void suffixesSkipNamesThatAreAlreadyTaken() {
+    CompositeComponent composite =
+        customBoard(pad("Pad1", "GND"), pad("Pad2", "GND_1"), pad("Pad3", "GND"));
+
+    assertEquals("GND_2", composite.getControlPointNodeName(0));
+    assertEquals("GND_1", composite.getControlPointNodeName(1));
+    assertEquals("GND_3", composite.getControlPointNodeName(2));
+  }
+
+  @Test
+  public void padClashingWithAPinOfAMultiPointChildKeepsItsName() {
+    CompositeComponent composite = customBoard(pad("Pad1", "1"), twoPoint("R1"));
+
+    assertEquals("1", composite.getControlPointNodeName(0));
+    assertEquals("R1.1", composite.getControlPointNodeName(1));
+    assertEquals("2", composite.getControlPointNodeName(2));
+  }
+
+  @Test
+  public void childrenSharingANameAreStillDistinguished() {
+    // renaming in the property editor does not enforce uniqueness, so a block can hold two "R1"s
+    CompositeComponent composite = customBoard(twoPoint("R1"), twoPoint("R1"));
+
+    assertEquals("R1.1_1", composite.getControlPointNodeName(0));
+    assertEquals("R1.2_1", composite.getControlPointNodeName(1));
+    assertEquals("R1.1_2", composite.getControlPointNodeName(2));
+    assertEquals("R1.2_2", composite.getControlPointNodeName(3));
+  }
+
+  @Test
+  public void unnamedPointsAreNotTerminals() {
+    CompositeComponent composite =
+        customBoard(pad("Pad1", null), pad("Pad2", null), pad("Pad3", "VCC"));
+
+    assertNull(composite.getControlPointNodeName(0));
+    assertNull(composite.getControlPointNodeName(1));
+    assertEquals("VCC", composite.getControlPointNodeName(2));
+  }
+
+  @Test
+  public void instancesOfTheSameBlockResolveIdenticalNames() throws Exception {
+    CompositeComponent original = customBoard(blankBoard(), pad("Pad1", "GND"),
+        pad("Pad2", "GND"), twoPoint("R1"), twoPoint("R2"));
+
+    CompositeComponent clone = (CompositeComponent) original.clone();
+
+    for (int i = 0; i < original.getControlPointCount(); i++) {
+      assertEquals(original.getControlPointNodeName(i), clone.getControlPointNodeName(i));
+    }
   }
 
   @Test
@@ -204,7 +309,7 @@ public class CompositeComponentTest {
     assertEquals(original.getBlockName(), reloaded.getBlockName());
     for (int i = 0; i < original.getControlPointCount(); i++)
       assertEquals(original.getControlPoint(i), reloaded.getControlPoint(i));
-    assertEquals("BLK1.2", new Node(reloaded, 3).toString());
+    assertEquals("BLK1.R2.2", new Node(reloaded, 3).toString());
   }
 
   @Test

diylc/diylc-core/src/test/java/org/diylc/testcomponents/OnePointTestComponent.java

@@ -0,0 +1,107 @@
+/*
+
+    DIY Layout Creator (DIYLC).
+    Copyright (c) 2009-2025 held jointly by the individual authors.
+
+    This file is part of DIYLC.
+
+    DIYLC is free software: you can redistribute it and/or modify
+    it under the terms of the GNU General Public License as published by
+    the Free Software Foundation, either version 3 of the License, or
+    (at your option) any later version.
+
+    DIYLC is distributed in the hope that it will be useful,
+    but WITHOUT ANY WARRANTY; without even the implied warranty of
+    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+    GNU General Public License for more details.
+
+    You should have received a copy of the GNU General Public License
+    along with DIYLC.  If not, see <http://www.gnu.org/licenses/>.
+
+*/
+package org.diylc.testcomponents;
+
+import java.awt.Graphics2D;
+import java.awt.geom.Point2D;
+
+import org.diylc.components.AbstractComponent;
+import org.diylc.core.ComponentState;
+import org.diylc.core.IDIYComponent;
+import org.diylc.core.IDrawingObserver;
+import org.diylc.core.Project;
+import org.diylc.core.VisibilityPolicy;
+import org.diylc.core.annotations.ComponentDescriptor;
+
+/**
+ * Minimal single-point, sticky component with a settable node name, standing in for a solder pad
+ * or turret when testing how {@code CompositeComponent} resolves the terminal names of a custom
+ * board. A {@code null} node name marks the point as not being a netlist node, like a pad whose
+ * Node Name is left blank.
+ */
+@ComponentDescriptor(name = "One Point Test", category = "Test", author = "Test",
+    description = "test", instanceNamePrefix = "OP", zOrder = IDIYComponent.COMPONENT)
+public class OnePointTestComponent extends AbstractComponent<Void> {
+
+  private static final long serialVersionUID = 1L;
+
+  private Point2D point;
+  private String nodeName;
+
+  public OnePointTestComponent() {
+    this("", null, new Point2D.Double(0, 0));
+  }
+
+  public OnePointTestComponent(String name, String nodeName, Point2D point) {
+    setName(name);
+    this.nodeName = nodeName;
+    this.point = point;
+  }
+
+  @Override
+  public int getControlPointCount() {
+    return 1;
+  }
+
+  @Override
+  public Point2D getControlPoint(int index) {
+    return point;
+  }
+
+  @Override
+  public void setControlPoint(Point2D point, int index) {
+    this.point = point;
+  }
+
+  @Override
+  public boolean isControlPointSticky(int index) {
+    return true;
+  }
+
+  @Override
+  public VisibilityPolicy getControlPointVisibilityPolicy(int index) {
+    return VisibilityPolicy.ALWAYS;
+  }
+
+  @Override
+  public String getControlPointNodeName(int index) {
+    return nodeName;
+  }
+
+  @Override
+  public Void getValue() {
+    return null;
+  }
+
+  @Override
+  public void setValue(Void value) {
+  }
+
+  @Override
+  public void draw(Graphics2D g2d, ComponentState componentState, boolean outlineMode,
+      Project project, IDrawingObserver drawingObserver) {
+  }
+
+  @Override
+  public void drawIcon(Graphics2D g2d, int width, int height) {
+  }
+}