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

Turn on and off DECCKM to modify keyboard mode for unix native commands to work correctly #14943

Merged
merged 3 commits into from
Apr 1, 2021

Conversation

SteveL-MSFT
Copy link
Member

@SteveL-MSFT SteveL-MSFT commented Mar 4, 2021

PR Summary

Dotnet runtime sets keyboard mode to application on Unix systems and does not currently reset on exit. This change emits the DECRTS 1 sequence on pwsh exit on Unix systems (if VT is supported and interactive) to put keyboard mode back to default so that arrow keys do not emit escape sequences.

In the case of an interactive shell, we turn on DECCKM before showing prompt and turn if off before execution. This follows what zsh does.

Validated manually with gh auth login works with these changes and fails without.

PR Context

Fix #12268

PR Checklist

@iSazonov
Copy link
Collaborator

iSazonov commented Mar 4, 2021

It is not full fix for #12268
We need to address external applications run. See #12268 (comment)

@SteveL-MSFT
Copy link
Member Author

I'll update to set and reset in the native command processor

@iSazonov
Copy link
Collaborator

iSazonov commented Mar 4, 2021

Start-Process?

@SteveL-MSFT
Copy link
Member Author

@iSazonov it seems that if we follow zsh, then the set/reset should be in console host repl loop and not in native command processor nor start-process.

@iSazonov
Copy link
Collaborator

iSazonov commented Mar 5, 2021

  1. Why do we only Unix fix if the issue was reported as Windows bug?
  2. I still think we could safely skip s_theConsoleHost.IsInteractive check.

@SteveL-MSFT
Copy link
Member Author

@iSazonov it's not a Windows bug, it was WSL. .NET on Windows doesn't use ANSI escape sequences for keyboard input. As for why check for interactive, the problem is that if you do something like: pwsh -command 1, then you get back 1ESC[?1l as the output from that command. Even if you don't explicitly use -noninteractive the console host knows that -command without -noexit is non-interactive so we don't want to emit the ESC sequence that will be part of the stdout output.

@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Mar 5, 2021
@SteveL-MSFT SteveL-MSFT changed the title Emit DECRST 1 sequence to restore keyboard mode on Unix systems on pwsh exit Turn on and off DECCKM to modify keyboard mode for unix native commands to work correctly Mar 10, 2021
@ghost ghost added the Review - Needed The PR is being reviewed label Mar 18, 2021
@ghost
Copy link

ghost commented Mar 18, 2021

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

@adityapatwardhan adityapatwardhan merged commit c008e3e into PowerShell:master Apr 1, 2021
@ghost ghost removed the Review - Needed The PR is being reviewed label Apr 1, 2021
@iSazonov iSazonov added this to the 7.2.0-preview.5 milestone Apr 2, 2021
@ghost
Copy link

ghost commented Apr 14, 2021

🎉v7.2.0-preview.5 has been released which incorporates this pull request.:tada:

Handy links:

TravisEz13 added a commit that referenced this pull request Apr 16, 2021
[7.2.0-preview.5] - 2021-04-14

* Breaking Changes

- Make PowerShell Linux deb and RPM packages universal (#15109)
- Enforce AppLocker Deny configuration before Execution Policy Bypass configuration (#15035)
- Disallow mixed dash and slash in command line parameter prefix (#15142) (Thanks @davidBar-On!)

* Experimental Features

- `PSNativeCommandArgumentPassing`: Use `ArgumentList` for native executable invocation (breaking change) (#14692)

* Engine Updates and Fixes

- Add `IArgumentCompleterFactory` for parameterized `ArgumentCompleters` (#12605) (Thanks @powercode!)

* General Cmdlet Updates and Fixes

- Fix SSH remoting connection never finishing with misconfigured endpoint (#15175)
- Respect `TERM` and `NO_COLOR` environment variables for `$PSStyle` rendering (#14969)
- Use `ProgressView.Classic` when Virtual Terminal is not supported (#15048)
- Fix `Get-Counter` issue with `-Computer` parameter (#15166) (Thanks @krishnayalavarthi!)
- Fix redundant iteration while splitting lines (#14851) (Thanks @hez2010!)
- Enhance `Remove-Item -Recurse` to work with OneDrive (#14902) (Thanks @iSazonov!)
- Change minimum depth to 0 for `ConvertTo-Json` (#14830) (Thanks @kvprasoon!)
- Allow `Set-Clipboard` to accept empty string (#14579)
- Turn on and off `DECCKM` to modify keyboard mode for Unix native commands to work correctly (#14943)
- Fall back to `CopyAndDelete()` when `MoveTo()` fails due to an `IOException` (#15077)

* Code Cleanup

<details>

<summary>

<p>We thank the following contributors!</p>
<p>@xtqqczze, @iSazonov, @ZhiZe-ZG</p>

</summary>

<ul>
<li>Update .NET to <code>6.0.0-preview.3</code> (#15221)</li>
<li>Add space before comma to hosting test to fix error reported by <code>SA1001</code> (#15224)</li>
<li>Add <code>SecureStringHelper.FromPlainTextString</code> helper method for efficient secure string creation (#14124) (Thanks @xtqqczze!)</li>
<li>Use static lambda keyword (#15154) (Thanks @iSazonov!)</li>
<li>Remove unnecessary <code>Array</code> -&gt; <code>List</code> -&gt; <code>Array</code> conversion in <code>ProcessBaseCommand.AllProcesses</code> (#15052) (Thanks @xtqqczze!)</li>
<li>Standardize grammar comments in Parser.cs (#15114) (Thanks @ZhiZe-ZG!)</li>
<li>Enable <code>SA1001</code>: Commas should be spaced correctly (#14171) (Thanks @xtqqczze!)</li>
<li>Refactor <code>MultipleServiceCommandBase.AllServices</code> (#15053) (Thanks @xtqqczze!)</li>
</ul>

</details>

* Tools

- Use Unix line endings for shell scripts (#15180) (Thanks @xtqqczze!)

* Tests

- Add the missing tag in Host Utilities tests (#14983)
- Update `copy-props` version in `package.json` (#15124)

* Build and Packaging Improvements

<details>

<summary>

<p>We thank the following contributors!</p>
<p>@JustinGrote</p>

</summary>

<ul>
<li>Fix <code>yarn-lock</code> for <code>copy-props</code> (#15225)</li>
<li>Make package validation regex accept universal Linux packages (#15226)</li>
<li>Bump NJsonSchema from 10.4.0 to 10.4.1 (#15190)</li>
<li>Make MSI and EXE signing always copy to fix daily build (#15191)</li>
<li>Sign internals of EXE package so that it works correctly when signed (#15132)</li>
<li>Bump Microsoft.NET.Test.Sdk from 16.9.1 to 16.9.4 (#15141)</li>
<li>Update daily release tag format to  work with new Microsoft Update work (#15164)</li>
<li>Feature: Add Ubuntu 20.04 Support to install-powershell.sh (#15095) (Thanks @JustinGrote!)</li>
<li>Treat rebuild branches like release branches (#15099)</li>
<li>Update WiX to 3.11.2 (#15097)</li>
<li>Bump NJsonSchema from 10.3.11 to 10.4.0 (#15092)</li>
<li>Allow patching of preview releases (#15074)</li>
<li>Bump Newtonsoft.Json from 12.0.3 to 13.0.1 (#15084, #15085)</li>
<li>Update the <code>minSize</code> build package filter to be explicit (#15055)</li>
<li>Bump NJsonSchema from 10.3.10 to 10.3.11 (#14965)</li>
</ul>

</details>

* Documentation and Help Content

- Merge `7.2.0-preview.4` changes to master (#15056)
- Update `README` and `metadata.json` (#15046)
- Fix broken links for `dotnet` CLI (#14937)

[7.2.0-preview.5]: v7.2.0-preview.4...v7.2.0-preview.5
@SteveL-MSFT SteveL-MSFT deleted the emit-decrst branch July 12, 2021 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log
Projects
None yet
3 participants