Remove PSStyle if terminal does not support escape sequence #426
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
BethanyZhou
changed the title
isra-fel
reviewed
Aug 2, 2024
src/Common/AzurePSCmdlet.cs
Outdated
Comment on lines
340
to
343
| if (null == PSStyle.SupportsVirtualTerminal) | ||
| { | ||
| PSStyle.SupportsVirtualTerminal = Host.UI.SupportsVirtualTerminal; | ||
| } |
There was a problem hiding this comment.
This logic is too specific to be put in AzurePSCmdlet. It should be in PSStyle.
There was a problem hiding this comment.
Host.UI.SupportsVirtualTerminal is a property of PSCmdlet, its value is unavailable beyond cmdlet.
isra-fel
reviewed
Aug 2, 2024
| @@ -20,6 +20,8 @@ namespace Microsoft.WindowsAzure.Commands.Common | |||
| /// </summary> | |||
| public sealed class PSStyle | |||
| { | |||
| public static bool? SupportsVirtualTerminal = null; | |||
|
|
|||
| /// <summary> | |||
There was a problem hiding this comment.
Try something like this
public static bool IsEscapeSequenceSupported {
if (!_isEscapeSequenceSupported.HasValue) { _isEscapeSequenceSupported = Host.UI.SupportsVirtualTerminal; }
return _isEscapeSequenceSupported.Value;
}
private static bool? _isEscapeSequenceSupported = null;
dolauli
reviewed
Aug 2, 2024
6 tasks
isra-fel
reviewed
Aug 5, 2024
src/Common/ColorAndFormat/PSStyle.cs
Outdated
| } | ||
|
|
||
| /// <summary> | ||
| /// This function should be invoked only in the Az.Accounts. |
There was a problem hiding this comment.
Suggested change
| /// This function should be invoked only in the Az.Accounts. | |
| /// Initializes default settings of PSStyle according to the environment. | |
| /// Note: This function should be invoked only in Az.Accounts. |
isra-fel
reviewed
Aug 5, 2024
src/Common/ColorAndFormat/PSStyle.cs
Outdated
| @@ -20,6 +22,29 @@ namespace Microsoft.WindowsAzure.Commands.Common | |||
| /// </summary> | |||
| public sealed class PSStyle | |||
| { | |||
| private static bool? _isEscapeSequenceSupported = null; | |||
|
|
|||
| public static bool IsEscapeSequenceSupported | |||
There was a problem hiding this comment.
Any reason to make this public? Otherwise it can be private.
BethanyZhou
changed the title
isra-fel
approved these changes
Aug 5, 2024
@dolauli thanks for asking. No change log is needed for common repo. could you help approve this PR if no other comments. |
dolauli
approved these changes
Aug 6, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix issue: Recent changes to Connect-AZAccount -DeviceCode cause display issues in the PowerShell ISE azure-powershell#24556
Group in Azure-PowerShell PR: [Account] Recent changes to Connect-AZAccount -DeviceCode cause display issues in the PowerShell ISE azure-powershell#25735
Current effect
Changed effect
