Copy paste multiple nodes #10194
Copy paste multiple nodes #10194
Conversation
We don’t need `copiedNodes` argument, it was a mistake
vitvakatu
added
CI: No changelog needed
vitvakatu
requested review from
Frizi,
farmaazon,
kazcw and
AdRiley
as code owners
| ) { | ||
| /** Copy the content of the selected node to the clipboard. */ | ||
| function copySelectionToClipboard() { | ||
| const nodes = new Array<Node>() | ||
| for (const id of toValue(selected)) { | ||
| const edit = graphStore.startEdit() |
There was a problem hiding this comment.
startEdit is a somewhat expensive operation that should only be needed for editing. pickInCodeOrder could use the current syncModule by default, so that snapshotting the module here is not required.
There was a problem hiding this comment.
My understanding was that starting editing is cheap, but applying changes is expensive. Unfortunately, we don’t have any documentation where it will be explained.
Removed the usage of the mutable module here in favor of syncModule inside of pickInCodeOrder.
There was a problem hiding this comment.
It does have some cost--it has to copy the state of the module.
app/gui2/src/stores/graph/index.ts
Outdated
| /** Iterate over code lines, return node IDs from `ids` set in the order of code positions. */ | ||
| function pickInCodeOrder(edit: MutableModule, ids: Set<NodeId>): NodeId[] { | ||
| const result: NodeId[] = [] | ||
| const body = edit.getVersion(unwrap(getExecutedMethodAst(edit))).bodyAsBlock() |
There was a problem hiding this comment.
It is not clear from the name or documentation why this function mutates the body.
There was a problem hiding this comment.
Yep, it does not. bodyAsBlock is not available for immutable Ast, so I hope current implementation is correct. Could you please take a look at it?
There was a problem hiding this comment.
bodyAsBlock does mutate--if the body is inline, it converts it to a single-line block. When a MutableBodyBlock isn't needed, there's a different helper for iterating the expressions of a function--suggested in the new review.
app/gui2/src/util/ast/abstract.ts
Outdated
| pattern: IdentifierOrOperatorIdentifier, | ||
| to: IdentifierOrOperatorIdentifier, | ||
| ) { | ||
| const expr = module.getVersion(expression) ?? expression |
There was a problem hiding this comment.
This is a strange fallback. Why not accept expression as a MutableAst and then use expression.module to get the module?
There was a problem hiding this comment.
Actually it was copied from substituteQualifiedName nearby. Fixed both methods.
There was a problem hiding this comment.
@kazcw please take a look at my usage of mutableChild in this module. I didn’t find better way of getting mutable ast from children().
There was a problem hiding this comment.
That works. Alternatively MutableAst could define a mutableChildren that works by casting the iterator returned by children (the MutableAst knows that its module is a MutableModule, and all children of a mutable module are mutable).
vitvakatu
removed
the
CI: No changelog needed
app/gui2/src/util/ast/abstract.ts
Outdated
| } else if (expr instanceof PropertyAccess && expr.lhs instanceof Ident) { | ||
| if (expr instanceof MutableIdent && expr.code() === pattern) { | ||
| expr.setToken(to) | ||
| } else if (expr instanceof MutablePropertyAccess && expr.lhs != null) { |
There was a problem hiding this comment.
The != null check should be inside the instanceof PropertyAccess conditional--a PropertyAccess can be an operator section like .foo and we still shouldn't change the RHS (needs another test).
app/gui2/src/stores/graph/index.ts
Outdated
| function pickInCodeOrder(ids: Set<NodeId>): NodeId[] { | ||
| assert(syncModule.value != null) | ||
| const func = unwrap(getExecutedMethodAst(syncModule.value)) | ||
| const lines = func.body instanceof Ast.BodyBlock ? func.body.lines : [] |
There was a problem hiding this comment.
Function has bodyExpressions that does the right thing if the function is single-line.
app/gui2/src/stores/graph/index.ts
Outdated
| /** Iterate over code lines, return node IDs from `ids` set in the order of code positions. */ | ||
| function pickInCodeOrder(edit: MutableModule, ids: Set<NodeId>): NodeId[] { | ||
| const result: NodeId[] = [] | ||
| const body = edit.getVersion(unwrap(getExecutedMethodAst(edit))).bodyAsBlock() |
There was a problem hiding this comment.
bodyAsBlock does mutate--if the body is inline, it converts it to a single-line block. When a MutableBodyBlock isn't needed, there's a different helper for iterating the expressions of a function--suggested in the new review.
Pull Request Description
Closes #9424
copy.paste.multiple.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.