Allow generated profiles to be deleted (#11007)

Re-enables the delete button for generated profiles in the settings UI.
Additionally fixes "Startup Profiles" to only list active profiles.

Profiles are considered deleted if they're absent from settings.json, but their
GUID has been encountered before. Or in other words, from a user's perspective:
Generated profiles are added to the settings.json automatically only once.
Thus if the user chooses to delete the profile (e.g. using the delete button)
they aren't re-added automatically and thus appear to have been deleted.

Meanwhile those generated profiles are actually only marked as "hidden"
as well as "deleted", but still exist in internal profile lists.
The "hidden" attribute hides them from all existing menus. The "deleted" one
hides them from the settings UI and prevents them from being written to disk.

It would've been preferrable of course to just not generate and
add deleted profile to internal profile lists in the first place.
But this would've required far more wide-reaching changes.
The settings UI for instance requires a list of _all_ profiles in order to
allow a user to re-create previously deleted profiles. Such an approach was
attempted but discarded because of it's current complexity overhead.

## References

* Part of #9997
* A sequel to 5d36e5d

## PR Checklist

* [x] Closes #10960
* [x] I work here
* [x] Tests added/passed

## Validation Steps Performed

* "Startup Profiles" doesn't list deleted profiles ✔️
* Manually removing an item from settings.json removes the profile ✔️
* Removing cmd.exe and saving doesn't create empty objects (#10960) ✔️
* "Add a new profile" lists deleted profiles ✔️
* "Duplicate" recreates previously deleted profiles ✔️
* Profiles are always created with GUIDs ✔️
This commit is contained in:
Leonard Hecker 2021-08-24 00:00:08 +02:00 committed by GitHub
parent 10992b77a0
commit 608a49e817
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
14 changed files with 167 additions and 160 deletions

View file

@ -47,4 +47,25 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
const auto profile{ winrt::unbox_value<Model::Profile>(value) };
_State.Settings().GlobalSettings().DefaultProfile(profile.Guid());
}
winrt::Windows::Foundation::Collections::IObservableVector<IInspectable> Launch::DefaultProfiles() const
{
const auto allProfiles = _State.Settings().AllProfiles();
std::vector<IInspectable> profiles;
profiles.reserve(allProfiles.Size());
// Remove profiles from the selection which have been explicitly deleted.
// We do want to show hidden profiles though, as they are just hidden
// from menus, but still work as the startup profile for instance.
for (const auto& profile : allProfiles)
{
if (!profile.Deleted())
{
profiles.emplace_back(profile);
}
}
return winrt::single_threaded_observable_vector(std::move(profiles));
}
}

View file

@ -27,6 +27,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
IInspectable CurrentDefaultProfile();
void CurrentDefaultProfile(const IInspectable& value);
winrt::Windows::Foundation::Collections::IObservableVector<IInspectable> DefaultProfiles() const;
WINRT_PROPERTY(Editor::LaunchPageNavigationState, State, nullptr);

View file

@ -16,6 +16,9 @@ namespace Microsoft.Terminal.Settings.Editor
LaunchPageNavigationState State { get; };
IInspectable CurrentDefaultProfile;
// I wish this was a IObservableVector<Microsoft.Terminal.Settings.Model.Profile>, but:
// https://github.com/microsoft/microsoft-ui-xaml/issues/5395
IObservableVector<IInspectable> DefaultProfiles { get; };
IInspectable CurrentLaunchMode;
IObservableVector<Microsoft.Terminal.Settings.Editor.EnumEntry> LaunchModeList { get; };

View file

@ -40,7 +40,7 @@
<local:SettingContainer x:Uid="Globals_DefaultProfile"
Margin="0">
<ComboBox x:Name="DefaultProfile"
ItemsSource="{x:Bind State.Settings.AllProfiles, Mode=OneWay}"
ItemsSource="{x:Bind DefaultProfiles}"
SelectedItem="{x:Bind CurrentDefaultProfile, Mode=TwoWay}"
Style="{StaticResource ComboBoxSettingStyle}">
<ComboBox.ItemTemplate>

View file

@ -387,14 +387,19 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
void MainPage::_InitializeProfilesList()
{
const auto menuItems = SettingsNav().MenuItems();
// Manually create a NavigationViewItem for each profile
// and keep a reference to them in a map so that we
// can easily modify the correct one when the associated
// profile changes.
for (const auto& profile : _settingsClone.AllProfiles())
{
auto navItem = _CreateProfileNavViewItem(_viewModelForProfile(profile, _settingsClone));
SettingsNav().MenuItems().Append(navItem);
if (!profile.Deleted())
{
auto navItem = _CreateProfileNavViewItem(_viewModelForProfile(profile, _settingsClone));
menuItems.Append(navItem);
}
}
// Top off (the end of the nav view) with the Add Profile item
@ -407,7 +412,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
icon.Glyph(L"\xE710");
addProfileItem.Icon(icon);
SettingsNav().MenuItems().Append(addProfileItem);
menuItems.Append(addProfileItem);
}
void MainPage::_CreateAndNavigateToNewProfile(const uint32_t index, const Model::Profile& profile)

View file

@ -18,11 +18,6 @@ using namespace winrt::Windows::Foundation;
using namespace winrt::Windows::Foundation::Collections;
using namespace winrt::Microsoft::Terminal::Settings::Model;
static const std::array<winrt::guid, 2> InBoxProfileGuids{
winrt::guid{ 0x61c54bbd, 0xc2c6, 0x5271, { 0x96, 0xe7, 0x00, 0x9a, 0x87, 0xff, 0x44, 0xbf } }, // Windows Powershell
winrt::guid{ 0x0caa0dad, 0x35be, 0x5f56, { 0xa8, 0xff, 0xaf, 0xce, 0xee, 0xaa, 0x61, 0x01 } } // Command Prompt
};
namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
{
Windows::Foundation::Collections::IObservableVector<Editor::Font> ProfileViewModel::_MonospaceFontList{ nullptr };
@ -221,25 +216,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
bool ProfileViewModel::CanDeleteProfile() const
{
const auto guid{ Guid() };
if (IsBaseLayer())
{
return false;
}
else if (std::find(std::begin(InBoxProfileGuids), std::end(InBoxProfileGuids), guid) != std::end(InBoxProfileGuids))
{
// in-box profile
return false;
}
else if (!Source().empty())
{
// dynamic profile
return false;
}
else
{
return true;
}
return !IsBaseLayer();
}
Editor::AppearanceViewModel ProfileViewModel::DefaultAppearance()
@ -383,21 +360,6 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
ProfileViewModel::UpdateFontList();
}
// Set the text disclaimer for the text box
hstring disclaimer{};
const auto guid{ _State.Profile().Guid() };
if (std::find(std::begin(InBoxProfileGuids), std::end(InBoxProfileGuids), guid) != std::end(InBoxProfileGuids))
{
// load disclaimer for in-box profiles
disclaimer = RS_(L"Profile_DeleteButtonDisclaimerInBox");
}
else if (!_State.Profile().Source().empty())
{
// load disclaimer for dynamic profiles
disclaimer = RS_(L"Profile_DeleteButtonDisclaimerDynamic");
}
DeleteButtonDisclaimer().Text(disclaimer);
// Check the use parent directory box if the starting directory is empty
if (_State.Profile().StartingDirectory().empty())
{

View file

@ -144,81 +144,77 @@
</local:SettingContainer>
<!-- Delete Button -->
<StackPanel Margin="{StaticResource StandardControlMargin}">
<Button x:Name="DeleteButton"
IsEnabled="{x:Bind State.Profile.CanDeleteProfile}"
Style="{StaticResource DeleteButtonStyle}">
<Button.Resources>
<ResourceDictionary>
<ResourceDictionary.ThemeDictionaries>
<ResourceDictionary x:Key="Light">
<SolidColorBrush x:Key="ButtonBackground"
Color="Firebrick" />
<SolidColorBrush x:Key="ButtonBackgroundPointerOver"
Color="#C23232" />
<SolidColorBrush x:Key="ButtonBackgroundPressed"
Color="#A21212" />
<SolidColorBrush x:Key="ButtonForeground"
Color="White" />
<SolidColorBrush x:Key="ButtonForegroundPointerOver"
Color="White" />
<SolidColorBrush x:Key="ButtonForegroundPressed"
Color="White" />
</ResourceDictionary>
<ResourceDictionary x:Key="Dark">
<SolidColorBrush x:Key="ButtonBackground"
Color="Firebrick" />
<SolidColorBrush x:Key="ButtonBackgroundPointerOver"
Color="#C23232" />
<SolidColorBrush x:Key="ButtonBackgroundPressed"
Color="#A21212" />
<SolidColorBrush x:Key="ButtonForeground"
Color="White" />
<SolidColorBrush x:Key="ButtonForegroundPointerOver"
Color="White" />
<SolidColorBrush x:Key="ButtonForegroundPressed"
Color="White" />
</ResourceDictionary>
<ResourceDictionary x:Key="HighContrast">
<SolidColorBrush x:Key="ButtonBackground"
Color="{ThemeResource SystemColorButtonFaceColor}" />
<SolidColorBrush x:Key="ButtonBackgroundPointerOver"
Color="{ThemeResource SystemColorHighlightColor}" />
<SolidColorBrush x:Key="ButtonBackgroundPressed"
Color="{ThemeResource SystemColorHighlightColor}" />
<SolidColorBrush x:Key="ButtonForeground"
Color="{ThemeResource SystemColorButtonTextColor}" />
<SolidColorBrush x:Key="ButtonForegroundPointerOver"
Color="{ThemeResource SystemColorHighlightTextColor}" />
<SolidColorBrush x:Key="ButtonForegroundPressed"
Color="{ThemeResource SystemColorHighlightTextColor}" />
</ResourceDictionary>
</ResourceDictionary.ThemeDictionaries>
</ResourceDictionary>
</Button.Resources>
<Button.Content>
<StackPanel Orientation="Horizontal">
<FontIcon FontSize="{StaticResource StandardIconSize}"
Glyph="&#xE74D;" />
<TextBlock x:Uid="Profile_DeleteButton"
Margin="10,0,0,0" />
<Button x:Name="DeleteButton"
Margin="{StaticResource StandardControlMargin}"
Style="{StaticResource DeleteButtonStyle}"
Visibility="{x:Bind State.Profile.CanDeleteProfile}">
<Button.Resources>
<ResourceDictionary>
<ResourceDictionary.ThemeDictionaries>
<ResourceDictionary x:Key="Light">
<SolidColorBrush x:Key="ButtonBackground"
Color="Firebrick" />
<SolidColorBrush x:Key="ButtonBackgroundPointerOver"
Color="#C23232" />
<SolidColorBrush x:Key="ButtonBackgroundPressed"
Color="#A21212" />
<SolidColorBrush x:Key="ButtonForeground"
Color="White" />
<SolidColorBrush x:Key="ButtonForegroundPointerOver"
Color="White" />
<SolidColorBrush x:Key="ButtonForegroundPressed"
Color="White" />
</ResourceDictionary>
<ResourceDictionary x:Key="Dark">
<SolidColorBrush x:Key="ButtonBackground"
Color="Firebrick" />
<SolidColorBrush x:Key="ButtonBackgroundPointerOver"
Color="#C23232" />
<SolidColorBrush x:Key="ButtonBackgroundPressed"
Color="#A21212" />
<SolidColorBrush x:Key="ButtonForeground"
Color="White" />
<SolidColorBrush x:Key="ButtonForegroundPointerOver"
Color="White" />
<SolidColorBrush x:Key="ButtonForegroundPressed"
Color="White" />
</ResourceDictionary>
<ResourceDictionary x:Key="HighContrast">
<SolidColorBrush x:Key="ButtonBackground"
Color="{ThemeResource SystemColorButtonFaceColor}" />
<SolidColorBrush x:Key="ButtonBackgroundPointerOver"
Color="{ThemeResource SystemColorHighlightColor}" />
<SolidColorBrush x:Key="ButtonBackgroundPressed"
Color="{ThemeResource SystemColorHighlightColor}" />
<SolidColorBrush x:Key="ButtonForeground"
Color="{ThemeResource SystemColorButtonTextColor}" />
<SolidColorBrush x:Key="ButtonForegroundPointerOver"
Color="{ThemeResource SystemColorHighlightTextColor}" />
<SolidColorBrush x:Key="ButtonForegroundPressed"
Color="{ThemeResource SystemColorHighlightTextColor}" />
</ResourceDictionary>
</ResourceDictionary.ThemeDictionaries>
</ResourceDictionary>
</Button.Resources>
<Button.Content>
<StackPanel Orientation="Horizontal">
<FontIcon FontSize="{StaticResource StandardIconSize}"
Glyph="&#xE74D;" />
<TextBlock x:Uid="Profile_DeleteButton"
Margin="10,0,0,0" />
</StackPanel>
</Button.Content>
<Button.Flyout>
<Flyout>
<StackPanel>
<TextBlock x:Uid="Profile_DeleteConfirmationMessage"
Style="{StaticResource CustomFlyoutTextStyle}" />
<Button x:Uid="Profile_DeleteConfirmationButton"
Click="DeleteConfirmation_Click" />
</StackPanel>
</Button.Content>
<Button.Flyout>
<Flyout>
<StackPanel>
<TextBlock x:Uid="Profile_DeleteConfirmationMessage"
Style="{StaticResource CustomFlyoutTextStyle}" />
<Button x:Uid="Profile_DeleteConfirmationButton"
Click="DeleteConfirmation_Click" />
</StackPanel>
</Flyout>
</Button.Flyout>
</Button>
<TextBlock x:Name="DeleteButtonDisclaimer"
VerticalAlignment="Center"
Style="{StaticResource DisclaimerStyle}" />
</StackPanel>
</Flyout>
</Button.Flyout>
</Button>
</StackPanel>
</ScrollViewer>
</PivotItem>

View file

@ -994,14 +994,6 @@
<value>Rename</value>
<comment>Text label for a button that can be used to begin the renaming process.</comment>
</data>
<data name="Profile_DeleteButtonDisclaimerInBox" xml:space="preserve">
<value>This profile cannot be deleted because it is included by default.</value>
<comment>Disclaimer presented next to the delete button when it is disabled. Some profiles are included in the app by default and cannot be deleted.</comment>
</data>
<data name="Profile_DeleteButtonDisclaimerDynamic" xml:space="preserve">
<value>This profile cannot be deleted because it is automatically generated.</value>
<comment>Disclaimer presented next to the delete button when it is disabled. Some profiles are automatically generated by the app and cannot be deleted.</comment>
</data>
<data name="ColorScheme_DeleteButtonDisclaimerInBox" xml:space="preserve">
<value>This color scheme cannot be deleted or renamed because it is included by default.</value>
<comment>Disclaimer presented next to the delete button when it is disabled.</comment>

View file

@ -225,20 +225,20 @@ winrt::Microsoft::Terminal::Settings::Model::Profile CascadiaSettings::CreateNew
return nullptr;
}
winrt::hstring newName{};
for (uint32_t candidateIndex = 0; candidateIndex < _allProfiles.Size() + 1; candidateIndex++)
std::wstring newName;
for (uint32_t candidateIndex = 0, count = _allProfiles.Size() + 1; candidateIndex < count; candidateIndex++)
{
// There is a theoretical unsigned integer wraparound, which is OK
newName = fmt::format(L"Profile {}", _allProfiles.Size() + 1 + candidateIndex);
newName = fmt::format(L"Profile {}", count + candidateIndex);
if (std::none_of(begin(_allProfiles), end(_allProfiles), [&](auto&& profile) { return profile.Name() == newName; }))
{
break;
}
}
auto newProfile{ _userDefaultProfileSettings->CreateChild() };
newProfile->Name(newName);
const auto newProfile = _CreateNewProfile(newName);
_allProfiles.Append(*newProfile);
_activeProfiles.Append(*newProfile);
return *newProfile;
}
@ -258,26 +258,10 @@ winrt::Microsoft::Terminal::Settings::Model::Profile CascadiaSettings::Duplicate
{
THROW_HR_IF_NULL(E_INVALIDARG, source);
winrt::com_ptr<Profile> duplicated;
if (_userDefaultProfileSettings)
{
duplicated = _userDefaultProfileSettings->CreateChild();
}
else
{
duplicated = winrt::make_self<Profile>();
}
_allProfiles.Append(*duplicated);
if (!source.Hidden())
{
_activeProfiles.Append(*duplicated);
}
winrt::hstring newName{ fmt::format(L"{} ({})", source.Name(), RS_(L"CopySuffix")) };
auto newName = fmt::format(L"{} ({})", source.Name(), RS_(L"CopySuffix"));
// Check if this name already exists and if so, append a number
for (uint32_t candidateIndex = 0; candidateIndex < _allProfiles.Size() + 1; ++candidateIndex)
for (uint32_t candidateIndex = 0, count = _allProfiles.Size() + 1; candidateIndex < count; ++candidateIndex)
{
if (std::none_of(begin(_allProfiles), end(_allProfiles), [&](auto&& profile) { return profile.Name() == newName; }))
{
@ -286,13 +270,14 @@ winrt::Microsoft::Terminal::Settings::Model::Profile CascadiaSettings::Duplicate
// There is a theoretical unsigned integer wraparound, which is OK
newName = fmt::format(L"{} ({} {})", source.Name(), RS_(L"CopySuffix"), candidateIndex + 2);
}
duplicated->Name(winrt::hstring(newName));
const auto isProfilesDefaultsOrigin = [](const auto& profile) -> bool {
const auto duplicated = _CreateNewProfile(newName);
static constexpr auto isProfilesDefaultsOrigin = [](const auto& profile) -> bool {
return profile && profile.Origin() != OriginTag::ProfilesDefaults;
};
const auto isProfilesDefaultsOriginSub = [=](const auto& sub) -> bool {
static constexpr auto isProfilesDefaultsOriginSub = [](const auto& sub) -> bool {
return sub && isProfilesDefaultsOrigin(sub.SourceProfile());
};
@ -308,7 +293,9 @@ winrt::Microsoft::Terminal::Settings::Model::Profile CascadiaSettings::Duplicate
target.settingName(source.settingName()); \
}
DUPLICATE_SETTING_MACRO(Hidden);
// If the source is hidden and the Settings UI creates a
// copy of it we don't want the copy to be hidden as well.
// --> Don't do DUPLICATE_SETTING_MACRO(Hidden);
DUPLICATE_SETTING_MACRO(Icon);
DUPLICATE_SETTING_MACRO(CloseOnExit);
DUPLICATE_SETTING_MACRO(TabTitle);
@ -388,6 +375,8 @@ winrt::Microsoft::Terminal::Settings::Model::Profile CascadiaSettings::Duplicate
duplicated->ConnectionType(source.ConnectionType());
}
_allProfiles.Append(*duplicated);
_activeProfiles.Append(*duplicated);
return *duplicated;
}
@ -421,6 +410,32 @@ winrt::hstring CascadiaSettings::GetSerializationErrorMessage()
return _deserializationErrorMessage;
}
// As used by CreateNewProfile and DuplicateProfile this function
// creates a new Profile instance with a random UUID and a given name.
winrt::com_ptr<Profile> CascadiaSettings::_CreateNewProfile(const std::wstring_view& name) const
{
winrt::com_ptr<Profile> profile;
if (_userDefaultProfileSettings)
{
profile = _userDefaultProfileSettings->CreateChild();
}
else
{
profile = winrt::make_self<Profile>();
}
// Technically there's Utils::CreateV5Uuid which we could use, but I wanted
// truly globally unique UUIDs for profiles created through the settings UI.
GUID guid{};
LOG_IF_FAILED(CoCreateGuid(&guid));
profile->Guid(guid);
profile->Name(winrt::hstring{ name });
return profile;
}
// Method Description:
// - Attempts to validate this settings structure. If there are critical errors
// found, they'll be thrown as a SettingsLoadError. Non-critical errors, such

View file

@ -124,6 +124,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
Json::Value _defaultSettings;
winrt::com_ptr<Profile> _userDefaultProfileSettings{ nullptr };
winrt::com_ptr<Profile> _CreateNewProfile(const std::wstring_view& name) const;
void _LayerOrCreateProfile(const Json::Value& profileJson);
winrt::com_ptr<implementation::Profile> _FindMatchingProfile(const Json::Value& profileJson);
std::optional<uint32_t> _FindMatchingProfileIndex(const Json::Value& profileJson);

View file

@ -214,15 +214,18 @@ winrt::Microsoft::Terminal::Settings::Model::CascadiaSettings CascadiaSettings::
auto generatedProfiles = state->GeneratedProfiles();
bool generatedProfilesChanged = false;
for (auto profile : resultPtr->_allProfiles)
for (const auto& profile : resultPtr->_allProfiles)
{
if (generatedProfiles.emplace(profile.Guid()).second)
const auto profileImpl = winrt::get_self<implementation::Profile>(profile);
if (generatedProfiles.emplace(profileImpl->Guid()).second)
{
generatedProfilesChanged = true;
}
else if (profile.Origin() != OriginTag::User)
else if (profileImpl->Origin() != OriginTag::User)
{
profile.Hidden(true);
profileImpl->Deleted(true);
profileImpl->Hidden(true);
}
}
@ -351,7 +354,7 @@ winrt::Microsoft::Terminal::Settings::Model::CascadiaSettings CascadiaSettings::
// tag these profiles as in-box
for (const auto& profile : resultPtr->AllProfiles())
{
const auto& profileImpl{ winrt::get_self<implementation::Profile>(profile) };
const auto profileImpl{ winrt::get_self<implementation::Profile>(profile) };
profileImpl->Origin(OriginTag::InBox);
}
@ -781,7 +784,7 @@ bool CascadiaSettings::_AppendDynamicProfilesToUserSettings()
// * in the user settings or the default settings
// Because we don't want to add profiles which are already
// in the settings.json (explicitly or implicitly).
if (profile.Hidden() || isInJsonObj(profile, _userSettings) || isInJsonObj(profile, _defaultSettings))
if (profile.Deleted() || isInJsonObj(profile, _userSettings) || isInJsonObj(profile, _defaultSettings))
{
continue;
}
@ -1250,8 +1253,11 @@ Json::Value CascadiaSettings::ToJson() const
Json::Value profilesList{ Json::ValueType::arrayValue };
for (const auto& entry : _allProfiles)
{
const auto prof{ winrt::get_self<implementation::Profile>(entry) };
profilesList.append(prof->ToJson());
if (!entry.Deleted())
{
const auto prof{ winrt::get_self<implementation::Profile>(entry) };
profilesList.append(prof->ToJson());
}
}
profiles[JsonKey(ProfilesListKey)] = profilesList;
json[JsonKey(ProfilesKey)] = profiles;

View file

@ -84,6 +84,7 @@ winrt::com_ptr<Profile> Profile::CopySettings(winrt::com_ptr<Profile> source)
{
auto profile{ winrt::make_self<Profile>() };
profile->_Deleted = source->_Deleted;
profile->_Guid = source->_Guid;
profile->_Name = source->_Name;
profile->_Source = source->_Source;

View file

@ -106,6 +106,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
void _FinalizeInheritance() override;
WINRT_PROPERTY(bool, Deleted, false);
WINRT_PROPERTY(OriginTag, Origin, OriginTag::None);
INHERITABLE_SETTING(Model::Profile, guid, Guid, _GenerateGuidForProfile(Name(), Source()));

View file

@ -46,6 +46,8 @@ namespace Microsoft.Terminal.Settings.Model
void CreateUnfocusedAppearance();
void DeleteUnfocusedAppearance();
// True if the user explicitly removed this Profile from settings.json.
Boolean Deleted { get; };
OriginTag Origin { get; };
INHERITABLE_PROFILE_SETTING(String, Name);