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

Add system clipboard yank and paste commands #310

Merged
merged 3 commits into from
Jun 20, 2021

Conversation

CBenoit
Copy link
Member

@CBenoit CBenoit commented Jun 18, 2021

This commit adds six new commands to interact with system clipboard:

  • clipboard-yank
  • clipboard-yank-join
  • clipboard-paste-after
  • clipboard-paste-before
  • clipboard-paste-replace
  • show-clipboard-provider

System clipboard provider is detected by checking a few environment
variables and executables. Currently only built-in detection is
supported.

clipboard-yank will only yank the "main" selection, which is currently the first
one. This will need to be revisited later.

Closes #76

@vv9k
Copy link
Contributor

vv9k commented Jun 19, 2021

I think you have to add the new commands to the commands list here:

commands!(

@archseer
Copy link
Member

Potentially controversial but I think space+y, space+p might be good default mappings. It's what I've always done and I've seen others do that too (either that or they replaced the default y/p).

@vv9k
Copy link
Contributor

vv9k commented Jun 19, 2021

I usually replace the default y and p but I think space+y and space+p fits in nicely to the default keybindings.

@sudormrfbin
Copy link
Member

I prefer the register approach TBH - "+y for yanking to primary selection and "*y for secondary which I think can be added later. space+y seems like a good default keybind, but I'd be wary of changing y itself to clipboard yank since it could mangle up other selections made outside of helix; it's better left to the user to decide whether they want that behavior or not (set clipboard=unnamedplus in vim)

@CBenoit
Copy link
Member Author

CBenoit commented Jun 19, 2021

I think you have to add the new commands to the commands list here

I will have to change the type of command for that (currently it's "typable").

Potentially controversial but I think space+y, space+p might be good default mappings. It's what I've always done and I've seen others do that too (either that or they replaced the default y/p).

I usually replace the default y and p but I think space+y and space+p fits in nicely to the default keybindings.

Oh, let me add this then!

I prefer the register approach TBH - "+y for yanking to primary selection and "*y for secondary which I think can be added later. space+y seems like a good default keybind, but I'd be wary of changing y itself to clipboard yank since it could mangle up other selections made outside of helix; it's better left to the user to decide whether they want that behavior or not (set clipboard=unnamedplus in vim)

@sudormrfbin

Regarding the register approach : it works well when multi selection is not a first class citizen like in (neo)vim, but it's not ideal in our case. System clipboard is unique and destroy distinction between selections, and you have multiple interpretations : you could join all selections together OR only take one of the selection when yanking to system clipboard. When pasting, your only option is pretty much to paste the same thing for all the selections. And when yanking and directly pasting from the register, you get a completely different behavior than the other registers, which while making sense, is arguably surprising. In (neo)vim, this register behavior is indeed a bit different because it will also paste from / yank to system clipboard, but you could use it like a normal vim register. In Helix, you couldn't use that register like a normal helix register.

That's why I decided to not implement this approach at least for now and the more I think about it, the more I don't see this as the right default approach for Helix.

However, I would like to enable people to implement this using user config through future plugins system (by either using hooks or defining a custom function).

@CBenoit
Copy link
Member Author

CBenoit commented Jun 19, 2021

  • I added mappable versions of the commands so that we can type space + y, etc. These can also be mapped through user config.

  • Typable command clipboard-yank-join now also takes an optional parameter to provide a custom separator (default is newline).

    e.g.: selecting A and B with two selections and then typing clipboard-yank-join - will insert A-B in the system clipboard.

@archseer
Copy link
Member

@sudormrfbin Yeah, we started with the + register but we decided to keep things separate for now. It might make sense to include it later so it's usable in more contexts than just :commands, but we'd need to clearly denote that the behavior will differ when multiple selections are used

helix-term/src/commands.rs Show resolved Hide resolved
use anyhow::Result;
use std::borrow::Cow;

pub trait ClipboardProvider: std::fmt::Debug {
Copy link
Member

Choose a reason for hiding this comment

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

I guess this trait gives us the option to introduce more providers later, is that the intent here? Otherwise we could also simply use Option<CommandProvider> and avoid the Box<dyn T>

Copy link
Member Author

@CBenoit CBenoit Jun 19, 2021

Choose a reason for hiding this comment

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

Yeah that was the intent, but I agree this could as well be Option<CommandProvider> right now. I can change that if you prefer. It was maybe a bit premature.

helix-term/src/commands.rs Outdated Show resolved Hide resolved
Comment on lines +2610 to +2625
let max_to = doc.text().len_chars().saturating_sub(1);
let to = std::cmp::min(max_to, range.to() + 1);
Copy link
Member

Choose a reason for hiding this comment

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

This pattern is now frequent enough that maybe we should add a helper function doc.clipped_range or something, it would help with #224. Can be done over there I guess

Copy link
Member Author

@CBenoit CBenoit Jun 19, 2021

Choose a reason for hiding this comment

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

If it helps in #224, it's probably more useful if done there indeed. I'll make sure to use the helper if it gets merged before my PR.

@sudormrfbin
Copy link
Member

Ahh yeah I didn't think about the disparity between yanking multiple selections and pasting a single selection. Makes sense 👍

@sudormrfbin sudormrfbin mentioned this pull request Jun 19, 2021
This commit adds six new commands to interact with system clipboard:
- clipboard-yank
- clipboard-yank-join
- clipboard-paste-after
- clipboard-paste-before
- clipboard-paste-replace
- show-clipboard-provider

System clipboard provider is detected by checking a few environment
variables and executables. Currently only built-in detection is
supported.

`clipboard-yank` will only yank the "main" selection, which is currently the first
one. This will need to be revisited later.

Closes helix-editor#76
System clipboard integration exists now in two favors: typable and
mappable.

Default mappings are:
- SPC p: paste clipboard after
- SPC P: paste clipboard before
- SPC y: join and yank selection to clipboard
- SPC Y: yank main selection to clipboard
- SPC R: replace selections by clipboard contents
@CBenoit
Copy link
Member Author

CBenoit commented Jun 20, 2021

PR updated and rebased on master.

Copy link
Member

@archseer archseer left a comment

Choose a reason for hiding this comment

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

Tested on wayland, works great!

@archseer archseer merged commit ffb54b4 into helix-editor:master Jun 20, 2021
@CBenoit CBenoit deleted the system-clipboard branch June 20, 2021 21:44
@pickfire
Copy link
Contributor

In (neo)vim, this register behavior is indeed a bit different because it will also paste from / yank to system clipboard, but you could use it like a normal vim register. In Helix, you couldn't use that register like a normal helix register.

What does that mean?

@CBenoit
Copy link
Member Author

CBenoit commented Jun 23, 2021

In (neo)vim, this register behavior is indeed a bit different because it will also paste from / yank to system clipboard, but you could use it like a normal vim register. In Helix, you couldn't use that register like a normal helix register.

What does that mean?

Let me try to clarify my paragraph.

In (neo)vim if you copy a selection "A" to " and paste from " you get "A". You get the same result by using + instead except it uses system clipboard.

The behavior would be the same in helix as well when working with a single selection, however helix supports multi selections and system clipboard does not supports multi values.
In helix if you copy "A" and "B" using multi selections to register ", you can paste "A" and "B" separately using multi selections again. However, if you do that using +, first, when yanking, a decision has to be made between "join all selections" or "use main selection only" (because only a single value can be yanked to system clipboard). Then, when you paste you would either get "AB" or "A" (assuming "A" was the main selection) for both selections, which is not how normal helix registers are working as shown above.

Since there is no right default choice for helix with +, I would rather see that as a plugin or user-defined behavior matching user's taste through configuration (pretty much like kakoune in fact).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Yank to system clipboard
5 participants