Add support for a profile to specify an "unfocused" appearance #8392

PankajBhojwani · 2020-11-24T23:22:11Z

This pull request adds an appearance configuration object to our
settings model and app lib, allowing the control to be rendered
differently depending on its state, and then uses it to add support for
an "unfocused" appearance that the terminal will use when it's not in
focus.

To accomplish this, we isolated the appearance-related settings from
Profile (into AppearanceConfig) and TerminalSettings (into the
IControlAppearance and ICoreAppearance interfaces). A bunch of work was
done to make inheritance work.

The unfocused appearance inherits from the focused one for that
profile. This is important: If you define a
defaults.unfocusedAppearance, it will apply all of defaults' settings to
any leaf profile when a terminal in that profile is out of focus.

Specified in #8345
Closes #3062
Closes #2316

… applied fully

carlos-zamora

Awesome! This is all much clearer now. Thanks you!

src/cascadia/TerminalApp/Pane.cpp

src/cascadia/TerminalApp/TabManagement.cpp

src/cascadia/TerminalApp/TerminalTab.cpp

src/cascadia/TerminalSettingsModel/IInheritable.h

src/cascadia/TerminalSettingsModel/Profile.h

carlos-zamora · 2021-04-05T17:29:53Z

src/cascadia/TerminalSettingsModel/Profile.cpp

+// Method Description:
+// - Inserts a parent profile into a child profile, at the specified index if one was provided
+// - Makes sure to call _FinalizeInheritance after inserting the parent
+// Arguments:
+// - child: the child profile to insert the parent into
+// - parent: the parent profile to insert into the child
+// - index: an optional index value to insert the parent into
+void Profile::InsertParentHelper(winrt::com_ptr<Profile> child, winrt::com_ptr<Profile> parent, std::optional<size_t> index)
+{
+    if (index)
+    {
+        child->InsertParent(index.value(), parent);
+    }
+    else
+    {
+        child->InsertParent(parent);
+    }
+    child->_FinalizeInheritance();
+}
+


I guess we don't need to right now, but would it make sense to move this to IInheritable?

Ah so we don't want every InsertParent call to call _FinalizeInheritance, that's why Profile implements a helper for it instead. (Adding _FinalizeInheritance in InsertParent will break GlobalAppSettings in the settings UI)

carlos-zamora · 2021-04-05T18:53:10Z

src/cascadia/TerminalSettingsModel/TerminalSettings.cpp

+        auto defaultChild = defaultImpl->CreateChild();
+        if (parent.UnfocusedSettings())
+        {
+            parent.UnfocusedSettings().SetParent(*defaultChild);


Discussed offline. Yeah, this makes sense. I guess what was confusing to me is that this function would make TerminalSettings inherit from another TerminalSettings. Here, we're doing the same thing. TerminalSettingsCreateResult is intended to just be a way to pass two objects at once. It is not inheritable, whereas TerminalSettings is.

src/cascadia/TerminalControl/TermControl.h

DHowett

Some explanation required around the locking changes. We have had no end of trouble with locking, so this seems ~ ~ dangerous ~ ~

DHowett · 2021-04-07T17:37:06Z

src/cascadia/TerminalControl/TermControl.h

@@ -26,6 +26,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
        TermControl(IControlSettings settings, TerminalConnection::ITerminalConnection connection);

        winrt::fire_and_forget UpdateSettings();
+        winrt::fire_and_forget UpdateAppearance(const IControlAppearance newAppearance);


Suggested change

winrt::fire_and_forget UpdateAppearance(const IControlAppearance newAppearance);

winrt::fire_and_forget UpdateAppearance(const IControlAppearance& newAppearance);

always one more & ;)

wait, we can't do this because its a fire_and_forget right?

i keep making this mistake! AGH.

DHowett · 2021-04-07T17:51:57Z

src/cascadia/TerminalControl/TermControl.cpp

-            // Initialization will handle the renderer settings.
-            return;
-        }
+        auto lock = _terminal->LockForWriting();


@PankajBhojwani yeah -- why did this lock move down?

DHowett · 2021-04-07T17:52:59Z

src/cascadia/TerminalControl/TermControl.cpp

-            // Initialization will handle the renderer settings.
-            return;
-        }
+        auto lock = _terminal->LockForWriting();


i am also very worried about the removal of the initialization check below. I didn't see it pop back up anywhere.

This lock guards access to the variable determining whether the terminal is initialized above all -- we will explode if we try to update settings while not initialized.

DHowett · 2021-04-07T22:08:00Z

src/cascadia/TerminalControl/TermControl.cpp

@@ -878,6 +910,9 @@ namespace winrt::Microsoft::Terminal::Control::implementation
            //      becomes a no-op.
            this->Focus(FocusState::Programmatic);

+            // Now that the renderer is set up, update the appearance for initialization
+            UpdateAppearance(_settings);


okay, so. It looks like we're doing a bunch of other things here that are touching XAML objects. I think that initialization happens on the UI thread already. I think that we should then in this case call the "from the UI thread" version. Otherwise, we will end up in a weird place. We're literally saying... "Update the appearance after I return from this function, having unlocked the terminal lock"

I bet that that would deadlock because _UpdateAppearanceFromUIThread takes the lock.

How were the appearance settings getting applied before this? o_O

So... I don't remember why I added this line here, and removing it doesn't cause any issues. I'm guessing this line was needed at first but somewhere down the line it wasn't required anymore and I didn't come back to update this! Its gone now :)

DHowett

I'm excited. Let's do it.

ghost · 2021-04-08T22:46:12Z

Hello @DHowett!

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 · 2021-04-14T17:09:36Z

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

Handy links:

## Summary of the Pull Request Allow schemes to be previewed as the user hovers over them in the Command Palette. ![preview-set-color-scheme](https://user-images.githubusercontent.com/18356694/114557761-9a3cbd80-9c2f-11eb-987f-eb0c89ee1fa6.gif) ## References * Branched off of #8392, which is why the commit history is so polluted. 330a8e8 : 544b2fd has the interesting commits * #5400: cmdpal megathread ### Potential follow-ups * changing the font size * changing the font face * changing the opacity of acrylic ## PR Checklist * [x] Closes #6689, a last straggling FHL PR * [x] I work here * [ ] Tests added/passed * [n/a] Requires documentation to be updated - I don't think so ## Detailed Description of the Pull Request / Additional comments This works by inserting a "preview" `TerminalSettings` into the settings hierarchy, before the `TermControl`'s runtime settings, and after the ones from the actual `CascadiaSettings`. This allows us to modify that preview settings object, then discard it when we're done with the preview. This could also be used for other settings in the future - I built it to be extensible to other `ShortcutAction`s, though I haven't implemented those yet. ## Validation Steps Performed * Select a colorscheme - it becomes the active one * `colortool -x <scheme>` after selecting a scheme - colortool overrides the selected scheme * Select a colorscheme after a `colortool -x <scheme>` after selecting a scheme - the scheme in the palette becomes the active one * Pressing <kbd>esc</kbd> at any point to dismiss the command palette - scheme returns to the previous one * reloading the settings - returns to the scheme in the settings

ghost · 2021-05-25T19:06:03Z

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

Handy links:

PankajBhojwani added 4 commits November 20, 2020 16:54

add object on Core side

c6432b6

object in settings model side

f806011

applib side object

5aa1e66

barebones prototype

c423e11

zadjii-msft self-assigned this Nov 30, 2020

PankajBhojwani mentioned this pull request Nov 30, 2020

Spec: Appearance configuration objects for profiles #8345

Merged

6 tasks

PankajBhojwani added 20 commits November 30, 2020 11:35

renaming

8f8e6e7

cursor shape

f4dd69a

background image works now

235bbd7

deduplicate code: UpdateSettings now calls UpdateAppearance

dc8328d

spell, Cl check

d3ad801

IAppearanceConfig interface in settings model side, color scheme gets…

43c6204

… applied fully

background image parameters

2960ba2

actually use the image params

bc12378

functions for update settings/appearance from ui settings

34621c1

conflict

45a9718

update with the new converged alignment

d9fbe56

IControl/CoreAppearance

9e1a9f9

inheritance step 1?

a118fe7

placeholder inheritance

90025ec

friend struct works

e0244d5

not experimental

95e679c

color scheme fix

6688c3c

dont update appearance if no unfocused config

9eba7ab

add comments to code

698cf41

update to mock term settings

8194aca

PankajBhojwani changed the title ~~[DRAFT] Appearance configuration objects for profiles~~ Appearance configuration objects for profiles Dec 11, 2020

PankajBhojwani marked this pull request as ready for review December 11, 2020 20:31

PankajBhojwani added 2 commits December 16, 2020 13:21

begin

8b64425

everything updates except color scheme?

403646f

This comment has been minimized.

Sign in to view

mock term

822fefe

carlos-zamora approved these changes Apr 5, 2021

View reviewed changes

address nits

d04c557

zadjii-msft mentioned this pull request Apr 6, 2021

Feature request: Ability to change window appearance when window is not in focus #9711

Closed

DHowett requested changes Apr 7, 2021

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 Apr 7, 2021

move lock, initialized check

e2130e6

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 7, 2021

DHowett reviewed Apr 7, 2021

View reviewed changes

remove update appearance call

0089065

DHowett approved these changes Apr 8, 2021

View reviewed changes

DHowett changed the title ~~Appearance configuration objects for profiles~~ Add support for a profile to specify an "unfocused" appearance Apr 8, 2021

ghost added Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Issue-Task It's a feature request, but it doesn't really need a major design. labels Apr 8, 2021

ghost merged commit 9e83655 into main Apr 8, 2021

ghost deleted the dev/pabhoj/unfocused_pane branch April 8, 2021 22:46

This was referenced Apr 13, 2021

Enable previewing the color scheme in the command palette #9794

Merged

Introduce abstraction layer between TermControl and TerminalCore (aka Interactivity) #6842

Closed

This was referenced Apr 14, 2021

Feature Request: Specify cursor behaviour on window focus / unfocus #2316

Closed

Add a configuration object for focused vs unfocused state #3062

Closed

Chaphasilor mentioned this pull request May 9, 2021

Investigate: AcrylicBlur always on in an inactive window #7158

Closed

sdsalyer mentioned this pull request Sep 15, 2021

unfocusedAppearance cursorShape doesn't work #10215

Open

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for a profile to specify an "unfocused" appearance #8392

Add support for a profile to specify an "unfocused" appearance #8392

PankajBhojwani commented Nov 24, 2020 •

edited by DHowett

Loading

This comment has been minimized.

carlos-zamora left a comment

carlos-zamora Apr 5, 2021

PankajBhojwani Apr 5, 2021

carlos-zamora Apr 5, 2021

DHowett left a comment

DHowett Apr 7, 2021

PankajBhojwani Apr 7, 2021

DHowett Apr 7, 2021

DHowett Apr 7, 2021

DHowett Apr 7, 2021

DHowett Apr 7, 2021

PankajBhojwani Apr 8, 2021

DHowett left a comment

ghost commented Apr 8, 2021

ghost commented Apr 14, 2021

ghost commented May 25, 2021

	winrt::fire_and_forget UpdateAppearance(const IControlAppearance newAppearance);
	winrt::fire_and_forget UpdateAppearance(const IControlAppearance& newAppearance);

Add support for a profile to specify an "unfocused" appearance #8392

Add support for a profile to specify an "unfocused" appearance #8392

Conversation

PankajBhojwani commented Nov 24, 2020 • edited by DHowett Loading

This comment has been minimized.

carlos-zamora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

ghost commented Apr 8, 2021

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 commented Apr 14, 2021

ghost commented May 25, 2021

PankajBhojwani commented Nov 24, 2020 •

edited by DHowett

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.