Add an ENUM setting for disabling rendering "intense" text as bold #10759

zadjii-msft · 2021-07-22T17:26:08Z

Summary of the Pull Request

This adds a new setting intenseTextStyle. It's a per-appearance, control setting, defaulting to "all".

When set to "all" or ["bold", "bright"], then we'll render text as both bold and bright (1.10 behavior)
When set to "bold", ["bold"], we'll render text formatted with ^[[1m as bold, but not bright
When set to "bright", ["bright"], we'll render text formatted with ^[[1m as bright, but not bold. This is the pre 1.10 behavior
When set to "none", we won't do anything special for it at all.

references

I last did this in Add a setting for disabling rendering "intense" text as bold #10648. This time it's an enum, so we can add bright in the future. It's got positive wording this time.
~~We will want to add "bright" as a value in the future, to disable the auto intense->bright conversion.~~ I just did that now.
Consider whether we need to support "bold foreground" (like gnome-terminal) as a separate color #5682 is related

PR Checklist

Closes Add a setting for disabling "intense is bold" #10576
I seriously don't think we have an issue for "disable intense is bright", but I'm not crazy, people wanted that, right? Add support for parsing Underlined and Doubly Underlined to the parser #2916 (comment) was the closest
I work here
Tests added/passed
add intenseTextStyle to the docs MicrosoftDocs/terminal#381

Validation Steps Performed

Yea that works. Printed some bold text, toggled it on, the text was no longer bold. hooray.

EDIT, 10 Aug

"intenseTextStyle": "none",
"intenseTextStyle": "bold",
"intenseTextStyle": "bright",
"intenseTextStyle": "all",
"intenseTextStyle": ["bold", "bright"],

all work now. Repro script:

printf "\e[1m[bold]\e[m[normal]\e[34m[blue]\e[1m[bold blue]\e[m\n"

…f a bool

src/cascadia/TerminalSettingsModel/IAppearanceConfig.idl

DHowett · 2021-07-22T23:18:05Z

src/renderer/dx/DxRenderer.hpp

@@ -256,6 +257,7 @@ namespace Microsoft::Console::Render
        D2D1_TEXT_ANTIALIAS_MODE _antialiasingMode;

        float _defaultTextBackgroundOpacity;
+        bool _intenseIsBold;


nit: you can initialize this in the class body if you want.

j4james · 2021-07-24T10:18:47Z

When set to "none", we won't. (the pre-1.10 behavior)

This sounds wrong to me. If the long term plan is to allow the choice of "none", "bright", "bold", and "all", then the pre-1.10 behavior is "bright" rather than "none".

zadjii-msft · 2021-07-26T16:34:58Z

This sounds wrong to me. If the long term plan is to allow the choice of "none", "bright", "bold", and "all", then the pre-1.10 behavior is "bright" rather than "none".

Sorry yea, that is a little confusing. Right now, there's only two values that the settings will actually accept - bold and none. none will still do the bright thing that we've always done right now, until we come through and actually add support for the bright setting. Then none will be (not bright, not bold).

I'm not sure I can fiddle with the enum parsing enough in this PR to make it so the only options are some set of bold, bright, and all. Maybe this PR doesn't make any sense at all without also doing the bright setting (though that's harder 😕)

carlos-zamora

Don't forget to update the schema and the docs! (only reason I'm blocking)

Everything else is a suggestion.

carlos-zamora · 2021-07-26T19:41:33Z

src/cascadia/TerminalControl/IControlAppearance.idl

+        Boolean IntenseIsBold;
+        // Boolean IntenseIsBright;


Why is IntenseStyle declared in the Microsoft.Terminal.Settings.Model instead of the Microsoft.Terminal.Control namespace? I guess you need to convert the enum into two bools at some point, but it's just weird to see two super similar settings in one place imo.

src/cascadia/TerminalSettingsModel/AppearanceConfig.h

j4james · 2021-07-26T22:30:31Z

Right now, there's only two values that the settings will actually accept - bold and none.

Maybe I'm not understanding how these mappings work, but if those are the actual values that are going to be saved in the settings file when a user toggles the "Render intense text as bold" option, then surely that setting is guaranteed to be incorrect at some point in the future?

For example, if a user turns this option off (so the setting becomes none), SGR 1 will still show as bright for now, but will eventually stop working altogether. Even when it's turned on, it's going to change from bold+bright to just bold. We're almost certain to receive a whole lot of bug reports when that happens.

If it can't be avoided, so be it. I just wanted to be sure you were aware of the problem. And if I've just misunderstood how it's going to work, sorry for the noise.

skyline75489 · 2021-07-27T04:09:17Z

none will still do the bright thing that we've always done right now, until we come through and actually add support for the bright setting. Then none will be will be (not bright, not bold).

Yea I agree with @j4james , this change of behavior will result in huge amount of bug reports.

this PR doesn't make any sense at all without also doing the bright setting

Adding the bright setting is likely the correct solution here 😦

ghost · 2021-08-03T06:02:51Z

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment.

zadjii-msft · 2021-08-03T12:17:38Z

shh bot, I'm coming back to this

…ng-try-2

carlos-zamora

Don't forget to update the schema and the docs! (only reason I'm blocking)

Everything else is a suggestion.

carlos-zamora · 2021-08-10T20:24:00Z

src/cascadia/TerminalControl/ControlCore.cpp

@@ -600,6 +601,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation

        _renderEngine->SetForceFullRepaintRendering(_settings.ForceFullRepaintRendering());
        _renderEngine->SetSoftwareRendering(_settings.SoftwareRendering());
+


Suggested change

nit: unnecessary change

…ng-try-2

ghost · 2021-08-12T20:02:42Z

Hello @zadjii-msft!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

DHowett · 2021-08-12T20:23:41Z

build failures :(

DHowett · 2021-08-12T20:25:31Z

src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw

+    <comment>Header for a control to select how "intense" text is formatted (bold, bright, both or none)</comment>
+  </data>
+  <data name="Appearance_IntenseTextStyleNone.Content" xml:space="preserve">
+    <value>None</value>
+    <comment>An option to choose from for the "intense text format" setting. When selected, "intense" text will not be rendered differently</comment>
+  </data>
+  <data name="Appearance_IntenseTextStyleBold.Content" xml:space="preserve">
+    <value>Bold</value>
+    <comment>An option to choose from for the "intense text format" setting. When selected, "intense" text will be rendered as bold text</comment>
+  </data>
+  <data name="Appearance_IntenseTextStyleBright.Content" xml:space="preserve">
+    <value>Bright</value>
+    <comment>An option to choose from for the "intense text format" setting. When selected, "intense" text will be rendered in a brighter color</comment>
+  </data>
+  <data name="Appearance_IntenseTextStyleAll.Content" xml:space="preserve">
+    <value>Both</value>
+    <comment>An option to choose from for the "intense text format" setting. When selected, "intense" text will be rendered as both bold text and in a brighter color</comment>


Worried about the wording here. The names are pretty short, and people may not know what "Intense Text Format" means. Should we do something more like Display "intense" text in... ...the normal font a bold font ...bright colors ...both bold font and bright colors?

Should we get more technical and mention SGR 1?

@cinnamon-msft what do you think? this is a button for users who "know what they're doing"

I'm gonna guess that .1% of our users will know what SGR 1 is. I be 1% of the folks that upvoted #109 in the first place do - people just want bold.

I'm not a wordsmith, I'm just a lowly engineer

As a point of comparison, Mintty has this option as a dropdown list with the label "Show bold", and with the choices "as font", "as color", or "as font & as color". XTerm just as the option to toggle the bold fonts (at least in the UI) with a checked menu item labelled "Bold Fonts". And Gnome Terminal just as the option to toggle the bright colors with a checkbox labelled "Show bold text in bright colors".

My recommendation would be something similar to what Mintty has, but with a little more detail, e.g.:

Show bold with brighter colors

Show bold with a heavier font weight

Show bold with both font weight and color

I think most people that are going to want to mess with this know that it's the "bold" functionality that they're tweaking. And what we're trying to differentiate is whether it affects the font or the color (or both).

@j4james I appreciate that! Thanks. We're gonna merge this as is to get the feature into selfhost/test builds and work on the wording from there. :)

(we're gonna work on the wording in post)

DHowett · 2021-08-12T20:25:57Z

src/cascadia/TerminalControl/IControlAppearance.idl

@@ -11,6 +11,8 @@ namespace Microsoft.Terminal.Control
        Windows.UI.Xaml.Media.Stretch BackgroundImageStretchMode;
        Windows.UI.Xaml.HorizontalAlignment BackgroundImageHorizontalAlignment;
        Windows.UI.Xaml.VerticalAlignment BackgroundImageVerticalAlignment;
+        Boolean IntenseIsBold;
+        // IntenseIsBright is in Core Appearance


technically both should be there, since eventually we'll push down the renderer right?

IMO eventually is not today, and I'd rather be self-consistent with the state of the world and not what we expect the world to be (which may lead to confusion)

skyline75489

DHowett · 2021-08-16T12:49:02Z

/azp run

azure-pipelines · 2021-08-16T12:49:15Z

Azure Pipelines successfully started running 1 pipeline(s).

…10759) This adds a new setting `intenseTextStyle`. It's a per-appearance, control setting, defaulting to `"all"`. * When set to `"all"` or `["bold", "bright"]`, then we'll render text as both **bold** and bright (1.10 behavior) * When set to `"bold"`, `["bold"]`, we'll render text formatted with `^[[1m` as **bold**, but not bright * When set to `"bright"`, `["bright"]`, we'll render text formatted with `^[[1m` as bright, but not bold. This is the pre 1.10 behavior * When set to `"none"`, we won't do anything special for it at all. * I last did this in #10648. This time it's an enum, so we can add bright in the future. It's got positive wording this time. * ~We will want to add `"bright"` as a value in the future, to disable the auto intense->bright conversion.~ I just did that now. * #5682 is related * [x] Closes #10576 * [x] I seriously don't think we have an issue for "disable intense is bright", but I'm not crazy, people wanted that, right? #2916 (comment) was the closest * [x] I work here * [x] Tests added/passed * [x] MicrosoftDocs/terminal#381  ![image](https://user-images.githubusercontent.com/18356694/128929228-504933ee-cf50-43a2-9982-55110ba39191.png) Yea that works. Printed some bold text, toggled it on, the text was no longer bold. hooray. ```json "intenseTextStyle": "none", "intenseTextStyle": "bold", "intenseTextStyle": "bright", "intenseTextStyle": "all", "intenseTextStyle": ["bold", "bright"], ``` all work now. Repro script: ```sh printf "\e[1m[bold]\e[m[normal]\e[34m[blue]\e[1m[bold blue]\e[m\n" ```

ghost · 2021-08-31T17:04:42Z

🎉Windows Terminal Preview v1.10.2383.0 has been released which incorporates this pull request.:tada:

Handy links:

ghost · 2021-08-31T17:14:36Z

🎉Windows Terminal Preview v1.11.2421.0 has been released which incorporates this pull request.:tada:

Handy links:

Add a setting for 'intense is bold', but backed by an enum, instead o…

ccf5a0e

…f a bool

DHowett reviewed Jul 22, 2021

View reviewed changes

src/cascadia/TerminalSettingsModel/IAppearanceConfig.idl Outdated Show resolved Hide resolved

DHowett approved these changes Jul 22, 2021

View reviewed changes

leonMSFT approved these changes Jul 23, 2021

View reviewed changes

carlos-zamora requested changes Jul 26, 2021

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 26, 2021

ghost added the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label Aug 3, 2021

zadjii-msft removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. labels Aug 3, 2021

zadjii-msft added 7 commits August 10, 2021 10:28

Merge remote-tracking branch 'origin/main' into dev/migrie/bold-setti…

c16412b

…ng-try-2

Implement support for bold-is-bright in TextAttribute, write test

8ab373b

plumb the setting through. The SUI for this is weird now though.

4b74320

pretty close, but we can't revert the setting?

8a3b7ee

derp

f9393ce

okay I don't know what I'm missing

55cbba3

Thanks carlos

cbf3022

carlos-zamora reviewed Aug 10, 2021

View reviewed changes

zadjii-msft added 2 commits August 11, 2021 05:41

schema for carlos

b8548fe

Merge remote-tracking branch 'origin/main' into dev/migrie/bold-setti…

6c59afd

…ng-try-2

zadjii-msft mentioned this pull request Aug 11, 2021

add intenseTextStyle to the docs MicrosoftDocs/terminal#381

Merged

zadjii-msft requested a review from carlos-zamora August 11, 2021 10:49

carlos-zamora approved these changes Aug 12, 2021

View reviewed changes

zadjii-msft added 2 commits August 12, 2021 14:26

Merge remote-tracking branch 'origin/main' into dev/migrie/bold-setti…

7a45b13

…ng-try-2

build the solution dummy

5c215b2

zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Aug 12, 2021

DHowett requested changes Aug 12, 2021

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Aug 12, 2021

skyline75489 approved these changes Aug 13, 2021

View reviewed changes

DHowett mentioned this pull request Aug 16, 2021

SUI: Reword the bold enum options #10955

Closed

DHowett approved these changes Aug 16, 2021

View reviewed changes

zadjii-msft added 2 commits August 16, 2021 08:10

forgot to push before the weekend

2d124e1

fricken VS

dffb37a

ghost merged commit a544f56 into main Aug 16, 2021

ghost deleted the dev/migrie/bold-setting-try-2 branch August 16, 2021 13:46

PankajBhojwani mentioned this pull request Aug 16, 2021

Render "intense" text as bright by default #10958

Merged

DHowett added the zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. label Aug 16, 2021

DHowett removed the zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. label Aug 25, 2021

ghost mentioned this pull request Aug 31, 2021

Add a setting for disabling "intense is bold" #10576

Closed

zadjii-msft mentioned this pull request Oct 27, 2021

Property intenseTextStyle is not allowed #11629

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an ENUM setting for disabling rendering "intense" text as bold #10759

Add an ENUM setting for disabling rendering "intense" text as bold #10759

zadjii-msft commented Jul 22, 2021 •

edited

Loading

DHowett Jul 22, 2021

j4james commented Jul 24, 2021

zadjii-msft commented Jul 26, 2021

carlos-zamora left a comment

carlos-zamora Jul 26, 2021

j4james commented Jul 26, 2021

skyline75489 commented Jul 27, 2021

ghost commented Aug 3, 2021

zadjii-msft commented Aug 3, 2021

carlos-zamora left a comment

carlos-zamora Aug 10, 2021

ghost commented Aug 12, 2021

DHowett commented Aug 12, 2021

DHowett Aug 12, 2021

DHowett Aug 12, 2021

zadjii-msft Aug 12, 2021

j4james Aug 12, 2021

DHowett Aug 16, 2021

zadjii-msft Aug 16, 2021

DHowett Aug 12, 2021

zadjii-msft Aug 12, 2021

skyline75489 left a comment

DHowett commented Aug 16, 2021

azure-pipelines bot commented Aug 16, 2021

ghost commented Aug 31, 2021

ghost commented Aug 31, 2021

		@@ -600,6 +601,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation

		_renderEngine->SetForceFullRepaintRendering(_settings.ForceFullRepaintRendering());
		_renderEngine->SetSoftwareRendering(_settings.SoftwareRendering());

Add an ENUM setting for disabling rendering "intense" text as bold #10759

Add an ENUM setting for disabling rendering "intense" text as bold #10759

Conversation

zadjii-msft commented Jul 22, 2021 • edited Loading

Summary of the Pull Request

references

PR Checklist

Validation Steps Performed

EDIT, 10 Aug

Choose a reason for hiding this comment

j4james commented Jul 24, 2021

zadjii-msft commented Jul 26, 2021

carlos-zamora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j4james commented Jul 26, 2021

skyline75489 commented Jul 27, 2021

ghost commented Aug 3, 2021

zadjii-msft commented Aug 3, 2021

carlos-zamora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Aug 12, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

DHowett commented Aug 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skyline75489 left a comment

Choose a reason for hiding this comment

DHowett commented Aug 16, 2021

azure-pipelines bot commented Aug 16, 2021

ghost commented Aug 31, 2021

ghost commented Aug 31, 2021

zadjii-msft commented Jul 22, 2021 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.