Destructing a Connection propagates empty data even without an output node

[vhutter]

Not sure if the behavior is intended, but it seems to me that it's unnecessary. I assumed that propagating empty data is only useful if there was a non-empty propagation for the respective connection directly prior to the empty one.

Steps to reproduce:

1. Click to an input node of a data node.
2. Move the mouse anywhere, but don't connect the other end to an output node.
3. Release the mouse.

This will result in the connection's destructor firing a signal, that will be handled by the respective node's data model type.

My suggestion is changing Connection::~Connection from

Connection::
~Connection()
{
  if (complete())
  {
    connectionMadeIncomplete(*this);
  }

  propagateEmptyData();

  if (_inNode)
  {
    _inNode->nodeGraphicsObject().update();
  }

  if (_outNode)
  {
    _outNode->nodeGraphicsObject().update();
  }
}

to

Connection::
~Connection()
{
  if (complete())
  {
    connectionMadeIncomplete(*this);
  }

  if (_inNode)
  {
    _inNode->nodeGraphicsObject().update();
  }

  if (_outNode)
  {
    propagateEmptyData();
    _outNode->nodeGraphicsObject().update();
  }
}

@paceholder If you approve, I can make a quick PR.

[paceholder]

Hi, thanks for your suggestion.

Don't we already have the desired behavior? Let's look at the function call chain:

propagateEmptyData(); propagateData("empty");

and now

void
Connection::
propagateData(std::shared_ptr<NodeData> nodeData) const
{
  if (_inNode)     <<< Here we check that we actually had a node on the right hand side.
  {

   ....

    _inNode->propagateData(nodeData, _inPortIndex);
  }
}

Am I missing something?

It might first look misleading, but the _inNode is the one "on the right" and the _outNode is the one "on the left".

         out node                                  in node
    _________________                         _________________
   |                 | out port      in port |                 |
   |                 O - - - - - - - - - - - O                 |
   |                 |                       |                 |
   |_________________|                       |_________________|

[vhutter]

Hi,

I think you misunderstood the issue. I also made a typo in my description. In "Step 1", I meant "input port of a data node", so as you correctly displayed it on your diagram, the in port of the node on the right. Step 2 is actually unimportant. The main point is to create a half-connection, only with the input port attached to it, and releasing the mouse before connecting it to an output port (left side).

To give another demonstrative example, if you simply click on an input node 10 times (not even moving the mouse), the respective Node's corresponding NodeDataType::setInData will be triggered 10 times by Connection::propagateData (with nullptr as an argument). This is what I found unnecessary, since it would be nice if this method was only called when an actual data change happens, so in either of these 3 cases:

• valid data -> another valid data
• valid data -> nullptr
• nullptr -> valid data

and NOT in case nullptr -> nullptr, which is currently happening. I think that's redundant to handle in client code.

[paceholder]

I see your point now, thanks for the explanation. Let's fix it.

[vhutter]

I opened a PR for it: https://github.com/paceholder/nodeeditor/pull/308

[paceholder]

Thanks