Change AdjustIndistinguishableColors to an enum setting instead of a boolean setting by PankajBhojwani · Pull Request #13512 · microsoft/terminal

PankajBhojwani · 2022-07-14T20:52:22Z

Summary of the Pull Request

AdjustIndistinguishableColors can now be set to:

Never: Never adjust the colors
Indexed: Only adjust colors that are part of the color scheme
Always: Always adjust the colors

References

#13343

PR Checklist

Closes #xxx
CLA signed. If not, go over here and sign the CLA
Tests added/passed
Documentation updated. If checked, please file a pull request on our docs repo and link it here: #xxx
Schema updated.
I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

For legacy purposes, true and false map to Indexed and Never respectively

Validation Steps Performed

Setting still works

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

carlos-zamora · 2022-07-14T23:32:05Z

src/renderer/base/RenderSettings.cpp

    // default color pairs within the color table
    if (Feature_AdjustIndistinguishableText::IsEnabled() &&
-        GetRenderMode(Mode::DistinguishableColors) &&
+        GetRenderMode(Mode::IndexedDistinguishableColors) &&


Shouldn't this be...

(GetRenderMode(Mode::IndexedDistinguishableColors) || GetRenderMode(Mode::IndexedDistinguishableColors)) &&

Shouldn't this be...

(GetRenderMode(Mode::IndexedDistinguishableColors) || GetRenderMode(Mode::IndexedDistinguishableColors)) &&

Shouldn't that be

(GetRenderMode(Mode::IndexedDistinguishableColors) || GetRenderMode(Mode::AlwaysDistinguishableColors)) &&

This block of code is for: "Indexed was the mode chosen, so use our precalculated 'adjusted' color table to determine the foreground color", but we don't want to enter this block of code at all if the mode is Always because in that case we just call the color adjustment function on whatever the final foreground is (see line 255)

carlos-zamora · 2022-07-14T23:33:16Z

src/cascadia/TerminalCore/Terminal.cpp

+        _renderSettings.SetRenderMode(RenderSettings::Mode::IndexedDistinguishableColors, false);
+        _renderSettings.SetRenderMode(RenderSettings::Mode::AlwaysDistinguishableColors, true);


Suggested change

_renderSettings.SetRenderMode(RenderSettings::Mode::IndexedDistinguishableColors, false);

_renderSettings.SetRenderMode(RenderSettings::Mode::AlwaysDistinguishableColors, true);

_renderSettings.SetRenderMode(RenderSettings::Mode::IndexedDistinguishableColors, true);

_renderSettings.SetRenderMode(RenderSettings::Mode::AlwaysDistinguishableColors, true);

Should we just set both to true just to be safe? That way if someone in the future adds a feature to indexed, but forgets to check for "always", nothing will break?

Yea I'm curious about this too

The reason I set this to false for now is that we don't want to bother with creating the pre-calculated color table at all if the mode is Always, since we don't even access the table and instead we calculate the adjusted foreground just before rendering (see line 255 in RenderSettings.cpp)

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

src/cascadia/TerminalSettingsModel/TerminalSettingsSerializationHelpers.h

src/renderer/base/RenderSettings.cpp

carlos-zamora · 2022-07-15T20:42:41Z

And don't forget to update the schema/docs plz

doc/cascadia/profiles.schema.json

Co-authored-by: Carlos Zamora <[email protected]>

ghost · 2022-07-22T11:41:05Z

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.

ghost · 2022-09-13T17:06:59Z

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

Handy links:

PankajBhojwani and others added 7 commits June 20, 2022 11:44

ignore brighten/invisible

86a7133

enable in feature, default setting is false

d86700f

add comment

230c2aa

setting is in

c4b0fb2

implement

4c7c187

conflict

332f8b6

legacy mappings

bde0937

PankajBhojwani marked this pull request as ready for review July 14, 2022 20:59

carlos-zamora reviewed Jul 14, 2022

View reviewed changes

string fixes

da5116a

carlos-zamora requested changes Jul 15, 2022

View reviewed changes

src/cascadia/TerminalSettingsModel/TerminalSettingsSerializationHelpers.h Show resolved Hide resolved

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

mapping fixes

4be97c8

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 15, 2022

DHowett reviewed Jul 15, 2022

View reviewed changes

src/renderer/base/RenderSettings.cpp Outdated Show resolved Hide resolved

don't break concealed

bd48da0

carlos-zamora reviewed Jul 15, 2022

View reviewed changes

src/renderer/base/RenderSettings.cpp Show resolved Hide resolved

Schema

059e714

DHowett reviewed Jul 15, 2022

View reviewed changes

doc/cascadia/profiles.schema.json Outdated Show resolved Hide resolved

DHowett reviewed Jul 15, 2022

View reviewed changes

doc/cascadia/profiles.schema.json Outdated Show resolved Hide resolved

case

659feba

carlos-zamora approved these changes Jul 15, 2022

View reviewed changes

doc/cascadia/profiles.schema.json Outdated Show resolved Hide resolved

zadjii-msft approved these changes Jul 18, 2022

View reviewed changes

doc/cascadia/profiles.schema.json Outdated Show resolved Hide resolved

Apply suggestions from code review

0acd810

Co-authored-by: Carlos Zamora <[email protected]>

zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jul 22, 2022

ghost merged commit d3ae00e into main Jul 22, 2022

ghost deleted the dev/pabhoj/delta_e_further branch July 22, 2022 11:41

DHowett mentioned this pull request Oct 20, 2022

ssh <start typing hostname> --- the hostname is invisible. No matter what color scheme I select. #14242

Closed

This was referenced Oct 27, 2022

adjustIndistinguishableColors needs to be updated MicrosoftDocs/terminal#606

Closed

Enable users to disable VT colors (for accessibility, visibility, contrast, etc.) #3066

Open

This pull request was closed.

		_renderSettings.SetRenderMode(RenderSettings::Mode::IndexedDistinguishableColors, false);
		_renderSettings.SetRenderMode(RenderSettings::Mode::AlwaysDistinguishableColors, true);

Conversation

PankajBhojwani commented Jul 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Uh oh!

Uh oh!

carlos-zamora Jul 14, 2022

Choose a reason for hiding this comment

Uh oh!

zadjii-msft Jul 15, 2022

Choose a reason for hiding this comment

Uh oh!

PankajBhojwani Jul 15, 2022

Choose a reason for hiding this comment

Uh oh!

carlos-zamora Jul 14, 2022

Choose a reason for hiding this comment

Uh oh!

zadjii-msft Jul 15, 2022

Choose a reason for hiding this comment

Uh oh!

PankajBhojwani Jul 15, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

carlos-zamora commented Jul 15, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ghost commented Jul 22, 2022

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.

Uh oh!

ghost commented Sep 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

PankajBhojwani commented Jul 14, 2022 •

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.