From 608a49e8178466ecdf6f6e83155b34de2628f25c Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Tue, 24 Aug 2021 00:00:08 +0200 Subject: [PATCH] Allow generated profiles to be deleted (#11007) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 ✔️ --- .../TerminalSettingsEditor/Launch.cpp | 21 +++ src/cascadia/TerminalSettingsEditor/Launch.h | 1 + .../TerminalSettingsEditor/Launch.idl | 3 + .../TerminalSettingsEditor/Launch.xaml | 2 +- .../TerminalSettingsEditor/MainPage.cpp | 11 +- .../TerminalSettingsEditor/Profiles.cpp | 40 +---- .../TerminalSettingsEditor/Profiles.xaml | 144 +++++++++--------- .../Resources/en-US/Resources.resw | 8 - .../CascadiaSettings.cpp | 69 +++++---- .../TerminalSettingsModel/CascadiaSettings.h | 2 + .../CascadiaSettingsSerialization.cpp | 22 ++- .../TerminalSettingsModel/Profile.cpp | 1 + src/cascadia/TerminalSettingsModel/Profile.h | 1 + .../TerminalSettingsModel/Profile.idl | 2 + 14 files changed, 167 insertions(+), 160 deletions(-) diff --git a/src/cascadia/TerminalSettingsEditor/Launch.cpp b/src/cascadia/TerminalSettingsEditor/Launch.cpp index 99b15cf73..b881b4e00 100644 --- a/src/cascadia/TerminalSettingsEditor/Launch.cpp +++ b/src/cascadia/TerminalSettingsEditor/Launch.cpp @@ -47,4 +47,25 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation const auto profile{ winrt::unbox_value(value) }; _State.Settings().GlobalSettings().DefaultProfile(profile.Guid()); } + + winrt::Windows::Foundation::Collections::IObservableVector Launch::DefaultProfiles() const + { + const auto allProfiles = _State.Settings().AllProfiles(); + + std::vector 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)); + } } diff --git a/src/cascadia/TerminalSettingsEditor/Launch.h b/src/cascadia/TerminalSettingsEditor/Launch.h index 799a4fc32..82fed6e41 100644 --- a/src/cascadia/TerminalSettingsEditor/Launch.h +++ b/src/cascadia/TerminalSettingsEditor/Launch.h @@ -27,6 +27,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation IInspectable CurrentDefaultProfile(); void CurrentDefaultProfile(const IInspectable& value); + winrt::Windows::Foundation::Collections::IObservableVector DefaultProfiles() const; WINRT_PROPERTY(Editor::LaunchPageNavigationState, State, nullptr); diff --git a/src/cascadia/TerminalSettingsEditor/Launch.idl b/src/cascadia/TerminalSettingsEditor/Launch.idl index d59228bad..a5b132eb2 100644 --- a/src/cascadia/TerminalSettingsEditor/Launch.idl +++ b/src/cascadia/TerminalSettingsEditor/Launch.idl @@ -16,6 +16,9 @@ namespace Microsoft.Terminal.Settings.Editor LaunchPageNavigationState State { get; }; IInspectable CurrentDefaultProfile; + // I wish this was a IObservableVector, but: + // https://github.com/microsoft/microsoft-ui-xaml/issues/5395 + IObservableVector DefaultProfiles { get; }; IInspectable CurrentLaunchMode; IObservableVector LaunchModeList { get; }; diff --git a/src/cascadia/TerminalSettingsEditor/Launch.xaml b/src/cascadia/TerminalSettingsEditor/Launch.xaml index b556a4889..caac364f4 100644 --- a/src/cascadia/TerminalSettingsEditor/Launch.xaml +++ b/src/cascadia/TerminalSettingsEditor/Launch.xaml @@ -40,7 +40,7 @@ diff --git a/src/cascadia/TerminalSettingsEditor/MainPage.cpp b/src/cascadia/TerminalSettingsEditor/MainPage.cpp index 71a775cb0..414bd4f0d 100644 --- a/src/cascadia/TerminalSettingsEditor/MainPage.cpp +++ b/src/cascadia/TerminalSettingsEditor/MainPage.cpp @@ -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) diff --git a/src/cascadia/TerminalSettingsEditor/Profiles.cpp b/src/cascadia/TerminalSettingsEditor/Profiles.cpp index 1c105798c..94b49c2fa 100644 --- a/src/cascadia/TerminalSettingsEditor/Profiles.cpp +++ b/src/cascadia/TerminalSettingsEditor/Profiles.cpp @@ -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 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 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()) { diff --git a/src/cascadia/TerminalSettingsEditor/Profiles.xaml b/src/cascadia/TerminalSettingsEditor/Profiles.xaml index 54afd38fd..46e7e87ef 100644 --- a/src/cascadia/TerminalSettingsEditor/Profiles.xaml +++ b/src/cascadia/TerminalSettingsEditor/Profiles.xaml @@ -144,81 +144,77 @@ - - - - + + + diff --git a/src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw b/src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw index c6875b08f..7b328dc4d 100644 --- a/src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw +++ b/src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw @@ -994,14 +994,6 @@ Rename Text label for a button that can be used to begin the renaming process. - - This profile cannot be deleted because it is included by default. - Disclaimer presented next to the delete button when it is disabled. Some profiles are included in the app by default and cannot be deleted. - - - This profile cannot be deleted because it is automatically generated. - Disclaimer presented next to the delete button when it is disabled. Some profiles are automatically generated by the app and cannot be deleted. - This color scheme cannot be deleted or renamed because it is included by default. Disclaimer presented next to the delete button when it is disabled. diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp index 03b05a766..052026fb9 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp @@ -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 duplicated; - if (_userDefaultProfileSettings) - { - duplicated = _userDefaultProfileSettings->CreateChild(); - } - else - { - duplicated = winrt::make_self(); - } - _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 CascadiaSettings::_CreateNewProfile(const std::wstring_view& name) const +{ + winrt::com_ptr profile; + + if (_userDefaultProfileSettings) + { + profile = _userDefaultProfileSettings->CreateChild(); + } + else + { + profile = winrt::make_self(); + } + + // 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 diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.h b/src/cascadia/TerminalSettingsModel/CascadiaSettings.h index 5a5735831..ee5782277 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.h +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.h @@ -124,6 +124,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation Json::Value _defaultSettings; winrt::com_ptr _userDefaultProfileSettings{ nullptr }; + winrt::com_ptr _CreateNewProfile(const std::wstring_view& name) const; + void _LayerOrCreateProfile(const Json::Value& profileJson); winrt::com_ptr _FindMatchingProfile(const Json::Value& profileJson); std::optional _FindMatchingProfileIndex(const Json::Value& profileJson); diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp index 9d6e5c1f8..36ca2a316 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp @@ -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(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(profile) }; + const auto profileImpl{ winrt::get_self(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(entry) }; - profilesList.append(prof->ToJson()); + if (!entry.Deleted()) + { + const auto prof{ winrt::get_self(entry) }; + profilesList.append(prof->ToJson()); + } } profiles[JsonKey(ProfilesListKey)] = profilesList; json[JsonKey(ProfilesKey)] = profiles; diff --git a/src/cascadia/TerminalSettingsModel/Profile.cpp b/src/cascadia/TerminalSettingsModel/Profile.cpp index d90a65884..338de21ac 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.cpp +++ b/src/cascadia/TerminalSettingsModel/Profile.cpp @@ -84,6 +84,7 @@ winrt::com_ptr Profile::CopySettings(winrt::com_ptr source) { auto profile{ winrt::make_self() }; + profile->_Deleted = source->_Deleted; profile->_Guid = source->_Guid; profile->_Name = source->_Name; profile->_Source = source->_Source; diff --git a/src/cascadia/TerminalSettingsModel/Profile.h b/src/cascadia/TerminalSettingsModel/Profile.h index 155dcec4c..c84dadefc 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.h +++ b/src/cascadia/TerminalSettingsModel/Profile.h @@ -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())); diff --git a/src/cascadia/TerminalSettingsModel/Profile.idl b/src/cascadia/TerminalSettingsModel/Profile.idl index 9572fdc17..5ad1ae933 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.idl +++ b/src/cascadia/TerminalSettingsModel/Profile.idl @@ -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);