Bind Launch Mode/Size, BI Opacity, Opacity, and FontWeight #8219

carlos-zamora · 2020-11-10T22:53:26Z

Summary of the Pull Request

Properly binds and polishes controls for a few settings in the Settings UI:

LaunchMode is now bound to a radio button
"Launch size" was added and bound as two number boxes
backgroundImageOpacity and acrylicOpacity are now bound to a Slider control
- A PercentageConverter was introduced to convert from decimal range (0.0-1.0) to whole numbers (0-100)
- Common styling was added to CommonResources
fontWeight is now bound to a combination of controls:
- ComboBox is used to display the named values (FontWeights docs)
- A special Custom option is included. When selected, a Slider appears that lets you choose a custom value.
AcrylicOpacity slider only appears if useAcrylic is enabled

References

#1564 - Settings UI

Detailed Description of the Pull Request / Additional comments

bf7d06d is the most complicated commit here. FontWeight is exposed as a FontWeight but is constantly treated as a uint16_t. The following changes were made to handle this:

FontWeightConverter: bind the slider value to the setting
ConversionTrait<FontWeight> stores the values as uint16_t instead of unsigned int
ConversionTrait<FontWeight> accepts a range of 1 to 999 in accordance to FontWeight docs
manually add and track the CustomFontWeight EnumEntry for the ComboBox. A sentinel value of 0 is used
manually implement getter/setter for CurrentFontWeight so that the Slider control is shown/hidden appropriately

Follow-up work for a later PR

The following work items are being added and tracked.

non-alphabetical enum ordering
- FontWeight struggles with this the most. We should order the options from lightest to heaviest.

Demo

src/cascadia/TerminalSettingsEditor/Profiles.cpp

src/cascadia/TerminalSettingsEditor/Profiles.xaml

src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw

DHowett · 2020-11-11T01:33:30Z

src/cascadia/TerminalSettingsModel/EnumMappings.cpp

+    winrt::Windows::Foundation::Collections::IMap<winrt::hstring, uint16_t> EnumMappings::FontWeight()
+    {
+        static IMap<winrt::hstring, uint16_t> enumMap = []() {
+            auto map = single_threaded_map<winrt::hstring, uint16_t>();
+            for (auto [enumStr, enumVal] : JsonUtils::ConversionTrait<Windows::UI::Text::FontWeight>::mappings)
+            {
+                map.Insert(winrt::to_hstring(enumStr), enumVal);
+            }
+            return map;
+        }();
+        return enumMap;
+    }


soooooooo what's up with this

Is it because of how weird FontWeight objects are

Our FontWeight ConversionTrait is weird. It spits out a uint16_t. So I need to manually create this so that I target the FontWeight trait, but store it as a uint16_t.

DHowett · 2020-11-11T01:34:00Z

src/cascadia/TerminalSettingsModel/EnumMappings.h

@@ -37,6 +37,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
        static winrt::Windows::Foundation::Collections::IMap<winrt::hstring, winrt::Microsoft::Terminal::TerminalControl::TextAntialiasingMode> TextAntialiasingMode();
        static winrt::Windows::Foundation::Collections::IMap<winrt::hstring, winrt::Microsoft::Terminal::TerminalControl::CursorStyle> CursorStyle();
        static winrt::Windows::Foundation::Collections::IMap<winrt::hstring, BellStyle> BellStyle();
+        static winrt::Windows::Foundation::Collections::IMap<winrt::hstring, uint16_t> FontWeight();


surprised this is legal!

ghost · 2020-11-18T03:00:08Z

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment.

src/cascadia/TerminalSettingsEditor/BooleanToVisibilityConverter.cpp

zadjii-msft

Okay the code of this looks fine, and the XAML generally looks okay? That's a lot harder to review but the gifs look god

src/cascadia/TerminalSettingsEditor/Launch.xaml

carlos-zamora · 2020-12-01T18:19:21Z

the gifs look god

(I had to hahaha)

DHowett · 2020-12-01T22:37:14Z

src/cascadia/TerminalSettingsEditor/Profiles.cpp

+    IInspectable Profiles::CurrentFontWeight()
+    {
+        // if no value was found, we have a custom value, and we show the slider.
+        const auto maybeEnumEntry{ _FontWeightMap.TryLookup(_State.Profile().FontWeight().Weight) };
+        CustomFontWeightControl().Visibility(maybeEnumEntry ? Visibility::Collapsed : Visibility::Visible);
+        return maybeEnumEntry ? maybeEnumEntry : _CustomFontWeight;


really don't love the fact that the getter for the current font weight also impacts the UI's visibility. That's not what a getter is for.

It makes sense though, no? Because the visibility of the slider is dependent on the value. So we change it when we get the value? Curious what other approach you suggest though

src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw

carlos-zamora · 2020-12-02T18:39:35Z

At some point, Kayla pointed out that "Base Layer" should be "Base layer". It's a super small change so I added it in here.

ghost · 2020-12-03T00:04:02Z

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.

carlos-zamora added 4 commits November 6, 2020 13:17

Bind LaunchMode enum

d469d59

Use slider for BI and Acrylic Opacity

8df2b30

Use slider for Profile.FontWeight

2559c7b

Add FontWeightComboBox (and interact w/ slider)

bf7d06d

carlos-zamora mentioned this pull request Nov 10, 2020

Create converter for fontweight #8099

Closed

6 tasks

code format

58611e3

DHowett reviewed Nov 11, 2020

View reviewed changes

src/cascadia/TerminalSettingsEditor/Profiles.cpp Outdated Show resolved Hide resolved

DHowett reviewed Nov 11, 2020

View reviewed changes

src/cascadia/TerminalSettingsEditor/Profiles.cpp Outdated Show resolved Hide resolved

DHowett requested changes Nov 11, 2020

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 Nov 11, 2020

ghost added the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label Nov 18, 2020

properly format sliders and add visual label

addb3b4

ghost removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. labels Nov 18, 2020

carlos-zamora added 4 commits November 18, 2020 14:38

Merge branch 'feature/settings-ui' into dev/cazamor/sui/bind-launch-mode

444a9ff

add comment for each font weight localization

e7c41c5

address PR comments in non-xaml code

1525bcc

add bind acrylic slider visibility

3ef16c8

DHowett reviewed Nov 19, 2020

View reviewed changes

src/cascadia/TerminalSettingsEditor/BooleanToVisibilityConverter.cpp Outdated Show resolved Hide resolved

remove unnecessary converter

f42737b

carlos-zamora assigned DHowett Nov 19, 2020

carlos-zamora assigned zadjii-msft Nov 30, 2020

carlos-zamora added 2 commits November 30, 2020 14:57

Bind launch size

a7ac47c

Merge branch 'feature/settings-ui' into dev/cazamor/sui/bind-launch-mode

83514e4

carlos-zamora changed the title ~~Bind LaunchMode, BI Opacity, Opacity, and FontWeight~~ Bind Launch Mode/Size, BI Opacity, Opacity, and FontWeight Nov 30, 2020

zadjii-msft approved these changes Dec 1, 2020

View reviewed changes

src/cascadia/TerminalSettingsEditor/Launch.xaml Outdated Show resolved Hide resolved

zadjii-msft removed their assignment Dec 1, 2020

zadjii-msft added the Area-Settings UI Anything specific to the SUI label Dec 1, 2020

change initial size delta to 1 & 10

17519cb

Merge branch 'feature/settings-ui' into dev/cazamor/sui/bind-launch-mode

b8b03e7

DHowett reviewed Dec 1, 2020

View reviewed changes

src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw Outdated Show resolved Hide resolved

DHowett requested changes Dec 1, 2020

View reviewed changes

src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw Outdated Show resolved Hide resolved

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Dec 1, 2020

carlos-zamora assigned carlos-zamora and unassigned DHowett Dec 2, 2020

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

fixed terminology; lowercase 'base layer'

0662419

carlos-zamora assigned DHowett and unassigned carlos-zamora Dec 2, 2020

improve FontWeight visibility logic

9b79935

DHowett approved these changes Dec 2, 2020

View reviewed changes

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

ghost merged commit 5770d23 into feature/settings-ui Dec 3, 2020

ghost deleted the dev/cazamor/sui/bind-launch-mode branch December 3, 2020 00:04

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bind Launch Mode/Size, BI Opacity, Opacity, and FontWeight #8219

Bind Launch Mode/Size, BI Opacity, Opacity, and FontWeight #8219

carlos-zamora commented Nov 10, 2020 •

edited

Loading

DHowett Nov 11, 2020

DHowett Nov 11, 2020

carlos-zamora Nov 18, 2020

DHowett Nov 11, 2020

ghost commented Nov 18, 2020

zadjii-msft left a comment

carlos-zamora commented Dec 1, 2020

DHowett Dec 1, 2020

carlos-zamora Dec 2, 2020

carlos-zamora commented Dec 2, 2020

ghost commented Dec 3, 2020

Bind Launch Mode/Size, BI Opacity, Opacity, and FontWeight #8219

Bind Launch Mode/Size, BI Opacity, Opacity, and FontWeight #8219

Conversation

carlos-zamora commented Nov 10, 2020 • edited Loading

Summary of the Pull Request

References

Detailed Description of the Pull Request / Additional comments

Follow-up work for a later PR

Demo

DHowett Nov 11, 2020

Choose a reason for hiding this comment

DHowett Nov 11, 2020

Choose a reason for hiding this comment

carlos-zamora Nov 18, 2020

Choose a reason for hiding this comment

DHowett Nov 11, 2020

Choose a reason for hiding this comment

ghost commented Nov 18, 2020

zadjii-msft left a comment

Choose a reason for hiding this comment

carlos-zamora commented Dec 1, 2020

DHowett Dec 1, 2020

Choose a reason for hiding this comment

carlos-zamora Dec 2, 2020

Choose a reason for hiding this comment

carlos-zamora commented Dec 2, 2020

ghost commented Dec 3, 2020

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.

carlos-zamora commented Nov 10, 2020 •

edited

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.