Repository: microsoft/terminal (language: cpp)

Issue:
[1.21] After changing font, features/axes can't be edited
Clicking add or delete does nothing to the UI


Discussion / hints:

Fix patch:
diff --git a/.github/actions/spelling/expect/expect.txt b/.github/actions/spelling/expect/expect.txt
index 775694924d3..5c0c14b0df9 100644
--- a/.github/actions/spelling/expect/expect.txt
+++ b/.github/actions/spelling/expect/expect.txt
@@ -245,6 +245,7 @@ CONKBD
 conlibk
 conmsgl
 CONNECTINFO
+connyection
 CONOUT
 conprops
 conpropsp
@@ -1420,6 +1421,8 @@ PUCHAR
 pvar
 pwch
 PWDDMCONSOLECONTEXT
+Pwease
+pweview
 pws
 pwstr
 pwsz
@@ -1898,6 +1901,7 @@ UVWXY
 UVWXYZ
 uwa
 uwp
+uwu
 uxtheme
 Vanara
 vararg
@@ -1971,6 +1975,7 @@ wdm
 webpage
 websites
 wekyb
+wewoad
 wex
 wextest
 wextestclass
diff --git a/src/cascadia/TerminalSettingsEditor/Appearances.cpp b/src/cascadia/TerminalSettingsEditor/Appearances.cpp
index c2823b955bc..2ed71afc5a3 100644
--- a/src/cascadia/TerminalSettingsEditor/Appearances.cpp
+++ b/src/cascadia/TerminalSettingsEditor/Appearances.cpp
@@ -544,14 +544,33 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
         return item;
     }
 
+    // Call this when all the _fontFaceDependents members have changed.
     void AppearanceViewModel::_notifyChangesForFontSettings()
     {
-        _NotifyChanges(
-            L"FontFaceDependents",
-            L"FontAxes",
-            L"FontFeatures",
-            L"HasFontAxes",
-            L"HasFontFeatures");
+        _NotifyChanges(L"FontFaceDependents");
+        _NotifyChanges(L"FontAxes");
+        _NotifyChanges(L"FontFeatures");
+        _NotifyChanges(L"HasFontAxes");
+        _NotifyChanges(L"HasFontFeatures");
+    }
+
+    // Call this when used items moved into unused and vice versa.
+    // Because this doesn't recreate the IObservableVector instances,
+    // we don't need to notify the UI about changes to the "FontAxes" property.
+    void AppearanceViewModel::_notifyChangesForFontSettingsReactive(FontSettingIndex fontSettingsIndex)
+    {
+        _NotifyChanges(L"FontFaceDependents");
+        switch (fontSettingsIndex)
+        {
+        case FontAxesIndex:
+            _NotifyChanges(L"HasFontAxes");
+            break;
+        case FontFeaturesIndex:
+            _NotifyChanges(L"HasFontFeatures");
+            break;
+        default:
+            break;
+        }
     }
 
     double AppearanceViewModel::LineHeight() const
@@ -616,6 +635,30 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
         FontWeight(winrt::Microsoft::Terminal::UI::Converters::DoubleToFontWeight(fontWeight));
     }
 
+    const AppearanceViewModel::FontFaceDependentsData& AppearanceViewModel::FontFaceDependents()
+    {
+        if (!_fontFaceDependents)
+        {
+            _refreshFontFaceDependents();
+        }
+        return *_fontFaceDependents;
+    }
+
+    winrt::hstring AppearanceViewModel::MissingFontFaces()
+    {
+        return FontFaceDependents().missingFontFaces;
+    }
+
+    winrt::hstring AppearanceViewModel::ProportionalFontFaces()
+    {
+        return FontFaceDependents().proportionalFontFaces;
+    }
+
+    bool AppearanceViewModel::HasPowerlineCharacters()
+    {
+        return FontFaceDependents().hasPowerlineCharacters;
+    }
+
     IObservableVector<Editor::FontKeyValuePair> AppearanceViewModel::FontAxes()
     {
         return FontFaceDependents().fontSettingsUsed[FontAxesIndex];
@@ -628,7 +671,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
 
     void AppearanceViewModel::ClearFontAxes()
     {
-        _deleteAllFontSettings(FontAxesIndex);
+        _deleteAllFontKeyValuePairs(FontAxesIndex);
     }
 
     Model::FontConfig AppearanceViewModel::FontAxesOverrideSource() const
@@ -648,7 +691,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
 
     void AppearanceViewModel::ClearFontFeatures()
     {
-        _deleteAllFontSettings(FontFeaturesIndex);
+        _deleteAllFontKeyValuePairs(FontFeaturesIndex);
     }
 
     Model::FontConfig AppearanceViewModel::FontFeaturesOverrideSource() const
@@ -664,7 +707,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
         }
 
         const auto kvImpl = winrt::get_self<FontKeyValuePair>(kv);
-        const auto fontSettingsIndex = kvImpl->IsFontFeature() ? 1 : 0;
+        const auto fontSettingsIndex = kvImpl->IsFontFeature() ? FontFeaturesIndex : FontAxesIndex;
         auto& d = *_fontFaceDependents;
         auto& used = d.fontSettingsUsed[fontSettingsIndex];
         auto& unused = d.fontSettingsUnused[fontSettingsIndex];
@@ -686,7 +729,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
 
         unused.erase(it);
 
-        _notifyChangesForFontSettings();
+        _notifyChangesForFontSettingsReactive(fontSettingsIndex);
     }
 
     void AppearanceViewModel::DeleteFontKeyValuePair(const Editor::FontKeyValuePair& kv)
@@ -699,10 +742,9 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
         const auto kvImpl = winrt::get_self<FontKeyValuePair>(kv);
         const auto tag = kvImpl->Key();
         const auto tagString = tagToString(tag);
-        const auto fontSettingsIndex = kvImpl->IsFontFeature() ? 1 : 0;
+        const auto fontSettingsIndex = kvImpl->IsFontFeature() ? FontFeaturesIndex : FontAxesIndex;
         auto& d = *_fontFaceDependents;
         auto& used = d.fontSettingsUsed[fontSettingsIndex];
-        auto& unused = d.fontSettingsUnused[fontSettingsIndex];
 
         const auto fontInfo = _appearance.SourceProfile().FontInfo();
         auto fontSettingsUser = kvImpl->IsFontFeature() ? fontInfo.FontFeatures() : fontInfo.FontAxes();
@@ -719,47 +761,13 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
 
         fontSettingsUser.Remove(std::wstring_view{ tagString });
 
-        // Insert the item into the unused list, keeping it sorted by the display text.
-        {
-            const auto item = _createFontSettingMenuItem(*it);
-            const auto it = std::lower_bound(unused.begin(), unused.end(), item, [](const MenuFlyoutItemBase& lhs, const MenuFlyoutItemBase& rhs) {
-                const auto& a = lhs.as<MenuFlyoutItem>().Text();
-                const auto& b = rhs.as<MenuFlyoutItem>().Text();
-                return til::compare_linguistic_insensitive(a, b) < 0;
-            });
-            unused.insert(it, item);
-        }
-
+        _addMenuFlyoutItemToUnused(fontSettingsIndex, _createFontSettingMenuItem(*it));
         used.RemoveAt(gsl::narrow<uint32_t>(it - used.begin()));
 
-        _notifyChangesForFontSettings();
-    }
-
-    void AppearanceViewModel::UpdateFontSetting(const FontKeyValuePair* kvImpl)
-    {
-        const auto tag = kvImpl->Key();
-        const auto value = kvImpl->Value();
-        const auto tagString = tagToString(tag);
-        const auto fontInfo = _appearance.SourceProfile().FontInfo();
-        auto fontSettingsUser = kvImpl->IsFontFeature() ? fontInfo.FontFeatures() : fontInfo.FontAxes();
-
-        if (!fontSettingsUser)
-        {
-            fontSettingsUser = winrt::single_threaded_map<hstring, float>();
-            if (kvImpl->IsFontFeature())
-            {
-                fontInfo.FontFeatures(fontSettingsUser);
-            }
-            else
-            {
-                fontInfo.FontAxes(fontSettingsUser);
-            }
-        }
-
-        std::ignore = fontSettingsUser.Insert(std::wstring_view{ tagString }, value);
+        _notifyChangesForFontSettingsReactive(fontSettingsIndex);
     }
 
-    void AppearanceViewModel::_deleteAllFontSettings(FontSettingIndex fontSettingsIndex)
+    void AppearanceViewModel::_deleteAllFontKeyValuePairs(FontSettingIndex fontSettingsIndex)
     {
         const auto fontInfo = _appearance.SourceProfile().FontInfo();
         if (fontSettingsIndex == FontFeaturesIndex)
@@ -778,16 +786,61 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
 
         auto& d = *_fontFaceDependents;
         auto& used = d.fontSettingsUsed[fontSettingsIndex];
-        auto& unused = d.fontSettingsUnused[fontSettingsIndex];
 
         for (const auto& kv : used)
         {
-            unused.emplace_back(_createFontSettingMenuItem(kv));
+            _addMenuFlyoutItemToUnused(fontSettingsIndex, _createFontSettingMenuItem(kv));
         }
 
         used.Clear();
 
-        _notifyChangesForFontSettings();
+        _notifyChangesForFontSettingsReactive(fontSettingsIndex);
+    }
+
+    // Inserts the given menu item into the unused list, while keeping it sorted by the display text.
+    void AppearanceViewModel::_addMenuFlyoutItemToUnused(FontSettingIndex index, MenuFlyoutItemBase item)
+    {
+        if (!_fontFaceDependents)
+        {
+            return;
+        }
+
+        auto& d = *_fontFaceDependents;
+        auto& unused = d.fontSettingsUnused[index];
+
+        const auto it = std::lower_bound(unused.begin(), unused.end(), item, [](const MenuFlyoutItemBase& lhs, const MenuFlyoutItemBase& rhs) {
+            const auto& a = lhs.as<MenuFlyoutItem>().Text();
+            const auto& b = rhs.as<MenuFlyoutItem>().Text();
+            return til::compare_linguistic_insensitive(a, b) < 0;
+        });
+        unused.insert(it, std::move(item));
+    }
+
+    void AppearanceViewModel::UpdateFontSetting(const FontKeyValuePair* kvImpl)
+    {
+        const auto tag = kvImpl->Key();
+        const auto value = kvImpl->Value();
+        const auto tagString = tagToString(tag);
+        const auto fontInfo = _appearance.SourceProfile().FontInfo();
+        auto fontSettingsUser = kvImpl->IsFontFeature() ? fontInfo.FontFeatures() : fontInfo.FontAxes();
+
+        if (!fontSettingsUser)
+        {
+            fontSettingsUser = winrt::single_threaded_map<hstring, float>();
+            if (kvImpl->IsFontFeature())
+            {
+                fontInfo.FontFeatures(fontSettingsUser);
+            }
+            else
+            {
+                fontInfo.FontAxes(fontSettingsUser);
+            }
+        }
+
+        std::ignore = fontSettingsUser.Insert(std::wstring_view{ tagString }, value);
+        // Pwease call Profiles_Appearance::_onProfilePropertyChanged to make the pweview connyection wewoad. Thanks!! uwu
+        // ...I hate this.
+        _NotifyChanges(L"uwu");
     }
 
     void AppearanceViewModel::SetBackgroundImageOpacityFromPercentageValue(double percentageValue)
diff --git a/src/cascadia/TerminalSettingsEditor/Appearances.h b/src/cascadia/TerminalSettingsEditor/Appearances.h
index 572b6240984..7ad8a03ce47 100644
--- a/src/cascadia/TerminalSettingsEditor/Appearances.h
+++ b/src/cascadia/TerminalSettingsEditor/Appearances.h
@@ -94,18 +94,10 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
 
         void SetFontWeightFromDouble(double fontWeight);
 
-        const FontFaceDependentsData& FontFaceDependents()
-        {
-            if (!_fontFaceDependents)
-            {
-                _refreshFontFaceDependents();
-            }
-            return *_fontFaceDependents;
-        }
-
-        winrt::hstring MissingFontFaces() { return FontFaceDependents().missingFontFaces; }
-        winrt::hstring ProportionalFontFaces() { return FontFaceDependents().proportionalFontFaces; }
-        bool HasPowerlineCharacters() { return FontFaceDependents().hasPowerlineCharacters; }
+        const FontFaceDependentsData& FontFaceDependents();
+        winrt::hstring MissingFontFaces();
+        winrt::hstring ProportionalFontFaces();
+        bool HasPowerlineCharacters();
 
         Windows::Foundation::Collections::IObservableVector<Editor::FontKeyValuePair> FontAxes();
         bool HasFontAxes() const;
@@ -165,7 +157,9 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
         void _generateFontFeatures(IDWriteFontFace* fontFace, std::vector<Editor::FontKeyValuePair>& list);
         Windows::UI::Xaml::Controls::MenuFlyoutItemBase _createFontSettingMenuItem(const Editor::FontKeyValuePair& kv);
         void _notifyChangesForFontSettings();
-        void _deleteAllFontSettings(FontSettingIndex index);
+        void _notifyChangesForFontSettingsReactive(FontSettingIndex fontSettingsIndex);
+        void _deleteAllFontKeyValuePairs(FontSettingIndex index);
+        void _addMenuFlyoutItemToUnused(FontSettingIndex index, Windows::UI::Xaml::Controls::MenuFlyoutItemBase item);
 
         Model::AppearanceConfig _appearance;
         winrt::hstring _lastBgImagePath;
diff --git a/src/cascadia/TerminalSettingsEditor/Appearances.xaml b/src/cascadia/TerminalSettingsEditor/Appearances.xaml
index 2f94cf4473b..a3f643c53c3 100644
--- a/src/cascadia/TerminalSettingsEditor/Appearances.xaml
+++ b/src/cascadia/TerminalSettingsEditor/Appearances.xaml
@@ -309,7 +309,7 @@
                 <StackPanel Spacing="16">
                     <ListView IsItemClickEnabled="False"
                               ItemTemplate="{StaticResource FontKeyValuePairTemplate}"
-                              ItemsSource="{x:Bind Appearance.FontAxes}"
+                              ItemsSource="{x:Bind Appearance.FontAxes, Mode=OneWay}"
                               SelectionMode="None" />
                     <muxc:DropDownButton x:Name="AddFontAxisButton"
                                          x:Uid="Profile_AddFontAxisButton">
@@ -336,7 +336,7 @@
                 <StackPanel Spacing="16">
                     <ListView IsItemClickEnabled="False"
                               ItemTemplate="{StaticResource FontKeyValuePairTemplate}"
-                              ItemsSource="{x:Bind Appearance.FontFeatures}"
+                              ItemsSource="{x:Bind Appearance.FontFeatures, Mode=OneWay}"
                               SelectionMode="None" />
                     <muxc:DropDownButton x:Name="AddFontFeatureButton"
                                          x:Uid="Profile_AddFontFeatureButton">
diff --git a/src/cascadia/TerminalSettingsEditor/Profiles_Appearance.cpp b/src/cascadia/TerminalSettingsEditor/Profiles_Appearance.cpp
index 5295d3042bb..cdf1e851ae2 100644
--- a/src/cascadia/TerminalSettingsEditor/Profiles_Appearance.cpp
+++ b/src/cascadia/TerminalSettingsEditor/Profiles_Appearance.cpp
@@ -3,13 +3,11 @@
 
 #include "pch.h"
 #include "Profiles_Appearance.h"
-#include "Profiles_Appearance.g.cpp"
+
 #include "ProfileViewModel.h"
 #include "PreviewConnection.h"
-#include "EnumEntry.h"
 
-#include <LibraryResources.h>
-#include "..\WinRTUtils\inc\Utils.h"
+#include "Profiles_Appearance.g.cpp"
 
 using namespace winrt::Windows::UI::Xaml;
 using namespace winrt::Windows::UI::Xaml::Navigation;
@@ -64,10 +62,20 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
         _Profile.DeleteUnfocusedAppearance();
     }
 
-    void Profiles_Appearance::_onProfilePropertyChanged(const IInspectable&, const PropertyChangedEventArgs&) const
+    void Profiles_Appearance::_onProfilePropertyChanged(const IInspectable&, const PropertyChangedEventArgs&)
     {
-        const auto settings = _Profile.TermSettings();
-        _previewConnection->DisplayPowerlineGlyphs(_Profile.DefaultAppearance().HasPowerlineCharacters());
-        _previewControl.UpdateControlSettings(settings, settings);
+        if (!_updatePreviewControl)
+        {
+            _updatePreviewControl = std::make_shared<ThrottledFuncTrailing<>>(
+                winrt::Windows::System::DispatcherQueue::GetForCurrentThread(),
+                std::chrono::milliseconds{ 100 },
+                [this]() {
+                    const auto settings = _Profile.TermSettings();
+                    _previewConnection->DisplayPowerlineGlyphs(_Profile.DefaultAppearance().HasPowerlineCharacters());
+                    _previewControl.UpdateControlSettings(settings, settings);
+                });
+        }
+
+        _updatePreviewControl->Run();
     }
 }
diff --git a/src/cascadia/TerminalSettingsEditor/Profiles_Appearance.h b/src/cascadia/TerminalSettingsEditor/Profiles_Appearance.h
index ab9ebf66fb0..27c8e455a52 100644
--- a/src/cascadia/TerminalSettingsEditor/Profiles_Appearance.h
+++ b/src/cascadia/TerminalSettingsEditor/Profiles_Appearance.h
@@ -3,9 +3,12 @@
 
 #pragma once
 
-#include "Profiles_Appearance.g.h"
-#include "Utils.h"
+#include <ThrottledFunc.h>
+
 #include "PreviewConnection.h"
+#include "Utils.h"
+
+#include "Profiles_Appearance.g.h"
 
 namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
 {
@@ -26,10 +29,11 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
         WINRT_PROPERTY(Editor::ProfileViewModel, Profile, nullptr);
 
     private:
-        void _onProfilePropertyChanged(const IInspectable&, const PropertyChangedEventArgs&) const;
+        void _onProfilePropertyChanged(const IInspectable&, const PropertyChangedEventArgs&);
 
         winrt::com_ptr<PreviewConnection> _previewConnection{ nullptr };
         Microsoft::Terminal::Control::TermControl _previewControl{ nullptr };
+        std::shared_ptr<ThrottledFuncTrailing<>> _updatePreviewControl;
         Windows::UI::Xaml::Data::INotifyPropertyChanged::PropertyChanged_revoker _ViewModelChangedRevoker;
         Windows::UI::Xaml::Data::INotifyPropertyChanged::PropertyChanged_revoker _AppearanceViewModelChangedRevoker;
         Editor::IHostedInWindow _windowRoot;


Test patch: