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

marcelwgn · 2021-08-01T22:20:18Z

Validation Steps Performed

Clicked around, validated that settings still behave the same (as far as
I can tell with my limited terminal configuration expertise)

Closes #10387

marcelwgn · 2021-08-01T22:20:58Z

OpenConsole.sln

+# Visual Studio Version 17
+VisualStudioVersion = 17.0.31410.414


If desired, I can also revert these changes.

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. 😅

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

carlos-zamora

Thanks for doing this! Big fan! haha

src/cascadia/TerminalSettingsEditor/Converters.h

carlos-zamora · 2021-08-02T16:57:11Z

src/cascadia/TerminalSettingsEditor/Microsoft.Terminal.Settings.Editor.vcxproj.filters

@@ -17,9 +17,6 @@
  <ItemGroup>
    <ClInclude Include="pch.h" />
    <ClInclude Include="Utils.h" />
-    <ClInclude Include="PercentageSignConverter.h">


You can probably also...

remove the reference to PercentageSignConverter.cpp on line 13 above

remove the Converters filter on line 50 below

Good point! Fixed that now.

carlos-zamora · 2021-08-02T17:00:01Z

src/cascadia/TerminalSettingsEditor/Microsoft.Terminal.Settings.Editor.vcxproj

@@ -330,7 +272,6 @@
    <ProjectReference Include="$(OpenConsoleDir)src\cascadia\TerminalConnection\TerminalConnection.vcxproj">
      <Private>false</Private>
    </ProjectReference>
-


nit: mind adding in these empty lines again?

src/cascadia/TerminalSettingsEditor/Converters.idl

carlos-zamora · 2021-08-02T17:28:57Z

src/cascadia/TerminalSettingsEditor/Appearances.idl

@@ -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; };


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

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.

carlos-zamora · 2021-08-02T17:42:21Z

src/cascadia/TerminalSettingsEditor/Appearances.cpp

@@ -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())


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

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

src/cascadia/TerminalSettingsEditor/Profiles.h

carlos-zamora · 2021-08-02T17:51:59Z

src/cascadia/TerminalSettingsEditor/Appearances.xaml

@@ -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}" />


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?

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.

src/cascadia/TerminalSettingsModel/GlobalAppSettings.h

src/cascadia/TerminalSettingsEditor/GlobalAppearance.xaml

Co-authored-by: Carlos Zamora <carlos.zamora@microsoft.com>

carlos-zamora

Thank you!!

marcelwgn · 2021-08-02T20:49:07Z

Thank you!!

Always happy to help!

zadjii-msft

You know, I think I'm okay with the desktopWallpaper thing. That's really not so bad, and the rest of this is all goodness. Thanks for doing this!

zadjii-msft · 2021-08-03T14:33:12Z

src/cascadia/TerminalSettingsEditor/Appearances.xaml

                                 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}" />


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 😋)

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.

DHowett

So much happier with this than what we had before. Excellent work @chingucoding!

ghost · 2021-08-03T22:25:00Z

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

marcelwgn · 2021-08-03T22:29:01Z

Thank you @DHowett !@zadjii-msft, should I create a follow up PR in the next few days to fix the behavior you noticed?

ghost · 2021-08-31T17:10:18Z

🎉Windows Terminal Preview v1.11.2421.0 has been released which incorporates this pull request.:tada:

Handy links:

marcelwgn added 6 commits June 9, 2021 00:25

Add percentage sign to opacity slider values

3fee481

Switch to function binding instead of Converter objects

3ea94fb

Merge main

9030618

Merge main

dfbaa36

Fix build errors and bugs and stuff

2e2b9b5

Fix missing bindbacks

4bd044b

marcelwgn commented Aug 1, 2021

View reviewed changes

Fix formatting

3e34e5d

carlos-zamora requested changes Aug 2, 2021

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 2, 2021

Update src/cascadia/TerminalSettingsEditor/Converters.h

92e7fa0

Co-authored-by: Carlos Zamora <carlos.zamora@microsoft.com>

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 2, 2021

marcelwgn and others added 6 commits August 2, 2021 21:26

Update src/cascadia/TerminalSettingsEditor/Appearances.xaml

b2f50b8

Co-authored-by: Carlos Zamora <carlos.zamora@microsoft.com>

Update src/cascadia/TerminalSettingsEditor/Profiles.h

7e9c7af

Co-authored-by: Carlos Zamora <carlos.zamora@microsoft.com>

Update src/cascadia/TerminalSettingsEditor/Converters.idl

d69429c

Co-authored-by: Carlos Zamora <carlos.zamora@microsoft.com>

Update src/cascadia/TerminalSettingsModel/GlobalAppSettings.h

363a310

Co-authored-by: Carlos Zamora <carlos.zamora@microsoft.com>

Update src/cascadia/TerminalSettingsEditor/GlobalAppearance.xaml

2583135

Co-authored-by: Carlos Zamora <carlos.zamora@microsoft.com>

CR Feedback

3e5aad1

carlos-zamora approved these changes Aug 2, 2021

View reviewed changes

zadjii-msft approved these changes Aug 3, 2021

View reviewed changes

DHowett approved these changes Aug 3, 2021

View reviewed changes

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Aug 3, 2021

ghost merged commit 8ab3422 into microsoft:main Aug 3, 2021

marcelwgn deleted the dev/switch-to-converter-functions branch August 3, 2021 22:26

ghost mentioned this pull request Aug 31, 2021

Replace class-based XAML converters with bound function converters #10387

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

marcelwgn commented Aug 1, 2021 •

edited by DHowett

Loading

marcelwgn Aug 1, 2021

lhecker Aug 2, 2021

marcelwgn Aug 2, 2021

carlos-zamora left a comment

carlos-zamora Aug 2, 2021

marcelwgn Aug 2, 2021

carlos-zamora Aug 2, 2021

carlos-zamora Aug 2, 2021

marcelwgn Aug 2, 2021

carlos-zamora Aug 2, 2021

marcelwgn Aug 2, 2021

carlos-zamora Aug 2, 2021

marcelwgn Aug 2, 2021

carlos-zamora left a comment

marcelwgn commented Aug 2, 2021

zadjii-msft left a comment

zadjii-msft Aug 3, 2021

zadjii-msft Aug 3, 2021

DHowett left a comment

ghost commented Aug 3, 2021

marcelwgn commented Aug 3, 2021

ghost commented Aug 31, 2021

		# Visual Studio Version 17
		VisualStudioVersion = 17.0.31410.414

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

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

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

Conversation

marcelwgn commented Aug 1, 2021 • edited by DHowett Loading

Validation Steps Performed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlos-zamora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlos-zamora left a comment

Choose a reason for hiding this comment

marcelwgn commented Aug 2, 2021

zadjii-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

ghost commented Aug 3, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

marcelwgn commented Aug 3, 2021

ghost commented Aug 31, 2021

marcelwgn commented Aug 1, 2021 •

edited by DHowett

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.