maxmustermann/terminal
1
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

Reintroduce the Defaults page and the Reset buttons (#10588)

Browse source 
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...

1. ... something in profiles.defaults.
2. ... something in a Fragment Extension profile.
3. ... something from a Dynamic Profile Generator.
4. ... something from the compiled-in defaults.

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.

¹ `XxxOverrideSource` returns the profile *above* the current profile
  that holds a value for setting `Xxx`. When the value is the
  compiled-in value, `XxxOverrideSource` will be `null`. Since it's
  supposed to be the profile above the current profile, it will also be
  `null` if the profile contains a setting at this layer.
  In short, `null` means "user specified" *or* "compiled in". Oops.

Fixes #10430

Validation
----------

* [x] Tested Release build to make sure it's mostly arrow-free (apart from fragments)
This commit is contained in:
Dustin L. Howett 2021-07-09 17:03:41 -05:00 committed by GitHub
parent be2b77653f
commit d57fb84557
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
15 changed files with 117 additions and 47 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

17
src/cascadia/TerminalSettingsEditor/MainPage.cpp
View file

@ -34,6 +34,7 @@ static const std::wstring_view launchTag{ L"Launch_Nav" };
static const std::wstring_view interactionTag{ L"Interaction_Nav" };
static const std::wstring_view renderingTag{ L"Rendering_Nav" };
static const std::wstring_view actionsTag{ L"Actions_Nav" };
static const std::wstring_view globalProfileTag{ L"GlobalProfile_Nav" };
static const std::wstring_view addProfileTag{ L"AddProfile" };
static const std::wstring_view colorSchemesTag{ L"ColorSchemes_Nav" };
static const std::wstring_view globalAppearanceTag{ L"GlobalAppearance_Nav" };
@ -310,6 +311,17 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
contentFrame().Navigate(xaml_typename<Editor::ReadOnlyActions>(), actionsState);
}
}
else if (clickedItemTag == globalProfileTag)
{
auto profileVM{ _viewModelForProfile(_settingsClone.ProfileDefaults(), _settingsClone) };
profileVM.IsBaseLayer(true);
_lastProfilesNavState = winrt::make<ProfilePageNavigationState>(profileVM,
_settingsClone.GlobalSettings().ColorSchemes(),
_lastProfilesNavState,
*this);
contentFrame().Navigate(xaml_typename<Editor::Profiles>(), _lastProfilesNavState);
}
else if (clickedItemTag == colorSchemesTag)
{
contentFrame().Navigate(xaml_typename<Editor::ColorSchemes>(), _colorSchemesNavState);
@ -471,4 +483,9 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
SettingsNav().SelectedItem(newSelectedItem);
_Navigate(newSelectedItem.try_as<MUX::Controls::NavigationViewItem>().Tag().try_as<Editor::ProfileViewModel>());
}
bool MainPage::ShowBaseLayerMenuItem() const noexcept
{
return Feature_ShowProfileDefaultsInSettings::IsEnabled();
}
}

2
src/cascadia/TerminalSettingsEditor/MainPage.h
View file

@ -26,6 +26,8 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
bool TryPropagateHostingWindow(IInspectable object) noexcept;
uint64_t GetHostingWindow() const noexcept;
bool ShowBaseLayerMenuItem() const noexcept;
TYPED_EVENT(OpenJson, Windows::Foundation::IInspectable, Model::SettingsTarget);
private:

2
src/cascadia/TerminalSettingsEditor/MainPage.idl
View file

@ -23,5 +23,7 @@ namespace Microsoft.Terminal.Settings.Editor
// Due to the aforementioned bug, we can't use IInitializeWithWindow _here_ either.
// Let's just smuggle the HWND in as a UInt64 :|
void SetHostingWindow(UInt64 window);
Boolean ShowBaseLayerMenuItem { get; };
}
}

9
src/cascadia/TerminalSettingsEditor/MainPage.xaml
View file

@ -88,6 +88,15 @@
<muxc:NavigationViewItemHeader x:Uid="Nav_Profiles" />
<muxc:NavigationViewItem x:Name="BaseLayerMenuItem"
x:Uid="Nav_ProfileDefaults"
Tag="GlobalProfile_Nav"
Visibility="{x:Bind ShowBaseLayerMenuItem}">
<muxc:NavigationViewItem.Icon>
<FontIcon Glyph="&#xE81E;" />
</muxc:NavigationViewItem.Icon>
</muxc:NavigationViewItem>
</muxc:NavigationView.MenuItems>
<muxc:NavigationView.PaneFooter>

2
src/cascadia/TerminalSettingsEditor/Profiles.cpp
View file

@ -67,7 +67,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
Model::TerminalSettings ProfileViewModel::TermSettings() const
{
return Model::TerminalSettings::CreateWithProfileByID(_appSettings, _profile.Guid(), nullptr).DefaultSettings();
return Model::TerminalSettings::CreateWithProfile(_appSettings, _profile, nullptr).DefaultSettings();
}
// Method Description:

10
src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw
View file

@ -452,8 +452,8 @@
<comment>Header for a menu item. This opens the JSON file that is used to log the app's settings.</comment>
</data>
<data name="Nav_ProfileDefaults.Content" xml:space="preserve">
<value>Base layer</value>
<comment>Header for the "base layer" menu item. This navigates to a page that lets you see and modify settings that affect profiles. The base layer is the lowest layer of profile settings that all other profile settings are based on. If a profile doesn't define a setting, base layer is responsible for figuring out what that setting is supposed to be.</comment>
<value>Defaults</value>
<comment>Header for the "defaults" menu item. This navigates to a page that lets you see and modify settings that affect profiles. This is the lowest layer of profile settings that all other profile settings are based on. If a profile doesn't define a setting, this page is responsible for figuring out what that setting is supposed to be.</comment>
</data>
<data name="Nav_Rendering.Content" xml:space="preserve">
<value>Rendering</value>
@ -1012,7 +1012,7 @@
</data>
<data name="Profile_BaseLayerDisclaimer.Text" xml:space="preserve">
<value>Settings defined here will apply to all profiles unless they are overridden by a profile's settings.</value>
<comment>A disclaimer presented at the top of a page. See "Nav_ProfileDefaults.Content" for a description on what the base layer does in the app.</comment>
<comment>A disclaimer presented at the top of a page. See "Nav_ProfileDefaults.Content" for a description on what the defaults layer does in the app.</comment>
</data>
<data name="Profile_DeleteConfirmationButton.Content" xml:space="preserve">
<value>Yes, delete profile</value>
@ -1087,8 +1087,8 @@
<comment>Header for a control to toggle animations on panes. "Enabled" value enables the animations.</comment>
</data>
<data name="SettingContainer_OverrideMessageBaseLayer" xml:space="preserve">
<value>Reset to base layer value.</value>
<comment>"base layer" should match Nav_ProfileDefaults.Content's text. This is a text label on a button.</comment>
<value>Reset to inherited value.</value>
<comment>This button will remove a user's customization from a given setting, restoring it to the value that the profile inherited. This is a text label on a button.</comment>
</data>
<data name="ColorScheme_TerminalColorsHeader.Text" xml:space="preserve">
<value>Terminal colors</value>

66
src/cascadia/TerminalSettingsEditor/SettingContainer.cpp
View file

@ -57,9 +57,9 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
_SettingOverrideSourceProperty =
DependencyProperty::Register(
L"SettingOverrideSource",
xaml_typename<bool>(),
xaml_typename<IInspectable>(),
xaml_typename<Editor::SettingContainer>(),
PropertyMetadata{ nullptr });
PropertyMetadata{ nullptr, PropertyChangedCallback{ &SettingContainer::_OnHasSettingValueChanged } });
}
}
@ -152,17 +152,9 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
// We want to be smart about showing the override system.
// Don't just show it if the user explicitly set the setting.
// If the tooltip is empty, we'll hide the entire override system.
hstring tooltip{};
const auto& settingSrc{ SettingOverrideSource() };
if (const auto& profile{ settingSrc.try_as<Model::Profile>() })
{
tooltip = _GenerateOverrideMessage(profile);
}
else if (const auto& appearanceConfig{ settingSrc.try_as<Model::AppearanceConfig>() })
{
tooltip = _GenerateOverrideMessage(appearanceConfig.SourceProfile());
}
const auto tooltip{ _GenerateOverrideMessage(settingSrc) };
Controls::ToolTipService::SetToolTip(button, box_value(tooltip));
button.Visibility(tooltip.empty() ? Visibility::Collapsed : Visibility::Visible);
@ -182,35 +174,43 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
// - profile: the profile that defines the setting (aka SettingOverrideSource)
// Return Value:
// - text specifying where the setting was defined. If empty, we don't want to show the system.
hstring SettingContainer::_GenerateOverrideMessage(const Model::Profile& profile)
hstring SettingContainer::_GenerateOverrideMessage(const IInspectable& settingOrigin)
{
const auto originTag{ profile.Origin() };
if (originTag == Model::OriginTag::InBox)
// We only get here if the user had an override in place.
Model::OriginTag originTag{ Model::OriginTag::None };
winrt::hstring source;
if (const auto& profile{ settingOrigin.try_as<Model::Profile>() })
{
// in-box profile
return {};
source = profile.Source();
originTag = profile.Origin();
}
else if (originTag == Model::OriginTag::Generated)
else if (const auto& appearanceConfig{ settingOrigin.try_as<Model::AppearanceConfig>() })
{
// from a dynamic profile generator
return {};
const auto profile = appearanceConfig.SourceProfile();
source = profile.Source();
originTag = profile.Origin();
}
else if (originTag == Model::OriginTag::Fragment)
if constexpr (Feature_ShowProfileDefaultsInSettings::IsEnabled())
{
// EXPERIMENTAL FEATURE
// We will display arrows for all origins, and informative tooltips for Fragments and Generated
if (originTag == Model::OriginTag::Fragment || originTag == Model::OriginTag::Generated)
{
// from a fragment extension or generated profile
return hstring{ fmt::format(std::wstring_view{ RS_(L"SettingContainer_OverrideMessageFragmentExtension") }, source) };
}
return RS_(L"SettingContainer_OverrideMessageBaseLayer");
}
// STABLE FEATURE
// We will only display arrows and informative tooltips for Fragments
if (originTag == Model::OriginTag::Fragment)
{
// from a fragment extension
return hstring{ fmt::format(std::wstring_view{ RS_(L"SettingContainer_OverrideMessageFragmentExtension") }, profile.Source()) };
}
else
{
// base layer
// TODO GH#3818: When we add profile inheritance as a setting,
// we'll need an extra conditional check to see if this
// is the base layer or some other profile
// GH#9539: Base Layer has been removed from the Settings UI.
// In the event that the Base Layer comes back,
// return RS_(L"SettingContainer_OverrideMessageBaseLayer") instead
return {};
return hstring{ fmt::format(std::wstring_view{ RS_(L"SettingContainer_OverrideMessageFragmentExtension") }, source) };
}
return {}; // no tooltip
}
}

2
src/cascadia/TerminalSettingsEditor/SettingContainer.h
View file

@ -38,7 +38,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
private:
static void _InitializeProperties();
static void _OnHasSettingValueChanged(Windows::UI::Xaml::DependencyObject const& d, Windows::UI::Xaml::DependencyPropertyChangedEventArgs const& e);
static hstring _GenerateOverrideMessage(const Model::Profile& profile);
static hstring _GenerateOverrideMessage(const IInspectable& settingOrigin);
void _UpdateOverrideSystem();
};
}

11
src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp
View file

@ -862,6 +862,7 @@ void CascadiaSettings::LayerJson(const Json::Value& json)
void CascadiaSettings::_LayerOrCreateProfile(const Json::Value& profileJson)
{
// Layer the json on top of an existing profile, if we have one:
winrt::com_ptr<implementation::Profile> profile{ nullptr };
auto profileIndex{ _FindMatchingProfileIndex(profileJson) };
if (profileIndex)
{
@ -874,6 +875,7 @@ void CascadiaSettings::_LayerOrCreateProfile(const Json::Value& profileJson)
// When we loaded Profile.Defaults, we created an empty child already.
// So this just populates the empty child
parent->LayerJson(profileJson);
profile.copy_from(parent);
}
else
{
@ -883,6 +885,7 @@ void CascadiaSettings::_LayerOrCreateProfile(const Json::Value& profileJson)
// replace parent in _profiles with child
_allProfiles.SetAt(*profileIndex, *childImpl);
profile = std::move(childImpl);
}
}
else
@ -892,7 +895,7 @@ void CascadiaSettings::_LayerOrCreateProfile(const Json::Value& profileJson)
// `source`. Dynamic profiles _must_ be layered on an existing profile.
if (!Profile::IsDynamicProfileObject(profileJson))
{
auto profile{ winrt::make_self<Profile>() };
profile = winrt::make_self<Profile>();
// GH#2325: If we have a set of default profile settings, set that as my parent.
// We _won't_ have these settings yet for defaults, dynamic profiles.
@ -905,6 +908,12 @@ void CascadiaSettings::_LayerOrCreateProfile(const Json::Value& profileJson)
_allProfiles.Append(*profile);
}
}
if (profile && _userDefaultProfileSettings)
{
// If we've loaded defaults{} we're in the "user settings" phase for sure
profile->Origin(OriginTag::User);
}
}
// Method Description:

2
src/cascadia/TerminalSettingsModel/Profile.h
View file

@ -103,7 +103,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
void _FinalizeInheritance() override;
WINRT_PROPERTY(OriginTag, Origin, OriginTag::Custom);
WINRT_PROPERTY(OriginTag, Origin, OriginTag::None);
INHERITABLE_SETTING(Model::Profile, guid, Guid, _GenerateGuidForProfile(Name(), Source()));
INHERITABLE_SETTING(Model::Profile, hstring, Name, L"Default");

3
src/cascadia/TerminalSettingsModel/Profile.idl
View file

@ -14,7 +14,8 @@ namespace Microsoft.Terminal.Settings.Model
// This tag is used to identify the context in which the Profile was created
enum OriginTag
{
Custom = 0,
None = 0,
User,
InBox,
Generated,
Fragment,

24
src/cascadia/TerminalSettingsModel/TerminalSettings.cpp
View file

@ -62,14 +62,11 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
// Return Value:
// - A TerminalSettingsCreateResult, which contains a pair of TerminalSettings objects,
// one for when the terminal is focused and the other for when the terminal is unfocused
Model::TerminalSettingsCreateResult TerminalSettings::CreateWithProfileByID(const Model::CascadiaSettings& appSettings, winrt::guid profileGuid, const IKeyBindings& keybindings)
Model::TerminalSettingsCreateResult TerminalSettings::CreateWithProfile(const Model::CascadiaSettings& appSettings, const Model::Profile& profile, const IKeyBindings& keybindings)
{
auto settings{ winrt::make_self<TerminalSettings>() };
settings->_KeyBindings = keybindings;
const auto profile = appSettings.FindProfile(profileGuid);
THROW_HR_IF_NULL(E_INVALIDARG, profile);
const auto globals = appSettings.GlobalSettings();
settings->_ApplyProfileSettings(profile);
settings->_ApplyGlobalSettings(globals);
@ -86,6 +83,25 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
return winrt::make<TerminalSettingsCreateResult>(*settings, child);
}
// Method Description:
// - Create a TerminalSettingsCreateResult for the provided profile guid. We'll
// use the guid to look up the profile that should be used to
// create these TerminalSettings. Then, we'll apply settings contained in the
// global and profile settings to the instance.
// Arguments:
// - appSettings: the set of settings being used to construct the new terminal
// - profileGuid: the unique identifier (guid) of the profile
// - keybindings: the keybinding handler
// Return Value:
// - A TerminalSettingsCreateResult, which contains a pair of TerminalSettings objects,
// one for when the terminal is focused and the other for when the terminal is unfocused
Model::TerminalSettingsCreateResult TerminalSettings::CreateWithProfileByID(const Model::CascadiaSettings& appSettings, winrt::guid profileGuid, const IKeyBindings& keybindings)
{
const auto profile = appSettings.FindProfile(profileGuid);
THROW_HR_IF_NULL(E_INVALIDARG, profile);
return CreateWithProfile(appSettings, profile, keybindings);
}
// Method Description:
// - Create a TerminalSettings object for the provided newTerminalArgs. We'll
// use the newTerminalArgs to look up the profile that should be used to

4
src/cascadia/TerminalSettingsModel/TerminalSettings.h
View file

@ -53,6 +53,10 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
{
TerminalSettings() = default;
static Model::TerminalSettingsCreateResult CreateWithProfile(const Model::CascadiaSettings& appSettings,
const Model::Profile& profile,
const Control::IKeyBindings& keybindings);
static Model::TerminalSettingsCreateResult CreateWithProfileByID(const Model::CascadiaSettings& appSettings,
guid profileGuid,
const Control::IKeyBindings& keybindings);

1
src/cascadia/TerminalSettingsModel/TerminalSettings.idl
View file

@ -26,6 +26,7 @@ namespace Microsoft.Terminal.Settings.Model
{
TerminalSettings();
static TerminalSettingsCreateResult CreateWithProfile(CascadiaSettings appSettings, Profile profile, Microsoft.Terminal.Control.IKeyBindings keybindings);
static TerminalSettingsCreateResult CreateWithProfileByID(CascadiaSettings appSettings, Guid profileGuid, Microsoft.Terminal.Control.IKeyBindings keybindings);
static TerminalSettingsCreateResult CreateWithNewTerminalArgs(CascadiaSettings appSettings, NewTerminalArgs newTerminalArgs, Microsoft.Terminal.Control.IKeyBindings keybindings);
static TerminalSettingsCreateResult CreateWithParent(TerminalSettingsCreateResult parent);

9
src/features.xml
View file

@ -56,4 +56,13 @@
<brandingToken>WindowsInbox</brandingToken>
</alwaysEnabledBrandingTokens>
</feature>
<feature>
<name>Feature_ShowProfileDefaultsInSettings</name>
<description>Whether to show the "defaults" page in the Terminal settings UI</description>
<id>10430</id>
<stage>AlwaysEnabled</stage>
<!-- This feature will not ship to Stable until it is complete. -->
<alwaysDisabledReleaseTokens />
</feature>
</featureStaging>