Make font name comparison case-insensitive #9494

eugenesmlv · 2021-03-14T17:37:45Z

Make font name comparison case-insensitive, so a warning message won't
be displayed when settings contain a font name with the wrong case.

Pros:

The warning message isn't shown.

Cons:

If one clicks the Save button on SUI, the fallback font will be
applied. That happens because the selected font face in SUI is
fallback font.

Should we also silently change the font face in the settings in that case?

Fixes #9360

DHowett · 2021-03-14T19:41:56Z

src/cascadia/TerminalControl/TermControl.cpp

+        const auto actualFontName = _actualFont.GetFaceName();
+        const auto desiredFontName = _desiredFont.GetFaceName();
+
+        auto caseInsensitiveEquality = [](wchar_t a, wchar_t b) { return ::towlower(a) == ::towlower(b); };


Worried that this uses the user's locale for a not-locale-specific font name. Maybe we can use CompareStringOrdinal with bIgnoreCase=TRUE to get around that?

DHowett

Thanks!

DHowett · 2021-03-15T19:46:10Z

@msftbot make sure @miniksa signs off on this

ghost · 2021-03-15T19:46:15Z

Hello @DHowett!

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 @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".

DHowett · 2021-03-15T19:47:37Z

When you say that the fallback font is displayed in the settings UI, do you mean that it's not selected in the dropdown box because it can't be found in the list (case sensitively)? @carlos-zamora -- can you point @eugenesmlv to the right place?

carlos-zamora · 2021-03-15T20:04:50Z

When you say that the fallback font is displayed in the settings UI, do you mean that it's not selected in the dropdown box because it can't be found in the list (case sensitively)? @carlos-zamora -- can you point @eugenesmlv to the right place?

@eugenesmlv If fallback is happening for settings UI, you may want to do the same kind of comparison here

DHowett · 2021-03-15T20:06:13Z

Oh no, we're using the localized name there. I might have led you astray. Carlos, do you know if we use localized or raw font names when looking up fonts? This could .. be .. important ...

eugenesmlv · 2021-03-15T20:13:49Z

@DHowett yes, I do. When I open the settings editor the profileFontFace in CurrentFontFace method is set to the value from settings.json. I've tried to call _settings.FontFace(actualFontName) after the string comparison, but it doesn't help.

terminal/src/cascadia/TerminalSettingsEditor/Profiles.cpp

Lines 429 to 442 in 99b09c0

    
           for (const auto& font : currentFontList) 
        
           { 
        
               if (font.LocalizedName() == profileFontFace) 
        
               { 
        
                   return box_value(font); 
        
               } 
        
               else if (font.LocalizedName() == L"Cascadia Mono") 
        
               { 
        
                   fallbackFont = box_value(font); 
        
               } 
        
           } 
        
           // we couldn't find the desired font, set to "Cascadia Mono" since that ships by default 
        
           return fallbackFont;

carlos-zamora · 2021-03-15T20:19:11Z

Oh no, we're using the localized name there. I might have led you astray. Carlos, do you know if we use localized or raw font names when looking up fonts? This could .. be .. important ...

Unfortunately, I actually don't know. I tracked it down to the renderer here:

terminal/src/renderer/dx/DxFontRenderData.cpp

Line 643 in 99b09c0

    
           THROW_IF_FAILED(fontCollection->FindFamilyName(familyName.data(), &familyIndex, &familyExists));

And the docs for IDWriteFontCollection::FindFamilyName don't say anything on it, it seems:

https://docs.microsoft.com/en-us/windows/win32/api/dwrite/nf-dwrite-idwritefontcollection-findfamilyname

miniksa · 2021-03-15T20:21:56Z

I'll look behind the FindFamilyName interface and see where it's pulling from...

DHowett · 2021-03-15T20:22:14Z

@miniksa this is really gonna ruin your day.

miniksa · 2021-03-15T20:22:36Z

@miniksa this is really gonna ruin your day.

NNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNN

miniksa · 2021-03-15T20:26:34Z

I'll look behind the FindFamilyName interface and see where it's pulling from...

It actually contains all names in that dictionary but will yield the en-us one first if one exists... UGH.

DHowett · 2021-03-15T20:28:02Z

We might have a couple things to do here.

Control/Warning: We probably cannot take the "easy way out" with the font name comparison any more. We need the DX Engine to tell TermControl that fallback took place. That's the only way to know if we actually hit the provided fallback table.
Settings UI: We probably need to store every localized name (or maybe just the current localized name AND the base unlocalized name) in the SUI ViewModel Font object so that we can map the user's request into an actual font name.

miniksa · 2021-03-15T20:32:34Z

To get the list of names... we have to...

IDWriteFontFamilyCollection::FindFamilyName which will give us the UINT32 of the index of the font family in that collection that matches the name. It will search its internal dictionary which contains a map of all the names it knows of to the indexes. And as an example both MS Gothic and MS　ゴシック should yield the same index.

Then we have to do IDWriteFontFamilyCollection::GetFontFamily with the index to get a IDWriteFontFamily.

Then IDWriteFontFamily::GetFamilyNames to get all the names that it knows about in all languages as IDWriteLocalizedStrings.

Then we need to loop IDWriteLocalizedStrings::GetStringLength and IDWriteLocalizedStrings::GetString for the count of IDWriteLocalizedStrings::GetCount to get all the possible names the family has in all locales, using ::GetLocaleName and ::GetLocaleNameLength if we want to know the matching locale name too.

DHowett · 2021-03-15T20:34:03Z

That'll be good for 2, the Settings UI work. It won't help us much for 1, the "we need to actually know about fallback instead of guessing based on the name" work.

miniksa · 2021-03-15T20:34:29Z

k let me dig to see if there's a better way to know of the fallback next.

carlos-zamora · 2021-03-15T20:36:21Z

@DHowett

Settings UI: We probably need to store every localized name (or maybe just the current localized name AND the base unlocalized name) in the SUI ViewModel Font object so that we can map the user's request into an actual font name.

We already store the current localized name and the base unlocalized name in Editor::Font. However, we only compare what the user wrote to the localized name (see below).

terminal/src/cascadia/TerminalSettingsEditor/Profiles.cpp

Lines 429 to 442 in 99b09c0

    
           for (const auto& font : currentFontList) 
        
           { 
        
               if (font.LocalizedName() == profileFontFace) 
        
               { 
        
                   return box_value(font); 
        
               } 
        
               else if (font.LocalizedName() == L"Cascadia Mono") 
        
               { 
        
                   fallbackFont = box_value(font); 
        
               } 
        
           } 
        
           // we couldn't find the desired font, set to "Cascadia Mono" since that ships by default 
        
           return fallbackFont;

Should we just make that look something like this then:

 for (const auto& font : currentFontList) 
 { 
     // GH#XXXX: check localized name and base name
     if (font.LocalizedName() == profileFontFace || font.Name() == profileFontFace) 
     { 
         return box_value(font); 
     } 
     else if (font.LocalizedName() == L"Cascadia Mono" || font.Name() == L"Cascadia Mono") 
     { 
         fallbackFont = box_value(font); 
     } 
 }

miniksa · 2021-03-15T20:37:40Z

@DHowett

Settings UI: We probably need to store every localized name (or maybe just the current localized name AND the base unlocalized name) in the SUI ViewModel Font object so that we can map the user's request into an actual font name.

We already store the current localized name and the base unlocalized name in Editor::Font. However, we only compare what the user wrote to the localized name (see below).

terminal/src/cascadia/TerminalSettingsEditor/Profiles.cpp

Lines 429 to 442 in 99b09c0

for (const auto& font : currentFontList)

{

if (font.LocalizedName() == profileFontFace)

{

return box_value(font);

}

else if (font.LocalizedName() == L"Cascadia Mono")

{

fallbackFont = box_value(font);

}

}

// we couldn't find the desired font, set to "Cascadia Mono" since that ships by default

return fallbackFont;

Should we just make that look something like this then:
 for (const auto& font : currentFontList) 
 { 
     // GH#XXXX: check localized name and base name
     if (font.LocalizedName() == profileFontFace || font.Name() == profileFontFace) 
     { 
         return box_value(font); 
     } 
     else if (font.LocalizedName() == L"Cascadia Mono" || font.Name() == L"Cascadia Mono") 
     { 
         fallbackFont = box_value(font); 
     } 
 } 

There could be N names. Not just one localized and one unlocalized. Or at least that's how DWrite stores it internally. :|

DHowett · 2021-03-15T20:46:06Z

And we're going to want to make the comparison case-insensitively.

DHowett · 2021-03-15T20:59:21Z

@eugenesmlv sorry we took over your pull request as our chat channel for this issue. There's a significant Teams outage, it seems, and GitHub has really worked out for us 😉

eugenesmlv · 2021-03-16T10:16:28Z

@DHowett it was interesting! Unfortunately, I don't know much about the terminal internals to participate in the discussion, but I'll be glad to help you fix that font comparison issue ☺️

miniksa · 2021-03-16T17:54:49Z

k let me dig to see if there's a better way to know of the fallback next.

We know that we fall back to a different face in DxFontRenderData::_ResolveFontFaceWithFallback after the first _FindFontFace call fails.

We also know that we couldn't find the name we were looking for in the current locale and did a fallback to a different name in DxFontRenderData::_GetFontFamilyName (and this function is also a great example of how to dig the names out like I was describing for the IDWriteLocalizedStrings business above.)

So I think just bubbling this information out of the lookups and to the interface is just what needs to be done.

miniksa · 2021-04-07T17:02:21Z

@eugenesmlv, thanks for this proposal. But I'm going to close this in favor of the solution I wrote in #9734 that handles a few more scenarios than this and avoids comparing strings at all. Thanks for lighting a fire under me. :)

Make font names comparison case insensitive

18b745b

eugenesmlv changed the title ~~Make font name comparison case insensitive~~ Make font name comparison case-insensitive Mar 14, 2021

DHowett reviewed Mar 14, 2021

View reviewed changes

Change string comparison

6eaef1a

DHowett approved these changes Mar 15, 2021

View reviewed changes

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

DHowett added zPreview-Service-Consider labels Mar 15, 2021

DHowett removed the AutoMerge Marked for automatic merge by the bot when requirements are met label Mar 15, 2021

miniksa approved these changes Mar 15, 2021

View reviewed changes

miniksa closed this Apr 7, 2021

eugenesmlv deleted the load-font-warning branch April 8, 2021 08:50

DHowett removed zPreview-Service-Consider labels Apr 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make font name comparison case-insensitive #9494

Make font name comparison case-insensitive #9494

eugenesmlv commented Mar 14, 2021 •

edited by DHowett

Loading

DHowett Mar 14, 2021

DHowett left a comment

DHowett commented Mar 15, 2021

ghost commented Mar 15, 2021

DHowett commented Mar 15, 2021

carlos-zamora commented Mar 15, 2021

DHowett commented Mar 15, 2021

eugenesmlv commented Mar 15, 2021

carlos-zamora commented Mar 15, 2021

miniksa commented Mar 15, 2021

DHowett commented Mar 15, 2021

miniksa commented Mar 15, 2021

miniksa commented Mar 15, 2021

DHowett commented Mar 15, 2021

miniksa commented Mar 15, 2021

DHowett commented Mar 15, 2021

miniksa commented Mar 15, 2021

carlos-zamora commented Mar 15, 2021

miniksa commented Mar 15, 2021

DHowett commented Mar 15, 2021

DHowett commented Mar 15, 2021

eugenesmlv commented Mar 16, 2021

miniksa commented Mar 16, 2021

miniksa commented Apr 7, 2021

Make font name comparison case-insensitive #9494

Make font name comparison case-insensitive #9494

Conversation

eugenesmlv commented Mar 14, 2021 • edited by DHowett Loading

DHowett Mar 14, 2021

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

DHowett commented Mar 15, 2021

ghost commented Mar 15, 2021

DHowett commented Mar 15, 2021

carlos-zamora commented Mar 15, 2021

DHowett commented Mar 15, 2021

eugenesmlv commented Mar 15, 2021

carlos-zamora commented Mar 15, 2021

miniksa commented Mar 15, 2021

DHowett commented Mar 15, 2021

miniksa commented Mar 15, 2021

miniksa commented Mar 15, 2021

DHowett commented Mar 15, 2021

miniksa commented Mar 15, 2021

DHowett commented Mar 15, 2021

miniksa commented Mar 15, 2021

carlos-zamora commented Mar 15, 2021

miniksa commented Mar 15, 2021

DHowett commented Mar 15, 2021

DHowett commented Mar 15, 2021

eugenesmlv commented Mar 16, 2021

miniksa commented Mar 16, 2021

miniksa commented Apr 7, 2021

eugenesmlv commented Mar 14, 2021 •

edited by DHowett

Loading