Prevent the creation of invalid hops in a visual manner #6281

Author: nadment

engine/src/main/java/org/apache/hop/pipeline/PipelineMeta.java

@@ -581,10 +581,10 @@ public void setTransform(int i, TransformMeta transformMeta) {
    * metadata at the specified index to the specified meta-data object.
    *
    * @param i The index into the hops list
-   * @param hi The hop meta-data to set
+   * @param hop The hop meta-data to set
    */
-  public void setPipelineHop(int i, PipelineHopMeta hi) {
-    hops.set(i, hi);
+  public void setPipelineHop(int i, PipelineHopMeta hop) {
+    hops.set(i, hop);
     clearCaches();
   }
 
@@ -649,15 +649,12 @@ public TransformMeta findTransform(String name, TransformMeta exclude) {
    * Searches the list of hops for a hop with a certain name.
    *
    * @param name The name of the hop to look for
-   * @return The hop information or null if nothing was found.
+   * @return The hop found or null if nothing was found.
    */
   public PipelineHopMeta findPipelineHop(String name) {
-    int i;
-
-    for (i = 0; i < nrPipelineHops(); i++) {
-      PipelineHopMeta hi = getPipelineHop(i);
-      if (hi.toString().equalsIgnoreCase(name)) {
-        return hi;
+    for (PipelineHopMeta hop : hops) {
+      if (hop.toString().equalsIgnoreCase(name)) {
+        return hop;
       }
     }
     return null;
@@ -667,13 +664,14 @@ public PipelineHopMeta findPipelineHop(String name) {
    * Search all hops for a hop where a certain transform is at the start.
    *
    * @param fromTransform The transform at the start of the hop.
-   * @return The hop or null if no hop was found.
+   * @return The first hop found or null if nothing was found.
    */
   public PipelineHopMeta findPipelineHopFrom(TransformMeta fromTransform) {
-    for (PipelineHopMeta hop : hops) {
-      if (hop.getFromTransform() != null
-          && hop.getFromTransform().equals(fromTransform)) { // return the first
-        return hop;
+    if (fromTransform != null) {
+      for (PipelineHopMeta hop : hops) {
+        if (fromTransform.equals(hop.getFromTransform())) { // return the first
+          return hop;
+        }
       }
     }
     return null;
@@ -689,11 +687,11 @@ public List<PipelineHopMeta> findAllPipelineHopFrom(TransformMeta fromTransform)
   /**
    * Find a certain hop in the pipeline.
    *
-   * @param hi The hop information to look for.
+   * @param hop The hop information to look for.
    * @return The hop or null if no hop was found.
    */
-  public PipelineHopMeta findPipelineHop(PipelineHopMeta hi) {
-    return findPipelineHop(hi.getFromTransform(), hi.getToTransform());
+  public PipelineHopMeta findPipelineHop(PipelineHopMeta hop) {
+    return findPipelineHop(hop.getFromTransform(), hop.getToTransform());
   }
 
   /**
@@ -712,13 +710,13 @@ public PipelineHopMeta findPipelineHop(TransformMeta from, TransformMeta to) {
    *
    * @param from The transform at the start of the hop.
    * @param to The transform at the end of the hop.
-   * @param disabledToo the disabled too
+   * @param includeDisabled include disabled hop
    * @return The hop or null if no hop was found.
    */
   public PipelineHopMeta findPipelineHop(
-      TransformMeta from, TransformMeta to, boolean disabledToo) {
+      TransformMeta from, TransformMeta to, boolean includeDisabled) {
     for (PipelineHopMeta hop : hops) {
-      if ((hop.isEnabled() || disabledToo)
+      if ((hop.isEnabled() || includeDisabled)
           && hop.getFromTransform() != null
           && hop.getToTransform() != null
           && hop.getFromTransform().equals(from)
@@ -733,13 +731,14 @@ public PipelineHopMeta findPipelineHop(
    * Search all hops for a hop where a certain transform is at the end.
    *
    * @param toTransform The transform at the end of the hop.
-   * @return The hop or null if no hop was found.
+   * @return The hop or null if nothing was found.
    */
   public PipelineHopMeta findPipelineHopTo(TransformMeta toTransform) {
-    for (PipelineHopMeta hop : hops) {
-      if (hop.getToTransform() != null
-          && hop.getToTransform().equals(toTransform)) { // Return the first!
-        return hop;
+    if (toTransform != null) {
+      for (PipelineHopMeta hop : hops) {
+        if (toTransform.equals(hop.getToTransform())) { // Return the first!
+          return hop;
+        }
       }
     }
     return null;

engine/src/main/java/org/apache/hop/pipeline/PipelinePainter.java

@@ -323,15 +323,23 @@ private void drawPipeline() throws HopException {
       drawTransformPerformanceTable(transformMeta);
     }
 
-    // Display an icon on the indicated location signaling to the user that the transform in
-    // question does not accept input
+    // Display a red cross on the indicated location signaling to the user that the transform in
+    // question does not accept input or is not a good candidate for a hop (duplicate hop or loop)
     //
     if (noInputTransform != null) {
       gc.setLineWidth(2);
       gc.setForeground(EColor.RED);
       Point n = noInputTransform.getLocation();
-      gc.drawLine(n.x - 5, n.y - 5, n.x + iconSize + 10, n.y + iconSize + 10);
-      gc.drawLine(n.x - 5, n.y + iconSize + 5, n.x + iconSize + 5, n.y - 5);
+      gc.drawLine(
+          round(offset.x + n.x - 1),
+          round(offset.y + n.y - 1),
+          round(offset.x + n.x + iconSize + 1),
+          round(offset.y + n.y + iconSize + 1));
+      gc.drawLine(
+          round(offset.x + n.x - 1),
+          round(offset.y + n.y + iconSize + 1),
+          round(offset.x + n.x + iconSize + 1),
+          round(offset.y + n.y - 1));
     }
 
     try {

engine/src/main/java/org/apache/hop/workflow/WorkflowMeta.java

@@ -662,7 +662,7 @@ public ActionMeta getAction(int i) {
   /**
    * Adds the action.
    *
-   * @param action the je
+   * @param action the action meta to add
    */
   public void addAction(ActionMeta action) {
     workflowActions.add(action);
@@ -673,7 +673,7 @@ public void addAction(ActionMeta action) {
   /**
    * Adds the workflow hop.
    *
-   * @param hop the hi
+   * @param hop the workflow hop meta to add
    */
   public void addWorkflowHop(WorkflowHopMeta hop) {
     workflowHops.add(hop);
@@ -683,23 +683,23 @@ public void addWorkflowHop(WorkflowHopMeta hop) {
   /**
    * Adds the action.
    *
-   * @param p the p
-   * @param action the si
+   * @param index index at which the specified action is to be inserted
+   * @param action the action meta to add
    */
-  public void addAction(int p, ActionMeta action) {
-    workflowActions.add(p, action);
+  public void addAction(int index, ActionMeta action) {
+    workflowActions.add(index, action);
     changedActions = true;
   }
 
   /**
    * Adds the workflow hop.
    *
-   * @param p the p
-   * @param hop the hi
+   * @param index index at which the specified hop is to be inserted
+   * @param hop the workflow hop meta to add
    */
-  public void addWorkflowHop(int p, WorkflowHopMeta hop) {
+  public void addWorkflowHop(int index, WorkflowHopMeta hop) {
     try {
-      workflowHops.add(p, hop);
+      workflowHops.add(index, hop);
     } catch (IndexOutOfBoundsException e) {
       workflowHops.add(hop);
     }
@@ -709,10 +709,10 @@ public void addWorkflowHop(int p, WorkflowHopMeta hop) {
   /**
    * Removes the action.
    *
-   * @param i the i
+   * @param index the index of the action to be removed
    */
-  public void removeAction(int i) {
-    ActionMeta deleted = workflowActions.remove(i);
+  public void removeAction(int index) {
+    ActionMeta deleted = workflowActions.remove(index);
     if (deleted != null) {
       // give transform a chance to cleanup
       deleted.setParentWorkflowMeta(null);
@@ -791,8 +791,8 @@ public ActionMeta findAction(String name) {
   /**
    * Find workflow hop.
    *
-   * @param name the name
-   * @return the workflow hop meta
+   * @param name the name of the hop to look for
+   * @return the workflow hop meta or null if nothing was found.
    */
   public WorkflowHopMeta findWorkflowHop(String name) {
     for (WorkflowHopMeta hop : workflowHops) {
@@ -806,18 +806,15 @@ public WorkflowHopMeta findWorkflowHop(String name) {
   }
 
   /**
-   * Find workflow hop from.
+   * Find the first workflow hop from.
    *
-   * @param action the action meta
-   * @return the workflow hop meta
+   * @param action the action meta at the start of the hop.
+   * @return the first hop found or null if nothing was found.
    */
   public WorkflowHopMeta findWorkflowHopFrom(ActionMeta action) {
     if (action != null) {
       for (WorkflowHopMeta hop : workflowHops) {
-
-        // Return the first we find!
-        //
-        if (hop != null && (hop.getFromAction() != null) && hop.getFromAction().equals(action)) {
+        if (action.equals(hop.getFromAction())) {
           return hop;
         }
       }
@@ -828,9 +825,9 @@ public WorkflowHopMeta findWorkflowHopFrom(ActionMeta action) {
   /**
    * Find workflow hop.
    *
-   * @param from the from
-   * @param to the to
-   * @return the workflow hop meta
+   * @param from the action meta at the start of the hop
+   * @param to the to action meta at the end of the hop.
+   * @return the workflow hop meta or null if nothing was found.
    */
   public WorkflowHopMeta findWorkflowHop(ActionMeta from, ActionMeta to) {
     return findWorkflowHop(from, to, false);
@@ -839,15 +836,14 @@ public WorkflowHopMeta findWorkflowHop(ActionMeta from, ActionMeta to) {
   /**
    * Find workflow hop.
    *
-   * @param from the from
-   * @param to the to
-   * @param includeDisabled the include disabled
-   * @return the workflow hop meta
+   * @param from the action meta at the start of the hop
+   * @param to the to action meta at the end of the hop.
+   * @param includeDisabled include disabled hop
+   * @return the workflow hop meta or null if nothing was found.
    */
   public WorkflowHopMeta findWorkflowHop(ActionMeta from, ActionMeta to, boolean includeDisabled) {
     for (WorkflowHopMeta hop : workflowHops) {
       if ((hop.isEnabled() || includeDisabled)
-          && hop != null
           && hop.getFromAction() != null
           && hop.getToAction() != null
           && hop.getFromAction().equals(from)
@@ -859,16 +855,17 @@ public WorkflowHopMeta findWorkflowHop(ActionMeta from, ActionMeta to, boolean i
   }
 
   /**
-   * Find workflow hop to.
+   * Find the first workflow hop to.
    *
-   * @param actionMeta the action meta
-   * @return the workflow hop meta
+   * @param action the to action meta at the end of the hop.
+   * @return the first workflow hop meta or null if nothing was found.
    */
-  public WorkflowHopMeta findWorkflowHopTo(ActionMeta actionMeta) {
-    for (WorkflowHopMeta hop : workflowHops) {
-      if (hop != null && hop.getToAction() != null && hop.getToAction().equals(actionMeta)) {
-        // Return the first!
-        return hop;
+  public WorkflowHopMeta findWorkflowHopTo(ActionMeta action) {
+    if (action != null) {
+      for (WorkflowHopMeta hop : workflowHops) {
+        if (action.equals(hop.getToAction())) {
+          return hop;
+        }
       }
     }
     return null;

engine/src/main/java/org/apache/hop/workflow/WorkflowPainter.java

@@ -194,23 +194,24 @@ private void drawActions() throws HopException {
       drawAction(actionMeta);
     }
 
-    // Display an icon on the indicated location signaling to the user that the action in
-    // question does not accept input
+    // Display a red cross on the indicated location signaling to the user that the action in
+    // question does not accept input or is not a good candidate for a hop (duplicate hop or
+    // workflow loop)
     //
     if (noInputAction != null) {
       gc.setLineWidth(2);
       gc.setForeground(EColor.RED);
       Point n = noInputAction.getLocation();
       gc.drawLine(
-          round(offset.x + n.x - 5),
-          round(offset.y + n.y - 5),
-          round(offset.x + n.x + iconSize + 5),
-          round(offset.y + n.y + iconSize + 5));
+          round(offset.x + n.x - 1),
+          round(offset.y + n.y - 1),
+          round(offset.x + n.x + iconSize + 1),
+          round(offset.y + n.y + iconSize + 1));
       gc.drawLine(
-          round(offset.x + n.x - 5),
-          round(offset.y + n.y + iconSize + 5),
-          round(offset.x + n.x + iconSize + 5),
-          round(offset.y + n.y - 5));
+          round(offset.x + n.x - 1),
+          round(offset.y + n.y + iconSize + 1),
+          round(offset.x + n.x + iconSize + 1),
+          round(offset.y + n.y - 1));
     }
 
     try {