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

[vi-mode] Add ViClipboardMode Option to Support Copying and Pasting to System Clipboard #3769

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

Conversation

WestRyanK
Copy link

@WestRyanK WestRyanK commented Aug 2, 2023

PR Summary

This pull request adds the ViClipboardMode option. This option has two values: ViRegister and SystemClipboard.

ViRegister, the default, maintains the original behavior for copying and pasting in Vi edit mode, which was to copy and paste using a buffer that was local to the current instance of PSReadLine. With this original behavior, text cannot be copied to/from other applications using Vi keys and it can't be copied between separate panes or windows of PowerShell.

The other value, SystemClipboard, changes the copy and paste behavior in Vi edit mode. When this value is used, text is copied and pasted from the system's clipboard when the user uses Vi key commands. This allows the user to copy and paste text from external applications using "y" or "p". It also allows the user to copy and paste text between different panes or windows of PowerShell.

PR Checklist

  • PR has a meaningful title
    • Use the present tense and imperative mood when describing your changes
  • Summarized changes
  • Make sure you've added one or more new tests
  • Make sure you've tested these changes in terminals that PowerShell is commonly used in (i.e. conhost.exe, Windows Terminal, Visual Studio Code Integrated Terminal, etc.)
  • User-facing changes
Microsoft Reviewers: codeflow:open?pullrequest=https://github.com/PowerShell/PSReadLine/pull/3769&drop=dogfoodAlpha

@WestRyanK
Copy link
Author

@microsoft-github-policy-service agree

This option allows the user to choose if they want the ViRegister to use
the system clipboard or the default local clipboard. By setting
ViClipboardMode to SystemClipboard, the user can easily copy and paste
text in the PowerShell prompt using Vi shortcuts in Command mode.
@WestRyanK WestRyanK marked this pull request as ready for review August 2, 2023 21:25
@springcomp
Copy link
Contributor

springcomp commented Aug 18, 2023

When I initially introduced the ViRegister class, I envisioned supporting copy/paste from the system clipboard as well.

In my mind, this would have been done using the " (double-quote) or + (plus-sign) named registers.
This way, at any single time, one can use the default unnamed register, or the system register as it is customary in vim and many VIM emulations.

May I respectfully ask what you think ? Would you mind me proposing my help to you on this feature ?

I see that you have done a ton of work for the documentation already, so this maybe a bit too late on my part… 😥

@WestRyanK
Copy link
Author

WestRyanK commented Aug 18, 2023

The docs weren't a lot of work, so I don't mind changing the feature and redoing them.

I've been using my branched PSReadLine personally since I posted the pull request. It works well, but there are some situations that have been annoying. If I delete a character, word, or line, the text always gets copied to the clipboard with my changes. Occasionally I actually do want to copy the text that I delete, but most of the time I don't. All of that to say... I want to change how my feature works before merging.

What if we did this compromise solution?

  • Create another set of Vi command functions that copy to the system clipboard instead of the Vi Register. So we'd have a function that Yanks to the ViRegister and another that Yanks to the system clipboard. Same with paste, and delete commands. The user could then map keys to either command.

Reasons why I like this solution:

  • Highly configurable. The user can decide which register to use for any keypress combination. It would provide them with the flexibility to do things like map p to paste from the system clipboard, map "p to paste from Vi register, but x deletes to the Vi register.
  • It fixes my issue of deletes going to the system clipboard
  • It could support my wish to default yy yank and p paste to system clipboard for fewer keypresses without needing a " prefix.
  • It could support your idea of using "p for or "yy to access the system clipboard and p or yy to use the Vi register.
  • Gets rid of the PSReadLineOption that I had created.

Thoughts?

@springcomp
Copy link
Contributor

springcomp commented Aug 19, 2023

@WestRyanK, thank you for your feedback.

That is an interesting proposal and I understand what you are trying to achieve. I do have some concerns though.

Mainly, I think if we strive to emulate the VIM keybindings and behaviour, using named registers would be the least surprising option. Deviating from this would create a kind of vim-but-not-quite-like emulation that may create a precedent for other behaviours that future users may want to include in all sorts of crazy ways.

Second, I think what you want could be achieved by using the builtin key-rebinding capabilities of PSRL. That is, you could create a custom profile that reassigns whatever key bindings you would like to the appropriate function or sequence of functions to use the default or the named register depending on your needs.

For instance, you would like to reassign yy to use the system clipboard instead of its default internal register. If we are careful enough to support public functions, that could be achieved using something like the following script in your $profile :

Set-PSReadLineKeyHandler ` 
    -ViMode Command `
    -Chord 'y,y' `
    -BriefDescription "Copy to clipboard" `
    -LongDescription "Yank a set of lines to the system clipboard" `
    -ScriptBlock {
    param($key, $arg)

    [Microsoft.PowerShell.PSConsoleReadLine]::ViSelectRegister($key, "+")
    [Microsoft.PowerShell.PSConsoleReadLine]::ViYankLine($key, $arg)
}

We could also include those snippets into the README.md of even in the sample profile file.

The way I see for implementing this would be:

  • Create a new IClipboard interface to abstract away talking to a clipboard, either internal or from the system.
  • Build a ViRegister with an injected IClipboard as it correctly handles linewise yanks and paste.
  • Create two new implementations InternalClipboard and SystemClipboard to store/retrieve text from a clipboard.
  • Hold a dictionary of ViRegister implementations, keyed by name:
private readonly ViRegister _clipboard;
private readonly ViRegister _systemClipboard;
private readonly IDictionary<string, ViRegister> _registers;
// ctor
static PSConsoleReadLine()
{
  _clipboard = new ViUnnamedRegister(_singleton);
  _systemClipboard = new ViSystemClipboard(_singleton);
  _registers  = new Dictionary<string, ViRegister>{
    [""] = _clipboard,
    ["\""] = _systemClipboard,
    ["+"] = _systemClipboard,
  }
}
  • Create public functions to select the register, like the ViSelectRegister function shown in the snippet above
  • Assign key-bindings to those functions

At this point, I think we would be mostly done.

It would be very easy to actually add named registers in the future: i.e creating public functions to create new registers on the fly but I would refrain from this in this PR.

Please, let me know how you feel about this.

@WestRyanK
Copy link
Author

Oh, I started implementing the feature based on your comments before I realized that you had actually implemented it yourself. I suppose I'll let you finish it then?

@springcomp
Copy link
Contributor

I would be more comfortable leaving the spirit of the changes to you.
Please, feel free to tear up my PR as you see fit and propose an iteration to the real project.
Would that work for you ?

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

2 participants