Skip to content

Conversation

@pgallino
Copy link
Collaborator

close #201

@pgallino pgallino self-assigned this May 13, 2025
@pgallino pgallino marked this pull request as ready for review May 14, 2025 02:43
Comment on lines 502 to 509
if (device && !device.visible) {
const edges = this.getConnections(nodeId);
for (const e of edges) {
if (!visitedEdges.has(e)) {
queue.push(e);
}
}
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (device && !device.visible) {
const edges = this.getConnections(nodeId);
for (const e of edges) {
if (!visitedEdges.has(e)) {
queue.push(e);
}
}
}
if (device && !device.visible) {
continue;
}
const edges = this.getConnections(nodeId);
for (const e of edges) {
if (!visitedEdges.has(e)) {
queue.push(e);
}
}

Copy link
Collaborator Author

@pgallino pgallino May 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the original code, the line:

if (device && !device.visible) {
  const edges = this.getConnections(nodeId);
  for (const e of edges) {
    if (!visitedEdges.has(e)) {
      queue.push(e);
    }
  }
}

is intentionally written this way because non-visible devices are the ones that connect multiple edges into larger "multi-edges".
The algorithm should only traverse through non-visible devices to find all edges that are part of the same multi-edge group.

If you were to use continue when a device is not visible (i.e., if (device && !device.visible) continue;), you would skip expanding through those invisible nodes, and the algorithm would not correctly join all related edges.

If you want to skip visible devices (so you only traverse through invisible ones), the correct condition would be:

if (device && device.visible) {
  continue;
}

This way, you only expand through invisible devices, which is necessary to correctly find all edges that are logically connected through hidden nodes.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I forgot to invert the checks. I meant that we should use early continues here, to reduce the amount of indentation.

@pgallino pgallino merged commit 0b7626b into main May 15, 2025
3 checks passed
@pgallino pgallino deleted the fix/Highlight-merged-edges branch May 15, 2025 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Highlight all merged edges and unify information when selecting a combined edge after hiding a device

3 participants