Introduce setting override tracking and update SettingContainer by carlos-zamora · Pull Request #9079 · microsoft/terminal

carlos-zamora · 2021-02-09T03:26:38Z

This PR adds improved override message generation for inheritance in
SUI. The settings model now has an OriginTag to be able to denote
where a Profile came from. This tag is used in the SettingContainer
to generate a more specific override message.

References

#6800 - SUI Epic
#8919 - SUI Inheritance PR
#8804 - SUI Inheritance (old issue)

Detailed Description of the Pull Request / Additional comments

Terminal Settings Model
- Introduced PROJECTED_SETTING as a macro to more easily declare the
  functions for each setting
- Introduced <setting>OverrideSource which finds the Profile that
  has <setting> defined
- Introduced OriginTag Profile::Origin {Custom, InBox, Generated} to
  trace where a profile came from
- DefaultProfileUtils creates profiles for profile generators. So
  that now sets the Origin tag to Generated
- CascadiaSettings::LoadDefaults() tags all profiles created as
  InBox.
- The view model had to ingest the API change to be able to interact
  with <setting>OverrideSource
Override Message Generation
- The reset button now has a more specific tooltip
- The reset button now only appears if base layer is being overridden
- We use the settings model changes to determine the message to
  display for the target

Validation Steps Performed

Tested the following cases:

overrides nothing (inherited setting)
overrides value inherited from...
- base layer
- a profile generator
- in-box profile
global settings should not have this feature

carlos-zamora

I'm chatting with @cinnamon-msft after lunch to do a bit of wordsmithing.

carlos-zamora · 2021-02-09T20:15:18Z

src/cascadia/TerminalSettingsEditor/SettingContainer.cpp

+                        // '{}' was not found
+                        // fallback to more ambiguous version
+                        hstring overrideMsg{ fmt::format(std::wstring_view{ RS_(L"SettingContainer_OverrideIntro") }, RS_(L"SettingContainer_OverrideTarget")) };
+                        tb.Text(overrideMsg);


This should only happen if the localization was messed up. Idk if there's a way to get around this? Or handle it better?

src/cascadia/TerminalSettingsModel/IInheritable.h

carlos-zamora · 2021-02-09T20:22:54Z

src/cascadia/TerminalSettingsEditor/Profiles.cpp

+        // Hook up the hyperlinks from SettingContainer
+        _RegisterNavigationHandlers(GeneralStack());
+        _RegisterNavigationHandlers(AppearanceStack());
+        _RegisterNavigationHandlers(AdvancedStack());


I'm not happy about this one. But we could leverage this to access the setting containers via code now.

carlos-zamora · 2021-02-09T20:26:08Z

src/cascadia/TerminalSettingsEditor/SettingContainer.cpp

+                                    }
+                                    else
+                                    {
+                                        // TODO #1690: add special handling for proto-extensions here


Chatted with @PankajBhojwani about proto-extensions. This should mostly just work. Only changes that would be needed for proto-extensions are...

tag profiles from proto-extensions

use Profile::Source to create msgTarget

add a hyperlink for a read-only view of the proto-extension

carlos-zamora · 2021-02-09T22:27:34Z

Some more annoying news. It looks like the screen reader doesn't read the hyperlink. I've tried explicitly setting the automation properties as well and no luck.

Hyperlinks seem accessible in XAML Controls gallery. I'm worried this may be a XAML Islands issue? I'm looking more into that now.

zadjii-msft · 2021-02-09T22:33:47Z

Hyperlinks seem accessible in XAML Controls gallery. I'm worried this may be a XAML Islands issue? I'm looking more into that now.

Check the About dialog

carlos-zamora · 2021-02-09T22:40:57Z

Hyperlinks seem accessible in XAML Controls gallery. I'm worried this may be a XAML Islands issue? I'm looking more into that now.

Check the About dialog

The links in there are HyperlinkButtons. They work just fine. I'm guessing they rely on the a11y stuff for buttons so that's why?

DHowett · 2021-02-09T22:45:57Z

I don't expect that this is an islands-specific issue.

src/cascadia/TerminalSettingsModel/Profile.idl

carlos-zamora · 2021-02-17T00:39:38Z

Now that the text block is removed, we don't have the accessibility issue.

zadjii-msft

I suppose these are all nits? But there's enough of them that I'm just gonna leave a comment for now.

src/cascadia/TerminalSettingsModel/Profile.idl

src/cascadia/TerminalSettingsModel/IInheritable.h

src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw

src/cascadia/TerminalSettingsEditor/SettingContainer.cpp

src/cascadia/TerminalSettingsModel/DefaultProfileUtils.cpp

DHowett · 2021-02-17T19:20:00Z

src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw

-    <value>a lower layer</value>
-    <comment>This is the object of "SettingContainer_OverrideIntro".</comment>
+  <data name="SettingContainer_OverrideMessageBaseLayer" xml:space="preserve">
+    <value>Reset to base layer value</value>


if we're only gonna do base layer, should we still add all of the source tracking? It seems our new needs are satisfied by the existence of Has/Clear in the model.

We still need source tracking for a few reasons:

need to specifically detect when we're overriding "base layer" as opposed to "in-box" or "generated" profile object

we'll probably use some of this for telemetry

in preparation for profile inheritance, we'll still need to distinguish a profile that exists from one that isn't exposed to the user

src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw

DHowett · 2021-02-18T20:09:51Z

src/cascadia/TerminalSettingsEditor/SettingContainer.cpp

 #include "SettingContainer.h"
 #include "SettingContainer.g.cpp"
 #include "LibraryResources.h"
+#include "../TerminalSettingsModel/LegacyProfileGeneratorNamespaces.h"


is this required? Rather not include things directly out of TSM

Ah, this was a relic of when we were detecting the specific profile generator. Removed.

Had to add this back in to be able to detect when we're overriding a setting from the proto-extension vs one of our profile generators

DHowett · 2021-02-18T20:10:39Z

src/cascadia/TerminalSettingsEditor/SettingContainer.cpp

+                        // no source; we're using the system-default value
+                        tooltip = L"";
+                    }
+                    else if (const auto& profile{ settingSrc.try_as<Model::Profile>() })


i </3 that SettingContainer knows what a profile is

I concur - almost seems like we should have ProfileSettingContainer : SettingContainer that allows for reverting to the parent, but only for settings in profiles.

All this is terrible, but let's not pretend our settings model was designed for a GUI.

Long-term, we should probably move more of this responsibility over the the view model in a post-#9207 world. But I think we're just not ready for that yet, unfortunately.

src/cascadia/TerminalSettingsEditor/SettingContainer.cpp

src/cascadia/TerminalSettingsEditor/ViewModelHelpers.idl.h

src/cascadia/TerminalSettingsEditor/pch.h

src/cascadia/TerminalSettingsEditor/ViewModelHelpers.idl.h

src/cascadia/TerminalSettingsModel/IInheritable.h

src/cascadia/TerminalSettingsModel/IInheritable.idl.h

zadjii-msft

You know what, Dustin's got a lot more relevant comments than I do. If you can satisfy his concerns, I'll be happy here too

zadjii-msft · 2021-02-18T22:33:42Z

src/cascadia/TerminalSettingsEditor/SettingContainer.cpp

+                        // no source; we're using the system-default value
+                        tooltip = L"";
+                    }
+                    else if (const auto& profile{ settingSrc.try_as<Model::Profile>() })


I concur - almost seems like we should have ProfileSettingContainer : SettingContainer that allows for reverting to the parent, but only for settings in profiles.

All this is terrible, but let's not pretend our settings model was designed for a GUI.

carlos-zamora · 2021-02-19T03:32:27Z

Main thing I'm unsure about is the macros. I've broken them up and moved them closer to where they're used. Let me know what you think.

carlos-zamora · 2021-02-19T18:58:12Z

There's some weird behavior here:

create new profile w/ proto-extension
that profile has a setting "color scheme = vintage" for example
it appears in SUI as vintage (good)
weird behavior:
- 1. we change vintage --> campbell
  - no reset button (by design)
- 1. we set a color scheme to "solarized" in base layer
  - value is now solarized
  - no way to reset it back to vintage
- 1. (do 1 and 2 concurrently)
  - reset button appears saying we're overriding base layer (good!)

@cinnamon-msft what should the proper behavior here be.

carlos-zamora · 2021-02-19T20:24:57Z

Had to make a minor modification for proto extension. If we're specifically overriding a value from a proto-extension, we want this reset button to appear. See the demo above for what that looks like.

Design was discussed with @cinnamon-msft.

@DHowett thoughts?

DHowett · 2021-02-19T20:25:19Z

src/cascadia/TerminalSettingsEditor/SettingContainer.cpp

+            const auto profileSource{ profile.Source() };
+            if (profileSource == WslGeneratorNamespace ||
+                profileSource == AzureGeneratorNamespace ||
+                profileSource == PowershellCoreGeneratorNamespace)
+            {
+                // from a dynamic profile generator
+                return {};
+            }
+            else
+            {
+                // proto-extensions
+                return hstring{ fmt::format(std::wstring_view(RS_(L"SettingContainer_OverrideMessageProtoExtension")), profileSource) };
+            }


I flatly reject a dependency from this UI element to an implementation detail in the settings model. You're going to have to find a better way to do this.

Maybe you need a "Fragment" origin.

ok... can I get a bit more feedback here?

Maybe you need to tag things that come from fragments as coming from fragments rather than coming from "Generated" to give them special behavior?

The name is not going to be particularly pretty if it comes from a package. It will say, "value from: CanonicalGroupLtd.UbuntuOnWindows_7yabmiilazx1"

ewwww. Understandable. Is there a way to detect that it came from a package or if it's going to have an ugly name? Maybe some way to polish the ugly name into a cleaner one?

I'm ok with doing this for now, then revisiting it after some user feedback. We're going to have to reevaluate all of this work anyways :(

src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw

src/cascadia/TerminalSettingsEditor/SettingContainer.cpp

Co-authored-by: Dustin L. Howett <[email protected]>

DHowett · 2021-02-19T22:18:46Z

old demo-

DHowett · 2021-02-19T22:30:25Z

Looks like you'll have a conflict with #8414

DHowett · 2021-02-19T22:30:30Z

Sorry, #9036.

ghost · 2021-02-19T23:11:46Z

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.

PankajBhojwani · 2021-02-25T22:14:07Z

src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp

                        auto childImpl{ matchingProfile->CreateChild() };
                        childImpl->LayerJson(profileStub);
+                        childImpl->Origin(OriginTag::Fragment);
+                        childImpl->Source(source);


I removed this call in #9293, @carlos-zamora do we need it?

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

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

Handy links:

carlos-zamora added 5 commits February 8, 2021 11:29

TSM: add getter for source object

dd764a8

TSM: implement tagging mechanism for profiles

4c2f59a

fix TSM

309746b

TSE: implement message generation

576a3f0

working prototype

64c776e

carlos-zamora added Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal. Area-SettingsUI Anything specific to the SUI labels Feb 9, 2021

carlos-zamora requested a review from DHowett February 9, 2021 03:26

polish; ready for review

ac9b549

carlos-zamora commented Feb 9, 2021

View reviewed changes

carlos-zamora marked this pull request as ready for review February 9, 2021 20:26

carlos-zamora added 2 commits February 9, 2021 13:20

Merge branch 'main' into dev/cazamor/sui/inheritance-hyperlinks

4f3ca9a

fix build; update TerminalSettings

a99afb6

Merge branch 'main' into dev/cazamor/sui/inheritance-hyperlinks

1ba476e

code format

08e1050

DHowett reviewed Feb 10, 2021

View reviewed changes

src/cascadia/TerminalSettingsModel/Profile.idl Outdated Show resolved Hide resolved

carlos-zamora added 2 commits February 16, 2021 16:21

redesign: remove text; update reset btn

40d8742

PROJECTED_SETTING --> INHERITABLE_SETTING

f8c4081

zadjii-msft reviewed Feb 17, 2021

View reviewed changes

carlos-zamora changed the title ~~Add hyperlinks to Settings UI inheritance mechanism~~ Introduce setting override tracking and update SettingContainer Feb 17, 2021

DHowett reviewed Feb 17, 2021

View reviewed changes

polish more now that we don't need hyperlinks

500c38e

carlos-zamora force-pushed the dev/cazamor/sui/inheritance-hyperlinks branch from 526eb92 to 500c38e Compare February 17, 2021 19:21

DHowett requested changes Feb 18, 2021

View reviewed changes

zadjii-msft approved these changes Feb 18, 2021

View reviewed changes

address PR feedback

73e708d

carlos-zamora assigned DHowett Feb 19, 2021

carlos-zamora added 2 commits February 19, 2021 09:45

Merge branch 'main' into dev/cazamor/sui/inheritance-hyperlinks

5e02669

tag proto-extension profiles as generated

db21da6

carlos-zamora added 2 commits February 19, 2021 11:03

Merge branch 'main' into dev/cazamor/sui/inheritance-hyperlinks

f49cdbf

include the reset button when we're overriding proto-extension

7e094c6

DHowett requested changes Feb 19, 2021

View reviewed changes

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 Feb 19, 2021

DHowett reviewed Feb 19, 2021

View reviewed changes

src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw Outdated Show resolved Hide resolved

introduce fragment origin tag

9edceb2

DHowett reviewed Feb 19, 2021

View reviewed changes

src/cascadia/TerminalSettingsEditor/SettingContainer.cpp Outdated Show resolved Hide resolved

carlos-zamora and others added 2 commits February 19, 2021 13:03

Prefer {} construction

f1cd288

Co-authored-by: Dustin L. Howett <[email protected]>

Merge branch 'main' into dev/cazamor/sui/inheritance-hyperlinks

f1a8b35

DHowett approved these changes Feb 19, 2021

View reviewed changes

Merge branch 'main' into dev/cazamor/sui/inheritance-hyperlinks

f396b16

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Feb 19, 2021

ghost merged commit eb0fb3e into main Feb 19, 2021

ghost deleted the dev/cazamor/sui/inheritance-hyperlinks branch February 19, 2021 23:50

PankajBhojwani reviewed Feb 25, 2021

View reviewed changes

PankajBhojwani mentioned this pull request Feb 25, 2021

Fix updating with fragments ignoring original profile settings #9293

Merged

This pull request was closed.

Conversation

carlos-zamora commented Feb 9, 2021 • edited by DHowett Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

References

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Uh oh!

carlos-zamora left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlos-zamora commented Feb 9, 2021

Uh oh!

zadjii-msft commented Feb 9, 2021

Uh oh!

carlos-zamora commented Feb 9, 2021

Uh oh!

DHowett commented Feb 9, 2021

Uh oh!

Uh oh!

carlos-zamora commented Feb 17, 2021

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlos-zamora commented Feb 19, 2021

Uh oh!

carlos-zamora commented Feb 19, 2021

Uh oh!

carlos-zamora commented Feb 19, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlos-zamora commented Feb 9, 2021 •

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.