Clean up the Converters implementation (#16820)

I thought the Converters.idl file had a really neat ordering, and I felt like the .cpp implementation fell short of this. This PR reorders the functions in the implementation to match the IDL. It also gets rid of some unnecessary math (int vs. float, clamping) and removes another use of `std::stringstream` (= bad STL class).
microsoft · Mar 18, 2024 · 287422b · 287422b
1 parent a494548
commit 287422b
Show file tree

Hide file tree

Showing 7 changed files with 64 additions and 68 deletions.
diff --git a/src/cascadia/TerminalSettingsEditor/Appearances.idl b/src/cascadia/TerminalSettingsEditor/Appearances.idl
@@ -122,7 +122,6 @@ namespace Microsoft.Terminal.Settings.Editor
         Windows.Foundation.Collections.IObservableVector<Microsoft.Terminal.Settings.Editor.EnumEntry> FontWeightList { get; };
 
         IInspectable CurrentFontFace { get; };
-        Windows.UI.Xaml.Controls.Slider BIOpacitySlider { get; };
 
         IInspectable CurrentIntenseTextStyle;
         Windows.Foundation.Collections.IObservableVector<Microsoft.Terminal.Settings.Editor.EnumEntry> IntenseTextStyleList { get; };

diff --git a/src/cascadia/TerminalSettingsEditor/Appearances.xaml b/src/cascadia/TerminalSettingsEditor/Appearances.xaml
@@ -739,13 +739,12 @@
                         <ColumnDefinition Width="*" />
                         <ColumnDefinition Width="Auto" />
                     </Grid.ColumnDefinitions>
-                    <Slider x:Name="BIOpacitySlider"
-                            x:Uid="Profile_BackgroundImageOpacitySlider"
+                    <Slider x:Uid="Profile_BackgroundImageOpacitySlider"
                             Grid.Column="0"
                             Value="{x:Bind mtu:Converters.PercentageToPercentageValue(Appearance.BackgroundImageOpacity), BindBack=Appearance.SetBackgroundImageOpacityFromPercentageValue, Mode=TwoWay}" />
                     <TextBlock Grid.Column="1"
                                Style="{StaticResource SliderValueLabelStyle}"
-                               Text="{x:Bind mtu:Converters.AppendPercentageSign(BIOpacitySlider.Value), Mode=OneWay}" />
+                               Text="{x:Bind mtu:Converters.PercentageToPercentageString(Appearance.BackgroundImageOpacity), Mode=OneWay}" />
                 </Grid>
             </local:SettingContainer>
         </StackPanel>

diff --git a/src/cascadia/TerminalSettingsEditor/Profiles_Appearance.idl b/src/cascadia/TerminalSettingsEditor/Profiles_Appearance.idl
@@ -10,7 +10,5 @@ namespace Microsoft.Terminal.Settings.Editor
         Profiles_Appearance();
         ProfileViewModel Profile { get; };
         IHostedInWindow WindowRoot { get; };
-
-        Windows.UI.Xaml.Controls.Slider OpacitySlider { get; };
     }
 }
diff --git a/src/cascadia/TerminalSettingsEditor/Profiles_Appearance.xaml b/src/cascadia/TerminalSettingsEditor/Profiles_Appearance.xaml
@@ -76,13 +76,12 @@
                                 <ColumnDefinition Width="*" />
                                 <ColumnDefinition Width="Auto" />
                             </Grid.ColumnDefinitions>
-                            <Slider x:Name="OpacitySlider"
-                                    x:Uid="Profile_OpacitySlider"
+                            <Slider x:Uid="Profile_OpacitySlider"
                                     Grid.Column="0"
                                     Value="{x:Bind mtu:Converters.PercentageToPercentageValue(Profile.Opacity), BindBack=Profile.SetAcrylicOpacityPercentageValue, Mode=TwoWay}" />
                             <TextBlock Grid.Column="1"
                                        Style="{StaticResource SliderValueLabelStyle}"
-                                       Text="{x:Bind mtu:Converters.AppendPercentageSign(OpacitySlider.Value), Mode=OneWay}" />
+                                       Text="{x:Bind mtu:Converters.PercentageToPercentageString(Profile.Opacity), Mode=OneWay}" />
                         </Grid>
                     </StackPanel>
                 </local:SettingContainer>

diff --git a/src/cascadia/UIHelpers/Converters.cpp b/src/cascadia/UIHelpers/Converters.cpp
@@ -7,58 +7,84 @@
 
 namespace winrt::Microsoft::Terminal::UI::implementation
 {
-    winrt::hstring Converters::AppendPercentageSign(double value)
+    // Booleans
+    bool Converters::InvertBoolean(bool value)
     {
-        return to_hstring(gsl::narrow_cast<uint32_t>(std::lrint(value))) + L"%";
+        return !value;
     }
 
-    winrt::Windows::UI::Xaml::Media::SolidColorBrush Converters::ColorToBrush(const winrt::Windows::UI::Color& color)
+    winrt::Windows::UI::Xaml::Visibility Converters::InvertedBooleanToVisibility(bool value)
     {
-        return Windows::UI::Xaml::Media::SolidColorBrush(color);
+        return value ? winrt::Windows::UI::Xaml::Visibility::Collapsed : winrt::Windows::UI::Xaml::Visibility::Visible;
     }
 
-    winrt::Windows::UI::Text::FontWeight Converters::DoubleToFontWeight(double value)
+    // Numbers
+    double Converters::PercentageToPercentageValue(double value)
     {
-        return winrt::Windows::UI::Text::FontWeight{ base::ClampedNumeric<uint16_t>(value) };
+        return value * 100.0;
     }
 
-    double Converters::FontWeightToDouble(const winrt::Windows::UI::Text::FontWeight& fontWeight)
+    double Converters::PercentageValueToPercentage(double value)
     {
-        return fontWeight.Weight;
+        return value / 100.0;
     }
 
-    bool Converters::InvertBoolean(bool value)
+    winrt::hstring Converters::PercentageToPercentageString(double value)
     {
-        return !value;
+        return winrt::hstring{ fmt::format(FMT_COMPILE(L"{:.0f}%"), value * 100.0) };
     }
 
-    winrt::Windows::UI::Xaml::Visibility Converters::InvertedBooleanToVisibility(bool value)
+    // Strings
+    bool Converters::StringsAreNotEqual(const winrt::hstring& expected, const winrt::hstring& actual)
     {
-        return value ? winrt::Windows::UI::Xaml::Visibility::Collapsed : winrt::Windows::UI::Xaml::Visibility::Visible;
+        return expected != actual;
+    }
+
+    winrt::Windows::UI::Xaml::Visibility Converters::StringNotEmptyToVisibility(const winrt::hstring& value)
+    {
+        return value.empty() ? winrt::Windows::UI::Xaml::Visibility::Collapsed : winrt::Windows::UI::Xaml::Visibility::Visible;
+    }
+
+    winrt::hstring Converters::StringOrEmptyIfPlaceholder(const winrt::hstring& placeholder, const winrt::hstring& value)
+    {
+        return placeholder == value ? L"" : value;
+    }
+
+    // Misc
+    winrt::Windows::UI::Text::FontWeight Converters::DoubleToFontWeight(double value)
+    {
+        return winrt::Windows::UI::Text::FontWeight{ base::ClampedNumeric<uint16_t>(value) };
+    }
+
+    winrt::Windows::UI::Xaml::Media::SolidColorBrush Converters::ColorToBrush(const winrt::Windows::UI::Color color)
+    {
+        return Windows::UI::Xaml::Media::SolidColorBrush(color);
+    }
+
+    double Converters::FontWeightToDouble(const winrt::Windows::UI::Text::FontWeight fontWeight)
+    {
+        return fontWeight.Weight;
     }
 
     double Converters::MaxValueFromPaddingString(const winrt::hstring& paddingString)
     {
-        const auto singleCharDelim = L',';
-        std::wstringstream tokenStream(paddingString.c_str());
-        std::wstring token;
+        std::wstring_view remaining{ paddingString };
         double maxVal = 0;
-        size_t* idx = nullptr;
 
         // Get padding values till we run out of delimiter separated values in the stream
         // Non-numeral values detected will default to 0
-        // std::getline will not throw exception unless flags are set on the wstringstream
         // std::stod will throw invalid_argument exception if the input is an invalid double value
         // std::stod will throw out_of_range exception if the input value is more than DBL_MAX
         try
         {
-            while (std::getline(tokenStream, token, singleCharDelim))
+            while (!remaining.empty())
             {
+                const std::wstring token{ til::prefix_split(remaining, L',') };
                 // std::stod internally calls wcstod which handles whitespace prefix (which is ignored)
                 //  & stops the scan when first char outside the range of radix is encountered
                 // We'll be permissive till the extent that stod function allows us to be by default
                 // Ex. a value like 100.3#535w2 will be read as 100.3, but ;df25 will fail
-                const auto curVal = std::stod(token, idx);
+                const auto curVal = std::stod(token);
                 if (curVal > maxVal)
                 {
                     maxVal = curVal;
@@ -74,35 +100,4 @@ namespace winrt::Microsoft::Terminal::UI::implementation
 
         return maxVal;
     }
-
-    int Converters::PercentageToPercentageValue(double value)
-    {
-        return base::ClampMul(value, 100u);
-    }
-
-    double Converters::PercentageValueToPercentage(double value)
-    {
-        return base::ClampDiv<double, double>(value, 100);
-    }
-
-    bool Converters::StringsAreNotEqual(const winrt::hstring& expected, const winrt::hstring& actual)
-    {
-        return expected != actual;
-    }
-    winrt::Windows::UI::Xaml::Visibility Converters::StringNotEmptyToVisibility(const winrt::hstring& value)
-    {
-        return value.empty() ? winrt::Windows::UI::Xaml::Visibility::Collapsed : winrt::Windows::UI::Xaml::Visibility::Visible;
-    }
-
-    // Method Description:
-    // - Returns the value string, unless it matches the placeholder in which case the empty string.
-    // Arguments:
-    // - placeholder - the placeholder string.
-    // - value - the value string.
-    // Return Value:
-    // - The value string, unless it matches the placeholder in which case the empty string.
-    winrt::hstring Converters::StringOrEmptyIfPlaceholder(const winrt::hstring& placeholder, const winrt::hstring& value)
-    {
-        return placeholder == value ? L"" : value;
-    }
 }
diff --git a/src/cascadia/UIHelpers/Converters.h b/src/cascadia/UIHelpers/Converters.h
@@ -9,19 +9,25 @@ namespace winrt::Microsoft::Terminal::UI::implementation
 {
     struct Converters
     {
-        Converters() = default;
-        static winrt::hstring AppendPercentageSign(double value);
-        static winrt::Windows::UI::Text::FontWeight DoubleToFontWeight(double value);
-        static winrt::Windows::UI::Xaml::Media::SolidColorBrush ColorToBrush(const winrt::Windows::UI::Color& color);
-        static double FontWeightToDouble(const winrt::Windows::UI::Text::FontWeight& fontWeight);
+        // Booleans
         static bool InvertBoolean(bool value);
         static winrt::Windows::UI::Xaml::Visibility InvertedBooleanToVisibility(bool value);
-        static double MaxValueFromPaddingString(const winrt::hstring& paddingString);
-        static int PercentageToPercentageValue(double value);
+
+        // Numbers
+        static double PercentageToPercentageValue(double value);
         static double PercentageValueToPercentage(double value);
+        static winrt::hstring PercentageToPercentageString(double value);
+
+        // Strings
         static bool StringsAreNotEqual(const winrt::hstring& expected, const winrt::hstring& actual);
         static winrt::Windows::UI::Xaml::Visibility StringNotEmptyToVisibility(const winrt::hstring& value);
         static winrt::hstring StringOrEmptyIfPlaceholder(const winrt::hstring& placeholder, const winrt::hstring& value);
+
+        // Misc
+        static winrt::Windows::UI::Text::FontWeight DoubleToFontWeight(double value);
+        static winrt::Windows::UI::Xaml::Media::SolidColorBrush ColorToBrush(winrt::Windows::UI::Color color);
+        static double FontWeightToDouble(winrt::Windows::UI::Text::FontWeight fontWeight);
+        static double MaxValueFromPaddingString(const winrt::hstring& paddingString);
     };
 }
 

diff --git a/src/cascadia/UIHelpers/Converters.idl b/src/cascadia/UIHelpers/Converters.idl
@@ -12,16 +12,16 @@ namespace Microsoft.Terminal.UI
         static Windows.UI.Xaml.Visibility InvertedBooleanToVisibility(Boolean value);
 
         // Numbers
-        static Int32 PercentageToPercentageValue(Double value);
+        static Double PercentageToPercentageValue(Double value);
         static Double PercentageValueToPercentage(Double value);
+        static String PercentageToPercentageString(Double value);
 
         // Strings
         static Boolean StringsAreNotEqual(String expected, String actual);
         static Windows.UI.Xaml.Visibility StringNotEmptyToVisibility(String value);
         static String StringOrEmptyIfPlaceholder(String placeholder, String value);
 
         // Misc
-        static String AppendPercentageSign(Double value);
         static Windows.UI.Text.FontWeight DoubleToFontWeight(Double value);
         static Windows.UI.Xaml.Media.SolidColorBrush ColorToBrush(Windows.UI.Color color);
         static Double FontWeightToDouble(Windows.UI.Text.FontWeight fontWeight);