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 OS clipboard only for explicit cut/copy/paste operations #761

Conversation

Tastaturtaste
Copy link
Contributor

This PR addresses #759.

This PR reverts most commands to using the local cut buffer for cut/copy/paste operations except for CutSelection and CopySelection. Those two commands now have a parameter to specify which clipboard to use. Current keybindings for those commands (CutSelection: Ctrl+Shift+X, CopySelection: Ctrl+Shift+V) still use the system clipboard. Currently, no keybindings are defaulted for these commands and the cut buffer.

We can't statically compile everything related to the system_clipboard feature out, as that would include the parametrized commands mentioned above. Activating the feature would not be purely additive anymore. Therefore those parameters have to stay even when the feature is disabled and the commands have to do something when the feature is not enabled. Currently, there is no facility in place for EditCommands to fail in a recoverable manner or indicate failure to the user. A few possibilities how to handle this situation that come to mind:

  1. Panic if the OS clipboard would have to be used. The user of the crate would be responsible to ensure commands using the OS clipboard are only used when the feature is enabled.
  2. Silently do nothing when the OS clipboard would have to be used
  3. Silently use the local clipboard, even when the command explicitly requests the OS clipboard

Of these options, I currently implement panicking. I think it is the least surprising thing to do for users of reedline. Requesting to use the OS clipboard while not having system_clipboard enabled is surely a logic bug. But I am open to discussion if someone sees it another way.

The changes can be used with this nushell branch, where I use this reedline branch to test things out.

@Tastaturtaste Tastaturtaste changed the title Bug/759 system clipboard not for everything Use the OS clipboard only for explicit cut/copy/paste operations Feb 27, 2024
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 for the quick turnaround!

Looks good apart from how we should handle the case if the feature is not enabled.

Comment on lines 538 to 531
fn cut_selection_to_system(&mut self) {
#[cfg(not(feature = "system_clipboard"))]
panic!("The OS clipboard is not enabled. Enable its use with the reedline/system_clipboard feature!");
#[cfg(feature = "system_clipboard")]
if let Some((start, end)) = self.get_selection() {
Copy link
Member

Choose a reason for hiding this comment

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

From the "don't panic" Nushell side this is a dangerous panic footgun.

EditCommand::Paste { system_clipboard: true } provides a possibly more aesthetic API over separate variants. But if you were to compile without system_clipboard and pass the same event this could panic the shell.

EditCommand::Paste {
system_clipboard: true,
} => self.paste_system_clipboard(),
EditCommand::Paste {
system_clipboard: false,
} => self.paste_cut_buffer(),

The match compiles either way. This would require extra care while parsing complex user configs.

Either we completely restrict it at compile time via separate variants or turn it into something handlable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume turning it into something handlable would involve returning a Result up the call stack so nushell can do something like inform the user? In general I think that might be a good idea, it also opens up for other failable commands should the need arise. Though I don't know if that is the right approach for handling this specific issue. What is your opinion on it? How should it be handled?

Copy link
Member

Choose a reason for hiding this comment

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

Let's keep it simple for now. As the problem only arises in connection with the choice of flags, we can keep those in separate variants that are cfged themelves.
If a library user provides the flag they can configure reedline to their pleasing but if they choose not to they don't get the added complexity.

@Tastaturtaste Tastaturtaste force-pushed the bug/759-system-clipboard-not-for-everything branch from 998a2f7 to 39ba760 Compare March 12, 2024 19:17
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 for championing this! Let's get this moving.

@sholderbach sholderbach merged commit dc7063e into nushell:main Mar 12, 2024
6 checks passed
@Tastaturtaste
Copy link
Contributor Author

Ok, that was fast :D

If you want to give it a try: https://github.com/Tastaturtaste/nushell/tree/bug/11907-system-clipboard-not-for-everything

@fdncred
Copy link
Collaborator

fdncred commented Mar 12, 2024

Thanks again @Tastaturtaste.

leftwo referenced this pull request in oxidecomputer/crucible Aug 17, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [reedline](https://togithub.com/nushell/reedline) |
workspace.dependencies | minor | `0.30.0` -> `0.33.0` |

---

### Release Notes

<details>
<summary>nushell/reedline (reedline)</summary>

###
[`v0.33.0`](https://togithub.com/nushell/reedline/releases/tag/v0.33.0)

[Compare
Source](https://togithub.com/nushell/reedline/compare/v0.32.0...v0.33.0)

#### What's Changed

- fix some new clippy warnings by
[@&#8203;fdncred](https://togithub.com/fdncred) in
[https://github.com/nushell/reedline/pull/790](https://togithub.com/nushell/reedline/pull/790)
- Add PWD to the `Reedline` state by
[@&#8203;YizhePKU](https://togithub.com/YizhePKU) in
[https://github.com/nushell/reedline/pull/796](https://togithub.com/nushell/reedline/pull/796)
- Fix [#&#8203;793](https://togithub.com/nushell/reedline/issues/793)
using width() for column menu alignements with special char by
[@&#8203;Jiogo18](https://togithub.com/Jiogo18) in
[https://github.com/nushell/reedline/pull/794](https://togithub.com/nushell/reedline/pull/794)
- Make menus process events before updating working details by
[@&#8203;ysthakur](https://togithub.com/ysthakur) in
[https://github.com/nushell/reedline/pull/799](https://togithub.com/nushell/reedline/pull/799)
- Feature: vi visual mode by
[@&#8203;adamschmalhofer](https://togithub.com/adamschmalhofer) in
[https://github.com/nushell/reedline/pull/800](https://togithub.com/nushell/reedline/pull/800)

#### New Contributors

- [@&#8203;YizhePKU](https://togithub.com/YizhePKU) made their first
contribution in
[https://github.com/nushell/reedline/pull/796](https://togithub.com/nushell/reedline/pull/796)
- [@&#8203;Jiogo18](https://togithub.com/Jiogo18) made their first
contribution in
[https://github.com/nushell/reedline/pull/794](https://togithub.com/nushell/reedline/pull/794)
- [@&#8203;adamschmalhofer](https://togithub.com/adamschmalhofer) made
their first contribution in
[https://github.com/nushell/reedline/pull/800](https://togithub.com/nushell/reedline/pull/800)

**Full Changelog**:
nushell/reedline@v0.32.0...v0.33.0

###
[`v0.32.0`](https://togithub.com/nushell/reedline/releases/tag/v0.32.0):
0.32.0

[Compare
Source](https://togithub.com/nushell/reedline/compare/v0.31.0...v0.32.0)

#### What's Changed

- add bashism `!term` to prefix search for last command beginning with
`term` by [@&#8203;fdncred](https://togithub.com/fdncred) in
[https://github.com/nushell/reedline/pull/779](https://togithub.com/nushell/reedline/pull/779)
- Remove debug print by
[@&#8203;sholderbach](https://togithub.com/sholderbach) in
[https://github.com/nushell/reedline/pull/784](https://togithub.com/nushell/reedline/pull/784)
- fix ide menu not reporting correct required_lines by
[@&#8203;maxomatic458](https://togithub.com/maxomatic458) in
[https://github.com/nushell/reedline/pull/781](https://togithub.com/nushell/reedline/pull/781)
- Fix (properly) the logic around prompt re-use & Host Command handling
by [@&#8203;bew](https://togithub.com/bew) in
[https://github.com/nushell/reedline/pull/770](https://togithub.com/nushell/reedline/pull/770)
- fix: unexpected spaces after large buffer input by
[@&#8203;sigoden](https://togithub.com/sigoden) in
[https://github.com/nushell/reedline/pull/783](https://togithub.com/nushell/reedline/pull/783)
- Bump version for `0.32.0` release by
[@&#8203;devyn](https://togithub.com/devyn) in
[https://github.com/nushell/reedline/pull/785](https://togithub.com/nushell/reedline/pull/785)

#### New Contributors

- [@&#8203;bew](https://togithub.com/bew) made their first contribution
in
[https://github.com/nushell/reedline/pull/770](https://togithub.com/nushell/reedline/pull/770)
- [@&#8203;sigoden](https://togithub.com/sigoden) made their first
contribution in
[https://github.com/nushell/reedline/pull/783](https://togithub.com/nushell/reedline/pull/783)
- [@&#8203;devyn](https://togithub.com/devyn) made their first
contribution in
[https://github.com/nushell/reedline/pull/785](https://togithub.com/nushell/reedline/pull/785)

**Full Changelog**:
nushell/reedline@v0.31.0...v0.32.0

###
[`v0.31.0`](https://togithub.com/nushell/reedline/releases/tag/v0.31.0):
0.31.0

[Compare
Source](https://togithub.com/nushell/reedline/compare/v0.30.0...v0.31.0)

New release for [Nushell](https://togithub.com/nushell/nushell) `0.92.0`

#### What's Changed

- Bump version of `strum`/`strum_macros` by
[@&#8203;sholderbach](https://togithub.com/sholderbach) in
[https://github.com/nushell/reedline/pull/768](https://togithub.com/nushell/reedline/pull/768)
- Use the OS clipboard only for explicit cut/copy/paste operations by
[@&#8203;Tastaturtaste](https://togithub.com/Tastaturtaste) in
[https://github.com/nushell/reedline/pull/761](https://togithub.com/nushell/reedline/pull/761)
- Revert "Move left when exiting insert mode" by
[@&#8203;fdncred](https://togithub.com/fdncred) in
[https://github.com/nushell/reedline/pull/773](https://togithub.com/nushell/reedline/pull/773)
- Fix `OpenOptions` clippy by
[@&#8203;sholderbach](https://togithub.com/sholderbach) in
[https://github.com/nushell/reedline/pull/776](https://togithub.com/nushell/reedline/pull/776)
- Bump `fd-lock` requirement and locked deps by
[@&#8203;sholderbach](https://togithub.com/sholderbach) in
[https://github.com/nushell/reedline/pull/775](https://togithub.com/nushell/reedline/pull/775)
- Fix case-consistency searching sqlite history by
[@&#8203;sholderbach](https://togithub.com/sholderbach) in
[https://github.com/nushell/reedline/pull/777](https://togithub.com/nushell/reedline/pull/777)
- Bump version for `0.31.0` release by
[@&#8203;sholderbach](https://togithub.com/sholderbach) in
[https://github.com/nushell/reedline/pull/780](https://togithub.com/nushell/reedline/pull/780)

**Full Changelog**:
nushell/reedline@v0.30.0...v0.31.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "after 8pm,before 6am" in timezone
America/Los_Angeles, Automerge - "after 8pm,before 6am" in timezone
America/Los_Angeles.

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR was generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View the
[repository job
log](https://developer.mend.io/github/oxidecomputer/crucible).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC4yNi4xIiwidXBkYXRlZEluVmVyIjoiMzguMjYuMSIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiZGVwZW5kZW5jaWVzIl19-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
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.

3 participants