Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix dangling node connection #8902

Merged
merged 3 commits into from
Feb 5, 2024
Merged

Conversation

vitvakatu
Copy link
Contributor

Pull Request Description

Fixes #8871

The issue was caused by invalid port registration. Because of the existing context switch expression (which is not visible in GUI), the port incorrectly considered itself to belong to another node. This happened because the port was only aware of the visible part of the node’s AST and considered it the whole node.

There are two fixes in this PR. Either of them fixes the issue, and they are both implemented for robustness:

  1. We provide nodeId information to the widget tree, so it no longer assumes the node ID from AST.
  2. The order of checks in getPortNodeId is swapped. Now we first search by AST, only then try to look up the port. It makes sense to me because the AST is a single root of truth, and we should only rely on registered ports if AST does not exist (which happens for unconnected ports).

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

@vitvakatu vitvakatu added CI: No changelog needed Do not require a changelog entry for this PR. -gui labels Jan 30, 2024
@vitvakatu vitvakatu self-assigned this Jan 30, 2024
@@ -418,7 +418,7 @@ export const useGraphStore = defineStore('graph', () => {
}

function getPortNodeId(id: PortId): NodeId | undefined {
return getPortPrimaryInstance(id)?.nodeId ?? db.getExpressionNodeId(id as string as Ast.AstId)
return db.getExpressionNodeId(id as string as Ast.AstId) ?? getPortPrimaryInstance(id)?.nodeId
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about this change. If we have port instances, and they define their nodeId, they could have a reason to change it from AST?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They don’t really “define” their nodeid, they use the node id derived from AST. In which case will they change it?

Copy link
Contributor

@farmaazon farmaazon Jan 31, 2024

Choose a reason for hiding this comment

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

so, the node id is derived from AST at some point, but not necessarily from Port Id. PortId is a broader than just AstId. Technically (even if it's an awful idea), some widget could create portIds which also resembles AstId, but are not bound to them. In that case, I would say the port information from PortId is more accurate than what we deduce from AstId. getExpressionNodeId feels more like a fallback than the main route.

Of course this all is nitpicking because it will probably make no difference in real scenarios.

@vitvakatu vitvakatu added the CI: Ready to merge This PR is eligible for automatic merge label Feb 1, 2024
@mwu-tow mwu-tow merged commit 801cc7b into develop Feb 5, 2024
26 of 28 checks passed
@mwu-tow mwu-tow deleted the wip/vitvakatu/fix-unconnected-edge branch February 5, 2024 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-gui CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dangling node connection after opening a project
3 participants