Fix default terminal setting dropdown (#11430)

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.

## PR Checklist
* [x] Closes #11424
* [x] I work here
* [x] Tests added/passed

## Validation Steps Performed
* Navigating between SUI pages maintains the current dropdown selection ✔️
* Saving the settings saves the correct terminal at `HKCU:\Console\%%Startup` ✔️

.github/actions/spelling/expect/expect.txt

@@ -189,13 +189,12 @@ cacafire
 callee
 capslock
 CARETBLINKINGENABLED
+carlos
 CARRIAGERETURN
 cascadia
 cassert
 castsi
 catid
-carlos
-zamora
 cazamor
 CBash
 cbegin
@@ -2724,6 +2723,7 @@ wixproj
 wline
 wlinestream
 wmain
+wmemory
 WMSZ
 wnd
 WNDALLOC
@@ -2850,6 +2850,7 @@ YSize
 YSubstantial
 YVIRTUALSCREEN
 YWalk
+zamora
 ZCmd
 ZCtrl
 zsh

src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp

@@ -5,6 +5,7 @@
 #include "CascadiaSettings.h"
 #include "CascadiaSettings.g.cpp"
 
+#include "DefaultTerminal.h"
 #include "FileUtils.h"
 
 #include <LibraryResources.h>
@@ -844,31 +845,21 @@ bool CascadiaSettings::IsDefaultTerminalAvailable() noexcept
 // - <none>
 // Return Value:
 // - an iterable collection of all available terminals that could be the default.
-IObservableVector<Model::DefaultTerminal> CascadiaSettings::DefaultTerminals() const noexcept
+IObservableVector<Model::DefaultTerminal> CascadiaSettings::DefaultTerminals() noexcept
 {
-    const auto available = DefaultTerminal::Available();
-    std::vector<Model::DefaultTerminal> terminals{ available.Size(), nullptr };
-    available.GetMany(0, terminals);
-    return winrt::single_threaded_observable_vector(std::move(terminals));
+    _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:
 // - <none>
 // Return Value:
 // - the selected default terminal application
 Settings::Model::DefaultTerminal CascadiaSettings::CurrentDefaultTerminal() noexcept
 {
-    if (!_currentDefaultTerminal)
-    {
-        _currentDefaultTerminal = DefaultTerminal::Current();
-    }
+    _refreshDefaultTerminals();
     return _currentDefaultTerminal;
 }
 
@@ -883,11 +874,27 @@ void CascadiaSettings::CurrentDefaultTerminal(const Model::DefaultTerminal& term
     _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);
+    }
+}
+
 void CascadiaSettings::ExportFile(winrt::hstring path, winrt::hstring content)
 {
     try
     {
-        winrt::Microsoft::Terminal::Settings::Model::WriteUTF8FileAtomic({ path.c_str() }, til::u16u8(content));
+        WriteUTF8FileAtomic({ path.c_str() }, til::u16u8(content));
     }
     CATCH_LOG();
 }

src/cascadia/TerminalSettingsModel/CascadiaSettings.h

@@ -134,7 +134,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
 
         // defterm
         static bool IsDefaultTerminalAvailable() noexcept;
-        winrt::Windows::Foundation::Collections::IObservableVector<Model::DefaultTerminal> DefaultTerminals() const noexcept;
+        winrt::Windows::Foundation::Collections::IObservableVector<Model::DefaultTerminal> DefaultTerminals() noexcept;
         Model::DefaultTerminal CurrentDefaultTerminal() noexcept;
         void CurrentDefaultTerminal(const Model::DefaultTerminal& terminal);
 
@@ -142,6 +142,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
         static const std::filesystem::path& _settingsPath();
 
         winrt::com_ptr<implementation::Profile> _createNewProfile(const std::wstring_view& name) const;
+        void _refreshDefaultTerminals();
 
         void _resolveDefaultProfile() const;
 
@@ -164,6 +165,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
         winrt::hstring _deserializationErrorMessage;
 
         // defterm
+        winrt::Windows::Foundation::Collections::IObservableVector<Model::DefaultTerminal> _defaultTerminals{ nullptr };
         Model::DefaultTerminal _currentDefaultTerminal{ nullptr };
     };
 }

src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp

@@ -24,6 +24,7 @@
 #include "userDefaults.h"
 
 #include "ApplicationState.h"
+#include "DefaultTerminal.h"
 #include "FileUtils.h"
 
 using namespace winrt::Microsoft::Terminal::Settings;
@@ -36,8 +37,6 @@ static constexpr std::string_view ProfilesKey{ "profiles" };
 static constexpr std::string_view DefaultSettingsKey{ "defaults" };
 static constexpr std::string_view ProfilesListKey{ "list" };
 static constexpr std::string_view SchemesKey{ "schemes" };
-static constexpr std::string_view NameKey{ "name" };
-static constexpr std::string_view GuidKey{ "guid" };
 
 static constexpr std::wstring_view jsonExtension{ L".json" };
 static constexpr std::wstring_view FragmentsSubDirectory{ L"\\Fragments" };

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<Model::DefaultTerminal> DefaultTerminal::_available = winrt::single_threaded_vector<Model::DefaultTerminal>();
-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<winrt::hstring::size_type>(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<std::vector<Model::DefaultTerminal>, 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<Model::DefaultTerminal> defaultTerminals;
+    Model::DefaultTerminal defaultTerminal{ nullptr };
+
     std::vector<DelegationConfig::DelegationPackage> 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<DefaultTerminal>(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<DefaultTerminal>(std::move(pkg));
 
-        _available.Append(p);
-
-        if (pkg == currentPackage)
+        if (isCurrent)
         {
-            _current = p;
+            defaultTerminal = p;
         }
-    }
-}
 
-winrt::Windows::Foundation::Collections::IVectorView<Model::DefaultTerminal> 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<DefaultTerminal>(term)->_pkg, true));
-    _current = term;
 }

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>
     {
-        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<Model::DefaultTerminal> Available();
-        static Model::DefaultTerminal Current();
+        static std::pair<std::vector<Model::DefaultTerminal>, Model::DefaultTerminal> Available();
         static void Current(const Model::DefaultTerminal& term);
 
     private:
         DelegationConfig::DelegationPackage _pkg;
-        static Windows::Foundation::Collections::IVector<Model::DefaultTerminal> _available;
-        static Model::DefaultTerminal _current;
     };
 }
-
-namespace winrt::Microsoft::Terminal::Settings::Model::factory_implementation
-{
-    BASIC_FACTORY(DefaultTerminal);
-}

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<DefaultTerminal> Available { get; };
     }
 }