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.

Sign up for GitHub

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

Jump to bottom

Use ProgressView.Classic if Virtual Terminal is not supported #15048

Merged
merged 1 commit into from
Apr 12, 2021
Merged

Use ProgressView.Classic if Virtual Terminal is not supported #15048

merged 1 commit into from
Apr 12, 2021

Conversation

SteveL-MSFT
Copy link
Member

@SteveL-MSFT SteveL-MSFT commented Mar 16, 2021 •
edited
Loading

PR Summary

On older Windows systems that don't support VT, they will get VT escape sequences if they enable certain ExperimentalFeatures that rely on VT. Change to check if VT is supported before emitting. In the case of Progress, we can't check deep in the Progress code as the host isn't available and it doesn't seem to make sense to pass it all the way down just for this. Instead, we check in the consolehost and change to Classic progress view if VT is not supported.

I also noticed that the OSCIndicator code wasn't properly protected against older systems that don't support VT, so added that.

PR Context

Fix #15047

PR Checklist

iSazonov
iSazonov reviewed Mar 17, 2021
View reviewed changes
src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostUserInterfaceProgress.cs
@@ -48,7 +48,7 @@ class ConsoleHostUserInterface : System.Management.Automation.Host.PSHostUserInt

_pendingProgress = null;

if (ExperimentalFeature.IsEnabled(ExperimentalFeature.PSAnsiProgressFeatureName) && PSStyle.Instance.Progress.UseOSCIndicator)
if (SupportsVirtualTerminal && ExperimentalFeature.IsEnabled(ExperimentalFeature.PSAnsiProgressFeatureName) && PSStyle.Instance.Progress.UseOSCIndicator)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why do not check SupportsVirtualTerminal in time we create PSStyle.Instance? It would seem be more generic.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah the way we decide within the methods what behaviour to have here isn't great design and while this change seems ok, I think we need to register the need to do this better.

An alternative perspective on how the code should be structured: PSHostUserInterface is an abstract class intended to be overridden with the implementation to actually provide. The right object-oriented approach would be to build the user interface object for the given settings, with shared functionality implemented in a common base class.

@ghost ghost added the Review - Needed The PR is being reviewed label Mar 24, 2021
@ghost
Copy link

ghost commented Mar 24, 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

rjmholt
rjmholt approved these changes Apr 12, 2021
View reviewed changes
src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostUserInterfaceProgress.cs
@@ -48,7 +48,7 @@ class ConsoleHostUserInterface : System.Management.Automation.Host.PSHostUserInt

_pendingProgress = null;

if (ExperimentalFeature.IsEnabled(ExperimentalFeature.PSAnsiProgressFeatureName) && PSStyle.Instance.Progress.UseOSCIndicator)
if (SupportsVirtualTerminal && ExperimentalFeature.IsEnabled(ExperimentalFeature.PSAnsiProgressFeatureName) && PSStyle.Instance.Progress.UseOSCIndicator)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah the way we decide within the methods what behaviour to have here isn't great design and while this change seems ok, I think we need to register the need to do this better.

An alternative perspective on how the code should be structured: PSHostUserInterface is an abstract class intended to be overridden with the implementation to actually provide. The right object-oriented approach would be to build the user interface object for the given settings, with shared functionality implemented in a common base class.

@rjmholt
Copy link
Collaborator

rjmholt commented Apr 12, 2021

I've rerun the Windows tests and they continue to fail. @SteveL-MSFT can you try rebasing this PR?

@ghost ghost removed the Review - Needed The PR is being reviewed label Apr 12, 2021
@rjmholt rjmholt merged commit 44d5ce9 into PowerShell:master Apr 12, 2021
@SteveL-MSFT SteveL-MSFT deleted the progress-fallback branch April 13, 2021 00:24
@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Apr 13, 2021
@iSazonov iSazonov added this to the 7.2.0-preview.6 milestone Apr 13, 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:

@ghost ghost mentioned this pull request Apr 14, 2021
Closed
@sdwheeler sdwheeler mentioned this pull request Apr 15, 2021
Closed
TravisEz13 added a commit that referenced this pull request Apr 16, 2021
@TravisEz13
Merge tag 'v7.2.0-preview.5'
32543c1 
[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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iSazonov iSazonov iSazonov left review comments

@rjmholt rjmholt rjmholt approved these changes

@anmenaga anmenaga Awaiting requested review from anmenaga anmenaga is a code owner

@daxian-dbw daxian-dbw Awaiting requested review from daxian-dbw daxian-dbw is a code owner

Assignees

@rjmholt rjmholt

Labels
CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log
Projects
None yet
Milestone
7.2.0-preview.5
Development

Successfully merging this pull request may close these issues.

Progress rendering is mangled in non-VT terminals
3 participants
@SteveL-MSFT @rjmholt @iSazonov