{Style} Refine style framework #16258

jiasli · 2020-12-14T07:14:25Z

Description

Refine the style framework.

Since BLUE and LIGHTBLUE_EX are not very visible/accessible under powershell.exe (see the demo at https://github.com/jiasli/color-demo), they are replaced with LIGHTWHITE_EX.

ℹ This doesn't apply to PowerShell sessions inside modern terminals, see item 5.
- Bring back get_parent_proc_name removed by {Packaging} Get rid of psutil dependency #11665. {Core} Add SessionId for telemetry #15076 has made sure that psutil is not installed under cygwin.

Honor cli_ctx.enable_color so that when color is disabled, it can remove color when formatting styled text. This can eliminate the necessity to make 2 versions of the same string, one with color, one without, like

azure-cli/src/azure-cli-core/azure/cli/core/commands/constants.py

Lines 35 to 37 in 026016e

    
           SURVEY_PROMPT = 'Please let us know how we are doing: https://aka.ms/azureclihats' 
        
           SURVEY_PROMPT_COLOR = Fore.YELLOW + Style.BRIGHT + 'Please let us know how we are doing: ' + Fore.BLUE + \ 
        
               'https://aka.ms/azureclihats' + Style.RESET_ALL

In the demo, you may easily turn off color with

az demo style --no-color

Change the color of primary and secondary text:
- Primary color: Use default foreground color instead of BRIGHT WHITE so that at least primary text is visible under both dark/light themes
- Secondary color: Use BRIGHT BLACK
Add light theme support which uses dark versions of colors. To try it, in light/dark-themed terminals, run
```
az demo style --theme light/dark
```
Add function is_modern_terminal to detect if the terminal is a modern terminal like VS Code, Windows Terminal.
print_styled_text wraps print and can pass kwargs.
format_styled_text supports more formats.

Testing Guide

az demo style

powershell.exe:

pwsh.exe:

yonzhan · 2020-12-14T07:22:00Z

Style

jiasli · 2020-12-15T09:04:11Z

src/azure-cli-core/setup.py

+# dependencies for specific OSes
+if not sys.platform.startswith('cygwin'):
+    DEPENDENCIES.append('psutil~=5.7')
+


Move the dependency of psutil to core as it is used by core, not command modules. @houk-ms

chenlomis · 2020-12-15T18:44:11Z

LGTM from the design side as per our latest discussion!

zhoxing-ms · 2020-12-28T08:41:58Z

May I ask could we support end parameter of print() in the print_styled_text() function?

At present, I have a requirement that the message of input() should also be colored, such as:

My previous implementation is:

print("Please input " + Fore.LIGHTBLUE_EX + param + Fore.RESET + ":", end='')
value = input()

Or do we have other better ways to implement this requirement, such as wrapping a specialized method?

jiasli · 2020-12-29T07:24:56Z

@zhoxing-ms, end is already supported. Note that kwargs is passed to print as is.

azure-cli/src/azure-cli-core/azure/cli/core/style.py

Line 82 in fa9985e

print(*formatted_list, file=file or sys.stderr, **kwargs)

Let me add some comments to make it clearer.

# Conflicts: # src/azure-cli-core/azure/cli/core/__init__.py

zhoxing-ms · 2021-01-04T10:33:04Z

src/azure-cli-core/azure/cli/core/style.py

+    # Convert str to the theme dict
+    if isinstance(theme, str):
+        if theme.lower() == 'none':
+            theme = THEME_NONE
+        elif theme.lower() == 'dark':
+            theme = THEME_DARK
+        elif theme.lower() == 'light':
+            theme = THEME_LIGHT
+        else:
+            from azure.cli.core.azclierror import CLIInternalError
+            raise CLIInternalError("Invalid theme. Supported themes: none, dark, light")


May I ask why do we need to support the case that theme is string?

By the way, could we consider setting a theme configuration as a enum for this mapping relationship instead of if-else logic? Such as:

class THEME(Enum): none = THEME_NONE, dark = THEME_DARK, light = THEME_LIGHT

If this enum is used as parameter, I personally think it is more readable and easier maintained~

May I ask why do we need to support the case that theme is string?

Because everything passed to CLI via the shell will be treated as str.

Moreover, the values of an Enum are usually basic types like str or int. Using dict as the value, like

class Theme(Enum): DARK = THEME_DARK LIGHT = THEME_LIGHT NONE = THEME_NONE

will result in:

> az demo style -h Arguments --theme : The theme to format styled text. Allowed values: {<Style.PRIMARY: 'primary'>: '\x1b[39m', <Style.SECONDARY: 'secondary'>: '\x1b[90m', <Style.IMPORTANT: 'important'>: '\x1b[35m', <Style.ACTION: 'action'>: '\x1b[34m', <Style.HYPERLINK: 'hyperlink'>: '\x1b[36m', <Style.ERROR: 'error'>: '\x1b[31m', <Style.SUCCESS: 'success'>: '\x1b[32m', <Style.WARNING: 'warning'>: '\x1b[33m'}, {<Style.PRIMARY: 'primary'>: '\x1b[39m', <Style.SECONDARY: 'secondary'>: '\x1b[90m', <Style.IMPORTANT: 'important'>: '\x1b[95m', <Style.ACTION: 'action'>: '\x1b[94m', <Style.HYPERLINK: 'hyperlink'>: '\x1b[96m', <Style.ERROR: 'error'>: '\x1b[91m', <Style.SUCCESS: 'success'>: '\x1b[92m', <Style.WARNING: 'warning'>: '\x1b[93m'}, {}. Default: dark.

But the idea of using Enum for Theme is good. Added as requested:

azure-cli/src/azure-cli-core/azure/cli/core/style.py

Lines 67 to 77 in 6240eaf

class Theme(str, Enum):

DARK = 'dark'

LIGHT = 'light'

NONE = 'none'

THEME_DEFINITIONS = {

Theme.NONE: THEME_NONE,

Theme.DARK: THEME_DARK,

Theme.LIGHT: THEME_LIGHT

}

zhoxing-ms · 2021-01-04T11:03:17Z

src/azure-cli-core/azure/cli/core/tests/test_util.py

+        # Linux
+        process.name.return_value = "python"
+        parent1.name.return_value = "bash"
+        parent2.name.return_value = "init"
+        parent3.name.return_value = "init"
+        self.assertEqual(_get_parent_proc_name(), "bash")


Out of curiosity, I just want to confirm that Mac and Linux are exactly the same, right?

Yes. Mac is also using bash, but it doesn't really mater as long as no ancestor process is called powershell.exe, pwsh.exe.

However, I think pwsh running on Linux can be a more complex scenario. I need to do more testing on that.

zhoxing-ms

LGTM

houk-ms

@jiasli When do you plan to get this PR merged? Is it possible to catch S181?

jiasli added 2 commits December 14, 2020 13:51

Disable blue in powershell

0d673e9

no_color

20847a2

jiasli requested review from chenlomis, houk-ms and zhoxing-ms December 14, 2020 07:14

jiasli self-assigned this Dec 14, 2020

jiasli added the style label Dec 14, 2020

jiasli requested a review from fengzhou-msft December 14, 2020 07:20

yonzhan added this to the S180 milestone Dec 14, 2020

Add comments

559e6bb

jiasli requested a review from evelyn-ys December 14, 2020 07:48

jiasli added 5 commits December 14, 2020 18:14

Add tests

325e9c4

Add tests for get_parent_proc_name

3962cc6

Use style for survey prompt

48a88bb

Cache get_parent_proc_name

d185678

CI

869ba5f

jiasli commented Dec 15, 2020

View reviewed changes

Send kwargs to print

98b6c61

jiasli mentioned this pull request Dec 15, 2020

[Hint] Post-output hint #16242

Draft

Light theme support

0945bad

jiasli mentioned this pull request Dec 16, 2020

[Core] New error output #16257

Merged

jiasli changed the title ~~{Style} Replace BLUE in powershell.exe, honor enable_color~~ {Style} Replace BLUE in powershell.exe, honor enable_color, light theme Dec 16, 2020

jiasli changed the title ~~{Style} Replace BLUE in powershell.exe, honor enable_color, light theme~~ {Style} Refine style framework Dec 16, 2020

jiasli added 3 commits December 17, 2020 13:32

Use default foreground color for PRIMARY

cad3673

Use foreground for powershell.exe

cbbca7f

Detect modern terminal

a44184c

jiasli mentioned this pull request Dec 23, 2020

{Core} Add util function highlight_command #16349

Merged

3 tasks

Merge branch 'dev' into style-color

0179277

jiasli added 3 commits December 24, 2020 11:04

is_legacy_powershell

b1c28d8

tests

7ec53fb

Remove no_color

fa9985e

yonzhan modified the milestones: S180, S181 Dec 26, 2020

jiasli added 2 commits January 4, 2021 16:53

Add comments for print_styled_text

2d62d81

Merge branch 'dev' into style-color

1e55d33

# Conflicts: # src/azure-cli-core/azure/cli/core/__init__.py

jiasli marked this pull request as ready for review January 4, 2021 08:58

jiasli requested review from Juliehzl and jsntcy as code owners January 4, 2021 08:58

zhoxing-ms reviewed Jan 4, 2021

View reviewed changes

jiasli added 2 commits January 5, 2021 10:33

Theme Enum

6240eaf

Fix test when no tty

9f89e0f

zhoxing-ms approved these changes Jan 5, 2021

View reviewed changes

jiasli added 2 commits January 11, 2021 16:55

pwsh in linux

36a1e9f

style

95dbeeb

houk-ms approved these changes Jan 12, 2021

View reviewed changes

Merge branch 'dev' into style-color

df6d6e7

yonzhan modified the milestones: S181, S182 Jan 16, 2021

jiasli merged commit 6f25505 into Azure:dev Jan 20, 2021

jiasli mentioned this pull request Jun 1, 2021

Remove colors from Azure CLI text output #18298

Closed

jiasli deleted the style-color branch July 15, 2021 06:12

	SURVEY_PROMPT = 'Please let us know how we are doing: https://aka.ms/azureclihats'
	SURVEY_PROMPT_COLOR = Fore.YELLOW + Style.BRIGHT + 'Please let us know how we are doing: ' + Fore.BLUE + \
	'https://aka.ms/azureclihats' + Style.RESET_ALL

	class Theme(str, Enum):
	DARK = 'dark'
	LIGHT = 'light'
	NONE = 'none'


	THEME_DEFINITIONS = {
	Theme.NONE: THEME_NONE,
	Theme.DARK: THEME_DARK,
	Theme.LIGHT: THEME_LIGHT
	}

{Style} Refine style framework #16258

{Style} Refine style framework #16258

Uh oh!

Conversation

jiasli commented Dec 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing Guide

Uh oh!

yonzhan commented Dec 14, 2020

Uh oh!

jiasli Dec 15, 2020

Choose a reason for hiding this comment

Uh oh!

chenlomis commented Dec 15, 2020

Uh oh!

zhoxing-ms commented Dec 28, 2020

Uh oh!

jiasli commented Dec 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhoxing-ms Jan 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiasli Jan 5, 2021

Choose a reason for hiding this comment

Uh oh!

zhoxing-ms Jan 5, 2021

Choose a reason for hiding this comment

Uh oh!

zhoxing-ms Jan 4, 2021

Choose a reason for hiding this comment

Uh oh!

jiasli Jan 5, 2021

Choose a reason for hiding this comment

Uh oh!

zhoxing-ms left a comment

Choose a reason for hiding this comment

Uh oh!

houk-ms left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jiasli commented Dec 14, 2020 •

edited

Loading

jiasli commented Dec 29, 2020 •

edited

Loading

zhoxing-ms Jan 4, 2021 •

edited

Loading

houk-ms left a comment •

edited

Loading