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

Implement CTRL-V support on Windows #521

Merged
merged 2 commits into from May 9, 2021
Merged

Implement CTRL-V support on Windows #521

merged 2 commits into from May 9, 2021

Conversation

marcbowes
Copy link
Contributor

This commit implements CTRL-V support on Windows. See #519 where this issue was raised.

Implementation notes

On Windows, we have 3 key changes:

  • There are new Windows-only dependencies on the clipboard-win and error-code crates.
  • tty/windows.rs implements read_pasted_text by calling the clipboard-win crate. Errors are mapped to a new Windows-only error variant.
  • When CTRL-V is pressed, QuotedInsert will perform a yank using the text from the clipboard (inserting the text and moving the cursor).

Testing

I made a new package:

cargo new testpaste
cd testpaste
cargo add rustyline
# edit Cargo.toml; make rustyline use my git fork
# edit src/lib.rs to be the contents of https://github.com/kkawakam/rustyline#example

As noted in #519, running this example in Windows Terminal works (CTRL-V pastes text from the system clipboard) while using Powershell or cmd.exe does not work (CTRL-V does nothing, while rightclick pastes text from the system clipboard).

After this change, CTRL-V works in all 3 shells. Mouse rightclick continues to work.

Other

See #520 for why I reverted the change to use fd-lock.

@gwenn
Copy link
Collaborator

gwenn commented May 8, 2021

  • fd-lock rollback should be done in a dedicated PR after investigation
  • Maybe we should put clipboard-win behind a feature
  • For me, QuotedInsert should not be used but a new dedicated Cmd should be added.

@marcbowes
Copy link
Contributor Author

fd-lock rollback should be done in a dedicated PR after investigation

Sure thing!

Maybe we should put clipboard-win behind a feature

What's the reasoning behind that? IMO, copy-paste should work out the box. Happy to make the change if you want, though. Let me know what the feature flag should be.

For me, QuotedInsert should not be used but a new dedicated Cmd should be added.

Ok. What do you want it to be called?

@marcbowes
Copy link
Contributor Author

Pushed 2 changes:

  • revert the revert
  • added cmd::ClipboardPaste. happy to accept new names

Note that C-q or C-v was used for QuotedInsert before. Now, I've bound C-v to ClipboardPaste by default on Windows. As far as I can tell, C-q doesn't really do anything my terminals on Windows anyways, but this is the smallest change I could make.

@marcbowes
Copy link
Contributor Author

I'm happy to squash commits once you're happy with the work.

@gwenn
Copy link
Collaborator

gwenn commented May 8, 2021

"paste-from-clipboard" is the name used by readline.
So PasteFromClipboard would be perfect but ClipboardPaste is fine...

@gwenn
Copy link
Collaborator

gwenn commented May 8, 2021

Note that C-q or C-v was used for QuotedInsert before. Now, I've bound C-v to ClipboardPaste by default on Windows. As far as I can tell, C-q doesn't really do anything my terminals on Windows anyways, but this is the smallest change I could make.

Don't worry because most of the time Ctrl-Q does nothing on unix platform because by default a signal is sent instead.

src/keymap.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
Fixes #519.

This commit adds a Windows-only dependency on clipboard-win to implement
`read_pasted_text()`. The keymap is updated such that CTRL-V maps
to (the new command) `PasteFromClipboard`.

Before this commit, using mouse rightclick to paste would work in all
terminals, but using CTRL-V to paste would only work in Windows
Terminal (i.e. did _not_ work in cmd.exe and Powershell).

A new Windows-only error variant is added. This can be returned if the
clipboard lookup fails more than 10 times.
@marcbowes
Copy link
Contributor Author

I squashed my commits and cleaned up the commit message. I addressed the two comments above.

src/keymap.rs Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
@marcbowes
Copy link
Contributor Author

After reverting 7055c27 again, I've tested that rustyline compiles and passes all tests on a Windows and macOS machine. I've tested that paste is fixed on my Windows machine and not affected on the macOS machine.

@marcbowes
Copy link
Contributor Author

I see a previous action run failed because of cargo-fmt failures (https://github.com/kkawakam/rustyline/runs/2533459886?check_suite_focus=true). Those are unrelated to my change. I can fix them if you'd like (I've actually been undoing them as I worked to avoid a noisy diff).

@gwenn
Copy link
Collaborator

gwenn commented May 8, 2021

I see a previous action run failed because of cargo-fmt failures (https://github.com/kkawakam/rustyline/runs/2533459886?check_suite_focus=true). Those are unrelated to my change. I can fix them if you'd like (I've actually been undoing them as I worked to avoid a noisy diff).

It may be related to rust 1.52.0: cargo fmt may have changed.

@gwenn gwenn merged commit d48d012 into kkawakam:master May 9, 2021
@gwenn
Copy link
Collaborator

gwenn commented May 9, 2021

Thanks.

@marcbowes
Copy link
Contributor Author

Thanks for helping me with this. Do you have any idea when the next release (to crates.io) will be?

@gwenn
Copy link
Collaborator

gwenn commented May 11, 2021

Nope.

@link2xt
Copy link

link2xt commented May 19, 2021

@marcbowes This PR pulled in str-buf with this issue: DoumanAsh/str-buf#1
It means rustyline now requires at least rust 1.51.0 to build. Maybe indeed put this behind the feature if you want to support older rust versions.

@marcbowes
Copy link
Contributor Author

Oh, that sucks. I'm not the maintainer of this library, so I'm tagging @gwenn for input.

I guess the options are:

  1. Do nothing
  2. Revert this commit and release 8.2.0 and/or:
  3. Re-add this commit either as a new MV (9.0.0) or behind a feature flag

Again, I'm not the maintainer, but if it were up to me I'd revert this commit, release 8.2.0, then readd the commit and release as 9.0.0. IMO, this was an important bugfix and consumers should not have to turn on a feature flag to disable bugs.

@link2xt
Copy link

link2xt commented May 19, 2021

Not sure it's important, but CI failed with rust 1.50.0 at deltachat/deltachat-core-rust#2448
There was one request to keep supporting Debian version of rustc, so I will not upgrade for now.

Probably makes sense to pin rust version in GitHub Actions workflow of rustyline or test across several versions to avoid such surprises later.

@DoumanAsh
Copy link
Contributor

Sorry for this.
I lowered compiler requirement in clipboard-win and error-code and yanked versions where bump was introduced.
Do cargo update and see if it works

@gwenn
Copy link
Collaborator

gwenn commented May 20, 2021

8.2.0 released.

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