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

Copy-pasting nodes #7618

Merged
merged 18 commits into from
Aug 30, 2023
Merged

Copy-pasting nodes #7618

merged 18 commits into from
Aug 30, 2023

Conversation

vitvakatu
Copy link
Contributor

Pull Request Description

Closes #6261

  • Adds support for copy-pasting nodes with cmd + C and cmd + V shortcuts.
  • Only a single, currently selected node will be copied. Adding support for multiple node copies seems easy, though (but was out of the scope of the task).
  • We use a custom data format for clipboard content. Node's metadata is also copied, so opened visualizations are preserved. However, the visualization's size is not preserved, as we do not store this info in metadata.
  • For custom format to work, we use a pretty new feature called Clipboard pickling, but it is available in Electron and in most browsers already.
  • Pasting plain text from other applications (or from Enso, if the code is copied in edit mode) is supported and is currently enabled. There are some security concerns related to this, though. I will create a separate issue/discussion for that.
  • Undo/redo works as you expect.
  • New node is pasted at the cursor position.
Kapture.2023-08-21.at.13.37.40.mp4

Important Notes

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.

Comment on lines 459 to 460
/// AST of the node's expression. Typically no external user wants to access it directly. Use
/// [`Self::expression`] instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

Docs accurate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, it's still not typical to use it, but in this case we need it.

lib/rust/web/js/clipboard.js Outdated Show resolved Hide resolved
@vitvakatu
Copy link
Contributor Author

vitvakatu commented Aug 21, 2023

On the demo, we discussed that it would be nice to copy nodes to external programs as plain text, but it turns out to be quite hard to implement, as multiple clipboard items are not supported on most platforms: https://bugs.chromium.org/p/chromium/issues/detail?id=1171260.

So we either need to step back from this feature, which is bad, or use plain text format, which is also bad, as we won't be able to copy metadata and do other cool things in the future. I need to research how it is done in other programs, as I can clearly copy syntax highlighting from some editors and at the same time operate with copied code as with plain text in other programs.

@wdanilo, what do you think?

Co-authored-by: Kaz Wesley <kaz@lambdaverse.org>
@vitvakatu
Copy link
Contributor Author

Ok, let's merge this one, and let's think about external apps support in a separate issue.

@vitvakatu
Copy link
Contributor Author

Thanks to @Frizi for helping with the investigation. Turns out I was confused by the documentation. Added support for pasting to external programs as plain text.

@MichaelMauderer
Copy link
Contributor

QA: 🔴

When pasting a node while zoomed out, it is created in the wrong position.

Peek 2023-08-24 15-06

Some additional smaller UX items thing that I noticed

  • When pasting illegal input (multiple lines, invalid identifiers), there is an error in the console, but not to the user. It might be good to show a user-facing error explaining why nothing is happening.
    Peek 2023-08-24 15-11
  • When pasting multiple times without moving the cursor, the items are created exactly on top of one another. This seems undesirable as it hides potential user error (accidental double key press).
    Peek 2023-08-24 15-10

@vitvakatu
Copy link
Contributor Author

All issues were addressed.

@farmaazon
Copy link
Contributor

QA Report: 🟢

Issues fixed, no more found.

@vitvakatu vitvakatu added the CI: Ready to merge This PR is eligible for automatic merge label Aug 30, 2023
@mergify mergify bot merged commit b49cc25 into develop Aug 30, 2023
27 checks passed
@mergify mergify bot deleted the wip/vitvakatu/copy-paste-nodes branch August 30, 2023 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Copy-paste a single node
4 participants