Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[settings-editor] Switch to function bindings instead of Converter objects #10846

Merged
14 commits merged into from
Aug 3, 2021
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions OpenConsole.sln
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@

Microsoft Visual Studio Solution File, Format Version 12.00
# Visual Studio Version 16
VisualStudioVersion = 16.0.29001.49
# Visual Studio Version 17
VisualStudioVersion = 17.0.31410.414
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If desired, I can also revert these changes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, if you could revert this change specifically, that would be nice.
I'm running into the same problem all the time, as I'm also using VS2022 already. 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted them now. Good to hear though, I'm not the only running into this 😄

MinimumVisualStudioVersion = 10.0.40219.1
Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Terminal", "Terminal", "{59840756-302F-44DF-AA47-441A9D673202}"
EndProject
Expand Down
7 changes: 3 additions & 4 deletions src/cascadia/TerminalSettingsEditor/Actions.xaml
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,6 @@

<!-- Converters & Misc. -->
<model:IconPathConverter x:Key="IconSourceConverter" />
<local:InvertedBooleanToVisibilityConverter x:Key="InvertedBooleanToVisibilityConverter" />
<SolidColorBrush x:Key="ActionContainerBackgroundEditing"
Color="{ThemeResource SystemListMediumColor}" />
<SolidColorBrush x:Key="ActionContainerBackground"
Expand Down Expand Up @@ -195,7 +194,7 @@
<TextBlock Grid.Column="0"
Style="{StaticResource KeyBindingNameTextBlockStyle}"
Text="{x:Bind Name, Mode=OneWay}"
Visibility="{x:Bind IsInEditMode, Mode=OneWay, Converter={StaticResource InvertedBooleanToVisibilityConverter}}" />
Visibility="{x:Bind local:Converters.InvertedBooleanToVisibility(IsInEditMode), Mode=OneWay}" />

<!-- Edit Mode: Action Combo-box -->
<ComboBox x:Uid="Actions_ActionComboBox"
Expand All @@ -211,7 +210,7 @@
HorizontalAlignment="Right"
VerticalAlignment="Center"
Style="{ThemeResource KeyChordBorderStyle}"
Visibility="{x:Bind IsInEditMode, Mode=OneWay, Converter={StaticResource InvertedBooleanToVisibilityConverter}}">
Visibility="{x:Bind local:Converters.InvertedBooleanToVisibility(IsInEditMode), Mode=OneWay}">

<TextBlock FontSize="14"
Style="{ThemeResource KeyChordTextBlockStyle}"
Expand Down Expand Up @@ -303,7 +302,7 @@
Margin="8,0,0,0"
AutomationProperties.Name="{x:Bind DeleteButtonName}"
Style="{StaticResource EditButtonStyle}"
Visibility="{x:Bind IsNewlyAdded, Mode=OneWay, Converter={StaticResource InvertedBooleanToVisibilityConverter}}">
Visibility="{x:Bind local:Converters.InvertedBooleanToVisibility(IsNewlyAdded), Mode=OneWay}">
<Button.Content>
<FontIcon FontSize="{StaticResource EditButtonIconSize}"
Glyph="&#xE74D;" />
Expand Down
9 changes: 6 additions & 3 deletions src/cascadia/TerminalSettingsEditor/Appearances.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -139,11 +139,14 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
{
bool result{ false };
const auto currentFont{ Appearance().FontFace() };
for (const auto& font : SourceProfile().MonospaceFontList())
if (!SourceProfile())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait... isn't this backwards? Don't we want to make sure SourceProfile is populated because we're using it on the line below?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That code change was not supposed to get into this PR, sorry about that ^^

{
if (font.LocalizedName() == currentFont)
for (const auto& font : SourceProfile().MonospaceFontList())
{
result = true;
if (font.LocalizedName() == currentFont)
{
result = true;
}
}
}
return result;
Expand Down
13 changes: 13 additions & 0 deletions src/cascadia/TerminalSettingsEditor/Appearances.h
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,19 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
public:
AppearanceViewModel(const Model::AppearanceConfig& appearance);

void SetFontWeightFromDouble(double fontWeight)
{
FontWeight(winrt::Microsoft::Terminal::Settings::Editor::Converters::DoubleToFontWeight(fontWeight));
}
void SetBackgroundImageOpacityFromPercentageValue(double percentageValue)
{
BackgroundImageOpacity(winrt::Microsoft::Terminal::Settings::Editor::Converters::PercentageValueToPercentage(percentageValue));
}
void SetBackgroundImagePath(winrt::hstring path)
{
BackgroundImagePath(path);
}

// background image
bool UseDesktopBGImage();
void UseDesktopBGImage(const bool useDesktop);
Expand Down
5 changes: 5 additions & 0 deletions src/cascadia/TerminalSettingsEditor/Appearances.idl
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@ namespace Microsoft.Terminal.Settings.Editor
{
Boolean IsDefault;

void SetFontWeightFromDouble(Double fontWeight);
void SetBackgroundImageOpacityFromPercentageValue(Double percentageValue);
void SetBackgroundImagePath(String path);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't you just use the setter BackgroundImagePath() directly? It's introduced by the OBSERVABLE_PROJECTED_APPEARANCE_SETTING macro.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That does not work, XAML Compiler gives us the error:

XamlCompiler error WMC1121: Invalid binding assignment : BindBack must point to a method

I'm afraid there really isn't a way around these bindback functions :/


Boolean UseDesktopBGImage;
Boolean BackgroundImageSettingsVisible { get; };

Expand Down Expand Up @@ -68,5 +72,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; };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait... is this just a way to get access to the element named BIOpacitySlider? 🤯

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly! Since we need to reference the control from XAML and the XAML Compiler only can access controls that are exposed through the winmd/metadata, we need to define them in IDL since otherwise they won't be available to the metadata provider.

}
}
24 changes: 7 additions & 17 deletions src/cascadia/TerminalSettingsEditor/Appearances.xaml
Original file line number Diff line number Diff line change
Expand Up @@ -34,16 +34,6 @@
<TextBlock FontFamily="{x:Bind Name}"
Text="{x:Bind LocalizedName}" />
</DataTemplate>

<local:ColorToBrushConverter x:Key="ColorToBrushConverter" />
<local:PercentageConverter x:Key="PercentageConverter" />
<local:PercentageSignConverter x:Key="PercentageSignConverter" />
<local:FontWeightConverter x:Key="FontWeightConverter" />
<local:InvertedBooleanToVisibilityConverter x:Key="InvertedBooleanToVisibilityConverter" />
<local:StringIsEmptyConverter x:Key="StringIsEmptyConverter" />
<local:PaddingConverter x:Key="PaddingConverter" />
<local:StringIsNotDesktopConverter x:Key="StringIsNotDesktopConverter" />
<local:DesktopWallpaperToEmptyStringConverter x:Key="DesktopWallpaperToEmptyStringConverter" />
</ResourceDictionary>
</UserControl.Resources>

Expand Down Expand Up @@ -87,7 +77,7 @@
SelectedItem="{x:Bind CurrentFontFace, Mode=OneWay}"
SelectionChanged="FontFace_SelectionChanged"
Style="{StaticResource ComboBoxSettingStyle}"
Visibility="{x:Bind ShowAllFonts, Mode=OneWay, Converter={StaticResource InvertedBooleanToVisibilityConverter}}" />
Visibility="{x:Bind local:Converters.InvertedBooleanToVisibility(ShowAllFonts), Mode=OneWay}" />
<ComboBox ItemTemplate="{StaticResource FontFaceComboBoxItemTemplate}"
ItemsSource="{x:Bind SourceProfile.CompleteFontList, Mode=OneWay}"
SelectedItem="{x:Bind CurrentFontFace, Mode=OneWay}"
Expand Down Expand Up @@ -144,7 +134,7 @@
Minimum="0"
TickFrequency="50"
TickPlacement="Outside"
Value="{x:Bind Appearance.FontWeight, Converter={StaticResource FontWeightConverter}, Mode=TwoWay}" />
Value="{x:Bind local:Converters.FontWeightToDouble(Appearance.FontWeight), BindBack=Appearance.SetFontWeightFromDouble, Mode=TwoWay}" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Value="{x:Bind local:Converters.FontWeightToDouble(Appearance.FontWeight), BindBack=Appearance.SetFontWeightFromDouble, Mode=TwoWay}" />
Value="{x:Bind Appearance.FontWeight.Weight, BindBack=Appearance.SetFontWeightFromDouble, Mode=TwoWay}" />

Can we extract the Weight directly and get rid of the converter entirely?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, this doesn't seem to work since two way binding either wants to bind to a property for both directions or have a function for both directions. Having a property for one direction (as would be the case with simply using Appearance.FontWeight.Weight) and a BindBack for the other direction does not seem to be supported.

<TextBlock Grid.Column="1"
Margin="10,0,0,0"
Style="{StaticResource SliderValueLabelStyle}"
Expand Down Expand Up @@ -215,12 +205,12 @@
SettingOverrideSource="{x:Bind Appearance.BackgroundImagePathOverrideSource, Mode=OneWay}">
<StackPanel Orientation="Vertical">
<StackPanel Orientation="Horizontal">
<TextBox IsEnabled="{x:Bind Appearance.BackgroundImagePath, Mode=OneWay, Converter={StaticResource StringIsNotDesktopConverter}}"
<TextBox IsEnabled="{x:Bind local:Converters.StringsAreNotEqual('desktopWallpaper', Appearance.BackgroundImagePath), Mode=OneWay}"
Style="{StaticResource TextBoxSettingStyle}"
Text="{x:Bind Appearance.BackgroundImagePath, Mode=TwoWay, Converter={StaticResource DesktopWallpaperToEmptyStringConverter}}" />
Text="{x:Bind local:Converters.StringFallBackToEmptyString('desktopWallpaper', Appearance.BackgroundImagePath), Mode=TwoWay, BindBack=Appearance.SetBackgroundImagePath}" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay so this TextBox is enabled when the BackgroundImagePath != 'desktopWallpaper'. When the BackgroundImagePath=='desktopWallpaper', StringFallBackToEmptyString will convert that to desktopWallpaper. When the string is anything else, it'll bind the text as the empty string? Is this right? This might need some comments adjacent to it to help explain what's going on here (though admittedly I didn't leave any behind when I wrote this the first time 😋)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think right now we're actually filling the box with the empty string, but still enabling the rest of background image settings. This PR updates it to actually fill the box with desktopWallpaper, which is a little weird, but probably okay? Everything else still seems to work fine.

<Button x:Uid="Profile_BackgroundImageBrowse"
Click="BackgroundImage_Click"
IsEnabled="{x:Bind Appearance.BackgroundImagePath, Mode=OneWay, Converter={StaticResource StringIsNotDesktopConverter}}"
IsEnabled="{x:Bind local:Converters.StringsAreNotEqual('desktopWallpaper', Appearance.BackgroundImagePath), Mode=OneWay}"
Style="{StaticResource BrowseButtonStyle}" />
</StackPanel>
<CheckBox x:Name="UseDesktopImageCheckBox"
Expand Down Expand Up @@ -431,10 +421,10 @@
</Grid.ColumnDefinitions>
<Slider x:Name="BIOpacitySlider"
Grid.Column="0"
Value="{x:Bind Appearance.BackgroundImageOpacity, Converter={StaticResource PercentageConverter}, Mode=TwoWay}" />
Value="{x:Bind local:Converters.PercentageToPercentageValue(Appearance.BackgroundImageOpacity), BindBack=Appearance.SetBackgroundImageOpacityFromPercentageValue, Mode=TwoWay}" />
<TextBlock Grid.Column="1"
Style="{StaticResource SliderValueLabelStyle}"
Text="{Binding ElementName=BIOpacitySlider, Path=Value, Mode=OneWay, Converter={StaticResource PercentageSignConverter}}" />
Text="{x:Bind local:Converters.AppendPercentageSign(BIOpacitySlider.Value), Mode=OneWay}" />
</Grid>
</local:SettingContainer>
</StackPanel>
Expand Down
33 changes: 0 additions & 33 deletions src/cascadia/TerminalSettingsEditor/ColorLightenConverter.cpp

This file was deleted.

9 changes: 0 additions & 9 deletions src/cascadia/TerminalSettingsEditor/ColorLightenConverter.h

This file was deleted.

13 changes: 4 additions & 9 deletions src/cascadia/TerminalSettingsEditor/ColorSchemes.xaml
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@
<DataTemplate x:Key="ColorTableEntryTemplate"
x:DataType="local:ColorTableEntry">
<Button AutomationProperties.Name="{x:Bind Name}"
Background="{x:Bind Color, Converter={StaticResource ColorToBrushConverter}, Mode=OneWay}"
Background="{x:Bind local:Converters.ColorToBrush(Color), Mode=OneWay}"
Style="{StaticResource ColorButtonStyle}"
ToolTipService.ToolTip="{x:Bind Name}">
<Button.Resources>
Expand All @@ -69,11 +69,11 @@
<ResourceDictionary.ThemeDictionaries>
<ResourceDictionary x:Key="Light">
<SolidColorBrush x:Key="ButtonBackgroundPointerOver"
Color="{x:Bind Color, Converter={StaticResource ColorLightenConverter}, Mode=OneWay}" />
Color="{x:Bind local:Converters.LightenColor(Color), Mode=OneWay}" />
</ResourceDictionary>
<ResourceDictionary x:Key="Dark">
<SolidColorBrush x:Key="ButtonBackgroundPointerOver"
Color="{x:Bind Color, Converter={StaticResource ColorLightenConverter}, Mode=OneWay}" />
Color="{x:Bind local:Converters.LightenColor(Color), Mode=OneWay}" />
</ResourceDictionary>
<!-- No High contrast dictionary, let's just leave that unchanged. -->
</ResourceDictionary.ThemeDictionaries>
Expand All @@ -90,11 +90,6 @@
</Button>
</DataTemplate>

<local:ColorToBrushConverter x:Key="ColorToBrushConverter" />
<local:ColorToHexConverter x:Key="ColorToHexConverter" />
<local:InvertedBooleanToVisibilityConverter x:Key="InvertedBooleanToVisibilityConverter" />
<local:ColorLightenConverter x:Key="ColorLightenConverter" />

</ResourceDictionary>
</Page.Resources>

Expand All @@ -106,7 +101,7 @@
<StackPanel Orientation="Horizontal">

<StackPanel Orientation="Horizontal"
Visibility="{x:Bind IsRenaming, Converter={StaticResource InvertedBooleanToVisibilityConverter}, Mode=OneWay}">
Visibility="{x:Bind local:Converters.InvertedBooleanToVisibility(IsRenaming), Mode=OneWay}">
<!-- Select a color scheme -->
<ComboBox x:Name="ColorSchemeComboBox"
ItemsSource="{x:Bind ColorSchemeList, Mode=OneWay}"
Expand Down
28 changes: 0 additions & 28 deletions src/cascadia/TerminalSettingsEditor/ColorToBrushConverter.cpp

This file was deleted.

30 changes: 0 additions & 30 deletions src/cascadia/TerminalSettingsEditor/ColorToBrushConverter.h

This file was deleted.

30 changes: 0 additions & 30 deletions src/cascadia/TerminalSettingsEditor/ColorToHexConverter.cpp

This file was deleted.

Loading