Delete-and-connect-around operation #14403
Conversation
farmaazon
requested review from
AdRiley,
Frizi,
PabloBuchu,
indiv0,
kazcw,
somebody1234 and
vitvakatu
as code owners
| const findMainSourceOf = (selectedNode: NodeId) => { | ||
| const findResult = (): Identifier | None => { | ||
| const alreadyKnown = knownMainSourceIdentifier.get(selectedNode) | ||
| if (alreadyKnown) return alreadyKnown | ||
| const selfPort = graphDb.nodeIdToNode.get(selectedNode)?.primaryApplication.selfArgument | ||
| if (!selfPort) return none | ||
| const mainSourceId = set.first(graphDb.connections.reverseLookup(selfPort)) | ||
| if (!mainSourceId) return none | ||
| const mainSourceNode = graphDb.getPatternExpressionNodeId(mainSourceId) | ||
| if (mainSourceNode && selected.has(mainSourceNode)) { | ||
| return findMainSourceOf(mainSourceNode) | ||
| } else { | ||
| const ident = graphDb.getOutputPortIdentifier(mainSourceId) | ||
| return ident != null && isIdentifier(ident) ? ident : none | ||
| } | ||
| } | ||
| const result = findResult() | ||
| knownMainSourceIdentifier.set(selectedNode, result) | ||
| return result | ||
| } |
There was a problem hiding this comment.
@kazcw I considered your comment in #14325 (comment) but sorting here would require bringing method function too (with guarantee that this is same method as graphDb), so I prefer to keep this, more self-contained solution.
There was a problem hiding this comment.
I don't think my proposed approach would introduce any greater reliance on the current method state than is already implicit in the use of graphDb.connections?
There was a problem hiding this comment.
I meant having two arguments (code and graphDb) where the caller would responsible for making sure they refer to same method.
But now I realized, this problem may be easily avoided by providing just the graph store.
| * input connection. If any connection could not be subsituted this way (e.g. no connection to self-port), | ||
| * an empty array will be returned. |
There was a problem hiding this comment.
I think an empty array is a bit subtle of a way to express an error condition probably needs to be reported to the user
| const findMainSourceOf = (selectedNode: NodeId) => { | ||
| const findResult = (): Identifier | None => { | ||
| const alreadyKnown = knownMainSourceIdentifier.get(selectedNode) | ||
| if (alreadyKnown) return alreadyKnown | ||
| const selfPort = graphDb.nodeIdToNode.get(selectedNode)?.primaryApplication.selfArgument | ||
| if (!selfPort) return none | ||
| const mainSourceId = set.first(graphDb.connections.reverseLookup(selfPort)) | ||
| if (!mainSourceId) return none | ||
| const mainSourceNode = graphDb.getPatternExpressionNodeId(mainSourceId) | ||
| if (mainSourceNode && selected.has(mainSourceNode)) { | ||
| return findMainSourceOf(mainSourceNode) | ||
| } else { | ||
| const ident = graphDb.getOutputPortIdentifier(mainSourceId) | ||
| return ident != null && isIdentifier(ident) ? ident : none | ||
| } | ||
| } | ||
| const result = findResult() | ||
| knownMainSourceIdentifier.set(selectedNode, result) | ||
| return result | ||
| } |
There was a problem hiding this comment.
I don't think my proposed approach would introduce any greater reliance on the current method state than is already implicit in the use of graphDb.connections?
| for (const { port, ident } of detachInfo.value) { | ||
| const result = await graphStore.updatePortValue(port, Ast.Ident.new(edit, ident), edit) | ||
| if (!result.ok) { | ||
| result.error.log('Failed to connect around') |
There was a problem hiding this comment.
We should expect this case to be reachable, as we have awaited during this edit and arbitrary graph edits may have taken place since the detachInfo analysis was performed. As such, we should carefully consider error recovery. I think maybe the best we could do is: Notify the user (e.g. with a toast) that an error occurred during the operation, and they may Undo if the result is wrong.
I think I've said this before, but I really don't like the complexity that results from allowing the edit operations to be async. It makes it so much harder to avoid bugs and crashes in hard-to-predict-or-test cases where multiple operations interact.
There was a problem hiding this comment.
Added a toast. AFAIK this async edit was needed in signature editor. I will make a task for sorting this out, as I agree async edits are very troublesome.
|
|
||
| for (const [source, targets] of graphDb.connections.allForward()) { | ||
| for (const target of targets) { | ||
| // TODO[ao]: copied from collapsing.ts. Worth merging? |
There was a problem hiding this comment.
TODO in code--let's create an issue (or add it to an existing issue) if it isn't addressed in this PR
| }), | ||
| ) | ||
| if (results.some((result) => !result.ok)) { | ||
| toasts.userActionFailed.show('Failed to connect removed component around') |
There was a problem hiding this comment.
I think we should be more clear that the action was attempted anyway, rather than aborted--something like "Errors occurred while connecting around removed components"
Pull Request Description
Fixes #14094
Screencast.From.2025-11-28.15-13-35.mp4
Important Notes
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
[ ] If meaningful changes were made to logic or tests affecting Enso Cloud integration in the libraries,or the Snowflake database integration, a run of the Extra Tests has been scheduled.