Enable switching app theme based off of OS theme #14497

zadjii-msft · 2022-12-06T16:06:40Z

This is basically just like #14064, but with the theme instead.

If you define a pair of theme names:

    "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.

…chemeSwitch

…me-theme-switch-switch

…eme-switch-switch

src/cascadia/TerminalSettingsModel/Theme.cpp

lhecker · 2022-12-16T14:28:07Z

src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp

    }

-    if (!_globals->Themes().HasKey(_globals->Theme()))
+    if (_globals->Theme().DarkName() == _globals->Theme().LightName())


BTW I think it'd be advantageous for us in the long term if we consistently stopped calling WinRT functions repeatedly like that. These calls are about 4x as expensive as regular, non-inlined function calls and returning a runtimeclass reference involves at least 2 such calls each time. I realize that even then these calls are plenty fast for "cold paths" like this, but I think that it might lead to impacts down the road, for instance by impacting Debug build performance to a point where it begins to be annoying (Debug build WinRT calls are over 20x more expensive), or just because one of these function calls ends up not being O(1) anymore but e.g. O(log N) or something (an ordered map lookup instead of an unordered one for instance).

Just to be clear, I'm not saying that this is a problem right now, but I'm hoping to make a case for what I think might be beneficial for our long term code health.

lhecker · 2022-12-16T14:28:52Z

src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp

@@ -1178,14 +1178,32 @@ void CascadiaSettings::_validateThemeExists()
        auto newTheme = winrt::make_self<Theme>();
        newTheme->Name(L"system");
        _globals->AddTheme(*newTheme);
-        _globals->Theme(L"system");
+        _globals->Theme(*winrt::make_self<ThemePair>(L"system"));


Why not just Model::ThemePair{ L"system" }?

lhecker · 2022-12-16T14:45:58Z

I know we need ThemePair to be class, because that's how it works, but...
I don't like it. It's a whole object for two members, something that is basically a std::pair, it gets an entire interface too and everyone got to QI it to get to the single implementor. Also I learned today that the proper name for this is "false dilemma" and is probably what best describes the design decisions that now forced us to write a class with more lines of boilerplate (45 LOC) than lines that actually use it (20 LOC).
...but I know that this is how it works and I would've written the exact same code you did.

…eme-switch-switch

ghost · 2023-01-12T13:16:34Z

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

✅

ghost · 2023-01-24T18:22:07Z

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

Handy links:

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

zadjii-msft · 2023-01-28T12:14:43Z

storing this here because I have to keep looking it up in teams chat history

* 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>

* 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>

* 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>

* 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>

…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>

Nicholas Bennett and others added 30 commits September 22, 2022 17:26

inintial buggy fix for syncing OS themes

3d5d88a

fixed control preview bug

5f54750

Code cleanup

43cc9fa

Code cleanup

e4c2d20

updated bug, where custom themes would not update the color scheme

b1c5342

updated bug, where custom themes would not update the color scheme

e4f3625

PR lhecker comments

c16b998

formatting fix

4e40eff

formatting fix

367d38c

Merge remote-tracking branch 'origin/main' into ThemeControlledColorS…

fdc2c80

…chemeSwitch

I think this is going to be way easier to manage

8e08d75

UI is better. Not good, but better

a0e28f1

hot reloading

b64b032

cleanup some todos

1b81da9

schema too folks

d71c56e

goodbye LoadSettings, ReloadSettings is my best friend now

e70ac18

yep. This is just as dumb as it seems

11460d6

tests man, tests (thread)

b969353

these weren't me!

c823fe6

Merge remote-tracking branch 'origin/main' into ThemeControlledColorS…

4e51484

…chemeSwitch

Merge remote-tracking branch 'origin/main' into ThemeControlledColorS…

9f251dd

…chemeSwitch

remove this scheme

1260705

Merge remote-tracking branch 'origin/main' into ThemeControlledColorS…

109ecd9

…chemeSwitch

first off, plumbing

1aa3675

secondly, this is _awesome

a14574a

Just a comment

fc6ef60

Merge remote-tracking branch 'origin/main' into ThemeControlledColorS…

ef6f660

…chemeSwitch

yea I dig it

c56a424

Merge branch 'ThemeControlledColorSchemeSwitch' into dev/migrie/f/the…

156e952

…me-theme-switch-switch

prep for review

77d00ee

ghost requested a review from lhecker December 9, 2022 20:41

zadjii-msft added 4 commits December 9, 2022 14:42

Merge remote-tracking branch 'origin/main' into dev/migrie/f/theme-th…

0fac998

…eme-switch-switch

why DONT we just not

85d6f92

Merge remote-tracking branch 'origin/main' into dev/migrie/f/theme-th…

c176c71

…eme-switch-switch

Merge remote-tracking branch 'origin/main' into dev/migrie/f/theme-th…

40afe5e

…eme-switch-switch

zadjii-msft mentioned this pull request Dec 12, 2022

Use a dark titlebar when we've requested dark theme #14536

Merged

carlos-zamora added a commit that referenced this pull request Dec 14, 2022

PRE-MERGE #14497 Enable switching app theme based off of OS theme

2c8855e

lhecker approved these changes Dec 16, 2022

View reviewed changes

zadjii-msft added this to the Terminal v1.17 milestone Jan 10, 2023

zadjii-msft added a commit that referenced this pull request Jan 12, 2023

I need this for #14497

1356c94

zadjii-msft added 2 commits January 12, 2023 06:24

Merge remote-tracking branch 'origin/main' into dev/migrie/f/theme-th…

bc1b385

…eme-switch-switch

minor nits

7666478

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

ghost merged commit 45a36cf into main Jan 12, 2023

ghost deleted the dev/migrie/f/theme-theme-switch-switch branch January 12, 2023 15:43

DHowett restored the dev/migrie/f/theme-theme-switch-switch branch January 13, 2023 00:15

DHowett deleted the dev/migrie/f/theme-theme-switch-switch branch January 13, 2023 00:16

DHowett reviewed Jan 13, 2023

View reviewed changes

ghost removed the Needs-Second It's a PR that needs another sign-off label Jan 13, 2023

zadjii-msft mentioned this pull request Jan 24, 2023

Themes updates for 1.17 MicrosoftDocs/terminal#634

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable switching app theme based off of OS theme #14497

Enable switching app theme based off of OS theme #14497

zadjii-msft commented Dec 6, 2022

lhecker Dec 16, 2022

lhecker Dec 16, 2022

lhecker commented Dec 16, 2022 •

edited

Loading

ghost commented Jan 12, 2023

DHowett left a comment

ghost commented Jan 24, 2023

zadjii-msft commented Jan 28, 2023

Enable switching app theme based off of OS theme #14497

Enable switching app theme based off of OS theme #14497

Conversation

zadjii-msft commented Dec 6, 2022

lhecker Dec 16, 2022

Choose a reason for hiding this comment

lhecker Dec 16, 2022

Choose a reason for hiding this comment

lhecker commented Dec 16, 2022 • edited Loading

ghost commented Jan 12, 2023

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 left a comment

Choose a reason for hiding this comment

ghost commented Jan 24, 2023

zadjii-msft commented Jan 28, 2023

lhecker commented Dec 16, 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.