From e502023922375e64a7e4087a09462afda1bda3a7 Mon Sep 17 00:00:00 2001 From: Ilya Bogdanov Date: Tue, 11 Jun 2024 12:26:55 +0300 Subject: [PATCH] Copy paste multiple nodes (#10194) Closes #9424 https://github.com/enso-org/enso/assets/6566674/c0c5a524-ea6a-41f6-9fea-c94431211f33 --- CHANGELOG.md | 2 + app/gui2/e2e/nodeClipboard.spec.ts | 18 +++- .../src/components/GraphEditor/clipboard.ts | 29 ++++-- app/gui2/src/composables/nodeCreation.ts | 88 ++++++++++++++++--- app/gui2/src/stores/graph/graphDatabase.ts | 10 ++- app/gui2/src/stores/graph/index.ts | 30 ++++++- .../src/util/ast/__tests__/abstract.test.ts | 83 ++++++++++++++--- app/gui2/src/util/ast/abstract.ts | 40 +++++++-- 8 files changed, 250 insertions(+), 50 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 99c5984873..f9559492b3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,11 +4,13 @@ - [Arrows navigation][10179] selected nodes may be moved around, or entire scene if no node is selected. +- [Copy-pasting multiple nodes][10194]. - The documentation editor has [formatting toolbars][10064]. - The documentation editor supports [rendering images][10205]. [10064]: https://github.com/enso-org/enso/pull/10064 [10179]: https://github.com/enso-org/enso/pull/10179 +[10194]: https://github.com/enso-org/enso/pull/10194 [10205]: https://github.com/enso-org/enso/pull/10205 #### Enso Standard Library diff --git a/app/gui2/e2e/nodeClipboard.spec.ts b/app/gui2/e2e/nodeClipboard.spec.ts index 6383fb3457..61efb1e0d9 100644 --- a/app/gui2/e2e/nodeClipboard.spec.ts +++ b/app/gui2/e2e/nodeClipboard.spec.ts @@ -3,6 +3,11 @@ import * as actions from './actions' import { expect } from './customExpect' import { CONTROL_KEY } from './keyboard' import * as locate from './locate' +import { edgesFromNodeWithBinding, edgesToNodeWithBinding } from './locate' + +/** Every edge consists of multiple parts. + * See e2e/edgeRendering.spec.ts for explanation. */ +const EDGE_PARTS = 2 test.beforeEach(async ({ page }) => { await page.addInitScript(() => { @@ -55,7 +60,7 @@ test('Copy multiple nodes', async ({ page }) => { // Select some nodes. const node1 = locate.graphNodeByBinding(page, 'final') await node1.click() - const node2 = locate.graphNodeByBinding(page, 'data') + const node2 = locate.graphNodeByBinding(page, 'prod') await node2.click({ modifiers: ['Shift'] }) await expect(node1).toBeSelected() await expect(node2).toBeSelected() @@ -68,5 +73,16 @@ test('Copy multiple nodes', async ({ page }) => { // Nodes and comment have been copied. await expect(locate.graphNode(page)).toHaveCount(originalNodes + 2) + // `final` node has a comment. await expect(page.locator('.GraphNodeComment')).toHaveCount(originalNodeComments + 1) + // Check that two copied nodes are isolated, i.e. connected to each other, not original nodes. + await expect(locate.graphNodeByBinding(page, 'prod1')).toBeVisible() + await expect(locate.graphNodeByBinding(page, 'final1')).toBeVisible() + await expect(await edgesFromNodeWithBinding(page, 'sum')).toHaveCount(2 * EDGE_PARTS) + await expect(await edgesFromNodeWithBinding(page, 'prod')).toHaveCount(1 * EDGE_PARTS) + + await expect(await edgesToNodeWithBinding(page, 'prod')).toHaveCount(1 * EDGE_PARTS) + await expect(await edgesToNodeWithBinding(page, 'final')).toHaveCount(1 * EDGE_PARTS) + await expect(await edgesToNodeWithBinding(page, 'prod1')).toHaveCount(1 * EDGE_PARTS) + await expect(await edgesToNodeWithBinding(page, 'final1')).toHaveCount(1 * EDGE_PARTS) }) diff --git a/app/gui2/src/components/GraphEditor/clipboard.ts b/app/gui2/src/components/GraphEditor/clipboard.ts index 2652e896fa..ded5ad4726 100644 --- a/app/gui2/src/components/GraphEditor/clipboard.ts +++ b/app/gui2/src/components/GraphEditor/clipboard.ts @@ -1,7 +1,8 @@ -import type { NodeCreation } from '@/composables/nodeCreation' +import type { NodeCreationOptions } from '@/composables/nodeCreation' import type { GraphStore, Node, NodeId } from '@/stores/graph' import { Ast } from '@/util/ast' import { Pattern } from '@/util/ast/match' +import { Vec2 } from '@/util/data/vec2' import type { ToValue } from '@/util/reactivity' import type { NodeMetadataFields } from 'shared/ast' import { computed, toValue } from 'vue' @@ -19,6 +20,7 @@ interface ClipboardData { /** Node data that is copied to the clipboard. Used for serializing and deserializing the node information. */ interface CopiedNode { expression: string + binding?: string documentation?: string | undefined metadata?: NodeMetadataFields } @@ -28,6 +30,7 @@ function nodeStructuredData(node: Node): CopiedNode { expression: node.innerExpr.code(), documentation: node.documentation, metadata: node.rootExpr.serializeMetadata(), + ...(node.pattern ? { binding: node.pattern.code() } : {}), } } @@ -120,12 +123,13 @@ function getClipboard() { export function useGraphEditorClipboard( graphStore: GraphStore, selected: ToValue>, - createNodes: NodeCreation['createNodes'], + createNodes: (nodesOptions: Iterable) => void, ) { /** Copy the content of the selected node to the clipboard. */ function copySelectionToClipboard() { const nodes = new Array() - for (const id of toValue(selected)) { + const ids = graphStore.pickInCodeOrder(toValue(selected)) + for (const id of ids) { const node = graphStore.db.nodeIdToNode.get(id) if (!node) continue nodes.push(node) @@ -144,13 +148,20 @@ export function useGraphEditorClipboard( console.warn('No valid node in clipboard.') return } + const firstNodePos = clipboardData[0]!.metadata?.position ?? { x: 0, y: 0 } + const originPos = new Vec2(firstNodePos.x, firstNodePos.y) createNodes( - clipboardData.map(({ expression, documentation, metadata }) => ({ - placement: { type: 'mouse' }, - expression, - metadata, - documentation, - })), + clipboardData.map(({ expression, binding, documentation, metadata }) => { + const pos = metadata?.position + const relativePos = pos ? new Vec2(pos.x, pos.y).sub(originPos) : new Vec2(0, 0) + return { + placement: { type: 'mouseRelative', posOffset: relativePos }, + expression, + binding, + metadata, + documentation, + } + }), ) } diff --git a/app/gui2/src/composables/nodeCreation.ts b/app/gui2/src/composables/nodeCreation.ts index 1458cdfeab..623d6a4c91 100644 --- a/app/gui2/src/composables/nodeCreation.ts +++ b/app/gui2/src/composables/nodeCreation.ts @@ -10,6 +10,7 @@ import { asNodeId } from '@/stores/graph/graphDatabase' import type { RequiredImport } from '@/stores/graph/imports' import type { Typename } from '@/stores/suggestionDatabase/entry' import { Ast } from '@/util/ast' +import { isIdentifier, substituteIdentifier, type Identifier } from '@/util/ast/abstract' import { partition } from '@/util/data/array' import { filterDefined } from '@/util/data/iterable' import { Rect } from '@/util/data/rect' @@ -25,6 +26,7 @@ export type NodeCreation = ReturnType type GraphIndependentPlacement = | { type: 'fixed'; position: Vec2 } | { type: 'mouse' } + | { type: 'mouseRelative'; posOffset: Vec2 } | { type: 'mouseEvent'; position: Vec2 } type GraphAwarePlacement = { type: 'viewport' } | { type: 'source'; node: NodeId } export type PlacementStrategy = GraphIndependentPlacement | GraphAwarePlacement @@ -44,6 +46,7 @@ function isIndependent( export interface NodeCreationOptions { placement: Placement expression: string + binding?: string | undefined type?: Typename | undefined documentation?: string | undefined metadata?: Ast.NodeMetadataFields | undefined @@ -61,10 +64,16 @@ export function useNodeCreation( return pos ? mouseDictatedPlacement(pos) : undefined } + function tryMouseRelative(offset: Vec2) { + const pos = toValue(sceneMousePos) + return pos ? mouseDictatedPlacement(pos.add(offset)) : undefined + } + function placeNode(placement: PlacementStrategy, place: (nodes?: Iterable) => Vec2): Vec2 { return ( placement.type === 'viewport' ? place() : placement.type === 'mouse' ? tryMouse() ?? place() + : placement.type === 'mouseRelative' ? tryMouseRelative(placement.posOffset) ?? place() : placement.type === 'mouseEvent' ? mouseDictatedPlacement(placement.position) : placement.type === 'source' ? place(filterDefined([graphStore.visibleArea(placement.node)])) : placement.type === 'fixed' ? placement.position @@ -113,10 +122,21 @@ export function useNodeCreation( return new Set() } const created = new Set() + const createdIdentifiers = new Set() + const identifiersRenameMap = new Map() graphStore.edit((edit) => { const statements = new Array() for (const options of placedNodes) { - const { rootExpression, id } = newAssignmentNode(edit, options) + const rhs = Ast.parse(options.expression, edit) + const ident = getIdentifier(rhs, options, createdIdentifiers) + createdIdentifiers.add(ident) + const { id, rootExpression } = newAssignmentNode( + edit, + ident, + rhs, + options, + identifiersRenameMap, + ) statements.push(rootExpression) created.add(id) assert(options.metadata?.position != null, 'Node should already be placed') @@ -127,29 +147,65 @@ export function useNodeCreation( onCreated(created) } + /** We resolve import conflicts and substitute identifiers if needed. */ + function afterCreation( + edit: Ast.MutableModule, + assignment: Ast.MutableAssignment, + ident: Ast.Identifier, + options: NodeCreationOptions, + identifiersRenameMap: Map, + ) { + // When nodes are copied, we need to substitute original names with newly assigned. + if (options.binding) { + if (isIdentifier(options.binding) && options.binding !== ident) + identifiersRenameMap.set(options.binding, ident) + } + for (const [old, replacement] of identifiersRenameMap.entries()) { + substituteIdentifier(assignment, old, replacement) + } + + // Resolve import conflicts. + const conflicts = graphStore.addMissingImports(edit, options.requiredImports ?? []) ?? [] + for (const _conflict of conflicts) { + // TODO: Substitution does not work, because we interpret imports wrongly. To be fixed in + // https://github.com/enso-org/enso/issues/9356 + // substituteQualifiedName(assignment, conflict.pattern, conflict.fullyQualified) + } + } + function createNode(options: NodeCreationOptions) { createNodes([options]) } - function newAssignmentNode(edit: Ast.MutableModule, options: NodeCreationOptions) { - const conflicts = graphStore.addMissingImports(edit, options.requiredImports ?? []) ?? [] - const rhs = Ast.parse(options.expression, edit) - const inferredPrefix = inferPrefixFromAst(rhs) - const namePrefix = options.type ? typeToPrefix(options.type) : inferredPrefix - const ident = graphStore.generateLocallyUniqueIdent(namePrefix) + function newAssignmentNode( + edit: Ast.MutableModule, + ident: Ast.Identifier, + rhs: Ast.Owned, + options: NodeCreationOptions, + identifiersRenameMap: Map, + ) { rhs.setNodeMetadata(options.metadata ?? {}) const assignment = Ast.Assignment.new(edit, ident, rhs) - for (const _conflict of conflicts) { - // TODO: Substitution does not work, because we interpret imports wrongly. To be fixed in - // https://github.com/enso-org/enso/issues/9356 - // substituteQualifiedName(edit, assignment, conflict.pattern, conflict.fullyQualified) - } + afterCreation(edit, assignment, ident, options, identifiersRenameMap) const id = asNodeId(rhs.id) const rootExpression = options.documentation != null ? Ast.Documented.new(options.documentation, assignment) : assignment - return { rootExpression, id, inferredType: inferredPrefix } + return { rootExpression, id } + } + + function getIdentifier( + expr: Ast.Ast, + options: NodeCreationOptions, + alreadyCreated: Set, + ): Ast.Identifier { + const namePrefix = + options.binding ? existingNameToPrefix(options.binding) + : options.type ? typeToPrefix(options.type) + : inferPrefixFromAst(expr) + const ident = graphStore.generateLocallyUniqueIdent(namePrefix, alreadyCreated) + return ident } return { createNode, createNodes, placeNode } @@ -187,6 +243,12 @@ function typeToPrefix(type: Typename): string { } } +/** Strip number suffix from binding name, effectively returning a valid prefix. + * The reverse of graphStore.generateLocallyUniqueIdent */ +function existingNameToPrefix(name: string): string { + return name.replace(/\d+$/, '') +} + /** Insert the given statements into the given block, at a location appropriate for new nodes. * * The location will be after any statements in the block that bind identifiers; if the block ends in an expression diff --git a/app/gui2/src/stores/graph/graphDatabase.ts b/app/gui2/src/stores/graph/graphDatabase.ts index 0420e0725e..ef53fa6a59 100644 --- a/app/gui2/src/stores/graph/graphDatabase.ts +++ b/app/gui2/src/stores/graph/graphDatabase.ts @@ -138,6 +138,10 @@ export class GraphDb { private valuesRegistry: ComputedValueRegistry, ) {} + private nodeIdToOuterExprIds = new ReactiveIndex(this.nodeIdToNode, (id, entry) => { + return [[id, entry.outerExpr.id]] + }) + private nodeIdToPatternExprIds = new ReactiveIndex(this.nodeIdToNode, (id, entry) => { const exprs: AstId[] = [] if (entry.pattern) entry.pattern.visitRecursiveAst((ast) => void exprs.push(ast.id)) @@ -202,7 +206,7 @@ export class GraphDb { return Array.from(ports, (port) => [id, port]) }) - nodeMainSuggestion = new ReactiveMapping(this.nodeIdToNode, (id, entry) => { + nodeMainSuggestion = new ReactiveMapping(this.nodeIdToNode, (_id, entry) => { const expressionInfo = this.getExpressionInfo(entry.innerExpr.id) const method = expressionInfo?.methodCall?.methodPointer if (method == null) return @@ -230,6 +234,10 @@ export class GraphDb { } } + getOuterExpressionNodeId(exprId: AstId | undefined): NodeId | undefined { + return exprId && set.first(this.nodeIdToOuterExprIds.reverseLookup(exprId)) + } + getExpressionNodeId(exprId: AstId | undefined): NodeId | undefined { return exprId && set.first(this.nodeIdToExprIds.reverseLookup(exprId)) } diff --git a/app/gui2/src/stores/graph/index.ts b/app/gui2/src/stores/graph/index.ts index ce6da66f3c..0d2bdef211 100644 --- a/app/gui2/src/stores/graph/index.ts +++ b/app/gui2/src/stores/graph/index.ts @@ -17,7 +17,7 @@ import { type SuggestionDbStore } from '@/stores/suggestionDatabase' import { assert, bail } from '@/util/assert' import { Ast } from '@/util/ast' import type { AstId } from '@/util/ast/abstract' -import { MutableModule, isIdentifier } from '@/util/ast/abstract' +import { MutableModule, isIdentifier, type Identifier } from '@/util/ast/abstract' import { RawAst, visitRecursive } from '@/util/ast/raw' import { partition } from '@/util/data/array' import { filterDefined } from '@/util/data/iterable' @@ -234,14 +234,21 @@ export const { injectFn: useGraphStore, provideFn: provideGraphStore } = createC return Ok(method) } - function generateLocallyUniqueIdent(prefix?: string | undefined) { + /** Generate unique identifier from `prefix` and some numeric suffix. + * @param prefix - of the identifier + * @param ignore - a list of identifiers to consider as unavailable. Useful when creating multiple identifiers in a batch. + * */ + function generateLocallyUniqueIdent( + prefix?: string | undefined, + ignore: Set = new Set(), + ): Identifier { // FIXME: This implementation is not robust in the context of a synchronized document, // as the same name can likely be assigned by multiple clients. // Consider implementing a mechanism to repair the document in case of name clashes. for (let i = 1; ; i++) { const ident = (prefix ?? FALLBACK_BINDING_PREFIX) + i assert(isIdentifier(ident)) - if (!db.identifierUsed(ident)) return ident + if (!db.identifierUsed(ident) && !ignore.has(ident)) return ident } } @@ -387,7 +394,7 @@ export const { injectFn: useGraphStore, provideFn: provideGraphStore } = createC for (const _conflict of conflicts) { // TODO: Substitution does not work, because we interpret imports wrongly. To be fixed in // https://github.com/enso-org/enso/issues/9356 - // substituteQualifiedName(edit, wholeAssignment, conflict.pattern, conflict.fullyQualified) + // substituteQualifiedName(wholeAssignment, conflict.pattern, conflict.fullyQualified) } } }) @@ -665,6 +672,20 @@ export const { injectFn: useGraphStore, provideFn: provideGraphStore } = createC proj.computedValueRegistry.processUpdates([update_]) } + /** Iterate over code lines, return node IDs from `ids` set in the order of code positions. */ + function pickInCodeOrder(ids: Set): NodeId[] { + assert(syncModule.value != null) + const func = unwrap(getExecutedMethodAst(syncModule.value)) + const body = func.bodyExpressions() + const result: NodeId[] = [] + for (const expr of body) { + const id = expr?.id + const nodeId = db.getOuterExpressionNodeId(id) + if (nodeId && ids.has(nodeId)) result.push(nodeId) + } + return result + } + /** * Reorders nodes so the `targetNodeId` node is placed after `sourceNodeId`. Does nothing if the * relative order is already correct. @@ -758,6 +779,7 @@ export const { injectFn: useGraphStore, provideFn: provideGraphStore } = createC disconnectTarget, moduleRoot, deleteNodes, + pickInCodeOrder, ensureCorrectNodeOrder, batchEdits, overrideNodeColor, diff --git a/app/gui2/src/util/ast/__tests__/abstract.test.ts b/app/gui2/src/util/ast/__tests__/abstract.test.ts index c06207af84..1d63c1a581 100644 --- a/app/gui2/src/util/ast/__tests__/abstract.test.ts +++ b/app/gui2/src/util/ast/__tests__/abstract.test.ts @@ -4,14 +4,13 @@ import { MutableModule, TextLiteral, escapeTextLiteral, + substituteIdentifier, substituteQualifiedName, unescapeTextLiteral, type Identifier, } from '@/util/ast/abstract' -import { tryQualifiedName } from '@/util/qualifiedName' import { fc, test } from '@fast-check/vitest' import { initializeFFI } from 'shared/ast/ffi' -import { unwrap } from 'shared/util/data/result' import { describe, expect } from 'vitest' import { findExpressions, testCase, tryFindExpressions } from './testCase' @@ -853,18 +852,74 @@ test.each([ substitution: 'ShouldNotWork', expected: 'Data.Table.new', }, -])('Substitute $pattern insde $original', ({ original, pattern, substitution, expected }) => { - const expression = Ast.parse(original) - expression.module.replaceRoot(expression) - const edit = expression.module.edit() - substituteQualifiedName( - edit, - expression, - pattern as Ast.QualifiedName, - unwrap(tryQualifiedName(substitution)), - ) - expect(edit.root()?.code()).toEqual(expected) -}) +])( + 'Substitute qualified name $pattern inside $original', + ({ original, pattern, substitution, expected }) => { + const expression = Ast.parse(original) + const module = expression.module + module.replaceRoot(expression) + const edit = expression.module.edit() + substituteQualifiedName(expression, pattern as Ast.Identifier, substitution as Ast.Identifier) + module.applyEdit(edit) + expect(module.root()?.code()).toEqual(expected) + }, +) + +test.each([ + { + original: 'some_name', + pattern: 'some_name', + substitution: 'other_name', + expected: 'other_name', + }, + { + original: 'x = Table.from_vec (node1.new 1 2 3)', + pattern: 'node1', + substitution: 'node2', + expected: 'x = Table.from_vec (node2.new 1 2 3)', + }, + { + original: 'x = some_func "node1"', + pattern: 'node1', + substitution: 'node2', + expected: 'x = some_func "node1"', + }, + { + original: 'x + y', + pattern: 'x', + substitution: 'z', + expected: 'z + y', + }, + { + original: 'node1.node2.node3', + pattern: 'node2', + substitution: 'ShouldNotWork', + expected: 'node1.node2.node3', + }, + { + original: 'node1.node2.node3', + pattern: 'node3', + substitution: 'ShouldNotWork', + expected: 'node1.node2.node3', + }, + { + original: '.node1', + pattern: 'node1', + substitution: 'ShouldNotWork', + expected: '.node1', + }, +])( + 'Substitute identifier $pattern inside $original', + ({ original, pattern, substitution, expected }) => { + const expression = Ast.parse(original) + const module = expression.module + module.replaceRoot(expression) + const edit = expression.module.edit() + substituteIdentifier(expression, pattern as Ast.Identifier, substitution as Ast.Identifier) + module.applyEdit(edit) + expect(module.root()?.code()).toEqual(expected) + }, +) test.each([ ['', ''], diff --git a/app/gui2/src/util/ast/abstract.ts b/app/gui2/src/util/ast/abstract.ts index 1c76221e34..6b35900f75 100644 --- a/app/gui2/src/util/ast/abstract.ts +++ b/app/gui2/src/util/ast/abstract.ts @@ -15,7 +15,9 @@ import { Function, Ident, MutableBodyBlock, + MutableIdent, MutableModule, + MutablePropertyAccess, NumericLiteral, OprApp, PropertyAccess, @@ -165,29 +167,51 @@ export function parseQualifiedName(ast: Ast): QualifiedName | null { return idents && normalizeQualifiedName(qnFromSegments(idents)) } -/* Substitute `pattern` inside `expression` with `to`. +/** Substitute `pattern` inside `expression` with `to`. + * Will only replace the first item in the property acccess chain. */ +export function substituteIdentifier( + expr: MutableAst, + pattern: IdentifierOrOperatorIdentifier, + to: IdentifierOrOperatorIdentifier, +) { + if (expr instanceof MutableIdent && expr.code() === pattern) { + expr.setToken(to) + } else if (expr instanceof MutablePropertyAccess) { + // Substitute only the first item in the property access chain. + if (expr.lhs != null) substituteIdentifier(expr.lhs, pattern, to) + } else { + for (const child of expr.children()) { + if (child instanceof Token) { + continue + } + const mutableChild = expr.module.getVersion(child) + substituteIdentifier(mutableChild, pattern, to) + } + } +} + +/** Substitute `pattern` inside `expression` with `to`. * Replaces identifier, the whole qualified name, or the beginning of the qualified name (first segments of property access chain). */ export function substituteQualifiedName( - module: MutableModule, - expression: Ast, + expr: MutableAst, pattern: QualifiedName | IdentifierOrOperatorIdentifier, to: QualifiedName, ) { - const expr = module.getVersion(expression) ?? expression - if (expr instanceof PropertyAccess || expr instanceof Ident) { + if (expr instanceof MutablePropertyAccess || expr instanceof MutableIdent) { const qn = parseQualifiedName(expr) if (qn === pattern) { - expr.updateValue(() => Ast.parse(to, module)) + expr.updateValue(() => Ast.parse(to, expr.module)) } else if (qn && qn.startsWith(pattern)) { const withoutPattern = qn.replace(pattern, '') - expr.updateValue(() => Ast.parse(to + withoutPattern, module)) + expr.updateValue(() => Ast.parse(to + withoutPattern, expr.module)) } } else { for (const child of expr.children()) { if (child instanceof Token) { continue } - substituteQualifiedName(module, child, pattern, to) + const mutableChild = expr.module.getVersion(child) + substituteQualifiedName(mutableChild, pattern, to) } } }