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

Merged
merged 1 commit into from
Nov 20, 2024

Conversation

AlfGalf
Copy link
Contributor

@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
Contributor 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
Contributor Author

AlfGalf commented May 28, 2024

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

@AlfGalf
Copy link
Contributor 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
Contributor 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
Contributor 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
Contributor 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
Contributor 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
@the-mikedavis the-mikedavis linked an issue Jun 3, 2024 that may be closed by this pull request
@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
Contributor Author

AlfGalf commented Jun 6, 2024

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

@AlfGalf AlfGalf force-pushed the add-clipboard-option branch from bbee06c to d26be6f Compare June 12, 2024 23:35
@AlfGalf
Copy link
Contributor 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
Contributor Author

AlfGalf commented Jul 5, 2024

Ping?

@AlfGalf
Copy link
Contributor 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
Contributor Author

AlfGalf commented Jul 5, 2024

Oh my apologies! I completely misread the log

@AlfGalf AlfGalf force-pushed the add-clipboard-option branch from d26be6f to af83df4 Compare September 9, 2024 10:26
@AlfGalf
Copy link
Contributor Author

AlfGalf commented Sep 9, 2024

Rebased this to master

@AlfGalf AlfGalf force-pushed the add-clipboard-option branch from af83df4 to 860f002 Compare September 9, 2024 10:47
@AlfGalf
Copy link
Contributor Author

AlfGalf commented Sep 9, 2024

I have also added documentation for this option.

@AlfGalf AlfGalf force-pushed the add-clipboard-option branch from c631b13 to 883a22e Compare October 16, 2024 20:33
@AlfGalf
Copy link
Contributor Author

AlfGalf commented Oct 16, 2024

Ahh yes, makes sense, resolved

@the-mikedavis the-mikedavis 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 Oct 16, 2024
@sbromberger
Copy link
Contributor

Just FYI, I tested this using blink.sh (ipad) over mosh and tmux, with provider termcode and it works perfectly. Thank you!

@AlfGalf
Copy link
Contributor Author

AlfGalf commented Oct 26, 2024

Just FYI, I tested this using blink.sh (ipad) over mosh and tmux, with provider termcode and it works perfectly. Thank you!

Awesome! I'm really glad to hear it!

@sbromberger
Copy link
Contributor

Any chance this could be merged? I've been using it for a week or so and have come to rely on it for clipboard access.

Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for working on this @AlfGalf! It ended up being a larger refactor than was strictly necessary to make config possible but I really like the way we have clipboard.rs laid out now.

@AlfGalf
Copy link
Contributor Author

AlfGalf commented Nov 14, 2024

Thanks for the help with it @the-mikedavis, it's been really fun and educational, I do still have a plan for a hideous asynchronous parsing patch but that will wait until work has quietened down.

@the-mikedavis the-mikedavis merged commit 68ee876 into helix-editor:master Nov 20, 2024
6 checks passed
@sbromberger
Copy link
Contributor

Thank you!!

@llakala
Copy link

llakala commented Nov 20, 2024

After an update to my flake inputs, the default value here doesn't seem to be properly set. It should be using wl-copy, but pasting was simply doing nothing. I've since reverted to an older commit before this, but I can try to get some more replication info.

Edit: can confirm it's this commit specifically that breaks it.

@the-mikedavis
Copy link
Member

Ah I see the issue, yank and paste commands are swapped. It's a bit unintuitive because to yank from the clipboard we want the clipboard program to paste and to paste we want it to yank.

the-mikedavis added a commit that referenced this pull request Nov 20, 2024
the-mikedavis added a commit that referenced this pull request Nov 20, 2024
The configuration here is not super intuitive - in order to yank from
a clipboard provider we want it to paste the clipboard and in order to
paste to the clipboard we want it to yank the contents we pass.

This swaps the programs for yank and paste to align with that.

Ref #10839
@llakala
Copy link

llakala commented Nov 21, 2024

Ah I see the issue, yank and paste commands are swapped. It's a bit unintuitive because to yank from the clipboard we want the clipboard program to paste and to paste we want it to yank.

Working great now! Thanks for the quick fix.

the-mikedavis added a commit that referenced this pull request Dec 2, 2024
This fixes reading from the clipboard when using the termcode provider.
Reading isn't implemented for the termcode provider so `get_contents`
returns `ClipboardError::ReadingNotSupported`. `read_from_clipboard`
needs to recognize this case and use the saved values instead of
emitting an error log and returning nothing.

Follow-up of #10839
Also see #12142
GladkihEgor pushed a commit to GladkihEgor/helix that referenced this pull request Jan 4, 2025
GladkihEgor pushed a commit to GladkihEgor/helix that referenced this pull request Jan 4, 2025
GladkihEgor pushed a commit to GladkihEgor/helix that referenced this pull request Jan 4, 2025
The configuration here is not super intuitive - in order to yank from
a clipboard provider we want it to paste the clipboard and in order to
paste to the clipboard we want it to yank the contents we pass.

This swaps the programs for yank and paste to align with that.

Ref helix-editor#10839
GladkihEgor pushed a commit to GladkihEgor/helix that referenced this pull request Jan 4, 2025
This fixes reading from the clipboard when using the termcode provider.
Reading isn't implemented for the termcode provider so `get_contents`
returns `ClipboardError::ReadingNotSupported`. `read_from_clipboard`
needs to recognize this case and use the saved values instead of
emitting an error log and returning nothing.

Follow-up of helix-editor#10839
Also see helix-editor#12142
diucicd pushed a commit to diucicd/helix that referenced this pull request Jan 8, 2025
diucicd pushed a commit to diucicd/helix that referenced this pull request Jan 8, 2025
diucicd pushed a commit to diucicd/helix that referenced this pull request Jan 8, 2025
The configuration here is not super intuitive - in order to yank from
a clipboard provider we want it to paste the clipboard and in order to
paste to the clipboard we want it to yank the contents we pass.

This swaps the programs for yank and paste to align with that.

Ref helix-editor#10839
diucicd pushed a commit to diucicd/helix that referenced this pull request Jan 8, 2025
This fixes reading from the clipboard when using the termcode provider.
Reading isn't implemented for the termcode provider so `get_contents`
returns `ClipboardError::ReadingNotSupported`. `read_from_clipboard`
needs to recognize this case and use the saved values instead of
emitting an error log and returning nothing.

Follow-up of helix-editor#10839
Also see helix-editor#12142
rmburg pushed a commit to rmburg/helix that referenced this pull request Jan 20, 2025
rmburg pushed a commit to rmburg/helix that referenced this pull request Jan 20, 2025
rmburg pushed a commit to rmburg/helix that referenced this pull request Jan 20, 2025
The configuration here is not super intuitive - in order to yank from
a clipboard provider we want it to paste the clipboard and in order to
paste to the clipboard we want it to yank the contents we pass.

This swaps the programs for yank and paste to align with that.

Ref helix-editor#10839
rmburg pushed a commit to rmburg/helix that referenced this pull request Jan 20, 2025
This fixes reading from the clipboard when using the termcode provider.
Reading isn't implemented for the termcode provider so `get_contents`
returns `ClipboardError::ReadingNotSupported`. `read_from_clipboard`
needs to recognize this case and use the saved values instead of
emitting an error log and returning nothing.

Follow-up of helix-editor#10839
Also see helix-editor#12142
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
6 participants