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

Use the system clipboard only for explicit copy/paste operations. Addresses issue 11907 #12179

Conversation

Tastaturtaste
Copy link
Contributor

Description

With the introduction of the system clipboard to nushell, many commands changed their behavior from using a local cut buffer to the system clipboard, perhaps surprisingly for many users. (See #11907)
This PR changes most of them back to using the local cut buffer and introduces three commands (CutSelectionSystem, CopySelectionSystem and PasteSystem) to explicitly use the system clipboard.

User-Facing Changes

Users who in the meantime already used the system clipboard now default back to the local clipboard. To be able to use the system clipboard again they have to append the suffix system to their current edit command specified in their keybindings.

Tests + Formatting

The commands themselves are tested in reedline. The changes introduces in nushell are minimal and simply forward from a match on the keybinding name to the command.

After Submitting

@Tastaturtaste
Copy link
Contributor Author

The reedline version is still patched via cargo to the github main branch at this time. I guess the dependency should be changed back to a regular reedline release before this PR gets merged?

@sholderbach
Copy link
Member

During development we will happily point to the main branch if we want to pull in changes from reedline. We will release a new version when necessary.

Copy link
Member

@sholderbach sholderbach left a comment

Choose a reason for hiding this comment

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

Small nit, but we should be pretty much good to go.

Thanks again!

@@ -825,18 +825,18 @@ $env.config = {
event: { edit: capitalizechar }
}
{
name: copy_selection
name: copy_selection_system
Copy link
Member

Choose a reason for hiding this comment

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

May be worth adding a comment that this requires the terminal to support the ctrl-shift bindings passing through (and the system-clipboard feature, if it is likely to be disabled on a build).

Cargo.toml Outdated
"which-support",
"trash-support",
"sqlite",
"mimalloc",
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be reverted to the formatting we use in the rest of the tomls, also to keep the git blame easier to read.

@fdncred
Copy link
Collaborator

fdncred commented Mar 14, 2024

@Tastaturtaste Do you have time to help with the suggestions made in this PR? We'd like to get this landed asap if possible.

@fdncred
Copy link
Collaborator

fdncred commented Mar 15, 2024

@sholderbach I took a swing at tweaking this PR so we can get it in and have more testing time. Let me know if it looks good to you.

Copy link
Member

@sholderbach sholderbach left a comment

Choose a reason for hiding this comment

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

Thanks this looks good to me!

@fdncred fdncred merged commit c7e0d4b into nushell:main Mar 15, 2024
16 checks passed
@Tastaturtaste
Copy link
Contributor Author

Hey, thanks for pushing this over the finish line. I was pretty swamped with work and other stuff the last couple of days.

@hustcer hustcer added this to the v0.92.0 milestone Mar 16, 2024
sholderbach added a commit to sholderbach/nushell that referenced this pull request Apr 3, 2024
Some platforms don't support the `system-clipboard` feature, notably
termux on android. The default config currently contained references to
`reedline` events that are only available with the feature enabled
(nushell#12179). This thus broke the out of the box config for those users. For
now be more defensive about this and only enable default events. Add the
alternative as commented out code you can quickly enable.
sholderbach added a commit that referenced this pull request Apr 4, 2024
Some platforms don't support the `system-clipboard` feature, notably
termux on android.
The default config currently contained references to `reedline` events
that are only available with the feature enabled (#12179). This thus
broke the out of the box config for those users.

For now be more defensive about this and only enable default events. Add
the alternative as commented out code you can quickly enable.

## Tested with:

```
cargo run --no-default-features --features default-no-clipboard -- --config crates/nu-utils/src/sample_config/default_config.nu
```
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.

None yet

4 participants