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

Ctrl C, when text is highlighted, should copy not cancel #2285

Closed
mikemaccana opened this issue Aug 6, 2019 · 30 comments · Fixed by #2446
Closed

Ctrl C, when text is highlighted, should copy not cancel #2285

mikemaccana opened this issue Aug 6, 2019 · 30 comments · Fixed by #2446
Assignees
Labels
Area-Interaction Interacting with the vintage console window (as opposed to driving via API or hooks) Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.

Comments

@mikemaccana
Copy link
Contributor

mikemaccana commented Aug 6, 2019

Environment

WIndows 10.0.18362.0 Microsoft Windows NT 10.0.18362.0
Windows Terminal Preview 3

Steps to reproduce

Select text.
Press Ctrl C

Expected behavior

Per Windows behavior, Ctrl C should copy the text.

Actual behavior

Ctrl C cancels, potentially terminating foreground jobs. This makes sense from a terminal point of view, but Windows users are used to hitting Ctrl C to copy text. That there is some text selected is a good indication that the user wishes to copy, not to cancel.

@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Aug 6, 2019
@CobusKruger
Copy link

CobusKruger commented Aug 6, 2019

Yes, you're spot-on about how it should work. Unfortunately, #1093 just "fixed" this by introducing a different keyboard shortcut for the copy function. The correct solution was mentioned several times like here, but kicked to the kerb.

@zadjii-msft
Copy link
Member

@mikemaccana What do you have copyText bound to in your profiles.json? (you may have it bound 0, 1, or many times).

@zadjii-msft zadjii-msft added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 6, 2019
@mikemaccana
Copy link
Contributor Author

@zadjii-msft copyText doesn't appear in my profiles.json. Want me to trash the file and let Terminal recreate it?

@ghost ghost added Needs-Attention The core contributors need to come back around and look at this ASAP. and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Aug 6, 2019
@zadjii-msft
Copy link
Member

zadjii-msft commented Aug 6, 2019

Don't do that, just add a keybinding for copy (that was my bad the action is copy not copyText).

{ "command": "copy", "keys": ["ctrl+c"] },
{ "command": "paste", "keys": ["ctrl+v"] }

should do the trick.

You can set the keybinding to whatever you want. Ctrl+C, Ctrl+Shift+C, Shift+Insert.

@mikemaccana
Copy link
Contributor Author

@zadjii-msft Done - adding that keybinding stops Ctrl C from cancelling when text is selected (yaay) but breaks cancelling when text isn't selected.

@mikemaccana
Copy link
Contributor Author

mikemaccana commented Aug 6, 2019

The comment mentioned earlier indicates a lot of other people have the same idea: #968 (comment)

Ctrl+C only copies when something is selected. If nothing is selected then Ctrl+C behaves as usual and sends an SIGINT.

There's 25 thumbs up.

@zadjii-msft
Copy link
Member

I'm gonna summon @carlos-zamora.

I thought we made copy only work if there was a selection, but I guess we still eat the keystroke. Maybe we shouldn't.

@Renha
Copy link

Renha commented Aug 6, 2019

you definitely should, keyboard input shouldn't depend on mouse input - user shouldn't waste time to grab a mouse for deselection if she needs to stop some program

@mikemaccana
Copy link
Contributor Author

Various thoughts:

  • 25 people voted for making Ctrl C copy if selected, cancel if not selected in the thread above

  • User can just bang Ctrl C and stop the program in most cases, deselection would only be needed if they explicitly selected something first.

  • Additionally, even if they had selected something, hitting Ctrl C twice would deselect/copy and then cancel

  • We could always make it configurable.

@carlos-zamora carlos-zamora added this to the Terminal 1908.1 milestone Aug 6, 2019
@carlos-zamora
Copy link
Member

As of now, we still have keystrokes eating the escape sequence. So you either have a keystroke hooked up to a command, or it emmits the escape sequence. Though it isn't ideal, the current workaround is to bind "copy" to something that won't conflict with the escape sequence you want to emit.

Though making a kind of "smart copy" would work (and I'm adding it to the coming milestone), this does show that there is a bigger issue: how can you emit escape sequences when keystrokes are bound to an action?

Accessibility was top-priority for me last month. Now that we have a somewhat working model (still plenty of work to do there), I'm hoping to spend more time shared between interactivity and accessibility. Please be patient and respectful as we make progress. We do accept PRs, as we can't get to everything between releases.

@carlos-zamora carlos-zamora added Area-Interaction Interacting with the vintage console window (as opposed to driving via API or hooks) Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal. and removed Needs-Attention The core contributors need to come back around and look at this ASAP. Needs-Tag-Fix Doesn't match tag requirements Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Aug 6, 2019
@miniksa
Copy link
Member

miniksa commented Aug 6, 2019

I can only assume it's being blocked by either ego or some internal spaghetti code.

@CobusKruger, please let me refresh you on the Microsoft Open Source Code of Conduct. We can disagree here and have debates, but please halt yourself before assuming the motivations of others and attacking individuals personally or their work.

We are happy to continue to discuss the way that this prerelease product should evolve before it meets v1.0 (and even after 1.0!) Please exercise patience and understanding when working with us and members of the community as the software develops.

@DHowett-MSFT
Copy link
Contributor

Design notes:
Each key binding handler needs to be augmented with a sender (maybe) and an event args (definitely). The event args should have a Handled() just like the key events at the Xaml layer.
This will let the Copy/Paste/MoveToTab (etc.) handlers report whether they did anything in response to the event being raised.
If a handler replies in the negatory, the event should fall through to the standard handlers.

Potential complexity:
We should be using event args to communicate everything about an event. For copy, that includes "filter newlines," and for "new tab" that includes "which profile index." We may cause a bit of an explosion in event argument types.

This will tie in with @zadjii-msft's work on key binding arguments, and may be a step towards implementing it.

@zadjii-msft
Copy link
Member

@DHowett-MSFT I'm pretty sure you just described like 80% of what #1349 has in it

@ViktorHofer
Copy link
Member

I don't understand the downvotes in the initial post. The described expected behavior is how cmd works today in Windows. An entire booth at the #build conference celebrated when the copy cmd+c functionality was announced.

@jtleaming
Copy link

I noticed selecting text with the mouse and copying works, but if I select the text using the cursor (shift+arrow-key) and then copy, nothing seems to be copied. Maybe shift+arrow-key doesn't seem to be selecting like I thought, but the issue seems to be related.

@ryzngard
Copy link
Member

ryzngard commented Aug 8, 2019

@DHowett-MSFT based on the design notes, should we follow #1349 to monitor this becoming available?

@Pilchie
Copy link
Member

Pilchie commented Aug 8, 2019

The lack of this paired with the poor formatting of pasting copied text from terminal into other apps has basically been a blocker to me adopting it. The improvements in conhost of the last few years have been so great, it's been impossible to go backwards.

@CobusKruger
Copy link

@miniksa Yes, the tone of that initial response was too harsh. I've edited to tone it down but still get the point across. Hopefully the balance is better now.

I apologize for any offense caused.

@zadjii-msft zadjii-msft self-assigned this Aug 15, 2019
zadjii-msft added a commit that referenced this issue Aug 15, 2019
  otherwise, let the key fallthrough

  Fixes #2285
@ghost ghost added the In-PR This issue has a related PR label Aug 16, 2019
DHowett-MSFT pushed a commit that referenced this issue Aug 16, 2019
This commit also transitions our keybinding events and event handlers to a
TypedEventHandler model with an "event args" class, as specified in the
keybinding arguments specification (#1349). In short, every event can be marked
Handled independently, and a Handled event will stop bubbling out to the
terminal. An unhandled event will be passed off to the terminal as a standard
keypress.

This unifies our keybinding event model and provides a convenient place for
binding arguments to live.

Fixes #2285.
Related to #1349, #1142.
@ghost ghost added Needs-Tag-Fix Doesn't match tag requirements Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Aug 16, 2019
@ghost
Copy link

ghost commented Aug 27, 2019

🎉This issue was addressed in #2446, which has now been successfully released as Windows Terminal Preview v0.4.2382.0.:tada:

Handy links:

@matthiasg
Copy link

As of Version 0.7.3451.0 this is not resolved. I deinstalled and reinstalled from the store and all settings were reset. Ctrl+C does not copy selected text.

@DHowett-MSFT
Copy link
Contributor

@matthiasg Binding Ctrl+C to copy is a user preference that can be chosen by adding {"command": "copy", "keys": ["ctrl+c"]} to your key bindings.

@matthiasg
Copy link

matthiasg commented Dec 20, 2019 via email

@Renha
Copy link

Renha commented Dec 20, 2019

I hope so, there is no reason to make "stop application" combination to copy text by default

@matthiasg
Copy link

@Renha When text is selected (which also stops all console output normally ) ? I mean it could just copy like on a unix shell which would be even better, but windows users would expect ctrl+c after selecting text to copy text.

@DHowett-MSFT
Copy link
Contributor

could just copy

You can setcopyOnSelect to true to get that behavior, actually. It’ll free up Ctrl+C to send Cancel if you had otherwise had it bound.

This is clearly a thorny issue.

@matthiasg
Copy link

Agreed. Obviously not ideal that Windows Shortcut for 'Copy'
is the same as the signal shortcut to command line processes but some working defaults should be in.

I believe once text is selected CTRL+C as copy makes sense. If someone accidentally forgets to selects text and then kills a process, I would file that as user error. It is a console after all. Can be disabled in settings but default does the right thing in both cases.

@ViktorHofer
Copy link
Member

It's unclear to me why Ctrl+C/V isn't the default keybinding like in cmd.exe with copyOnSelect set to true.

@mikemaccana
Copy link
Contributor Author

mikemaccana commented Jan 7, 2020 via email

@smflorentino
Copy link

Is there a UserVoice for Windows Terminal? I hit the same issue. It's one of those things I've just come to expect from consoles now.

@zadjii-msft
Copy link
Member

@smflorentino this GitHub repo is essentially our UserVoice. Also note that ctrl+c to copy when text is selected has worked for a number of releases now, _as long as you add {"command": "copy", "keys": ["ctrl+c"]} to your keybindings. Are you seeing some new issue where that's not working for you?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Interaction Interacting with the vintage console window (as opposed to driving via API or hooks) Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.