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.

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

Add support for switching the scheme based on the app's theme #14064

Merged
25 commits merged into from
Dec 6, 2022

Conversation

bennettnicholas
Copy link
Contributor

@bennettnicholas bennettnicholas commented Sep 23, 2022

Summary of the Pull Request

This pull request solved the problem of users not being able to set color schemes specifically for dark or light mode. Now the code has been updated to accept a dark and light color scheme in the json. The old setting is still compatible. Keep in mind if you update your color scheme through the settings UI, it will set both dark and light to the color scheme selected. This is because the settings UI update for selecting both Dark and Light color schemes is not supported yet.

This also solves the problem of the UI not using the system OS theme. Now you can select system theme and your color scheme will be selected based on if the system theme is dark or light.

References

#4066

PR Checklist

Detailed Description of the Pull Request / Additional comments

-Removed ColorSchemeName from MTSMSettings.h in order to process the setting for both string and object.
-Added DarkColorSchemeName and LightColorSchemeName properties to the AppearanceConfig to replace ColorSchemeName.
-Hacked a few processes to play nice with all 3 properties listed above as in some cases around the UI, we need to still use the ColorSchemeName. Once we change the UI I believe we can go back to just Dark and LightColorSchemeName
-Added and Updated Test to align to the new code.

Acceptable Json values,

"colorScheme":
{
"dark": "Campbell",
"light": "Campbell"
}
or

"colorScheme": "Campbell"

Validation Steps Performed

Individual testing along with the test case added.

@carlos-zamora carlos-zamora added the Needs-Discussion Something that requires a team discussion before we can proceed label Sep 26, 2022
@carlos-zamora
Copy link
Member

Hi @bennettnicholas,

Chatted with the team today. Here's some notes from our discussion:

  • don't worry about the color scheme not changing when you update the OS theme. We can throw that in a follow-up.
  • Settings UI:
    • I'm ok with this being another case (like window padding) where we don't overwrite the setting if the user didn't change it. It's not great at all, but it should suffice until we get the new one designed (others may disagree here, so we'll leave that up for discussion when they review this PR). Just make sure we track this in Epic: Settings UI v2 #10000.

@bennettnicholas bennettnicholas changed the title Syncing OS Tthemes Syncing OS Themes Sep 29, 2022
@ghost ghost added Area-Settings Issues related to settings and customizability, for console or terminal Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal. labels Oct 3, 2022
@bennettnicholas bennettnicholas marked this pull request as ready for review October 3, 2022 22:12
@bennettnicholas bennettnicholas mentioned this pull request Oct 3, 2022
65 tasks
@carlos-zamora carlos-zamora removed the Needs-Discussion Something that requires a team discussion before we can proceed label Oct 4, 2022
carlos-zamora added a commit that referenced this pull request Oct 8, 2022
@carlos-zamora
Copy link
Member

@bennettnicholas FYI found this this (sometimes) breaks color schemes: #14174

Please take a look when you get the chance. 😊

@bennettnicholas
Copy link
Contributor Author

bennettnicholas commented Oct 11, 2022

@carlos-zamora The bug has been identified and fixed. Along with another bug was discovered. Thanks for catching the bug. Let me know if I need to document the bug / fix

carlos-zamora added a commit that referenced this pull request Oct 11, 2022
Copy link
Member

@lhecker lhecker left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me! Just a few comments/questions...

src/cascadia/TerminalSettingsModel/AppearanceConfig.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsModel/AppearanceConfig.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsModel/AppearanceConfig.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsModel/AppearanceConfig.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsModel/TerminalSettings.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsModel/TerminalSettings.cpp Outdated Show resolved Hide resolved
@bennettnicholas
Copy link
Contributor Author

Looks pretty good to me! Just a few comments/questions...

@lhecker sorry for the delay, been fairly busy. I was able to incorporate most of the comments. The biggest thing around some of the logic in the code is because we are hacking the backend together until we update the UI. Once the UI is updated, a lot of this logic can be dropped. In many cases we have to process the color scheme name differently depending on if it's coming from the JSON or the UI. Please take a look at some of the changes and let me know what you think.

Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

  • please update the schema
  • please update the docs
  • We shouldn't need ColorSchemeName in the settings model anymore. We should only need the light/dark variants. Upon loading the JSON, if we encounter "colorScheme": "<name>", we should copy that name into both variants. We'll probably need special handling in Profile.cpp::LayerJson to accomplish this.
  • I think you need to modify MTSMSettings.h more. This is where we do the magic of defining a new setting "once" and it goes basically everywhere in the settings model (removes the need to implement a lot of boiler plate code)
  • update TerminalSettingsEditor. Specifically, the UI shouldn't change, but the text passed in should directly set the dark/light variant. Urgh, this still puts us in the situation of us not being sure which one to display depending on the theme. I say, look at the theme for the settings model that is in use, and display the relevant color scheme name. It's a bit weird, but I think that's a good intermediate step. CC @DHowett

src/cascadia/TerminalControl/IControlAppearance.idl Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsModel/TerminalSettings.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsModel/TerminalSettings.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsModel/IAppearanceConfig.idl Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp Outdated Show resolved Hide resolved
src/cascadia/inc/ControlProperties.h Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsModel/AppearanceConfig.h 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 Oct 20, 2022
@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Dec 6, 2022
Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

Awesome. Thanks @zadjii-msft and @bennettnicholas!

@carlos-zamora carlos-zamora removed their assignment Dec 6, 2022
@carlos-zamora
Copy link
Member

@zadjii-msft don't forget to document this plz

@carlos-zamora carlos-zamora added the AutoMerge Marked for automatic merge by the bot when requirements are met label Dec 6, 2022
@ghost
Copy link

ghost commented Dec 6, 2022

Hello @carlos-zamora!

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 ghost merged commit da2b80b into microsoft:main Dec 6, 2022
ghost pushed a commit that referenced this pull request Dec 12, 2022
I should have checked the `wparam`. 

Regressed in #14064.

Tested manually. No more crashy. 

Closes #14504
ghost pushed a commit that referenced this pull request Jan 12, 2023
This is basically just like #14064, but with the `theme` instead.

If you define a pair of `theme` names:

```json
    "theme": { "dark": "light", "light": "dark" },
```

then the Terminal will use the one relevant for the current OS theme. This cooperates with #14064, who sets the `scheme` based on the app's theme. 

This was spec'd as a part of #3327 / #12530, but never promoted to its own issue. 
Gif below.
@ghost
Copy link

ghost commented Jan 24, 2023

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

Handy links:

zadjii-msft added a commit to MicrosoftDocs/terminal that referenced this pull request Jan 25, 2023
Documents a whole pile of stuff from 1.17:

* `useMica` microsoft/terminal#13935
* theme pairs microsoft/terminal#14497
* scheme pairs microsoft/terminal#14064
* closes #626
mattwojo added a commit to MicrosoftDocs/terminal that referenced this pull request Mar 11, 2023
* fix(powerline): update oh-my-posh CLI (#632)

* Themes updates for 1.17 (#634)

Documents a whole pile of stuff from 1.17:

* `useMica` microsoft/terminal#13935
* theme pairs microsoft/terminal#14497
* scheme pairs microsoft/terminal#14064
* closes #626

* Default prompt icon info, resolves #625 (#642)

* Add app execution alias section (#643)

* Add autoHideWindows

Resolves #633

* Add color theme FAQs

* Remove code brackets from headers

Resolves #637

* Fix default if no command specified

Called out in #638

* Add note differentiating null from no setting 

for Starting Directory. 
Resolves #624

---------

Co-authored-by: Jan De Dobbeleer <2492783+JanDeDobbeleer@users.noreply.github.com>
Co-authored-by: Mike Griese <migrie@microsoft.com>
mattwojo added a commit to MicrosoftDocs/terminal that referenced this pull request Mar 21, 2023
* fix(powerline): update oh-my-posh CLI (#632)

* Themes updates for 1.17 (#634)

Documents a whole pile of stuff from 1.17:

* `useMica` microsoft/terminal#13935
* theme pairs microsoft/terminal#14497
* scheme pairs microsoft/terminal#14064
* closes #626

* Default prompt icon info, resolves #625 (#642)

* Add app execution alias section (#643)

* Add autoHideWindows

Resolves #633

* Add color theme FAQs

* Remove code brackets from headers

Resolves #637

* Fix default if no command specified

Called out in #638

* Add note differentiating null from no setting 

for Starting Directory. 
Resolves #624

* add the adobe-target metadata for A/B testing (#646)

could you please help to merge this metadata change (adobe-target=true) to main branch and then to live? This is to enable A/B testing ability across many url base paths so we can improve Learn user experience based on data-driven decisions. This meta (adobe-target) itself will not change how your content is displayed or modify any other UI behaviors so it is safe to merge.

---------

Co-authored-by: Jan De Dobbeleer <2492783+JanDeDobbeleer@users.noreply.github.com>
Co-authored-by: Mike Griese <migrie@microsoft.com>
Co-authored-by: Matt Wojciakowski <mattwoj@microsoft.com>
nguyen-dows added a commit to MicrosoftDocs/terminal that referenced this pull request May 19, 2023
* fix(powerline): update oh-my-posh CLI (#632)

* Themes updates for 1.17 (#634)

Documents a whole pile of stuff from 1.17:

* `useMica` microsoft/terminal#13935
* theme pairs microsoft/terminal#14497
* scheme pairs microsoft/terminal#14064
* closes #626

* Default prompt icon info, resolves #625 (#642)

* Add app execution alias section (#643)

* Add autoHideWindows

Resolves #633

* Add color theme FAQs

* Remove code brackets from headers

Resolves #637

* Fix default if no command specified

Called out in #638

* Add note differentiating null from no setting 

for Starting Directory. 
Resolves #624

* add the adobe-target metadata for A/B testing (#646)

could you please help to merge this metadata change (adobe-target=true) to main branch and then to live? This is to enable A/B testing ability across many url base paths so we can improve Learn user experience based on data-driven decisions. This meta (adobe-target) itself will not change how your content is displayed or modify any other UI behaviors so it is safe to merge.

* Add documentation to include information about enable/disable read-only mode functionality (#645)

* Update actions to include new enable and disable functionality

* Update panes to include new enable and disable functionality

* Many updates to commandline args docs, and some missing actions too (#649)

* Many updates to commandline args docs, and some missing actions too

* Fix anchor link warnings

---------

Co-authored-by: Matt Wojciakowski <mattwoj@microsoft.com>

* Remove Dynamic SSH profiles from doc since it has not been implemente… (#650)

* Remove Dynamic SSH profiles from doc since it has not been implemented yet.

* Updated authors and date

* Shelfing read-only mode docs until it goes in a future release (#653)

* Added maximum history size (#657)

* added maximum history size

* empty commit to poke the bot

* remove instances of Preview for 1.15 features (#669)

* remove enableReadOnlyMode and disableReadyOnlyMode docs in panes until it goes live in a future release (#672)

---------

Co-authored-by: Jan De Dobbeleer <2492783+JanDeDobbeleer@users.noreply.github.com>
Co-authored-by: Mike Griese <migrie@microsoft.com>
Co-authored-by: Matt Wojciakowski <mattwoj@microsoft.com>
Co-authored-by: juchen-ms <partychen.acm@gmail.com>
Co-authored-by: Alex Noble <6237394+Swinkid@users.noreply.github.com>
nguyen-dows added a commit to MicrosoftDocs/terminal that referenced this pull request May 23, 2023
* fix(powerline): update oh-my-posh CLI (#632)

* Themes updates for 1.17 (#634)

Documents a whole pile of stuff from 1.17:

* `useMica` microsoft/terminal#13935
* theme pairs microsoft/terminal#14497
* scheme pairs microsoft/terminal#14064
* closes #626

* Default prompt icon info, resolves #625 (#642)

* Add app execution alias section (#643)

* Add autoHideWindows

Resolves #633

* Add color theme FAQs

* Remove code brackets from headers

Resolves #637

* Fix default if no command specified

Called out in #638

* Add note differentiating null from no setting 

for Starting Directory. 
Resolves #624

* add the adobe-target metadata for A/B testing (#646)

could you please help to merge this metadata change (adobe-target=true) to main branch and then to live? This is to enable A/B testing ability across many url base paths so we can improve Learn user experience based on data-driven decisions. This meta (adobe-target) itself will not change how your content is displayed or modify any other UI behaviors so it is safe to merge.

* Add documentation to include information about enable/disable read-only mode functionality (#645)

* Update actions to include new enable and disable functionality

* Update panes to include new enable and disable functionality

* Many updates to commandline args docs, and some missing actions too (#649)

* Many updates to commandline args docs, and some missing actions too

* Fix anchor link warnings

---------

Co-authored-by: Matt Wojciakowski <mattwoj@microsoft.com>

* Remove Dynamic SSH profiles from doc since it has not been implemente… (#650)

* Remove Dynamic SSH profiles from doc since it has not been implemented yet.

* Updated authors and date

* Shelfing read-only mode docs until it goes in a future release (#653)

* Added maximum history size (#657)

* added maximum history size

* empty commit to poke the bot

* remove instances of Preview for 1.15 features (#669)

* remove enableReadOnlyMode and disableReadyOnlyMode docs in panes until it goes live in a future release (#672)

* Update Docs with Release 1.18 (#674)

* Stashed dynamic SSH profiles doc

* Add portable mode docs to release branch 1.18 (#655)

* added first draft of docs

* changed headings and subheadings

* Reword, add an Upgrading section

---------

Co-authored-by: Dustin Howett <duhowett@microsoft.com>

* Retool the portable mode documentation to explain our distributions (#656)

* Retool portable mode to be 'distributions'

* Fix warnings

* add preinstallation to the feature matrix

* add dci to matrix

* reword

* couple more notes

* better version note

* Use a data matrix table, re-alt-text the portable mode image

* add distributions to the toc

* address L's feedbacc

* Document 'activeOnly' value for 'showCloseButton' property. (#660)

* Shelfing read-only mode docs until it goes in a future release (#653)

* Added maximum history size (#657)

* added maximum history size

* empty commit to poke the bot

* Document 'activeOnly' value for 'showCloseButton' property.

---------

Co-authored-by: Christopher Nguyen <91625426+nguyen-dows@users.noreply.github.com>

* Add a small section on tab tearout in the overview page (#666)

* Shelfing read-only mode docs until it goes in a future release (#653)

* Added maximum history size (#657)

* added maximum history size

* empty commit to poke the bot

* initial commit

* added preview links and small grammatical fixes

* added important preview flag

* moved Important flag to the end of the section to fit Actions docs

* omit unrelated changes that got pulled in from master

* omit actions and profiles changes

* Add preview text and re-add missing enableReadOnlyMode docs (#673)

* add preview text and readd enableReadOnlyMode that was lost in the most recent commit

* add preview text for activeOnly value in showCloseButton property docs

---------

Co-authored-by: Dustin Howett <duhowett@microsoft.com>
Co-authored-by: kovdu <koen.vandurme@gmail.com>

---------

Co-authored-by: Jan De Dobbeleer <2492783+JanDeDobbeleer@users.noreply.github.com>
Co-authored-by: Mike Griese <migrie@microsoft.com>
Co-authored-by: Matt Wojciakowski <mattwoj@microsoft.com>
Co-authored-by: juchen-ms <partychen.acm@gmail.com>
Co-authored-by: Alex Noble <6237394+Swinkid@users.noreply.github.com>
Co-authored-by: Dustin Howett <duhowett@microsoft.com>
Co-authored-by: kovdu <koen.vandurme@gmail.com>
nguyen-dows added a commit to MicrosoftDocs/terminal that referenced this pull request May 24, 2023
…ocs (#678)

* fix(powerline): update oh-my-posh CLI (#632)

* Themes updates for 1.17 (#634)

Documents a whole pile of stuff from 1.17:

* `useMica` microsoft/terminal#13935
* theme pairs microsoft/terminal#14497
* scheme pairs microsoft/terminal#14064
* closes #626

* Default prompt icon info, resolves #625 (#642)

* Add app execution alias section (#643)

* Add autoHideWindows

Resolves #633

* Add color theme FAQs

* Remove code brackets from headers

Resolves #637

* Fix default if no command specified

Called out in #638

* Add note differentiating null from no setting 

for Starting Directory. 
Resolves #624

* add the adobe-target metadata for A/B testing (#646)

could you please help to merge this metadata change (adobe-target=true) to main branch and then to live? This is to enable A/B testing ability across many url base paths so we can improve Learn user experience based on data-driven decisions. This meta (adobe-target) itself will not change how your content is displayed or modify any other UI behaviors so it is safe to merge.

* Add documentation to include information about enable/disable read-only mode functionality (#645)

* Update actions to include new enable and disable functionality

* Update panes to include new enable and disable functionality

* Many updates to commandline args docs, and some missing actions too (#649)

* Many updates to commandline args docs, and some missing actions too

* Fix anchor link warnings

---------

Co-authored-by: Matt Wojciakowski <mattwoj@microsoft.com>

* Remove Dynamic SSH profiles from doc since it has not been implemente… (#650)

* Remove Dynamic SSH profiles from doc since it has not been implemented yet.

* Updated authors and date

* Shelfing read-only mode docs until it goes in a future release (#653)

* Added maximum history size (#657)

* added maximum history size

* empty commit to poke the bot

* remove instances of Preview for 1.15 features (#669)

* remove enableReadOnlyMode and disableReadyOnlyMode docs in panes until it goes live in a future release (#672)

* Update Docs with Release 1.18 (#674)

* Stashed dynamic SSH profiles doc

* Add portable mode docs to release branch 1.18 (#655)

* added first draft of docs

* changed headings and subheadings

* Reword, add an Upgrading section

---------

Co-authored-by: Dustin Howett <duhowett@microsoft.com>

* Retool the portable mode documentation to explain our distributions (#656)

* Retool portable mode to be 'distributions'

* Fix warnings

* add preinstallation to the feature matrix

* add dci to matrix

* reword

* couple more notes

* better version note

* Use a data matrix table, re-alt-text the portable mode image

* add distributions to the toc

* address L's feedbacc

* Document 'activeOnly' value for 'showCloseButton' property. (#660)

* Shelfing read-only mode docs until it goes in a future release (#653)

* Added maximum history size (#657)

* added maximum history size

* empty commit to poke the bot

* Document 'activeOnly' value for 'showCloseButton' property.

---------

Co-authored-by: Christopher Nguyen <91625426+nguyen-dows@users.noreply.github.com>

* Add a small section on tab tearout in the overview page (#666)

* Shelfing read-only mode docs until it goes in a future release (#653)

* Added maximum history size (#657)

* added maximum history size

* empty commit to poke the bot

* initial commit

* added preview links and small grammatical fixes

* added important preview flag

* moved Important flag to the end of the section to fit Actions docs

* omit unrelated changes that got pulled in from master

* omit actions and profiles changes

* Add preview text and re-add missing enableReadOnlyMode docs (#673)

* add preview text and readd enableReadOnlyMode that was lost in the most recent commit

* add preview text for activeOnly value in showCloseButton property docs

---------

Co-authored-by: Dustin Howett <duhowett@microsoft.com>
Co-authored-by: kovdu <koen.vandurme@gmail.com>

* Add a tutorial for enabling shell integration (#676)

* it's basically just the blog post

* fix urls

* Right-click context menu, allowHeadless docs (#677)

* right-click context menu docs

* allowHeadless too while I'm here

---------

Co-authored-by: Jan De Dobbeleer <2492783+JanDeDobbeleer@users.noreply.github.com>
Co-authored-by: Mike Griese <migrie@microsoft.com>
Co-authored-by: Matt Wojciakowski <mattwoj@microsoft.com>
Co-authored-by: juchen-ms <partychen.acm@gmail.com>
Co-authored-by: Alex Noble <6237394+Swinkid@users.noreply.github.com>
Co-authored-by: Dustin Howett <duhowett@microsoft.com>
Co-authored-by: kovdu <koen.vandurme@gmail.com>
@MatejKafka
Copy link

MatejKafka commented Jul 6, 2023

Is there a way to use a dark theme, but still switch the color scheme of a profile based on the system-wide theme? The dark theme is more readable for me, but I like to have the color scheme follow the system, since that affects a much larger area of the UI.

@zadjii-msft
Copy link
Member

yes and no.

You could do something like

    "theme": { "dark": "my light", "light": "dark" },
    "themes":
    [
        {
            "name": "my light",
            "tab":
            {
                "background": "terminalBackground",
                "unfocusedBackground": "#00000000"
            },
            "tabRow":
            {
                "background": "#2e2e2e",
                "unfocusedBackground": "#333333FF"
            },
            "window":
            {
                "applicationTheme": "light"
            }
        },

so that the tabs were dark, even if the rest of the UI was light. The applicationTheme being light would them let the scheme pick the light value.


But more completely - trying to support that scenario (dark UI, OS-following color scheme) proved a bit to confusing for the settings to support, hence the current approach. (as discussed around #4066 (comment))

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Area-Settings Issues related to settings and customizability, for console or terminal Area-UserInterface Issues pertaining to the user interface of the Console or Terminal AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Bug It either shouldn't be doing this or needs an investigation. Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SettingsModelLocalTests failures are on the rise Theme-controlled color scheme switch
7 participants