CmdPal: Improvements and fixes for icon loading (#45460)

## Summary of the Pull Request

This PR is a follow-up for Icon cache:

- Adds decoding and rasterization limit (by width) to reduce memory
usage and improves throughput (noticeably)
- Fixes timing issue when setting padding for font icons
- Resolves race condition in IconBox caused by incorrect guard

<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist

- [x] Closes: #45460
<!-- - [ ] Closes: #yyy (add separate lines for additional resolved
issues) -->
- [ ] **Communication:** I've discussed this with core contributors
already. If the work hasn't been agreed, this work might be rejected
- [ ] **Tests:** Added/updated and all pass
- [ ] **Localization:** All end-user-facing strings can be localized
- [ ] **Dev docs:** Added/updated
- [ ] **New binaries:** Added on the required places
- [ ] [JSON for
signing](https://github.com/microsoft/PowerToys/blob/main/.pipelines/ESRPSigning_core.json)
for new binaries
- [ ] [WXS for
installer](https://github.com/microsoft/PowerToys/blob/main/installer/PowerToysSetup/Product.wxs)
for new binaries and localization folder
- [ ] [YML for CI
pipeline](https://github.com/microsoft/PowerToys/blob/main/.pipelines/ci/templates/build-powertoys-steps.yml)
for new test projects
- [ ] [YML for signed
pipeline](https://github.com/microsoft/PowerToys/blob/main/.pipelines/release.yml)
- [ ] **Documentation updated:** If checked, please file a pull request
on [our docs
repo](https://github.com/MicrosoftDocs/windows-uwp/tree/docs/hub/powertoys)
and link it here: #xxx

<!-- Provide a more detailed description of the PR, other things fixed,
or any additional comments/features here -->
## Detailed Description of the Pull Request / Additional comments

<!-- Describe how you validated the behavior. Add automated tests
wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed

.github/actions/spell-check/expect.txt

@@ -1527,6 +1527,7 @@ randi
 RAquadrant
 rasterization
 Rasterize
+rasterizing
 RAWINPUTDEVICE
 RAWINPUTHEADER
 RAWMODE

src/modules/cmdpal/Microsoft.CmdPal.UI/Controls/IconBox.cs

@@ -16,12 +16,12 @@ namespace Microsoft.CmdPal.UI.Controls;
 /// </summary>
 public partial class IconBox : ContentControl
 {
+    private const double DefaultIconFontSize = 16.0;
+
     private double _lastScale;
     private ElementTheme _lastTheme;
     private double _lastFontSize;
 
-    private const double DefaultIconFontSize = 16.0;
-
     /// <summary>
     /// Gets or sets the <see cref="IconSource"/> to display within the <see cref="IconBox"/>. Overwritten, if <see cref="SourceKey"/> is used instead.
     /// </summary>
@@ -62,6 +62,12 @@ public partial class IconBox : ContentControl
             {
                 Refresh();
             }
+#if DEBUG
+            if (_sourceRequested?.GetInvocationList().Length > 1)
+            {
+                Logger.LogWarning("There shouldn't be more than one handler for IconBox.SourceRequested");
+            }
+#endif
         }
         remove => _sourceRequested -= value;
     }
@@ -102,9 +108,12 @@ public partial class IconBox : ContentControl
         if (Source is FontIconSource fontIcon)
         {
             fontIcon.FontSize = _lastFontSize;
+            UpdatePaddingForFontIcon();
         }
     }
 
+    private void UpdatePaddingForFontIcon() => Padding = new Thickness(Math.Round(_lastFontSize * -0.2));
+
     private void OnActualThemeChanged(FrameworkElement sender, object args)
     {
         if (_lastTheme == ActualTheme)
@@ -150,10 +159,7 @@ public partial class IconBox : ContentControl
 
     private void Refresh()
     {
-        if (SourceKey is not null)
+        UpdateSourceKey(this, SourceKey);
-        {
-            UpdateSourceKey(this, SourceKey);
-        }
     }
 
     private static void OnSourcePropertyChanged(DependencyObject d, DependencyPropertyChangedEventArgs e)
@@ -170,8 +176,10 @@ public partial class IconBox : ContentControl
                 self.Padding = default;
                 break;
             case FontIconSource fontIcon:
+                self.UpdateLastFontSize();
                 if (self.Content is IconSourceElement iconSourceElement)
                 {
+                    fontIcon.FontSize = self._lastFontSize;
                     iconSourceElement.IconSource = fontIcon;
                 }
                 else
@@ -190,7 +198,7 @@ public partial class IconBox : ContentControl
                     self.Content = elem;
                 }
 
-                self.Padding = new Thickness(Math.Round(self._lastFontSize * -0.2));
+                self.UpdatePaddingForFontIcon();
 
                 break;
             case BitmapIconSource bitmapIcon:
@@ -206,10 +214,12 @@ public partial class IconBox : ContentControl
                 self.Padding = default;
 
                 break;
+
             case IconSource source:
                 self.Content = source.CreateIconElement();
                 self.Padding = default;
                 break;
+
             default:
                 throw new InvalidOperationException($"New value of {e.NewValue} is not of type IconSource.");
         }
@@ -233,10 +243,10 @@ public partial class IconBox : ContentControl
             return;
         }
 
-        Callback(iconBox, sourceKey);
+        RequestIconFromSource(iconBox, sourceKey);
     }
 
-    private static async void Callback(IconBox iconBox, object? sourceKey)
+    private static async void RequestIconFromSource(IconBox iconBox, object? sourceKey)
     {
         try
         {
@@ -256,17 +266,12 @@ public partial class IconBox : ContentControl
             // list virtualization situation, it's very possible we
             // may have already been set to a new icon before we
             // even got back from the await.
-            if (eventArgs.Key != sourceKey)
+            if (!ReferenceEquals(sourceKey, iconBox.SourceKey))
             {
                 // If the requested icon has changed, then just bail
                 return;
             }
 
-            if (eventArgs.Value == iconBox.Source)
-            {
-                return;
-            }
-
             iconBox.Source = eventArgs.Value;
         }
         catch (Exception ex)

src/modules/cmdpal/Microsoft.CmdPal.UI/Helpers/Icons/IconLoaderService.cs

@@ -219,6 +219,6 @@ internal sealed partial class IconLoaderService : IIconLoaderService
             iconSize = DefaultIconSize;
         }
 
-        return IconPathConverter.IconSourceMUX(iconString, false, fontFamily, iconSize);
+        return IconPathConverter.IconSourceMUX(iconString, fontFamily, iconSize);
     }
 }

src/modules/cmdpal/Microsoft.Terminal.UI/IconPathConverter.cpp

@@ -94,40 +94,49 @@ namespace winrt::Microsoft::Terminal::UI::implementation
     // - <TIconSource>: The type of IconSource (MUX, WUX) to generate.
     // Arguments:
     // - path: the full, expanded path to the icon.
+    // - targetSize: the target size for decoding/rasterizing the icon.
     // Return Value:
     // - An IconElement with its IconSource set, if possible.
     template<typename TIconSource>
-    TIconSource _getColoredBitmapIcon(const winrt::hstring& path, bool monochrome)
+    TIconSource _getColoredBitmapIcon(const winrt::hstring& path, int targetSize)
     {
         // FontIcon uses glyphs in the private use area, whereas valid URIs only contain ASCII characters.
         // To skip throwing on Uri construction, we can quickly check if the first character is ASCII.
-        if (!path.empty() && path.front() < 128)
+        if (path.empty() || path.front() >= 128)
         {
-            try
+            return nullptr;
-            {
-                winrt::Windows::Foundation::Uri iconUri{ path };
-
-                if (til::equals_insensitive_ascii(iconUri.Extension(), L".svg"))
-                {
-                    typename ImageIconSource<TIconSource>::type iconSource;
-                    winrt::Microsoft::UI::Xaml::Media::Imaging::SvgImageSource source{ iconUri };
-                    iconSource.ImageSource(source);
-                    return iconSource;
-                }
-                else
-                {
-                    typename BitmapIconSource<TIconSource>::type iconSource;
-                    // Make sure to set this to false, so we keep the RGB data of the
-                    // image. Otherwise, the icon will be white for all the
-                    // non-transparent pixels in the image.
-                    iconSource.ShowAsMonochrome(monochrome);
-                    iconSource.UriSource(iconUri);
-                    return iconSource;
-                }
-            }
-            CATCH_LOG();
         }
 
+        try
+        {
+            winrt::Windows::Foundation::Uri iconUri{ path };
+
+            if (til::equals_insensitive_ascii(iconUri.Extension(), L".svg"))
+            {
+                typename ImageIconSource<TIconSource>::type iconSource;
+                winrt::Microsoft::UI::Xaml::Media::Imaging::SvgImageSource source{ iconUri };
+                source.RasterizePixelWidth(static_cast<double>(targetSize));
+                // Set only single dimension here; the image might not be square and 
+                // this will preserve the aspect ratio (for the price of keeping height unbound).
+                // source.RasterizePixelHeight(static_cast<double>(targetSize));
+                iconSource.ImageSource(source);
+                return iconSource;
+            }
+            else
+            {
+                typename ImageIconSource<TIconSource>::type iconSource;
+                winrt::Microsoft::UI::Xaml::Media::Imaging::BitmapImage bitmapImage;
+                bitmapImage.DecodePixelWidth(targetSize);
+                // Set only single dimension here; the image might not be square and
+                // this will preserve the aspect ratio (for the price of keeping height unbound).
+                // bitmapImage.DecodePixelHeight(targetSize);
+                bitmapImage.UriSource(iconUri);
+                iconSource.ImageSource(bitmapImage);
+                return iconSource;
+            }
+        }
+        CATCH_LOG();
+
         return nullptr;
     }
 
@@ -158,14 +167,14 @@ namespace winrt::Microsoft::Terminal::UI::implementation
     // Return Value:
     // - An IconElement with its IconSource set, if possible.
     template<typename TIconSource>
-    TIconSource _getIconSource(const winrt::hstring& iconPath, bool monochrome, const winrt::hstring& fontFamily, const int targetSize)
+    TIconSource _getIconSource(const winrt::hstring& iconPath, const winrt::hstring& fontFamily, const int targetSize)
     {
         TIconSource iconSource{ nullptr };
 
         if (iconPath.size() != 0)
         {
             const auto expandedIconPath{ _expandIconPath(iconPath) };
-            iconSource = _getColoredBitmapIcon<TIconSource>(expandedIconPath, monochrome);
+            iconSource = _getColoredBitmapIcon<TIconSource>(expandedIconPath, targetSize);
 
             // If we fail to set the icon source using the "icon" as a path,
             // let's try it as a symbol/emoji.
@@ -235,9 +244,9 @@ namespace winrt::Microsoft::Terminal::UI::implementation
     //     return _getIconSource<Windows::UI::Xaml::Controls::IconSource>(path, false);
     // }
 
-    static Microsoft::UI::Xaml::Controls::IconSource _IconSourceMUX(const hstring& path, bool monochrome, const winrt::hstring& fontFamily, const int targetSize)
+    static Microsoft::UI::Xaml::Controls::IconSource _IconSourceMUX(const hstring& path, const winrt::hstring& fontFamily, const int targetSize)
     {
-        return _getIconSource<Microsoft::UI::Xaml::Controls::IconSource>(path, monochrome, fontFamily, targetSize);
+        return _getIconSource<Microsoft::UI::Xaml::Controls::IconSource>(path, fontFamily, targetSize);
     }
 
     static SoftwareBitmap _convertToSoftwareBitmap(HICON hicon,
@@ -352,7 +361,6 @@ namespace winrt::Microsoft::Terminal::UI::implementation
     }
 
     MUX::Controls::IconSource IconPathConverter::IconSourceMUX(const winrt::hstring& iconPath,
-                                                               const bool monochrome,
                                                                const winrt::hstring& fontFamily,
                                                                const int targetSize)
     {
@@ -360,7 +368,7 @@ namespace winrt::Microsoft::Terminal::UI::implementation
         const auto indexOpt = _getIconIndex(iconPath, iconPathWithoutIndex);
         if (!indexOpt.has_value())
         {
-            return _IconSourceMUX(iconPath, monochrome, fontFamily, targetSize);
+            return _IconSourceMUX(iconPath, fontFamily, targetSize);
         }
 
         const auto bitmapSource = _getImageIconSourceForBinary(iconPathWithoutIndex, indexOpt.value(), targetSize);
@@ -374,13 +382,14 @@ namespace winrt::Microsoft::Terminal::UI::implementation
     Microsoft::UI::Xaml::Controls::IconElement IconPathConverter::IconMUX(const winrt::hstring& iconPath) {
         return IconMUX(iconPath, 24);
     }
+
     Microsoft::UI::Xaml::Controls::IconElement IconPathConverter::IconMUX(const winrt::hstring& iconPath, const int targetSize)
     {
         std::wstring_view iconPathWithoutIndex;
         const auto indexOpt = _getIconIndex(iconPath, iconPathWithoutIndex);
         if (!indexOpt.has_value())
         {
-            auto source = IconSourceMUX(iconPath, false, L"", targetSize);
+            auto source = IconSourceMUX(iconPath, L"", targetSize);
             Microsoft::UI::Xaml::Controls::IconSourceElement icon;
             icon.IconSource(source);
             return icon;

src/modules/cmdpal/Microsoft.Terminal.UI/IconPathConverter.h

@@ -10,7 +10,7 @@ namespace winrt::Microsoft::Terminal::UI::implementation
 
         //static Windows::UI::Xaml::Controls::IconElement IconWUX(const winrt::hstring& iconPath);
         //static Windows::UI::Xaml::Controls::IconSource IconSourceWUX(const winrt::hstring& iconPath);
-        static Microsoft::UI::Xaml::Controls::IconSource IconSourceMUX(const winrt::hstring& iconPath, bool convertToGrayscale, const winrt::hstring& fontFamily, const int targetSize=24);
+        static Microsoft::UI::Xaml::Controls::IconSource IconSourceMUX(const winrt::hstring& iconPath, const winrt::hstring& fontFamily, const int targetSize=24);
         static Microsoft::UI::Xaml::Controls::IconElement IconMUX(const winrt::hstring& iconPath);
         static Microsoft::UI::Xaml::Controls::IconElement IconMUX(const winrt::hstring& iconPath, const int targetSize);
 

src/modules/cmdpal/Microsoft.Terminal.UI/IconPathConverter.idl

@@ -7,7 +7,7 @@ namespace Microsoft.Terminal.UI
     {
         // static Windows.UI.Xaml.Controls.IconElement IconWUX(String path);
         // static Windows.UI.Xaml.Controls.IconSource IconSourceWUX(String path);
-        static Microsoft.UI.Xaml.Controls.IconSource IconSourceMUX(String path, Boolean convertToGrayscale, String fontFamily, Int32 targetSize);
+        static Microsoft.UI.Xaml.Controls.IconSource IconSourceMUX(String path, String fontFamily, Int32 targetSize);
         static Microsoft.UI.Xaml.Controls.IconElement IconMUX(String path);
         static Microsoft.UI.Xaml.Controls.IconElement IconMUX(String path, Int32 targetSize);
     };