Represent font face as a combo box in SUI by carlos-zamora · Pull Request #9275 · microsoft/terminal

Carlos Zamora (carlos-zamora) · 2021-02-24T22:54:01Z

Summary of the Pull Request

This replaces the Profiles > Font Face text box with a combo box.

References

#6800 - Settings UI Epic

Detailed Description of the Pull Request / Additional comments

Enumerating the fonts
- This doc was the main reference used to enumerate the fonts. It was mildly adapted to use WinRT instead of WRL.
Updating the UI
- Similar to other combo box settings, Profiles keeps a reference to the current value. We use that as a way to update the settings model. If an invalid value is used, we fallback to Cascadia Mono.
- A checkbox was added to let the user select from all of the installed fonts, or just the monospace ones.

Demo

Mike Griese (zadjii-msft) · 2021-02-25T16:00:56Z

With this proposed approach, if the user has a monospaced font selected, maybe I could just set the combobox as "custom" or something? Or create an item for the non-monospaced font? (this should be a rare case)

IMO, if the user has Papyrus in their settings, then we should add that one in the combobox, along with the other monospace fonts. They can see clearly that Papyrus is selected, and they can keep that selection, or pick a sane, monospace one.

Mike Griese (zadjii-msft)

blocking over the memory leak. I think Michael Niksa (@miniksa) should review the "collect all the fonts" bit, since he's the dwrite expert.

Mike Griese (zadjii-msft) · 2021-02-26T12:03:29Z

-                                <TextBox Text="{x:Bind State.Profile.FontFace, Mode=TwoWay}"
-                                         Style="{StaticResource TextBoxSettingStyle}"/>
+                                <ComboBox ItemsSource="{x:Bind FontList, Mode=OneWay}"
+                                          SelectedItem="{x:Bind CurrentFontFace, Mode=TwoWay}"


What happens if CurrentFontFace is something that's not an installed font? Like if I set my fontFace to asdf, what happens then?

So, there's a two fold approach (it's more developed in the upcoming commit):

We have 2 lists: all installed fonts and monospace fonts. If we find that the current font face is not in the list of monospace fonts, assume that it's in the list of all fonts. So we update the UI to force the user to look at all of the installed fonts.

If the user still selected a font that doesn't exist, CurrentFontFace() returns Cascadia Mono. This has to be valid since it is shipped with Windows Terminal.

I'd rather there be an error or warning instead of a whole "it automatically edits the setting if I look at the settings page" :/

Huh, I think we might be ok, but let me run through it out loud to double check.

JSON - invalid font face in settings.json --> show "Cascadia Mono" in SUI:

SUI - limited to autogenerated list

So the only way to get an invalid font is if you modified the JSON. That said, if you then go to SUI, the change won't go into effect until you click on the combo box.

Maybe the behavior should be that we add another "font" called "Invalid Font"? But I feel like this case is fairly rare, and this is more of a request for input validation in SUI. So I'm inclined to say we should punt on this one or throw it in the backlog? Whatcha think?

Carlos Zamora (carlos-zamora) · 2021-02-26T18:59:00Z

@msftbot make sure Michael Niksa (@miniksa) signs off

Deleted user (ghost) · 2021-02-26T18:59:04Z

Hello Carlos Zamora (@carlos-zamora)!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I'll only merge this pull request if it's approved by Michael Niksa (@miniksa)

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

Carlos Zamora (carlos-zamora) · 2021-02-26T19:01:46Z

+  <data name="Profile_FontFaceShowAllFonts.Content" xml:space="preserve">
+    <value>Show all fonts</value>
+  </data>
+  <data name="Profile_FontFaceShowAllFonts.[using:Windows.UI.Xaml.Controls]ToolTipService.ToolTip" xml:space="preserve">
+    <value>If checked, show all installed fonts in the list above. Otherwise, only show the list of monospace fonts.</value>
+  </data>


Kayla Cinnamon (@cinnamon-msft) (or others) feel free to suggest changes for this part.

Michael Niksa (miniksa) · 2021-02-26T23:50:22Z

            </DataTemplate>

+            <DataTemplate x:DataType="x:String" x:Key="FontFaceComboBoxItemTemplate">
+                <TextBlock Text="{x:Bind}" FontFamily="{x:Bind}"/>


Did you double check that this is fine receiving the localized name on other languages? I know you're keeping the localized name in the list for sorting and presentation.... but I have no idea what code TextBlock is calling to turn that localized string into its display font. I just want to make sure you double checked this on at least one non-English scenario.

I don't know how to test this, but I have a feeling that this is definitely going to fail. Working on a solution rn.

Michael Niksa (miniksa)

Good with the DWrite stuff. A few odds and ends comments, but I'll let the other blocks be the definitive ones.

Martin Anderson (mdtauk) · 2021-03-01T22:17:58Z

If it were an Editable ComboBox, that makes it the best of both worlds.

You could also present a text warning, if the FontName read from the JSON, does not match a font available to use.

Carlos Zamora (carlos-zamora) · 2021-03-02T00:25:27Z

If it were an Editable ComboBox, that makes it the best of both worlds.

You could also present a text warning, if the FontName read from the JSON, does not match a font available to use.

My concern with it being editable is that you could choose a font that doesn't exist. Yeah, we could present a warning, but if the list fully encompasses the accepted fonts, I think this is fine?

That said, making it editable would be "the cherry on top", so I'm gonna open a follow-up task for somebody to do that (most of the work would be creating the warning and validation after this PR).

Mike Griese (zadjii-msft)

Uhg I feel conflicted - these feel like nits, but there's enough that I'm gonna block.

Generally looks good to me though

Mike Griese (zadjii-msft) · 2021-03-04T15:46:57Z

+        // generate the font list, if we don't have one
+        if (!_State.Profile().CompleteFontList() || !_State.Profile().MonospaceFontList())
+        {
+            ProfileViewModel::UpdateFontList();


Should we do this here, or should we instead have the font lists initialized in the ProfileViewModel ctor?

actually wait this is done in the ctor. So is this ever hit?

I only have this here in case loading the fonts fails for some reason. The one in the constructor is hit when the Settings UI is opened, but then we check if we have it when we navigate to the profiles page.

Mike Griese (zadjii-msft) · 2021-03-04T16:01:37Z

+            _ShowAllFonts = value;
+            _NotifyChanges(L"ShowAllFonts");
+        }
    }


an aside: almost starting to feel like PVM warrants its own file

agreed. I think this'll be a part of #9207.

Mike Griese (zadjii-msft) · 2021-03-04T16:07:28Z

+                        monospaceFontList.emplace_back(fontEntry);
+                    }
+                }
+                CATCH_LOG();


This isn't gonna log 100 warnings as we iterate over all the fonts, is it?

switched from .as to .try_as. So I think the only one we are really logging here is GetFirstMatchingFont. I think that's fair because we'd want to know which fonts we can't get the IDWriteFont version of?

Deleted user (ghost) · 2021-03-08T16:44:59Z

Hello Mike Griese (@zadjii-msft)!

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.

Deleted user (ghost) · 2021-04-14T17:08:24Z

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

Handy links:

Adam Stachowicz (Saibamen) · 2021-04-14T20:24:55Z

    </data>
-
-    There are any number of "resheader" rows that contain simple
+                


Trailing whitespaces...

Yep. Turns out that Visual Studio will automatically add those trailing spaces to a .resw file pretty much always. That's why every other commit in this repo removes them, then adds them back, depending on what editor the author is using. It really doesn't matter in the end though.

(thanks for the review though? :P)

Represent font face as a combo box in SUI

505d6c1

Mike Griese (zadjii-msft) assigned Michael Niksa (miniksa) Feb 24, 2021

This comment has been minimized.

Sign in to view

limit list to monospace fonts

301a3e6

Carlos Zamora (carlos-zamora) assigned Carlos Zamora (carlos-zamora) and unassigned Michael Niksa (miniksa) Feb 25, 2021

Mike Griese (zadjii-msft) requested changes Feb 26, 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 Feb 26, 2021

polish UI and code

7084404

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

address PR comments

9b8cad0

Carlos Zamora (carlos-zamora) assigned Michael Niksa (miniksa) and Mike Griese (zadjii-msft) and unassigned Carlos Zamora (carlos-zamora) Feb 26, 2021

ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Feb 26, 2021

Carlos Zamora (carlos-zamora) commented Feb 26, 2021

View reviewed changes

Dustin L. Howett (DHowett) removed the AutoMerge Marked for automatic merge by the bot when requirements are met label Feb 26, 2021

accessibility

f62a15b

Dustin L. Howett (DHowett) reviewed Feb 26, 2021

View reviewed changes

Comment thread src/cascadia/TerminalSettingsEditor/Profiles.cpp Resolved

Carlos Zamora added 2 commits February 26, 2021 12:10

fix memory leak

1cb6df1

better fix for memory leak

fb5a18c

Michael Niksa (miniksa) reviewed Feb 26, 2021

View reviewed changes

Comment thread src/cascadia/TerminalSettingsEditor/Profiles.cpp Resolved

Michael Niksa (miniksa) reviewed Feb 26, 2021

View reviewed changes

Comment thread src/cascadia/TerminalSettingsEditor/Profiles.cpp Resolved

Michael Niksa (miniksa) reviewed Feb 26, 2021

View reviewed changes

Michael Niksa (miniksa) approved these changes Feb 26, 2021

View reviewed changes

Carlos Zamora (carlos-zamora) unassigned Michael Niksa (miniksa) Feb 26, 2021

address Michael's comments

6ecf3af

introduce Font WinRT object

0bc12b5

This comment has been minimized.

Sign in to view

Carlos Zamora (carlos-zamora) mentioned this pull request Mar 2, 2021

Make FontFace ComboBox editable #9322

Open

I hate the spellcheck sometimes

8d798b4

Mike Griese (zadjii-msft) requested changes Mar 4, 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 Mar 4, 2021

Mike Griese (zadjii-msft) removed their assignment Mar 4, 2021

Mike Griese (zadjii-msft) added Area-SettingsUI Anything specific to the SUI Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Mar 4, 2021

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

Carlos Zamora added 3 commits March 4, 2021 12:28

address Mike's PR comments

1dd35c5

Merge branch 'main' into dev/cazamor/sui/font-face-combobox

a3cecd1

fix build failure

3a18ea3

Mike Griese (zadjii-msft) self-assigned this Mar 8, 2021

Mike Griese (zadjii-msft) approved these changes Mar 8, 2021

View reviewed changes

Mike Griese (zadjii-msft) added the AutoMerge Marked for automatic merge by the bot when requirements are met label Mar 8, 2021

ghost merged commit 87fa526 into main Mar 8, 2021

ghost deleted the dev/cazamor/sui/font-face-combobox branch March 8, 2021 16:45

Mike Griese (zadjii-msft) mentioned this pull request Mar 30, 2021

Fonts in font face dropdown are not alphabetized #9594

Closed

Adam Stachowicz (Saibamen) reviewed Apr 14, 2021

View reviewed changes

This pull request was closed.

		</data>

		There are any number of "resheader" rows that contain simple

Conversation

Carlos Zamora (carlos-zamora) commented Feb 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of the Pull Request

References

Detailed Description of the Pull Request / Additional comments

Demo

Uh oh!

This comment has been minimized.

Mike Griese (zadjii-msft) commented Feb 25, 2021

Uh oh!

Mike Griese (zadjii-msft) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Carlos Zamora (carlos-zamora) commented Feb 26, 2021

Uh oh!

Deleted user (ghost) commented Feb 26, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Michael Niksa (miniksa) left a comment

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

Martin Anderson (mdtauk) commented Mar 1, 2021

Uh oh!

Carlos Zamora (carlos-zamora) commented Mar 2, 2021

Uh oh!

Mike Griese (zadjii-msft) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Deleted user (ghost) commented Mar 8, 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.

Uh oh!

Deleted user (ghost) commented Apr 14, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Carlos Zamora (carlos-zamora) commented Feb 24, 2021 •

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.