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 clipboard provider configuration #10839

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

AlfGalf
Copy link

@AlfGalf AlfGalf commented May 28, 2024

This change adds the clipboard-provider setting to the editor section of configuration.

This option can have values of:

  • none (use an internal buffer) (on windows only)
  • windows (use native windows clipboard) (on MacOS only)
  • macos (use pbcopy/pbpaste) (on neiter of the above)
  • wayland
  • xclip
  • xsel
  • win23yank (for wsl) (on all targets with "term")
  • termux
  • tmux
  • term (osc codes)
  • custom (see below for the configuration)

Note for a custom provider the configurations should look like:

[editor.clipboard-provider.config]
copy = ["tee", "test.txt"]
paste = ["cat", "test.txt"]
primary-copy = ["tee", "test-primary.txt"] # optional
primary-copy = ["cat", "test-primary.txt"] # optional

This can be configured at runtime with the usual:

set clipboard-provider term

Note: I was unable to work out a syntax expression for setting a custom provider at runtime. In my opinion this is probably a fine limitation to have but I am curious if there is a correct way I couldn't work out.

This ports over the previous provider selection logic so hopefully the same default behaviour should apply.

I updated the health command to reflect the provider. Note: this required reading the user configurations within the health command which warrants discussion as this seems to not have been done before.

This is my first contribution, I am a C++ developer by profession and a rust hobyist at best so nits and style updates very welcome.

Note: This adds the nonempty crate as a new dependency. This is because I wanted a way to fail parsing custom commands if they were empty and didn't want to write custom parsign Serde code. I do not know what the process is for considering new dependencies and am very willing to change this for an alternative solution when presented with a better one! I looked for a serde annotation as I thought that was likely but couldn't find one.

@AlfGalf
Copy link
Author

AlfGalf commented May 28, 2024

Predictably it wont build on machines other than mine 🙃. I'm going to look into cross compiling and clean this up

@AlfGalf AlfGalf force-pushed the add-clipboard-option branch 2 times, most recently from 8eb67a2 to fda31de Compare May 28, 2024 19:58
@AlfGalf
Copy link
Author

AlfGalf commented May 28, 2024

Okay as far as I can tell this is working on Windows, Ubuntu, and MacOS

@AlfGalf
Copy link
Author

AlfGalf commented May 29, 2024

This is a fix for #8826 by the way

@markstos
Copy link
Contributor

I would personally put even fewer defaults in the core: target covering 80 to 95% of users. So: Mac, Windows, X11, Wayland, maybe term... and that's it? Once the plugin system is online, I imagine people will be able to provide plugins that support less common options.

@AlfGalf
Copy link
Author

AlfGalf commented May 29, 2024

I would personally put even fewer defaults in the core: target covering 80 to 95% of users. So: Mac, Windows, X11, Wayland, maybe term... and that's it? Once the plugin system is online, I imagine people will be able to provide plugins that support less common options.

Ive matched the previous modes and default behavior.
I would be hesitant to remove any supported mode as that may create regressions for users. (Though maybe that comes from too much compiler development)

@AlfGalf
Copy link
Author

AlfGalf commented May 30, 2024

Ping for this PR?
Do I need to do anything to request reviews?

@kirawi
Copy link
Member

kirawi commented May 31, 2024

There's nothing you need to do. It seems like two of the main maintainers are away from Helix right now, so you may have to wait a little.

@kirawi kirawi added the A-helix-term Area: Helix term improvements label May 31, 2024
@AlfGalf
Copy link
Author

AlfGalf commented May 31, 2024

There's nothing you need to do. It seems like two of the main maintainers are away from Helix right now, so you may have to wait a little.

Ahhh okay great thank you

helix-view/Cargo.toml Outdated Show resolved Hide resolved
@@ -1169,6 +1173,7 @@ impl Editor {
pub fn refresh_config(&mut self) {
let config = self.config();
self.auto_pairs = (&config.auto_pairs).into();
self.registers.clipboard_provider = config.clipboard_provider.get_provider();
Copy link
Member

Choose a reason for hiding this comment

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

I believe this will reset the internal buffers (within NoneProvider for example) when you reload config even if you don't change your clipboard config. I'm not sure there's an easy way to fix this though. We could consider a much larger refactor for clipboard providers to remove state from the clipboard providers (since we store yanks and pastes on the Registers type anyways), and probably remove the clipboard provider trait and just use the enum directly. I'm not sure how large of a change this would take though - I will try out some changes locally and revisit this

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I can think of some gross solutions to this, but this seemed like the least bad option?

@the-mikedavis the-mikedavis changed the title Add clipboard provider configuration (#8826) Add clipboard provider configuration Jun 3, 2024
@kirawi kirawi added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 4, 2024
@AlfGalf
Copy link
Author

AlfGalf commented Jun 6, 2024

Sorry for the delay, Ive been traveling, I will try address those comments!

This change adds the `clipboard-provider` setting to the `editor` section
of configuration.

This option can have values of:
- `none` (use an internal buffer)
(on windows only)
- `windows` (use native windows clipboard)
(on MacOS only)
- `macos` (use pbcopy/pbpaste)
(on neiter of the above)
- `wayland`
- `xclip`
- `xsel`
- `win23yank` (for wsl)
(on all targets with "term")
- `termux`
- `tmux`
- `term` (osc codes)
- `custom` (see below for the configuration)

Note for a custom provider the configurations should look like:
```toml
[editor.clipboard-provider.config]
copy = ["tee", "test.txt"]
paste = ["cat", "test.txt"]
primary-copy = ["tee", "test-primary.txt"] // optional
primary-copy = ["cat", "test-primary.txt"] // optional
```

This can be configured at runtime with the usual:
```
set clipboard-provider term
```
Note: I was unable to work out a syntax expression for setting a `custom`
provider at runtime. In my opinion this is probably a fine limitation to
have but I am curious if there is a correct way I couldn't work out.

This ports over the previous provider selection logic so hopefully the
same default behaviour should apply.

I updated the health command to reflect the provider.
Note: this required reading the user configurations within the health command
which warrants discussion as this seems to not have been done before.

This is my first contribution, I am a C++ developer by profession and
a rust hobyist at best so nits and style updates very welcome.

Note: This adds the `nonempty` crate as a new dependency.
This is because I wanted a way to fail parsing custom commands if they
were empty and didn't want to write custom parsign Serde code.
I do not know what the process is for considering new dependencies and
am very willing to change this for an alternative solution when presented
with a better one! I looked for a `serde` annotation as I thought that
was likely but couldn't find one.
- Cleaned up clippy lints
- Removed `nonempty` dependency in favor of `helix-stdx-nonempty.rs`
- Changed Register initialisation signature to take clipboard provider
  rather than configuration struct.
@AlfGalf
Copy link
Author

AlfGalf commented Jun 12, 2024

Just rebased this onto master

@kirawi kirawi added S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 16, 2024
@AlfGalf
Copy link
Author

AlfGalf commented Jul 5, 2024

Ping?

@AlfGalf
Copy link
Author

AlfGalf commented Jul 5, 2024

I have made the required changes so I think the "waiting on author" isn't correct

@the-mikedavis
Copy link
Member

The current tag is waiting-on-review

@AlfGalf
Copy link
Author

AlfGalf commented Jul 5, 2024

Oh my apologies! I completely misread the log

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ability to control which backend Helix uses for clipboards?
4 participants