Connect clipboard functionality to their keybindings

[d-bingham]

Summary of the Pull Request

Connects copy and paste keybindings to the appropriate clipboard functionality within Terminal.

References

#1072, #968

PR Checklist

• Closes Copy & Paste Keybindings #968
• CLA signed. If not, go over here and sign the CLA
• Tests added/passed
• Requires documentation to be updated
• I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: Implement copy and paste for keyboard shortcuts #1072, Copy & Paste Keybindings #968 his connects the (mostly) pre-existing copy/paste mechanics to the keyboard shortcut mechanics.

Detailed Description of the Pull Request / Additional comments

Changes:

• Added TermControl::PasteTextFromClipboard which does what it says on the tin. Refactored the paste code from the right-click handling in TermControl here.
• Added App::_PasteText, mirroring the pre-existing App::_CopyText
• Added connections from keybindings to copy/paste functionality in App::_HookupKeyBindings

Notes:

• This PR interprets the keybinding "copy" as a copy trimming trailing whitespace (the operation performed by a right-click with a selection active). It may be appropriate to add a keybinding for a whitespace-preserving copy (which is now done as a shift-right-click with a selection active).

[carlos-zamora]

The other major change is that we need to make sure the VT sequences for these keybindings go through to the terminal. This is important because in CMD, CTRL+C stops the output. So if we prevent that from coming in, we're gonna be creating a MASSIVE bug :(. Not sure if this got resolved by the PR adding keybindings to the settings.

[d-bingham]

If I recall correctly, the parameter for _CopyText() decides whether or not to copy new lines (usually determined by the SHIFT button). So we'll have to detect that too and use that for this parameter (we'll probably need to make that button rebindable too).

Right-click checks for shift and toggles the copy mode. If we implement a toggle key for the keyboard shortcut, one of the following happens:

• we choose a non-bindable toggle key, and invalidate various keybindings as a result (e.g., I want the trim-whitespace-copy to fire on ctrl-shift-c, and if the toggle is shift, that doesn't work)
• we implement a keybinding for "toggle whitespace-trimming on copy" which means there's a toggle keybinding for exactly one command that can use the toggle and that implementation is both more complex and is less configurable than just adding a keybinding for "copy-without-whitespace-trimming".

So I think the best (or, at a minimum, "least bad") solution here is to add a separate keybinding for whitespace-preserving copy. I'm also unclear how much actual use this whitespace-preserving copy sees.

Regarding passing ctrl-c to the terminal in all cases... that's not really appropriate to clipboard stuff per se, and should be handled generically in the keybinding code. But in any case, I'd hope the default keybinding for copy is ctrl+ins or ctrl+shift+c precisely because (in my opinion) there is no elegant handling for copy being ctrl+c anyway -- you either cannot send a ^C to the terminal, or you are forced to do so when copying to the clipboard from the keyboard...

[zadjii-msft]

This seems fine to me for now - it doesn't add a default keybinding for these, so we can discuss that later (though I think we're all pretty highly in favor of ctrl+shift+c/v).

I think we can discuss the potential for adding a non-trimming copy keybinding later

[carlos-zamora]

If I recall correctly, the parameter for _CopyText() decides whether or not to copy new lines (usually determined by the SHIFT button). So we'll have to detect that too and use that for this parameter (we'll probably need to make that button rebindable too).

Right-click checks for shift and toggles the copy mode. If we implement a toggle key for the keyboard shortcut, one of the following happens:

• we choose a non-bindable toggle key, and invalidate various keybindings as a result (e.g., I want the trim-whitespace-copy to fire on ctrl-shift-c, and if the toggle is shift, that doesn't work)
• we implement a keybinding for "toggle whitespace-trimming on copy" which means there's a toggle keybinding for exactly one command that can use the toggle and that implementation is both more complex and is less configurable than just adding a keybinding for "copy-without-whitespace-trimming".

So I think the best (or, at a minimum, "least bad") solution here is to add a separate keybinding for whitespace-preserving copy. I'm also unclear how much actual use this whitespace-preserving copy sees.

Hmm... well, SHIFT serves as a modifier to a classic copy. So I guess this is more of a discussion of "should modifiers be rebindable (and how to represent that)". In that case, let's just hard-code SHIFT like we did with mouse selection copy/paste. That way that feature is still in. You can take a look at TermControl.cpp::_MouseClickHandler(). I'll create an issue to talk about this in a separate thread.

[d-bingham]

Hmm... well, SHIFT serves as a modifier to a classic copy. So I guess this is more of a discussion of "should modifiers be rebindable (and how to represent that)". In that case, let's just hard-code SHIFT like we did with mouse selection copy/paste. That way that feature is still in. You can take a look at TermControl.cpp::_MouseClickHandler(). I'll create an issue to talk about this in a separate thread.

Do you mean in the right-click copy, or for any copy key binding? If the latter, the "standard" copy key binding of ctrl-shift-c (per @zadjii-msft above) doesn't function correctly.

I don't think using hard-coded modifiers on bindable keys is a good idea for that reason. And if there's only one shortcut affected by the modifier, implementing it as a bindable modifier is kinda pointless.

[carlos-zamora]

Regarding passing ctrl-c to the terminal in all cases... that's not really appropriate to clipboard stuff per se, and should be handled generically in the keybinding code. But in any case, I'd hope the default keybinding for copy is ctrl+ins or ctrl+shift+c precisely because (in my opinion) there is no elegant handling for copy being ctrl+c anyway -- you either cannot send a ^C to the terminal, or you are forced to do so when copying to the clipboard from the keyboard...

Well, the current behavior for non-Windows Terminal is (in pseudocode):

if (selectionActive)
{
    // copy selection
}
else
{
    // send ^C through
}

So I think I just figured out how we can get around this issue. We'll have to do that check when we commit that action in AppKeyBindings.cpp::_DoAction(). I should be able to do it and move stuff around appropriately so don't worry about it. 😊 I'll write it and submit that as a PR soon.

[carlos-zamora]

Hmm... well, SHIFT serves as a modifier to a classic copy. So I guess this is more of a discussion of "should modifiers be rebindable (and how to represent that)". In that case, let's just hard-code SHIFT like we did with mouse selection copy/paste. That way that feature is still in. You can take a look at TermControl.cpp::_MouseClickHandler(). I'll create an issue to talk about this in a separate thread.

Do you mean in the right-click copy, or for any copy key binding? If the latter, the "standard" copy key binding of ctrl-shift-c (per @zadjii-msft Mike Griese FTE above) doesn't function correctly.

I don't think using hard-coded modifiers on bindable keys is a good idea for that reason. And if there's only one shortcut affected by the modifier, implementing it as a bindable modifier is kinda pointless.

Yeah. So then let's not worry about making the modifier re-bindable. But would you be able to figure out how to detect if the SHIFT key was pressed so that we can pass that in to the _CopyText(bool) function?

After that, we should be good. 😊

[d-bingham]

Yeah. So then let's not worry about making the modifier re-bindable. But would you be able to figure out how to detect if the SHIFT key was pressed so that we can pass that in to the _CopyText(bool) function?

I can do that, but that literally breaks the keybinding, since you could no longer bind whitespace-trimming copy to ctrl-shift-c. Can we do it in a way that actually allows whitespace-trimming copy to be able to be bound to ctrl-shift-c?

[carlos-zamora]

Yeah. So then let's not worry about making the modifier re-bindable. But would you be able to figure out how to detect if the SHIFT key was pressed so that we can pass that in to the _CopyText(bool) function?

I can do that, but that literally breaks the keybinding, since you could no longer bind whitespace-trimming copy to ctrl-shift-c. Can we do it in a way that actually allows whitespace-trimming copy to be able to be bound to ctrl-shift-c?

Ok. Sorry. I've been jumping around all day. Just talked it over with @DHowett-MSFT and @zadjii-msft. Here's the plan:

• let's create a new keybinding action titled something like "CopyTextWithoutNewlines"
• then we'll hook that up like we did here, but it'll be slightly different and we'll just call _CopyText(false)

That way, that specific action can be bindable and we can get both functionalities in. If you feel comfortable working on it, add it in. If not, let me know and I can just add it in after this PR goes in 😊 Up to you

[d-bingham]

I can get that this evening.

[d-bingham]

Ok, "copyTextWithoutNewlines" option added.

Is it safe to add ctrl+shift+c/ctrl+shift+v as defaults for copy (with newlines) and paste?

[carlos-zamora]

Ok, "copyTextWithoutNewlines" option added.

Is it safe to add ctrl+shift+c/ctrl+shift+v as defaults for copy (with newlines) and paste?

Awesome! Thanks!

Go for it. That shouldn't interfere with passing the ^C along.

[carlos-zamora]

[carlos-zamora]

Just fix the "Terminal CI (Static Analysis Build x64)" and it should be good:
"code formatting bad, run Invoke-CodeFormat on branch"

[d-bingham]

Got it.

[ghost]

🎉Windows Terminal Preview v0.3.2142.0 has been released which incorporates this pull request.:tada:

Handy links:

• Release Notes
• Store Download