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.

Sign up for GitHub

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

Jump to bottom

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

Merged
14 commits merged into from
Aug 3, 2021
Merged

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

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
3fee481
Add percentage sign to opacity slider values
marcelwgn Jun 8, 2021
3ea94fb
Switch to function binding instead of Converter objects
marcelwgn Aug 1, 2021
9030618
Merge main
marcelwgn Aug 1, 2021
dfbaa36
Merge main
marcelwgn Aug 1, 2021
2e2b9b5
Fix build errors and bugs and stuff
marcelwgn Aug 1, 2021
4bd044b
Fix missing bindbacks
marcelwgn Aug 1, 2021
3e34e5d
Fix formatting
marcelwgn Aug 1, 2021
92e7fa0
Update src/cascadia/TerminalSettingsEditor/Converters.h
marcelwgn Aug 2, 2021
b2f50b8
Update src/cascadia/TerminalSettingsEditor/Appearances.xaml
marcelwgn Aug 2, 2021
7e9c7af
Update src/cascadia/TerminalSettingsEditor/Profiles.h
marcelwgn Aug 2, 2021
d69429c
Update src/cascadia/TerminalSettingsEditor/Converters.idl
marcelwgn Aug 2, 2021
363a310
Update src/cascadia/TerminalSettingsModel/GlobalAppSettings.h
marcelwgn Aug 2, 2021
2583135
Update src/cascadia/TerminalSettingsEditor/GlobalAppearance.xaml
marcelwgn Aug 2, 2021
3e5aad1
CR Feedback
marcelwgn Aug 2, 2021
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop

This file was deleted.

9 changes: 0 additions & 9 deletions src/cascadia/TerminalSettingsEditor/ColorLightenConverter.h
View file
Open in desktop

This file was deleted.

13 changes: 4 additions & 9 deletions src/cascadia/TerminalSettingsEditor/ColorSchemes.xaml
View file
Open in desktop
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
View file
Open in desktop

This file was deleted.

30 changes: 0 additions & 30 deletions src/cascadia/TerminalSettingsEditor/ColorToBrushConverter.h
View file
Open in desktop

This file was deleted.

30 changes: 0 additions & 30 deletions src/cascadia/TerminalSettingsEditor/ColorToHexConverter.cpp
View file
Open in desktop

This file was deleted.

Loading