Skip to content
This repository was archived by the owner on May 28, 2025. It is now read-only.

Return a default font when a font name cannot be found#1253

Merged
ms-jihua merged 1 commit into
microsoft:developfrom
ms-jihua:default_font_return
Oct 28, 2016
Merged

Return a default font when a font name cannot be found#1253
ms-jihua merged 1 commit into
microsoft:developfrom
ms-jihua:default_font_return

Conversation

@ms-jihua

@ms-jihua ms-jihua commented Oct 27, 2016

Copy link
Copy Markdown
Contributor

Fixes #1251


This change is Reviewable

 - Disabled a related unit test
 - Revert this change when microsoft#1250 is finished

Fixes microsoft#1251
// For now return a default font to avoid crashes in case of missing fonts
// When #1250 is completed, remove this
if (!properties.familyName) {
name = CFSTR("Segoe UI");

@msft-Jeyaram msft-Jeyaram Oct 27, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Segoe UI [](start = 22, length = 8)

make this into a defined cost. #WontFix

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no.

  • we're reverting this soon anyway, no reason to make this change bigger and messier to revert.
  • this isn't intended to be our go-to default font functionality. we have other default font constants elsewhere. this just has to be any default font. (@rajsesh-msft and I were throwing around the idea of using something ugly so it'd stick out)

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in that case :shipit:

@aballway aballway left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved, seconding font name as a const

@msft-Jeyaram

msft-Jeyaram commented Oct 27, 2016

Copy link
Copy Markdown

remove? #WontFix


Refers to: Frameworks/CoreGraphics/DWriteWrapper.mm:317 in a31c45e. [](commit_id = a31c45e, deletion_comment = False)

@msft-Jeyaram

msft-Jeyaram commented Oct 27, 2016

Copy link
Copy Markdown
        //     properties->style = DWRITE_FONT_STYLE_NORMAL;

curious why these are commented out, are they to come back sometime in the future?
if so TODO? #WontFix


Refers to: Frameworks/CoreGraphics/DWriteWrapper.mm:329 in a31c45e. [](commit_id = a31c45e, deletion_comment = False)

@ms-jihua

Copy link
Copy Markdown
Contributor Author

out of scope


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


Refers to: Frameworks/CoreGraphics/DWriteWrapper.mm:317 in a31c45e. [](commit_id = a31c45e, deletion_comment = False)

@ms-jihua

Copy link
Copy Markdown
Contributor Author
        //     properties->style = DWRITE_FONT_STYLE_NORMAL;

no, just communicates that we didn't forget about "normal"


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


Refers to: Frameworks/CoreGraphics/DWriteWrapper.mm:329 in a31c45e. [](commit_id = a31c45e, deletion_comment = False)

@msft-Jeyaram

Copy link
Copy Markdown

still curious why this code is commented out and still present?


In reply to: 256795517 [](ancestors = 256795517,256795157)


Refers to: Frameworks/CoreGraphics/DWriteWrapper.mm:317 in a31c45e. [](commit_id = a31c45e, deletion_comment = False)

@ms-jihua

ms-jihua commented Oct 27, 2016

Copy link
Copy Markdown
Contributor Author

same as below - it'd be a no-op, so there's no reason to have the code actually execute, but it's a useful reminder that DWRITE_FONT_STYLE, STRETCH, WEIGHT, _NORMAL exists and we didn't forget about it


In reply to: 256795601 [](ancestors = 256795601,256795517,256795157)


Refers to: Frameworks/CoreGraphics/DWriteWrapper.mm:317 in a31c45e. [](commit_id = a31c45e, deletion_comment = False)

@msft-Jeyaram

Copy link
Copy Markdown

:shipit:

@msft-Jeyaram

Copy link
Copy Markdown

the commented out else code just looks ugly being present.
When it's in scope, it should get wiped.


In reply to: 256795654 [](ancestors = 256795654,256795601,256795517,256795157)


Refers to: Frameworks/CoreGraphics/DWriteWrapper.mm:317 in a31c45e. [](commit_id = a31c45e, deletion_comment = False)

@msft-Jeyaram

msft-Jeyaram commented Oct 27, 2016

Copy link
Copy Markdown

#endif

I know this is not part of the CR, but I'm curious why
Courier New Bold Italic maps to Courier-BoldOblique here , where as in the static consts above maps Courier New Bold Italic to CourierNewPS-BoldItalicMT.


Refers to: tests/unittests/CoreText/CTFontTests.mm:227 in a31c45e. [](commit_id = a31c45e, deletion_comment = False)

@ms-jihua

Copy link
Copy Markdown
Contributor Author

I'll do it as part of #1212


In reply to: 256795789 [](ancestors = 256795789,256795654,256795601,256795517,256795157)


Refers to: Frameworks/CoreGraphics/DWriteWrapper.mm:317 in a31c45e. [](commit_id = a31c45e, deletion_comment = False)

@ms-jihua

Copy link
Copy Markdown
Contributor Author

#endif

I wrote this one before the rest and I missed that the reference platform also has Courier New and not just Courier as default fonts. I'll fix this in #1212.


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


Refers to: tests/unittests/CoreText/CTFontTests.mm:227 in a31c45e. [](commit_id = a31c45e, deletion_comment = False)

@rajsesh

rajsesh commented Oct 27, 2016

Copy link
Copy Markdown
Contributor

:shipit:

@ms-jihua ms-jihua merged commit b426a70 into microsoft:develop Oct 28, 2016
@ms-jihua ms-jihua deleted the default_font_return branch February 1, 2017 22:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants