Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add Selection Background Color as a setting to Profiles and Col… #3471

Merged
merged 20 commits into from
Nov 13, 2019
Merged

Add Selection Background Color as a setting to Profiles and Col… #3471

merged 20 commits into from
Nov 13, 2019

Conversation

leonMSFT
Copy link
Contributor

@leonMSFT leonMSFT commented Nov 7, 2019

Summary of the Pull Request

This introduces a setting to both Profiles and ColorSchemes called selectionBackground that allows you to change the selection background color to what's specified. If selectionBackground isn't set in either the profile or color scheme, it'll default to what it was before - white.

PR Checklist

Validation Steps Performed

  • Added selectionBackground to existing profile and colorscheme tests.
  • Verified that the color does change to what I expect it to be when I add "selectionBackground" to either/both a profile and a color scheme.

leonMSFT and others added 13 commits October 30, 2019 16:57
@leonMSFT
adding selectionBackground to ColorScheme and TerminalSettings
db9c4c1
@leonMSFT
Changing PaintSelection inside the renderers to take a SelectionBackg…
3ea3bbc 
…round COLORREF
@leonMSFT
changes to conhost and terminal renderdata, and to terminal settings …
1d2cd97 
…and core
@leonMSFT
IT WORKS
ea3cc5b
@leonMSFT
modification of unit tests, json schemas, reordering of functions
3df73bc
@leonMSFT
more movement
04d9857
@leonMSFT
changed a couple of unit tests to add selectionBackground, added the …
a745812 
…setting to schemas, also added the optional setting to profiles
@leonMSFT
default selection background should be slightly offwhite like the def…
4a055de 
…ault foreground is
@leonMSFT
reverting changes to .sln
1f17ab1
@leonMSFT
cleaning up
23d8ebd
@leonMSFT
adding comment
3368d09
@leonMSFT
oops
ce2ef34
@leonMSFT
Merge branch 'master' into dev/lelian/selectionBackgroundColor
bee39c9
@mdtauk
Copy link

mdtauk commented Nov 7, 2019

Is there already a SelectionForeground colour?

@leonMSFT
Copy link
Contributor Author

leonMSFT commented Nov 7, 2019

@mdtauk Nope! There is not currently a setting for SelectionForeground.

zadjii-msft
zadjii-msft requested changes Nov 11, 2019
View reviewed changes
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This definitely looks good. Setting is plumbed through all the places it needs to be plumbed through.

Though, maybe it makes more sense to just have this be a property on the DxEngine, and not necessarily a part of TerminalCore. Maybe the setting should be moved to IControlSettings, and just set a property of the DxRenderer, instead of needing to plumb it all the way down to the TerminalCore and having it then tell the renderer each frame what the BG color should be. Further, then each of the other engines wouldn't need to have that added param on their PaintSelection methods. Plus, conhost wouldn't need to implement anything special at all to handle this, it would just leave the value unchanged (with a default of DEFAULT_FOREGROUND). Thoughts?

src/cascadia/TerminalApp/ColorScheme.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/ColorScheme.cpp Outdated Show resolved Hide resolved
@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Nov 11, 2019
@zadjii-msft zadjii-msft added Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Area-Settings Issues related to settings and customizability, for console or terminal Product-Terminal The new Windows Terminal. labels Nov 11, 2019
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Nov 12, 2019
@leonMSFT
Copy link
Contributor Author

@zadjii-msft Thanks for the suggestions! Putting the setting in IControlSettings and letting TermControl update the color in DxEngine really cut down on the amount of files I needed to touch.

zadjii-msft
zadjii-msft requested changes Nov 13, 2019
View reviewed changes
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is all okay except for that one _selectionBackground thing.

src/renderer/dx/DxRenderer.cpp Outdated Show resolved Hide resolved
@ghost 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 Nov 13, 2019
@zadjii-msft zadjii-msft changed the title Add Selection Background Color as a setting to Profiles and ColorSchemes Add Selection Background Color as a setting to Profiles and Col… Nov 13, 2019
@zadjii-msft zadjii-msft merged commit a404778 into master Nov 13, 2019
@javierdlg
Copy link
Member

Thanks for the quick turnaround!

@miniksa miniksa mentioned this pull request Nov 13, 2019
Closed
@zadjii-msft zadjii-msft mentioned this pull request Nov 14, 2019
Open
@ghost
Copy link

ghost commented Nov 26, 2019

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

Handy links:

@ghost ghost mentioned this pull request Nov 26, 2019
Closed
@leonMSFT leonMSFT deleted the dev/lelian/selectionBackgroundColor branch November 26, 2019 21:45
@zadjii-msft zadjii-msft mentioned this pull request May 4, 2020
Closed
@zadjii-msft zadjii-msft mentioned this pull request Sep 3, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdtauk mdtauk mdtauk left review comments

@zadjii-msft zadjii-msft zadjii-msft approved these changes

@miniksa miniksa miniksa approved these changes

@DHowett-MSFT DHowett-MSFT Awaiting requested review from DHowett-MSFT

@carlos-zamora carlos-zamora Awaiting requested review from carlos-zamora

Assignees
No one assigned
Labels
Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Area-Settings Issues related to settings and customizability, for console or terminal Product-Terminal The new Windows Terminal.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add a setting to control selection background color
5 participants
@leonMSFT @mdtauk @javierdlg @miniksa @zadjii-msft