Properly get font family name for UILabels by aballway · Pull Request #1243 · microsoft/WinObjC

aballway · 2016-10-27T16:12:41Z

Getting the Font Family Name from DWrite would for certain fonts return a different value than Xaml expects, causing Segue to be used as a default.

Partially fixes #1217

This change is

rajsesh · 2016-10-27T16:25:12Z

        _isBold = (mask & UIFontDescriptorTraitBold) > 0;
        _isItalic = (mask & UIFontDescriptorTraitItalic) > 0;
        std::wstring wideBuffer = Strings::NarrowToWide<std::wstring>(text);
-        ConstructGlyphs([[font fontName] UTF8String], wideBuffer.c_str(), wideBuffer.length());


Why not font fontFamilyName? We should be returning the right family name there.

do you mean familyName? That will return an incompatible name

The question I am really asking is, why aren't we fixing that method?

The method returns the "correct" value, which is taken directly from the font file as I understand it. The issue is Xaml expects the family name to be a different value than the one on the file in some cases. A developer may expect familyName to be the actual value from the file, in which case changing it in the CoreText/UIKit calls would be incorrect.

I don't know if it is "correct" if it works with one framework and not the other. If you really want to keep this separate go with a UIFont private property that rest of UIKit can use where it integrates with xaml.

The correct fix IMO would be for coretext to also handle font family names like xaml does (xaml is also built on dwrite), but that would likely be bigger change than what we want to do at this point.

Let me try to clarify on correctness here. [UIFont familyName] for this on the reference platform would return "Franklin Gothic Medium Cond". This is the same with XAML and DWrite, as well as several websites such as http://fontsgeek.com/fonts/Franklin-Gothic-Medium-Cond-Regular which enumerate family names for fonts, suggesting that it's a property of the ttf file itself.

However, XAML/DWrite only accept "Franklin Gothic" as the input family name, even though they give something else as output. This is fundamentally a mismatch on their end, so I don't know that there's anything "more correct" than this UIFont private function right now.

jaredhms · 2016-10-27T17:05:32Z

class DisplayTextureXamlGlyphs : public DisplayTexture {

Please don't make any changes to the compositor; this class doesn't even exist anymore in the refactor branch (https://github.com/jaredhms/WinObjC/tree/compositor_latest2). Let's discuss how I can best roll this into my changes; it looks like I should be calling _DWriteGetFamilyNameForFontName when I need to grab the font family name?

Refers to: Frameworks/UIKit/StarboardXaml/CompositorInterface.h:150 in e193e65. [](commit_id = e193e65, deletion_comment = False)

jaredhms · 2016-10-27T17:31:19Z

jaredhms · 2016-10-27T17:32:11Z

class DisplayTextureXamlGlyphs : public DisplayTexture {

I talked with Raj offline; we can take this if needed, and I already have a fix for it queued up in my branch.

In reply to: 256707554 [](ancestors = 256707554)

Refers to: Frameworks/UIKit/StarboardXaml/CompositorInterface.h:150 in e193e65. [](commit_id = e193e65, deletion_comment = False)

msft-Jeyaram · 2016-10-27T18:13:03Z

// Copyright (c) 2016 Microsoft Corporation. All rights reserved.

update #WontFix

Refers to: Frameworks/UIKit/StarboardXaml/CompositorInterface.mm:4 in e193e65. [](commit_id = e193e65, deletion_comment = False)

aballway · 2016-10-27T18:29:15Z

// Copyright (c) 2016 Microsoft Corporation. All rights reserved.

this file is going away

In reply to: 256725429 [](ancestors = 256725429)

Refers to: Frameworks/UIKit/StarboardXaml/CompositorInterface.mm:4 in e193e65. [](commit_id = e193e65, deletion_comment = False)

ms-jihua · 2016-10-27T18:32:56Z

    EXPECT_OBJCEQ(@"Times New Roman", (id)_DWriteGetFamilyNameForFontName(CFSTR("Times New Roman Italic")));
    EXPECT_OBJCEQ(@"Times New Roman", (id)_DWriteGetFamilyNameForFontName(CFSTR("Times New Roman Bold")));
    EXPECT_OBJCEQ(@"Times New Roman", (id)_DWriteGetFamilyNameForFontName(CFSTR("Times New Roman Bold Italic")));
+    EXPECT_OBJCEQ(@"Franklin Gothic", (id)_DWriteGetFamilyNameForFontName(CFSTR("Franklin Gothic Medium Cond")));


uhhh don't check this in, our lab machines don't have this font (though our build machines do)

I second this.

rajsesh

contingent on the unit test being fixed.

rajsesh · 2016-10-27T18:40:59Z

    EXPECT_OBJCEQ(@"Times New Roman", (id)_DWriteGetFamilyNameForFontName(CFSTR("Times New Roman Italic")));
    EXPECT_OBJCEQ(@"Times New Roman", (id)_DWriteGetFamilyNameForFontName(CFSTR("Times New Roman Bold")));
    EXPECT_OBJCEQ(@"Times New Roman", (id)_DWriteGetFamilyNameForFontName(CFSTR("Times New Roman Bold Italic")));
+    EXPECT_OBJCEQ(@"Franklin Gothic", (id)_DWriteGetFamilyNameForFontName(CFSTR("Franklin Gothic Medium Cond")));


I second this.

ms-jihua · 2016-10-27T18:43:53Z

bbowman · 2016-10-27T19:05:24Z

    float _desiredWidth, _desiredHeight;
    void Measure(float width, float height);
-    void ConstructGlyphs(const char* fontName, const wchar_t* str, int len);
+    void ConstructGlyphs(Microsoft::WRL::Wrappers::HString fontFamilyName, const wchar_t* str, int len);


This needs to be a const & otherwise it's pass by Val which will copy the inner string

Properly get font family name for uilabels

e193e65

msftclas added the cla-not-required label Oct 27, 2016

aballway assigned jaredhms, ms-jihua and rajsesh Oct 27, 2016

rajsesh suggested changes Oct 27, 2016

View reviewed changes

Make private extension for compatible family name

fafe18b

ms-jihua reviewed Oct 27, 2016

View reviewed changes

rajsesh approved these changes Oct 27, 2016

View reviewed changes

Remove extra test

db98853

bbowman suggested changes Oct 27, 2016

View reviewed changes

aballway added 2 commits October 27, 2016 12:37

Stop copying string

d789a42

Merge branch 'develop' of github.com:aballway/WinObjC into develop

c7d5fdf

aballway merged commit c2e9246 into microsoft:develop Oct 27, 2016

Conversation

aballway commented Oct 27, 2016 • edited by rajsesh Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aballway Oct 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

jaredhms commented Oct 27, 2016

Uh oh!

jaredhms commented Oct 27, 2016

Uh oh!

jaredhms commented Oct 27, 2016

Uh oh!

msft-Jeyaram commented Oct 27, 2016 • edited by aballway Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aballway commented Oct 27, 2016

Uh oh!

ms-jihua Oct 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajsesh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ms-jihua commented Oct 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

aballway commented Oct 27, 2016 •

edited by rajsesh

Loading

aballway Oct 27, 2016 •

edited

Loading

msft-Jeyaram commented Oct 27, 2016 •

edited by aballway

Loading

ms-jihua Oct 27, 2016 •

edited

Loading