Skip to content

Add "after" delimiter-adding support to "paste" #128

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

Closed
pokey opened this issue Jul 21, 2021 · 12 comments · Fixed by #771
Closed

Add "after" delimiter-adding support to "paste" #128

pokey opened this issue Jul 21, 2021 · 12 comments · Fixed by #771
Labels
enhancement New feature or request
Milestone

Comments

@pokey
Copy link
Member

pokey commented Jul 21, 2021

See https://github.com/pokey/cursorless-vscode/blob/5eada65c0a7cb6c34ba0682d8f06cc2f76fed0a1/src/actions/BringMoveSwap.ts#L115-L121

We'll probably do this one in tandem with #202

@AndreasArvidsson AndreasArvidsson self-assigned this Jul 21, 2021
@AndreasArvidsson
Copy link
Member

That can be problematic combined with #127

@AndreasArvidsson AndreasArvidsson removed their assignment Aug 4, 2021
@pokey pokey added the enhancement New feature or request label Aug 6, 2021
@pokey
Copy link
Member Author

pokey commented Aug 6, 2021

@AndreasArvidsson is this one not solved in latest slew of PRs?

@AndreasArvidsson
Copy link
Member

@pokey Not at all. Since we are using the built in paste now it would require large rewrites to get it working.

@pokey
Copy link
Member Author

pokey commented Aug 7, 2021

Ah right fair point. I really go back and forth on the built-in paste vs custom. Another argument for custom is that I think I'd argue that "bring" and "paste" should behave roughly the same. But yeah then we'd need to recreate the multi-line multi-selection copy tracking logic. Idk

@pokey
Copy link
Member Author

pokey commented Aug 7, 2021

Although couldn't we solve this one fairly easily by inserting delimiter, then moving cursor before / after it then issuing paste?

@AndreasArvidsson
Copy link
Member

Yes that's the only viable solution but that's not easy right now with how the code looks. Then we will have one edit we control and one edit we don't and we need to update selections on both. Doable of course but not that easy.

@AndreasArvidsson
Copy link
Member

I think the best way to solve this is

  • Revert to our own implementation of copy/cut/paste
  • Try to leverage as much from bring/move as we can
  • Use the same implementation that vscode does with an internal clipboard per cursor copied
  • Support clipboard history.
  • Contribute our own ctrl/cmd+c/x/v keyboard bindings with a when clause that the editor is focused

@pokey Sounds good?

@pokey
Copy link
Member Author

pokey commented Jan 28, 2022

Looks good to me! Worth mentioning that we should make sure clipboard state is kept in global storage so it can be shared across windows

Also would be awesome if you're able to scare up that link to the VSCode implementation again

@AndreasArvidsson
Copy link
Member

Some of the interesting parts are here

https://github.com/microsoft/vscode/blob/1498d0f34053f854e75e1364adaca6f99e43de08/src/vs/editor/contrib/clipboard/browser/clipboard.ts#L230

https://github.com/microsoft/vscode/blob/1fa4c98b130a9c70d4baeb59979db0873bee93ed/src/vs/editor/common/cursor/cursorTypeOperations.ts#L109

I can't right now find where they actually check that you haven't updated the clipboard outside of vscode. But I would assume it's something like:

if (clipboard.text === multiCursor.expectedText) { .. }

And if we want a clipboard history we definitely need to do something like this.

@pokey
Copy link
Member Author

pokey commented Jan 28, 2022

I was never able to find that check either fwiw. Slightly worrying 🤔

@AndreasArvidsson
Copy link
Member

According to my tests they must have a check like that or a hook whenever the clipboard changes because if you copy something out of vscode the paste behavior/content is updated.

@pokey
Copy link
Member Author

pokey commented Jan 29, 2022

I mean yeah I can definitely believe it, just slightly worrying neither of us can find the code. Would be good if we understand their code in order to make sure there isn't some fancy corner case handling that we miss by rolling our own

thetomcraig-aya pushed a commit to thetomcraig/cursorless that referenced this issue Mar 27, 2024
* Fall back to full hat enablement on error

* Refactor

* Black
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants