Reintroduce the Defaults page and the Reset buttons #10588
Conversation
This reverts commit 19fb9b2.
Why do we pass a profile's guid into TermSettings::CreateWithProfileByID just so it can look up the profile object we are already holding? Let's not do that.
This commit introduces a new origin and makes sure that we set it to "User" when we're processing a user's profiles. It sucks.
ghost
added
Area-Settings UI
| if (_userDefaultProfileSettings) | ||
| { | ||
| // If we've loaded defaults{} we're in the "user settings" phase for sure | ||
| profile->Origin(OriginTag::User); | ||
| } |
There was a problem hiding this comment.
@carlos-zamora this is the part that I am iffy on. I don't like it, but it is the easiest way to know that we are currently processing the user profile list compartment :|
There was a problem hiding this comment.
This crashes when profile is null -- why is profile null? i guess it's.. a first-run scenario? I'm not actually sure...
There was a problem hiding this comment.
ah, it's a profile that my DPG no longer emits
| <name>Feature_ShowProfileDefaultsInSettings</name> | ||
| <description>Whether to show the "base layer" ("defaults") page in the Terminal settings UI</description> | ||
| <stage>AlwaysEnabled</stage> | ||
| <!-- This feature will not ship to Stable until it is complete. --> |
There was a problem hiding this comment.
Project management question:
Do we (want to) have an issue tracking this? Technically it'd be #10430, but I know we hope to iterate on this before going to stable. Idk if you want to keep some kind of issue open to track "get this in a position we're ready for it to go into stable"?
There was a problem hiding this comment.
I feel like we're not gonna forget about this one no matter how we do it
There was a problem hiding this comment.
i added 10430 to the <id> field in the feature doc
| if (name() != value) \ | ||
| { \ | ||
| target.name(value); \ | ||
| _NotifyChanges(L"Has" #name, L#name, L#name "OverrideSource"); \ |
There was a problem hiding this comment.
Wait. These shouldn't be necessary right? "OverrideSource" is the deduced value from our parent. So changing any setting shouldn't affect the value we get from our parent.
There was a problem hiding this comment.
XxxOverrideSource is our parent. It changes (goes from returning null to non-null) if the value of HasXxx changes. Technically it doesn't matter if we notify on it because the listener is also listening for HasXxx changing.
Co-authored-by: Carlos Zamora <carlos.zamora@microsoft.com>
ghost
requested review from
DHowett
added
the
AutoMerge
|
Hello @DHowett! Because this pull request has the 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 (
|
|
🎉 Handy links: |
ghost
mentioned this pull request
Right now, we store GUIDs in panes and most of the functions for interacting with profiles on the settings model take GUIDs and look up profiles. This pull request changes how we store and look up profiles to prefer profile objects. Panes store strong references to their originating profiles, which simplifies settings lookup for CloseOnExit and the bell settings. In fact, deleting a pane's profile no longer causes it to forget which CloseOnExit setting applies to it. Duplicating a pane that is hosting a deleted profile (#5047) now duplicates the profile, even though it is otherwise unreachable. This makes the world more consistent and allows us to _eventually_ support panes hosting profiles that do not have GUIDs that can be looked up in the profile list. This is a gateway to #6776 and #10669, and consolidating the profile lookup logic will help with #10952. PR #10588 introduced TerminalSettings::CreateWithProfile and made ...CreateWithProfileByID a thin wrapper over top it, which looked up the profile by GUID before proceeding. It has also been removed, as its last caller is gone. Closes #5047
|
🎉 Handy links: |
This pull request brings back the "Base Layer" page, now renamed to
"Defaults", and the "Reset to inherited value" buttons. The scope of
inheritance for which buttons will display has been widened.
The button will be visible in the following cases:
The user has set a setting for the current profile, and it overrides...
Compared to the original implementation of reset arrows, cases (1), (3)
and (4) are new. Rationale:
(1) The user can see a setting on the Defaults page, and they need a way
to reset back to it.
(3) Dynamic profiles are not meaningfully different from fragments, and
users may need a way to reset back to the default value generated
for WSL or PowerShell.
(4) The user can see a setting on the Defaults page, BUT they are
not the one who created it. They still need a way to get back to
it.
To support this, I've introduced another origin tag, "User", and renamed
"Custom" to "None". Due to the way origin/override detection works¹, we
cannot otherwise disambiguate between settings that came from the user
and settings that came from the compiled-in defaults.
Changes were required in TerminalSettings such that we could construct a
settings object with a profile that does not have a GUID. In making this
change, I fixed a bit of silliness where we took a profile, extracted
its guid, and used that guid to look up the same profile object. Oops.
I also fixed the PropertyChanged notifier to include the
XxxOverrideSource property.
The presence of the page and the reset arrows is restricted to
Preview- or Dev-branded builds. Stable builds will retain their current
behavior.
¹
XxxOverrideSourcereturns the profile above the current profilethat holds a value for setting
Xxx. When the value is thecompiled-in value,
XxxOverrideSourcewill benull. Since it'ssupposed to be the profile above the current profile, it will also be
nullif the profile contains a setting at this layer.In short,
nullmeans "user specified" or "compiled in". Oops.Fixes #10430
Validation