Put deferred "insert" operators in correct place

This rewires the deferred "insertion" operators (operators that are
being inserted somewhere in an existing pipeline) so that they visually
appear where they are going to actually be placed when they execute. Before,
they would appear as if a new branch was being formed, but then move to their
insertion location when applied.

When "Cancel" is clicked, or escape/x button so that the dialog is closed, the
connected operators are rewired according to their initial wiring (before the
operator was added), and the pipeline is not re-executed.

Signed-off-by: Patrick Avery <patrick.avery@kitware.com>

Author: psavery

tomviz/AddPythonSourceReaction.cxx

@@ -40,7 +40,7 @@ void AddPythonSourceReaction::onTriggered()
 
   // Add the source to the pipeline before opening the dialog. This is
   // what gives us the cancel-rollback symmetry with transform
-  // insertion: NodeEditDialog::onCancel removes the node from the
+  // insertion: NodeEditDialog::reject() removes the node from the
   // pipeline (and deletes it). On OK we finish the standard source
   // scaffolding (sinks, color map, execute) via completeSourceSetup.
   LoadDataReaction::addSourceToPipeline(source);

tomviz/TransformUtils.cxx

@@ -18,6 +18,7 @@
 #include "pipeline/TransformNode.h"
 
 #include <QApplication>
+#include <QSet>
 #include <QtDebug>
 
 namespace tomviz {
@@ -43,6 +44,49 @@ static pipeline::OutputPort* findCompatibleOutputPort(
   return nullptr;
 }
 
+/// All nodes reachable downstream from @a start (inclusive), following output
+/// links.  This is exactly the set Node::markStale() cascades over when @a
+/// start is marked stale, so it is the set whose state we must snapshot to be
+/// able to undo an eager insertion.
+static QSet<pipeline::Node*> downstreamClosure(pipeline::Node* start)
+{
+  QSet<pipeline::Node*> result;
+  if (!start) {
+    return result;
+  }
+  QList<pipeline::Node*> stack;
+  stack.append(start);
+  while (!stack.isEmpty()) {
+    auto* node = stack.takeLast();
+    if (result.contains(node)) {
+      continue;
+    }
+    result.insert(node);
+    for (auto* out : node->outputPorts()) {
+      for (auto* link : out->links()) {
+        if (link->to() && link->to()->node()) {
+          stack.append(link->to()->node());
+        }
+      }
+    }
+  }
+  return result;
+}
+
+/// Snapshot the node and output-port states of @a nodes into @a deferred so
+/// they can be restored verbatim if an eager insertion is canceled.  Must be
+/// called before the insertion mutates the pipeline.
+static void captureStates(const QSet<pipeline::Node*>& nodes,
+                          pipeline::DeferredLinkInfo& deferred)
+{
+  for (auto* node : nodes) {
+    deferred.nodeStates.append({ node, node->state() });
+    for (auto* out : node->outputPorts()) {
+      deferred.portStaleStates.append({ out, out->isStale() });
+    }
+  }
+}
+
 /// Append a transform at the given targetPort, moving sink/group links to a
 /// compatible output of the new transform.  Sinks with no compatible output
 /// on the new transform are left connected to @a targetPort.
@@ -80,17 +124,32 @@ static void appendTransformAtPort(
   }
 }
 
-/// Deferred variant: adds node and input link only, returns deferred info
-/// for the output links to be completed later.
+/// Deferred variant of appendTransformAtPort: performs the full append
+/// eagerly (so the preview shows the final topology) and returns the info
+/// needed to undo it if the user cancels.
 static pipeline::DeferredLinkInfo appendTransformAtPortDeferred(
   pipeline::Pipeline* pip,
   pipeline::TransformNode* transform,
   pipeline::OutputPort* targetPort)
 {
+  // Snapshot the downstream subtree before mutating anything so a cancel can
+  // restore it exactly (the sink moves below mark it stale).
+  pipeline::DeferredLinkInfo deferred;
+  captureStates(downstreamClosure(targetPort->node()), deferred);
+
   pip->addNode(transform);
   pip->createLink(targetPort, transform->inputPorts()[0]);
 
-  pipeline::DeferredLinkInfo deferred;
+  // Move terminal (sink) links from targetPort onto the new transform's
+  // compatible outputs now, instead of deferring it to commit, so the preview
+  // shows the sinks already routed through the transform.  Record each
+  // original link so it can be recreated on cancel.
+  struct Move
+  {
+    pipeline::Link* link;
+    pipeline::OutputPort* newOut;
+  };
+  QList<Move> moves;
   for (auto* link : targetPort->links()) {
     if (link->to()->node() == transform) {
       continue; // skip the link we just created
@@ -102,8 +161,13 @@ static pipeline::DeferredLinkInfo appendTransformAtPortDeferred(
     if (!newOut) {
       continue;
     }
-    deferred.linksToBreak.append({ targetPort, link->to() });
-    deferred.linksToCreate.append({ newOut, link->to() });
+    moves.append({ link, newOut });
+  }
+  for (const auto& m : moves) {
+    auto* sinkInput = m.link->to();
+    deferred.linksToRestore.append({ targetPort, sinkInput });
+    pip->removeLink(m.link);
+    pip->createLink(m.newOut, sinkInput);
   }
   return deferred;
 }
@@ -140,7 +204,9 @@ static void insertTransformAtLink(
   pip->createLink(transform->outputPorts()[0], toPort);
 }
 
-/// Deferred variant for insert-at-link.
+/// Deferred variant of insertTransformAtLink: performs the full insertion
+/// eagerly (so the preview shows the transform in its final, inserted
+/// position) and returns the info needed to undo it if the user cancels.
 static pipeline::DeferredLinkInfo insertTransformAtLinkDeferred(
   pipeline::Pipeline* pip,
   pipeline::TransformNode* transform,
@@ -149,12 +215,19 @@ static pipeline::DeferredLinkInfo insertTransformAtLinkDeferred(
   auto* fromPort = link->from();
   auto* toPort = link->to();
 
+  // Snapshot the downstream subtree before mutating anything so a cancel can
+  // restore it exactly (createLink() below marks this subtree stale).
+  pipeline::DeferredLinkInfo deferred;
+  captureStates(downstreamClosure(toPort->node()), deferred);
+  deferred.linksToRestore.append({ fromPort, toPort });
+
+  // Perform the full insertion now (break from->to, splice the transform in)
+  // rather than only connecting the input and deferring the rest to commit.
+  pip->removeLink(link);
   pip->addNode(transform);
   pip->createLink(fromPort, transform->inputPorts()[0]);
+  pip->createLink(transform->outputPorts()[0], toPort);
 
-  pipeline::DeferredLinkInfo deferred;
-  deferred.linksToBreak.append({ fromPort, toPort });
-  deferred.linksToCreate.append({ transform->outputPorts()[0], toPort });
   return deferred;
 }
 

tomviz/pipeline/DeferredLinkInfo.h

@@ -4,35 +4,61 @@
 #ifndef tomvizPipelineDeferredLinkInfo_h
 #define tomvizPipelineDeferredLinkInfo_h
 
+#include "InputPort.h"
+#include "Node.h"
+#include "NodeState.h"
+#include "OutputPort.h"
+
 #include <QList>
+#include <QPointer>
 
 namespace tomviz {
 namespace pipeline {
 
-class InputPort;
-class OutputPort;
-
-/// Captures deferred link operations for a pending transform insertion.
-/// When the transform is accepted, old links are removed and new links
-/// are created to complete the insertion.
+/// Captures everything needed to undo an eagerly-performed transform
+/// insertion if the user cancels the edit dialog.
+///
+/// The insertion is applied to the pipeline immediately so the preview shows
+/// the transform in its final, inserted position rather than branching off
+/// the upstream port.  If the user cancels, the new node is removed (which
+/// drops its own links) and this struct is used to put the pipeline back
+/// exactly as it was: the original links are recreated and the node/port
+/// states captured before the insertion are restored, so cancel is a true
+/// no-op and nothing is left spuriously stale.
+///
+/// For source-shaped insertions (no links to break) this is left empty; the
+/// dialog then just removes the node on cancel.
 struct DeferredLinkInfo
 {
   struct LinkEndpoints
   {
-    OutputPort* from = nullptr;
-    InputPort* to = nullptr;
+    QPointer<OutputPort> from;
+    QPointer<InputPort> to;
   };
 
-  /// Old links to remove on commit (looked up by matching from/to ports).
-  QList<LinkEndpoints> linksToBreak;
-
-  /// New output links to create from the new transform's output ports.
-  QList<LinkEndpoints> linksToCreate;
+  struct NodeStateSnapshot
+  {
+    QPointer<Node> node;
+    NodeState state;
+  };
 
-  bool isEmpty() const
+  struct PortStaleSnapshot
   {
-    return linksToBreak.isEmpty() && linksToCreate.isEmpty();
-  }
+    QPointer<OutputPort> port;
+    bool stale;
+  };
+
+  /// Original links to recreate on cancel.
+  QList<LinkEndpoints> linksToRestore;
+
+  /// Node states captured before the insertion, restored on cancel.
+  QList<NodeStateSnapshot> nodeStates;
+
+  /// Output-port stale flags captured before the insertion, restored on
+  /// cancel.
+  QList<PortStaleSnapshot> portStaleStates;
+
+  bool isEmpty() const { return linksToRestore.isEmpty(); }
 };
 
 } // namespace pipeline

tomviz/pipeline/NodeEditDialog.cxx

@@ -4,8 +4,10 @@
 #include "NodeEditDialog.h"
 
 #include "EditNodeWidget.h"
+#include "InputPort.h"
 #include "Link.h"
 #include "Node.h"
+#include "OutputPort.h"
 #include "Pipeline.h"
 
 #include "Utilities.h"
@@ -77,7 +79,7 @@ void NodeEditDialog::init()
   connect(m_buttonBox, &QDialogButtonBox::accepted, this,
           &NodeEditDialog::onOkay);
   connect(m_buttonBox, &QDialogButtonBox::rejected, this,
-          &NodeEditDialog::onCancel);
+          &NodeEditDialog::reject);
   connect(m_buttonBox->button(QDialogButtonBox::Apply), &QPushButton::clicked,
           this, &NodeEditDialog::onApply);
 
@@ -136,18 +138,40 @@ void NodeEditDialog::onOkay()
   accept();
 }
 
-void NodeEditDialog::onCancel()
+void NodeEditDialog::reject()
 {
   if (m_isNewInsertion && !m_insertionCompleted) {
-    // Remove the node and its links. For transforms the input link
-    // exists; for sources there are no inputs. Output links were
-    // never created, so the pipeline remains valid.
+    // The insertion was applied eagerly when the dialog opened.  Undo it so
+    // that cancel is a true no-op.  Removing the new node also drops its own
+    // input/output links (for sources there are none).
     m_pipeline->removeNode(m_node);
     m_node = nullptr;
+
+    // Recreate the original links that were broken to insert the node.
+    for (const auto& ep : m_deferred.linksToRestore) {
+      if (ep.from && ep.to) {
+        m_pipeline->createLink(ep.from, ep.to);
+      }
+    }
+
+    // createLink() above marks the affected downstream subtree stale.
+    // Restore the states captured before the insertion so nothing is left
+    // spuriously stale (which would otherwise force a needless re-run).
+    for (const auto& ps : m_deferred.portStaleStates) {
+      if (ps.port) {
+        ps.port->setStale(ps.stale);
+      }
+    }
+    for (const auto& ns : m_deferred.nodeStates) {
+      if (ns.node) {
+        ns.node->setStateNoCascade(ns.state);
+      }
+    }
+
     emit insertionCanceled();
   }
 
-  reject();
+  QDialog::reject();
 }
 
 void NodeEditDialog::showEvent(QShowEvent* event)
@@ -208,22 +232,10 @@ void NodeEditDialog::restoreGeometry()
 
 void NodeEditDialog::completeInsertion()
 {
-  // Break old links
-  for (const auto& ep : m_deferred.linksToBreak) {
-    // Find the actual Link* by matching from/to ports
-    for (auto* link : m_pipeline->links()) {
-      if (link->from() == ep.from && link->to() == ep.to) {
-        m_pipeline->removeLink(link);
-        break;
-      }
-    }
-  }
-
-  // Create new output links
-  for (const auto& ep : m_deferred.linksToCreate) {
-    m_pipeline->createLink(ep.from, ep.to);
-  }
-
+  // The insertion (node + link rewiring) was performed eagerly when the
+  // dialog opened, so the pipeline already has its final topology.  There is
+  // nothing left to rewire here -- just mark the insertion committed so the
+  // cancel path will not try to roll it back.
   m_insertionCompleted = true;
   m_isNewInsertion = false;
   emit insertionCompleted(m_node);

tomviz/pipeline/NodeEditDialog.h

@@ -21,9 +21,11 @@ class Pipeline;
 /// A dialog for configuring node parameters with Apply/OK/Cancel.
 ///
 /// Two modes:
-///   - **Insertion mode**: The node is being added. Apply/OK completes
-///     the insertion and executes the pipeline. Cancel removes the node
-///     (and any input/output links described by DeferredLinkInfo).
+///   - **Insertion mode**: The node has already been added and spliced into
+///     the pipeline (eagerly, so the preview shows its final position).
+///     Apply/OK executes the pipeline and commits. Cancel removes the node
+///     and restores the original links and node states captured in
+///     DeferredLinkInfo, so it is a true no-op.
 ///   - **Edit mode**: The node already exists. Apply/OK re-applies
 ///     parameters and executes. Cancel just closes the dialog.
 ///
@@ -48,6 +50,11 @@ class NodeEditDialog : public QDialog
 
   Node* node() const;
 
+  /// Overridden so that every cancel path -- the Cancel button, the Escape
+  /// key, and the window close button -- funnels through here and rolls back
+  /// an in-progress insertion.
+  void reject() override;
+
 signals:
   /// Emitted after Apply/OK completes an insertion.
   void insertionCompleted(Node* node);
@@ -58,7 +65,6 @@ class NodeEditDialog : public QDialog
 private slots:
   void onApply();
   void onOkay();
-  void onCancel();
 
 protected:
   void showEvent(QShowEvent* event) override;

tomviz/pipeline/PipelineStripWidget.cxx

@@ -1052,10 +1052,16 @@ Link* PipelineStripWidget::linkHitTest(const QPoint& pos) const
 {
   QPainterPathStroker stroker;
   stroker.setWidth(8.0);
-  for (auto& lg : m_linkGeometries) {
-    QPainterPath hitPath = stroker.createStroke(lg.path);
+  // Iterate in reverse so the topmost-painted link wins when hit areas
+  // overlap. paintConnections() draws m_linkGeometries front-to-back, so the
+  // last entry is on top. Links between non-adjacent nodes share a routing
+  // gutter and become collinear there, so a forward search would return a
+  // link drawn underneath the one the user actually clicked.
+  for (auto it = m_linkGeometries.rbegin(); it != m_linkGeometries.rend();
+       ++it) {
+    QPainterPath hitPath = stroker.createStroke(it->path);
     if (hitPath.contains(pos)) {
-      return lg.link;
+      return it->link;
     }
   }
   return nullptr;