Copy/paste improvements #9734

kazcw · 2024-04-17T20:07:22Z

Pull Request Description

Copying nodes:

Multiple nodes supported.
Node comments and user-specified colors included.
Google Sheets data can be pasted to produce a Table node, handled the same way as Excel data.

Important Notes

Fix E2E tests on OS X.
Add E2E and unit tests for clipboard.
Use the lexer to test text escaping; fix text escaping issues and inconsistencies.

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.

kazcw · 2024-04-17T20:10:32Z

app/gui2/src/components/GraphEditor/clipboard.ts

+    for (const copiedNode of clipboardData) {
+      const { expression, documentation, visualization, colorOverride } = copiedNode
+      graphStore.createNode(
+        (clipboardData.length === 1 ? graphNavigator.sceneMousePos : null) ?? Vec2.Zero,


Positioning of multiple nodes is currently rudimentary; this will be fixed with the new API that will be introduced for #9703.

kazcw · 2024-04-17T20:12:23Z

app/gui2/e2e/nodeClipboard.spec.ts

+  // Copy and paste it.
+  await page.keyboard.press(`${CONTROL_KEY}+C`)
+  await page.keyboard.press(`${CONTROL_KEY}+V`)
+  await expect(nodeToCopy).toBeSelected()


I think newly-created nodes should always be selected; I'll implement this as part of #9703.

kazcw · 2024-04-17T20:13:50Z

Depends on #9731 for tests to pass.

farmaazon · 2024-04-18T10:02:32Z

app/gui2/e2e/collapsingAndEntering.spec.ts

 import * as locate from './locate'

 const MAIN_FILE_NODES = 11

-const COLLAPSE_SHORTCUT = os.platform() === 'darwin' ? 'Meta+G' : 'Control+G'
+const COLLAPSE_SHORTCUT = `${CONTROL_KEY}+G`


After some changes ago, now it's possible to read COLLAPSE shortcut from bindings.ts (used for example in https://github.com/enso-org/enso/blob/e1893b65af1774bacd4b862cff11d28886bb81c2/app/gui2/src/composables/__tests__/selection.test.ts) You may try to use it here.

I don't think E2E tests have access to the main app source tree

farmaazon · 2024-04-18T10:04:06Z

app/gui2/e2e/undoRedo.spec.ts

@@ -10,18 +11,18 @@ test('Adding new node', async ({ page }) => {
  await locate.addNewNodeButton(page).click()
  await expect(locate.componentBrowserInput(page)).toBeVisible()
  await page.keyboard.insertText('foo')
-  await page.keyboard.press('Control+Enter')
+  await page.keyboard.press(`${CONTROL_KEY}+Enter`)


Interesting. Does it mean that E2E tests never worked on mac?

This is one of the most recent tests, but yeah, it has never worked on mac

farmaazon · 2024-04-18T10:12:10Z

app/gui2/shared/ast/text.ts

-    ')',
-  'gu',
-)
+const escapeRegex = new RegExp(`${[...fixedEscapes, '\\p{Surrogate}'].join('|')}`, 'gu')


Why is it inside ``? join should return string already.

What is this {Surrogate} tag?

Added a comment:

Unpaired-surrogate codepoints are not technically valid in Unicode, but they are allowed in Javascript strings.
Enso source files must be strictly UTF-8 conformant.

farmaazon · 2024-04-18T10:24:44Z

app/gui2/src/components/GraphEditor/clipboard.ts

+  documentation?: string | undefined
+  visualization?: VisualizationMetadata | undefined
+  colorOverride?: string | undefined


It would be nice if, by default, we would copy all the metadata. So we could keep metadata field as Omit<NodeMetadataFields, 'position'>

farmaazon · 2024-04-19T11:55:30Z

app/gui2/shared/ast/text.ts

+    // Unpaired-surrogate codepoints are not technically valid in Unicode, but they are allowed in Javascript strings.
+    // Enso source files must be strictly UTF-8 conformant.
+    '\\p{Surrogate}',


Oh, I actually missed that this is Regex feature (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Regular_expressions/Unicode_character_class_escape)

farmaazon · 2024-04-19T12:09:12Z

app/gui2/shared/ast/tree.ts

+function isNodeMetadataField(property: string) {
+  return ['position', 'visualization', 'colorOverride'].includes(property)
+}


What about https://www.npmjs.com/package/ts-transformer-keys? You could use keys<NodeMetadataFields>()

We can't use compiler plugins without switching from vite's default transpiler (esbuild) to tsc, which would have a build-time cost: evanw/esbuild#1963.

We have a utility function allKeys that adds type-level correctness checking; I'll use that here. It is not as convenient as the TS-transformer (it requires an "example object" as an input), but it doesn't impose any unusual requirements on our build process.

farmaazon · 2024-04-19T12:10:46Z

app/gui2/src/components/GraphEditor/clipboard.ts

@@ -22,16 +22,14 @@ interface ClipboardData {
 interface CopiedNode {
  expression: string
  documentation?: string | undefined
-  visualization?: VisualizationMetadata | undefined
-  colorOverride?: string | undefined
+  metadata?: NodeMetadataFields


So we don't want to exclude position from copied metadata?

position is ignored in the metadata supplied for node creation because a separate parameter overrides it.

kazcw added 5 commits April 17, 2024 13:02

Refactor

288df3a

Use lexer to interpret text literals

17f2442

Remove unnecessary double-quote escaping

30f447a

Fix undo/redo tests on Mac

1f683f6

Copy multiple nodes, comments, user colors; clipboard E2E & unit tests

9a1a00f

kazcw added the CI: No changelog needed Do not require a changelog entry for this PR. label Apr 17, 2024

kazcw self-assigned this Apr 17, 2024

kazcw requested review from Frizi, farmaazon and vitvakatu as code owners April 17, 2024 20:07

kazcw commented Apr 17, 2024

View reviewed changes

This was linked to issues Apr 17, 2024

Copy-pasting nodes does not copy comments. #9456

Closed

Copy/Paste mutiple components #9424

Closed

farmaazon reviewed Apr 18, 2024

View reviewed changes

Review

cecd9b9

This was unlinked from issues Apr 18, 2024

Copy/Paste mutiple components #9424

Closed

Copy-pasting nodes does not copy comments. #9456

Closed

Review

cf05f6d

kazcw requested a review from farmaazon April 18, 2024 15:54

farmaazon approved these changes Apr 19, 2024

View reviewed changes

Review

a2c96e6

kazcw added the CI: Ready to merge This PR is eligible for automatic merge label Apr 19, 2024

mergify bot merged commit 6426478 into develop Apr 19, 2024
37 checks passed

mergify bot deleted the wip/kw/copy-comments branch April 19, 2024 16:33

kazcw linked an issue Apr 23, 2024 that may be closed by this pull request

Copy-pasting nodes does not copy comments. #9456

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Copy/paste improvements #9734

Copy/paste improvements #9734

kazcw commented Apr 17, 2024

kazcw Apr 17, 2024

kazcw Apr 17, 2024

kazcw commented Apr 17, 2024

farmaazon Apr 18, 2024

kazcw Apr 18, 2024

farmaazon Apr 18, 2024

kazcw Apr 18, 2024 •

edited

Loading

farmaazon Apr 18, 2024

kazcw Apr 18, 2024

farmaazon Apr 18, 2024

farmaazon Apr 19, 2024

farmaazon Apr 19, 2024

kazcw Apr 19, 2024

farmaazon Apr 19, 2024

kazcw Apr 19, 2024

Copy/paste improvements #9734

Copy/paste improvements #9734

Conversation

kazcw commented Apr 17, 2024

Pull Request Description

Important Notes

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kazcw commented Apr 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kazcw Apr 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kazcw Apr 18, 2024 •

edited

Loading