From 39d16ba94cfe537f82b1eacf76bf8fc675776136 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Wed, 6 Oct 2021 18:58:09 +0200 Subject: [PATCH] PREREQ: Fix default terminal setting dropdown (#11430) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit WinUI/XAML requires the `SelectedItem` to be member of the list of `ItemsSource`. `CascadiaSettings::DefaultTerminals()` is such an `ItemsSource` and is called every time the launch settings page is visited. It calls `DefaultTerminal::Available()` which in turn calls `Refresh()`. While the `SelectedItem` was cached in `CascadiaSettings`, the value of `DefaultTerminals()` wasn't. Thus every time the page was visited, it refreshed the `ItemsSource` list without invalidating the current `SelectedItem`. This commit prevents such accidental mishaps from occurring in the future, by moving the responsibility of caching solely to the `CascadiaSettings` class. * [x] Closes #11424 * [x] I work here * [x] Tests added/passed * Navigating between SUI pages maintains the current dropdown selection ✔️ * Saving the settings saves the correct terminal at `HKCU:\Console\%%Startup` ✔️ (cherry picked from commit 35ce8cc8580e8ff5f642e569d94953f90c06a196) --- .github/actions/spelling/expect/expect.txt | 3 + .../CascadiaSettings.cpp | 27 ++++++-- .../TerminalSettingsModel/CascadiaSettings.h | 13 ++-- .../CascadiaSettingsSerialization.cpp | 1 + .../TerminalSettingsModel/DefaultTerminal.cpp | 65 +++++++------------ .../TerminalSettingsModel/DefaultTerminal.h | 14 +--- .../TerminalSettingsModel/DefaultTerminal.idl | 3 - 7 files changed, 60 insertions(+), 66 deletions(-) diff --git a/.github/actions/spelling/expect/expect.txt b/.github/actions/spelling/expect/expect.txt index 8ed94f9d4..6386b273b 100644 --- a/.github/actions/spelling/expect/expect.txt +++ b/.github/actions/spelling/expect/expect.txt @@ -189,6 +189,7 @@ cacafire callee capslock CARETBLINKINGENABLED +carlos CARRIAGERETURN cascadia cassert @@ -2716,6 +2717,7 @@ wixproj wline wlinestream wmain +wmemory WMSZ wnd WNDALLOC @@ -2842,6 +2844,7 @@ YSize YSubstantial YVIRTUALSCREEN YWalk +zamora ZCmd ZCtrl zsh diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp index aff7cba52..53a60baeb 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp @@ -5,6 +5,8 @@ #include "CascadiaSettings.h" #include "CascadiaSettings.g.cpp" +#include "DefaultTerminal.h" + #include #include "AzureCloudShellGenerator.h" @@ -1239,24 +1241,21 @@ bool CascadiaSettings::IsDefaultTerminalAvailable() noexcept // - // Return Value: // - an iterable collection of all available terminals that could be the default. -IObservableVector CascadiaSettings::DefaultTerminals() const noexcept +IObservableVector CascadiaSettings::DefaultTerminals() noexcept { + _refreshDefaultTerminals(); return _defaultTerminals; } // Method Description: // - Returns the currently selected default terminal application. -// - DANGER! This will be null unless you've called -// CascadiaSettings::RefreshDefaultTerminals. At the time of this comment (May - -// 2021), only the Launch page in the settings UI calls that method, so this -// value is unset unless you've navigated to that page. // Arguments: // - // Return Value: // - the selected default terminal application Settings::Model::DefaultTerminal CascadiaSettings::CurrentDefaultTerminal() const noexcept { + _refreshDefaultTerminals(); return _currentDefaultTerminal; } @@ -1270,3 +1269,19 @@ void CascadiaSettings::CurrentDefaultTerminal(Settings::Model::DefaultTerminal t { _currentDefaultTerminal = terminal; } + +// This function is implicitly called by DefaultTerminals/CurrentDefaultTerminal(). +// It reloads the selection of available, installed terminals and caches them. +// WinUI requires us that the `SelectedItem` of a collection is member of the list given to `ItemsSource`. +// It's thus important that _currentDefaultTerminal is a member of _defaultTerminals. +// Right now this is implicitly the case thanks to DefaultTerminal::Available(), +// but in the future it might be worthwhile to change the code to use list indices instead. +void CascadiaSettings::_refreshDefaultTerminals() +{ + if (!_defaultTerminals) + { + auto [defaultTerminals, defaultTerminal] = DefaultTerminal::Available(); + _defaultTerminals = winrt::single_threaded_observable_vector(std::move(defaultTerminals)); + _currentDefaultTerminal = std::move(defaultTerminal); + } +} diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.h b/src/cascadia/TerminalSettingsModel/CascadiaSettings.h index ee5782277..174e6f895 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.h +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.h @@ -102,9 +102,9 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation void RefreshDefaultTerminals(); static bool IsDefaultTerminalAvailable() noexcept; - Windows::Foundation::Collections::IObservableVector DefaultTerminals() const noexcept; - Model::DefaultTerminal CurrentDefaultTerminal() const noexcept; - void CurrentDefaultTerminal(Model::DefaultTerminal terminal); + winrt::Windows::Foundation::Collections::IObservableVector DefaultTerminals() noexcept; + Model::DefaultTerminal CurrentDefaultTerminal() noexcept; + void CurrentDefaultTerminal(const Model::DefaultTerminal& terminal); private: com_ptr _globals; @@ -114,8 +114,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation Windows::Foundation::IReference _loadError; hstring _deserializationErrorMessage; - Windows::Foundation::Collections::IObservableVector _defaultTerminals; - Model::DefaultTerminal _currentDefaultTerminal; + void _refreshDefaultTerminals(); std::vector> _profileGenerators; @@ -177,6 +176,10 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation friend class SettingsModelLocalTests::KeyBindingsTests; friend class TerminalAppUnitTests::DynamicProfileTests; friend class TerminalAppUnitTests::JsonTests; + + // defterm + winrt::Windows::Foundation::Collections::IObservableVector _defaultTerminals{ nullptr }; + Model::DefaultTerminal _currentDefaultTerminal{ nullptr }; }; } diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp index 36ca2a316..f7ebd3031 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp @@ -16,6 +16,7 @@ // "Generated Files" directory. #include "ApplicationState.h" +#include "DefaultTerminal.h" #include "FileUtils.h" using namespace winrt::Microsoft::Terminal::Settings::Model::implementation; diff --git a/src/cascadia/TerminalSettingsModel/DefaultTerminal.cpp b/src/cascadia/TerminalSettingsModel/DefaultTerminal.cpp index 5f5079d2f..9a614baf3 100644 --- a/src/cascadia/TerminalSettingsModel/DefaultTerminal.cpp +++ b/src/cascadia/TerminalSettingsModel/DefaultTerminal.cpp @@ -10,85 +10,70 @@ using namespace winrt::Microsoft::Terminal::Settings; using namespace winrt::Microsoft::Terminal::Settings::Model::implementation; -winrt::Windows::Foundation::Collections::IVector DefaultTerminal::_available = winrt::single_threaded_vector(); -Model::DefaultTerminal DefaultTerminal::_current = nullptr; - -DefaultTerminal::DefaultTerminal(DelegationConfig::DelegationPackage pkg) : - _pkg(pkg) +DefaultTerminal::DefaultTerminal(DelegationConfig::DelegationPackage&& pkg) : + _pkg{ pkg } { } winrt::hstring DefaultTerminal::Name() const { - static const std::wstring def{ RS_(L"InboxWindowsConsoleName") }; - return _pkg.terminal.name.empty() ? winrt::hstring{ def } : winrt::hstring{ _pkg.terminal.name }; + return _pkg.terminal.name.empty() ? winrt::hstring{ RS_(L"InboxWindowsConsoleName") } : winrt::hstring{ _pkg.terminal.name }; } winrt::hstring DefaultTerminal::Version() const { // If there's no version information... return empty string instead. - if (DelegationConfig::PkgVersion{} == _pkg.terminal.version) + const auto& version = _pkg.terminal.version; + if (DelegationConfig::PkgVersion{} == version) { return winrt::hstring{}; } - const auto name = fmt::format(L"{}.{}.{}.{}", _pkg.terminal.version.major, _pkg.terminal.version.minor, _pkg.terminal.version.build, _pkg.terminal.version.revision); - return winrt::hstring{ name }; + fmt::wmemory_buffer buffer; + fmt::format_to(buffer, L"{}.{}.{}.{}", version.major, version.minor, version.build, version.revision); + return winrt::hstring{ buffer.data(), gsl::narrow_cast(buffer.size()) }; } winrt::hstring DefaultTerminal::Author() const { - static const std::wstring def{ RS_(L"InboxWindowsConsoleAuthor") }; - return _pkg.terminal.author.empty() ? winrt::hstring{ def } : winrt::hstring{ _pkg.terminal.author }; + return _pkg.terminal.author.empty() ? winrt::hstring{ RS_(L"InboxWindowsConsoleAuthor") } : winrt::hstring{ _pkg.terminal.author }; } winrt::hstring DefaultTerminal::Icon() const { - static const std::wstring_view def{ L"\uE756" }; - return _pkg.terminal.logo.empty() ? winrt::hstring{ def } : winrt::hstring{ _pkg.terminal.logo }; + return _pkg.terminal.logo.empty() ? winrt::hstring{ L"\uE756" } : winrt::hstring{ _pkg.terminal.logo }; } -void DefaultTerminal::Refresh() +std::pair, Model::DefaultTerminal> DefaultTerminal::Available() { + // The potential of returning nullptr for defaultTerminal feels weird, but XAML can + // handle that appropriately and will select nothing as current in the dropdown. + std::vector defaultTerminals; + Model::DefaultTerminal defaultTerminal{ nullptr }; + std::vector allPackages; DelegationConfig::DelegationPackage currentPackage; - LOG_IF_FAILED(DelegationConfig::s_GetAvailablePackages(allPackages, currentPackage)); - _available.Clear(); - for (const auto& pkg : allPackages) + for (auto& pkg : allPackages) { - auto p = winrt::make(pkg); + // Be a bit careful here: We're moving pkg into the constructor. + // Afterwards it'll be invalid, so we need to cache isCurrent. + const auto isCurrent = pkg == currentPackage; + auto p = winrt::make(std::move(pkg)); - _available.Append(p); - - if (pkg == currentPackage) + if (isCurrent) { - _current = p; + defaultTerminal = p; } - } -} -winrt::Windows::Foundation::Collections::IVectorView DefaultTerminal::Available() -{ - Refresh(); - return _available.GetView(); -} - -Model::DefaultTerminal DefaultTerminal::Current() -{ - if (!_current) - { - Refresh(); + defaultTerminals.emplace_back(std::move(p)); } - // The potential of returning nullptr feels weird, but XAML can handle that appropriately - // and will select nothing as current in the dropdown. - return _current; + return { std::move(defaultTerminals), std::move(defaultTerminal) }; } void DefaultTerminal::Current(const Model::DefaultTerminal& term) { THROW_IF_FAILED(DelegationConfig::s_SetDefaultByPackage(winrt::get_self(term)->_pkg, true)); - _current = term; } diff --git a/src/cascadia/TerminalSettingsModel/DefaultTerminal.h b/src/cascadia/TerminalSettingsModel/DefaultTerminal.h index ddce7d576..19f3ae14c 100644 --- a/src/cascadia/TerminalSettingsModel/DefaultTerminal.h +++ b/src/cascadia/TerminalSettingsModel/DefaultTerminal.h @@ -19,7 +19,6 @@ Author(s): #pragma once #include "DefaultTerminal.g.h" -#include "../inc/cppwinrt_utils.h" #include "../../propslib/DelegationConfig.hpp" @@ -27,26 +26,17 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation { struct DefaultTerminal : public DefaultTerminalT { - DefaultTerminal(DelegationConfig::DelegationPackage pkg); + explicit DefaultTerminal(DelegationConfig::DelegationPackage&& pkg); hstring Name() const; hstring Author() const; hstring Version() const; hstring Icon() const; - static void Refresh(); - static Windows::Foundation::Collections::IVectorView Available(); - static Model::DefaultTerminal Current(); + static std::pair, Model::DefaultTerminal> Available(); static void Current(const Model::DefaultTerminal& term); private: DelegationConfig::DelegationPackage _pkg; - static Windows::Foundation::Collections::IVector _available; - static Model::DefaultTerminal _current; }; } - -namespace winrt::Microsoft::Terminal::Settings::Model::factory_implementation -{ - BASIC_FACTORY(DefaultTerminal); -} diff --git a/src/cascadia/TerminalSettingsModel/DefaultTerminal.idl b/src/cascadia/TerminalSettingsModel/DefaultTerminal.idl index 9b3cc662d..94ceb48dd 100644 --- a/src/cascadia/TerminalSettingsModel/DefaultTerminal.idl +++ b/src/cascadia/TerminalSettingsModel/DefaultTerminal.idl @@ -9,8 +9,5 @@ namespace Microsoft.Terminal.Settings.Model String Author { get; }; String Version { get; }; String Icon { get; }; - - static DefaultTerminal Current; - static Windows.Foundation.Collections.IVectorView Available { get; }; } }