Separate runtime TerminalSettings from profile-TerminalSettings by PankajBhojwani · Pull Request #8602 · microsoft/terminal

PankajBhojwani · 2020-12-17T01:56:51Z

Summary of the Pull Request

The TerminalSettings object we create from profiles no longer gets passed into the control, instead, a child of that object gets passed into the control. Any overrides the control makes to the settings then live in the child. So, when we do a settings reload, we simply update the child's parent and the overrides will remain.

PR Checklist

Closes #xxx
CLA signed. If not, go over here and sign the CLA
Tests added/passed
Documentation updated. If checked, please file a pull request on our docs repo and link it here: #xxx
Schema updated.
I work here

Validation Steps Performed

Manual testing

…/pabhoj/terminal_settings

carlos-zamora

This is really cool!

src/cascadia/TerminalControl/TermControl.cpp

DHowett · 2020-12-24T07:42:20Z

It's weird -- I'm running a build of this with default settings (like, I cleared my app settings) and all my colors are black. Indicates the color table is all 0s.

carlos-zamora · 2020-12-30T00:15:34Z

src/cascadia/TerminalApp/TerminalSettings.h

 #pragma once

 #include "TerminalSettings.g.h"
+#include "../TerminalSettingsModel/IInheritable.h"


(This is a much larger discussion that doesn't have to be implemented as a part of this PR. So really I'm thinking out loud.)
I'm wondering if we should move TerminalSettings over to TerminalSettingsModel. Hear me out:

if we're making TerminalSettings IInheritable, might as well move TerminalSettings over to TSM. It's a bit weird for us to grab this random file from a separate project, right? haha

With regards to creating a TermControl preview in Settings UI...

I hit a problem today. TSE cannot depend on TermApp because that'd create a circular dependency (TermApp <--> TSE). So I tried making it so that TSE builds a TerminalSettings by firing an event to TermApp, but then TermApp would need to somehow get the synthesized TerminalSettings object down to the profile page and update on each change. Having TerminalSettings be its own project or a part of TSM makes it it so much easier to construct TerminalSettings and just have one readily available for a TermControl preview.

What do you think?

CC @DHowett for thoughts on this matter too. I might just be approaching the two things above completely wrong haha

You might have a point here (but it should probably be a follow up)

Bumping this thread for @DHowett because this is important for the terminal preview in SUI.

(Chatted in meeting.)

…/pabhoj/terminal_settings

PankajBhojwani · 2021-01-05T00:45:43Z

It's weird -- I'm running a build of this with default settings (like, I cleared my app settings) and all my colors are black. Indicates the color table is all 0s.

Fixed with latest push - I did need to implement a separate macro to implement an array setting though (if you try GETSET_SETTING(std::array<uint32_t, 16>, ColorTable) for example, the macro thinks that "16" is the name of the setting instead of "ColorTable")

zadjii-msft · 2021-01-08T13:52:50Z

@msftbot wait 8 hours before merging this

ghost · 2021-01-08T13:52:54Z

Hello @zadjii-msft!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I won't merge this pull request until after the UTC date Fri, 08 Jan 2021 21:52:53 GMT, which is in 8 hours

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

zadjii-msft

You know, I was about to sign off on this, but now I'm all worried about the copying of the array, and I think it's best if we let Dustin or Michael be the second on it

src/cascadia/TerminalApp/AppActionHandlers.cpp

src/cascadia/TerminalApp/Pane.cpp

zadjii-msft · 2021-01-08T13:42:06Z

src/cascadia/TerminalApp/TerminalSettings.h

 #pragma once

 #include "TerminalSettings.g.h"
+#include "../TerminalSettingsModel/IInheritable.h"


You might have a point here (but it should probably be a follow up)

zadjii-msft · 2021-01-08T13:54:31Z

src/cascadia/TerminalSettingsModel/IInheritable.h

+                                                                            \
+private:                                                                    \
+    std::optional<std::array<type, size>> _##name{ std::nullopt };          \
+    std::optional<std::array<type, size>> _get##name##Impl() const          \


I wanna make sure Dustin takes a look at the template magic going on in this macro. An optional<array<T>> seems to make sense to me, but there might be weird template quirks I'm missing.

zadjii-msft · 2021-01-08T13:56:50Z

src/cascadia/TerminalSettingsModel/IInheritable.h

+        /*iterate through parents to find a value*/                         \
+        for (auto parent : _parents)                                        \
+        {                                                                   \
+            if (auto val{ parent->_get##name##Impl() })                     \


Specifically this bit I'm worried about - are we returning a copy of the parent's result?

…/pabhoj/terminal_settings

DHowett · 2021-02-05T02:00:50Z

Touching to say we had a discussion about doing the array without copies.

…/pabhoj/terminal_settings

DHowett · 2021-02-08T19:15:59Z

src/cascadia/TerminalApp/TerminalSettings.cpp


 namespace winrt::TerminalApp::implementation
 {
+    static constexpr std::array<til::color, 16> campbellColorTable{


Why can't we use the one from Utils? Is it because it's a filler-style method?

Maybe we can just expose a function that returns it directly... as a gsl::span.

(Really don't love the duplication)

Yeah I don't like the duplication either - will add a function to colorTable.hpp to expose the campbell color table

DHowett · 2021-02-08T19:16:09Z

src/cascadia/TerminalApp/TerminalSettings.cpp


 namespace winrt::TerminalApp::implementation
 {
+    static constexpr std::array<til::color, 16> campbellColorTable{


(Really don't love the duplication)

DHowett · 2021-02-08T19:16:58Z

src/cascadia/TerminalApp/TerminalSettings.cpp

+        // issues:
+        // - converting span to array without brute force?
+        // - campbellColorTable conversion from color to uint32
+        // - including colortable.cpp causes errors


we don't import cpp files, like... ever. 😄

DHowett · 2021-02-08T22:53:20Z

src/cascadia/TerminalApp/TerminalSettings.h

 #include <DefaultSettings.h>
 #include <conattrs.hpp>

+using namespace Microsoft::Console::Utils;


ooh can you put out a PR that deletes this? we should never put a using in a header.

/cc @carlos-zamora @zadjii-msft 😄

In fact, only the cpp file needs to know about colorTable.hpp and the using block, correct?

Ah sorry my bad, made a PR to fix it

There was a mistake in #8602, this change fixes that. (`using` and `include` moved to cpp file)

In #8602, we started passing a child of the `TerminalSettings` to the control upon tab initialization, but forgot to do the same when new controls get created on a pane split. ## Validation Steps Performed Settings reload with multiple panes works Closes #9280

In #8602, we started passing a child of the `TerminalSettings` to the control upon tab initialization, but forgot to do the same when new controls get created on a pane split. ## Validation Steps Performed Settings reload with multiple panes works Closes #9280 (cherry picked from commit a930aa3)

ghost · 2021-03-01T18:02:54Z

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

Handy links:

Since #8602 merged, we need to pass a child of the settings object to the TermControl upon initializing it. Since this happens in a few places in `TerminalPage`, its probably best to use a helper. Closes #9292

PankajBhojwani added 7 commits December 16, 2020 13:21

begin

8b64425

everything updates except color scheme?

403646f

color scheme fix

7b5e04e

dead code

21139f5

remove unnecessary comment

7079e60

format

1fc3e8b

Merge branch 'main' of https://github.com/microsoft/terminal into dev…

5151a7f

…/pabhoj/terminal_settings

carlos-zamora approved these changes Dec 24, 2020

View reviewed changes

src/cascadia/TerminalControl/TermControl.cpp Outdated Show resolved Hide resolved

carlos-zamora reviewed Dec 30, 2020

View reviewed changes

PankajBhojwani added 4 commits January 4, 2021 11:30

Merge branch 'main' of https://github.com/microsoft/terminal into dev…

8d2c583

…/pabhoj/terminal_settings

pragma warning

a288001

conflict

ceb1d00

color table fix

b57ee23

ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jan 8, 2021

zadjii-msft reviewed Jan 8, 2021

View reviewed changes

zadjii-msft assigned DHowett and miniksa Jan 8, 2021

PankajBhojwani added 3 commits January 8, 2021 10:46

fix comment

ab36124

Merge branch 'main' of https://github.com/microsoft/terminal into dev…

b9ef9f1

…/pabhoj/terminal_settings

incomplete

54e5123

zadjii-msft requested a review from miniksa February 4, 2021 13:54

DHowett assigned PankajBhojwani and unassigned DHowett and miniksa Feb 5, 2021

comments

8d27531

PankajBhojwani added 3 commits February 8, 2021 10:32

couple of clarifications left

efd9176

Merge branch 'main' of https://github.com/microsoft/terminal into dev…

b29183f

…/pabhoj/terminal_settings

fix ambiguous symbols in tests

8cb6501

DHowett reviewed Feb 8, 2021

View reviewed changes

expose campbell color table

f402a8c

zadjii-msft approved these changes Feb 8, 2021

View reviewed changes

ghost merged commit 9047bbb into main Feb 8, 2021

ghost deleted the dev/pabhoj/terminal_settings branch February 8, 2021 22:01

DHowett reviewed Feb 8, 2021

View reviewed changes

PankajBhojwani mentioned this pull request Feb 8, 2021

Quickly fix a mistake in #8602 #9078

Merged

DHowett pushed a commit that referenced this pull request Feb 9, 2021

Quickly fix a mistake in #8602 (#9078)

a902895

There was a mistake in #8602, this change fixes that. (`using` and `include` moved to cpp file)

carlos-zamora mentioned this pull request Feb 11, 2021

Add a preview of your terminal to the Settings UI #9122

Closed

This was referenced Feb 25, 2021

Fix settings not updating on reload #9289

Merged

Code health: update every call of initializing TermControl with settings #9292

Closed

PankajBhojwani mentioned this pull request Feb 25, 2021

Add a helper function to initialize TermControls in TerminalPage #9296

Merged

PankajBhojwani mentioned this pull request Feb 26, 2021

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

Merged

This pull request was closed.

Conversation

PankajBhojwani commented Dec 17, 2020

Summary of the Pull Request

PR Checklist

Validation Steps Performed

Uh oh!

carlos-zamora left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DHowett commented Dec 24, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PankajBhojwani commented Jan 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zadjii-msft commented Jan 8, 2021

Uh oh!

ghost commented Jan 8, 2021

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DHowett commented Feb 5, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Mar 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

PankajBhojwani commented Jan 5, 2021 •

edited

Loading