search

microsoft / terminal

The new Windows Terminal and the original Windows console host, all in the same place!
MIT License
95.87k stars 8.34k forks source link

Ctrl+Insert does not copy the selected text from Command Palette #9520

Open KalleOlaviNiemitalo opened 3 years ago

KalleOlaviNiemitalo commented 3 years ago

Environment

Windows build number: 10.0.19042.867
Windows Terminal version (if applicable): 1.6.10571.0

Steps to reproduce

  1. Delete any saved Windows Terminal settings.
  2. Start Windows Terminal.
  3. Use the mouse to select some text in the pane.
  4. Press Ctrl+Shift+P to open the command palette.
  5. Type abc.
  6. Press Ctrl+A to select the text abc.
  7. Press Ctrl+Insert.

Expected behavior

The text abc should be copied to the clipboard, and the command palette should remain open.

Actual behavior

The command palette closes, and the text that you selected in the pane is copied to the clipboard.

Notes

If you copy by pressing Ctrl+C instead of Ctrl+Insert, then it works as expected.

KalleOlaviNiemitalo commented 3 years ago

I see, https://github.com/microsoft/terminal/pull/9056 added special handling for Ctrl+C and Ctrl+V in the command palette, but not for Ctrl+Insert and Shift+Insert.

zadjii-msft commented 3 years ago

Wait holy what now - Does ctrl+Ins just work (pretty much) everywhere in the OS? I did not know that. TIL! That sems like an easy enough fix.

KalleOlaviNiemitalo commented 3 years ago

Wikipedia lists Ctrl+Insert copying as being part of the IBM Common User Access standard.

DHowett commented 3 years ago

We have got to figure out what to do about key bindings and how they impact or do not impact certain scopes.

DHowett commented 3 years ago

I'm inclined to reject a pull request at this point that adds a fourth band-aid to the command palette (AND THE SEARCH BOX) by introducing another "did the user press THIS key?" check.

KalleOlaviNiemitalo commented 3 years ago

Maybe the precedence could depend on the action to which the key is bound.

Don-Vito commented 3 years ago

What a headache. Everything starts with us not getting some keys in KeyDown handler (as they get handled by inner controls). As a result we register to PreviewKeyDown, that propagates top down. And this gives the key bindings a priority over the controls (e.g., ctrl+c key binding would get priority over ctrl+c in search box). To address this we have added special flows in the PreviewKeyDown handler, covering ctr+c, but not ctrl+insert (which I would expect to work if it was not bound).

As Dustin mentioned we need to find a good approach.

I really don't understand the decision not to bubble up some keys.

KalleOlaviNiemitalo commented 3 years ago

If I am renaming a tab in Windows Terminal Preview 1.7.572.0 and press the Left arrow key when the insertion point is already at the beginning of the edit box, then it closes the edit box and commits the change as if I had pressed Enter. Is that a consequence of keys being bubbled up and the edit box deciding not to handle the key? If so, I hope this behavior won't spread to the command palette and the find box.

Don-Vito commented 3 years ago

If I am renaming a tab in Windows Terminal Preview 1.7.572.0 and press the Left arrow key when the insertion point is already at the beginning of the edit box, then it closes the edit box and commits the change as if I had pressed Enter. Is that a consequence of keys being bubbled up and the edit box deciding not to handle the key? If so, I hope this behavior won't spread to the command palette and the find box.

@KalleOlaviNiemitalo - not sure how you find all these :smile:. I think this one is unrelated, the Left key switches to another tab (I guess the TabView catches it and decides to switch to an adjacent), causing the renamer box to lose focus. If we want to fix this, we should probably open a separate ticket

eleadufresne commented 1 week ago

Hello everyone! I'd like to address this issue by adding a conditional check for CTRL with either Insert or C for the copy action, similar to the approach used in PR #9056.

I noticed that Insert is just a virtual key (rather than a modifier), which means it doesn't trigger the same way on "KeyDown." So, while this doesn't solve the underlying issues mentioned in the comments, it should fix the copy behaviour in the command palette with minimal changes.

Please let me know if this direction works for you! If this sounds good, I can create a new branch and a draft PR. 😊

JodhwaniMadhur commented 1 week ago

I would also like to take this up.