Add cursorColor to color scheme #4651

yitzhaks · 2020-02-20T00:55:34Z

Add the option to set the cursor color as part of the color scheme.

This is very useful for light themes, where the cursor disappears unless its color
is set in the profile.

Related to issue #764, but doesn't fully resolve it.

Validation

I tested this manually by creating a light color scheme, setting the cursor color
to black and setting the profile color scheme to the newly created color scheme.
I validated the cursor is black, then set the cursor color in the profile (to red)
and saw it trumps the cursor color from the color scheme.

Add the option to set the cursor color as part of the color scheme. This is very useful for light themes, where the cursor disappears unless its color is set in the profile.

yitzhaks · 2020-02-20T00:56:29Z

While locally validating, I didn't see my changes to the built-in color schemes reflected. Is this expected?

zadjii-msft

oof fat-fingered ctrl+enter

zadjii-msft

This looks good to me. I'd love some extra tests to check scenarios like:

a scheme doesn't have a cursorColor set
A profile doesn't have a cursorColor but it's scheme does
A profile doesn't have a cursorColor and neither does it's scheme
A profile does have a cursorColor but it's scheme doesn't

But I'm still happy with this without those extra tests 😄

carlos-zamora

Yeah, I wanna see the tests above too please.
I want to double check something. This doesn't make cursorColor required for a colorScheme right? Like, when this goes in, existing users that have custom colorSchemes without cursorColor will be unaffected by this change?
Could you please update doc/cascadia/SettingsSchema.md too?

carlos-zamora · 2020-02-20T21:16:06Z

@DHowett-MSFT Also, the discussion on #764 sounds like cursorColor could/should be added to color schemes, but the others (i.e.: cursorShape, fontFace, and fontSize) should not. I agree with that reasoning. Should we just close that issue once this PR goes in?

Add a test that checks the resulting cursor color for all combinations of setting cursor color or not setting it in profile and color scheme, and for not setting a color scheme at all.

yitzhaks · 2020-02-23T00:54:57Z

I added the tests @zadjii-msft suggested (and a tiny bit more).
@carlos-zamora, regarding existing users, they shouldn't be negatively impacted by this change.
If they didn't set a cursorColor in their profile they'll:

If they didn't set a colorScheme, they'll get #FFFFFF (from the default built-in color scheme).
If they set a built-in colorScheme, they'll get the cursor color from that color scheme. This is the only case users should see any changes, and only for the good (in the case of light color schemes where the cursor was invisible they'll now see their cursor, which is what spurred the discussion in the first place).
If they set a custom colorScheme, they'll get DEFAULT_CURSOR_COLOR == #FFFFFF (as the default for cursorColor when it's not set in their color scheme).

If they set a cursorColor in their profile, it will trump any cursorColor in the color scheme.
All these cases are covered by the tests I added, so I think we're good here.

There is no default cursor color for profile, and the documentation shouldn't say there is. The default cursor color is only relevant for color schemes (where it's correctly documented).

Align json and md on "of the" (instead of "for the").

DHowett-MSFT

I love this. Really sorry we took so long to review it. Thanks for the work!

Happy to merge after conflict resolution.

DHowett-MSFT · 2020-03-13T07:22:19Z

And yes, this should just close the referenced issue. Cursor shape shouldn’t be part of a color palette. :)

zadjii-msft · 2020-03-16T12:57:15Z

@carlos-zamora You should make another pass on this one, so we can get it merged

yitzhaks · 2020-03-17T16:57:15Z

@DHowett-MSFT, whenever you're ready :)

ghost · 2020-03-17T20:10:57Z

Hello @DHowett-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-MSFT · 2020-03-17T20:11:03Z

Thanks for the contribution!

ghost · 2020-04-22T17:15:33Z

🎉Windows Terminal Preview v0.11.1121.0 has been released which incorporates this pull request.:tada:

Handy links:

Add cursorColor to color scheme

e326375

Add the option to set the cursor color as part of the color scheme. This is very useful for light themes, where the cursor disappears unless its color is set in the profile.

yitzhaks requested review from DHowett-MSFT and zadjii-msft February 20, 2020 00:56

Make the formatter happy

163f586

zadjii-msft reviewed Feb 20, 2020

View reviewed changes

zadjii-msft approved these changes Feb 20, 2020

View reviewed changes

carlos-zamora requested changes Feb 20, 2020

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 Feb 20, 2020

Update the profiles schema documentation

692924e

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Feb 22, 2020

Add a test

ec21c51

Add a test that checks the resulting cursor color for all combinations of setting cursor color or not setting it in profile and color scheme, and for not setting a color scheme at all.

yitzhaks requested a review from carlos-zamora February 23, 2020 00:55

Yitzhak Steinmetz added 2 commits February 23, 2020 00:59

Fix json schema

8fc3e18

There is no default cursor color for profile, and the documentation shouldn't say there is. The default cursor color is only relevant for color schemes (where it's correctly documented).

Fix wording

c8e4268

Align json and md on "of the" (instead of "for the").

zadjii-msft added Area-Settings Issues related to settings and customizability, for console or terminal Product-Terminal The new Windows Terminal. Needs-Second It's a PR that needs another sign-off labels Feb 24, 2020

ghost requested review from miniksa and leonMSFT February 24, 2020 14:38

MCrank mentioned this pull request Feb 29, 2020

Add cursorColor to light them pending PR #4651 in Windows Terminal fedfigca/thanatos-windows-terminal#1

Closed

DHowett-MSFT approved these changes Mar 13, 2020

View reviewed changes

Merge branch 'master' into dev/yitzhaks/CursorColorInColorScheme

e1eb664

carlos-zamora approved these changes Mar 17, 2020

View reviewed changes

DHowett-MSFT added the AutoMerge Marked for automatic merge by the bot when requirements are met label Mar 17, 2020

ghost merged commit c0d704e into microsoft:master Mar 17, 2020

yitzhaks deleted the dev/yitzhaks/CursorColorInColorScheme branch March 17, 2020 20:25

DHowett-MSFT mentioned this pull request Mar 20, 2020

cursorColor only works in profiles{} #5044

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cursorColor to color scheme #4651

Add cursorColor to color scheme #4651

yitzhaks commented Feb 20, 2020 •

edited by DHowett-MSFT

Loading

yitzhaks commented Feb 20, 2020

zadjii-msft left a comment •

edited

Loading

zadjii-msft left a comment

carlos-zamora left a comment

carlos-zamora commented Feb 20, 2020

yitzhaks commented Feb 23, 2020

DHowett-MSFT left a comment

DHowett-MSFT commented Mar 13, 2020

zadjii-msft commented Mar 16, 2020

yitzhaks commented Mar 17, 2020

ghost commented Mar 17, 2020

DHowett-MSFT commented Mar 17, 2020

ghost commented Apr 22, 2020

Add cursorColor to color scheme #4651

Add cursorColor to color scheme #4651

Conversation

yitzhaks commented Feb 20, 2020 • edited by DHowett-MSFT Loading

Validation

yitzhaks commented Feb 20, 2020

zadjii-msft left a comment • edited Loading

Choose a reason for hiding this comment

zadjii-msft left a comment

Choose a reason for hiding this comment

carlos-zamora left a comment

Choose a reason for hiding this comment

carlos-zamora commented Feb 20, 2020

yitzhaks commented Feb 23, 2020

DHowett-MSFT left a comment

Choose a reason for hiding this comment

DHowett-MSFT commented Mar 13, 2020

zadjii-msft commented Mar 16, 2020

yitzhaks commented Mar 17, 2020

ghost commented Mar 17, 2020

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-MSFT commented Mar 17, 2020

ghost commented Apr 22, 2020

yitzhaks commented Feb 20, 2020 •

edited by DHowett-MSFT

Loading

zadjii-msft left a comment •

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.