Implement CTFontCopyLocalizedName and CTFontCopyAvailableTables #2058

aballway · 2017-02-23T17:38:17Z

Fixes #2023

msft-Jeyaram · 2017-02-23T17:48:12Z

Frameworks/CoreGraphics/CGFont.mm

@@ -151,7 +151,7 @@ CGFontRef CGFontCreateCopyWithVariations(CGFontRef font, CFDictionaryRef variati
 */
 CFStringRef CGFontCopyPostScriptName(CGFontRef font) {
    RETURN_NULL_IF(!font);
-    return _DWriteFontCopyInformationalString(font->_dwriteFontFace, DWRITE_INFORMATIONAL_STRING_POSTSCRIPT_NAME);
+    return _DWriteFontCopyInformationalString(font->_dwriteFontFace, DWRITE_INFORMATIONAL_STRING_POSTSCRIPT_NAME, nullptr);


why don't use just use default parameter?
You can leave these as is. #Resolved

msft-Jeyaram · 2017-02-23T17:52:49Z

Frameworks/include/CoreGraphics/DWriteWrapper.h

@@ -35,7 +35,7 @@ static inline CFStringRef _CFStringCreateUppercaseCopy(CFStringRef string) {
    return ret;
 }

-COREGRAPHICS_EXPORT CFStringRef _CFStringFromLocalizedString(IDWriteLocalizedStrings* localizedString);
+COREGRAPHICS_EXPORT CFStringRef _CFStringFromLocalizedString(IDWriteLocalizedStrings* localizedString, CFStringRef* language);


CFStringRef* language = nullptr
This saves you from all the extra null addition changes. #Resolved

msft-Jeyaram · 2017-02-23T17:53:55Z

Frameworks/include/CoreGraphics/DWriteWrapper.h

@@ -65,7 +65,8 @@ COREGRAPHICS_EXPORT HRESULT _DWriteCreateTextFormatWithFontNameAndSize(CFStringR

 // DWriteFont getters that convert to a CF/CG object or struct
 COREGRAPHICS_EXPORT CFStringRef _DWriteFontCopyInformationalString(const Microsoft::WRL::ComPtr<IDWriteFontFace>& fontFace,


_DWriteFontCopyInformationalString [](start = 32, length = 34)

same here
CFStringRef* language = nullptr #Resolved

given these are private methods, this should make life easier.

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

msft-Jeyaram · 2017-02-23T17:55:05Z

tests/unittests/CoreText/CTFontTests.mm

+}
+
+const NSString* c_localeName = @"en-us";


c_localeName [](start = 16, length = 12)

static #Resolved

msft-Jeyaram · 2017-02-23T17:55:49Z

tests/unittests/CoreText/CTFontTests.mm

+TEST(CTFont, CopyLocalizedName) {
+    auto fontName = woc::MakeAutoCF<CFStringRef>(CFSTR("Metadata Test"));
+    StrongId<NSURL> testFileURL = __GetURLFromPathRelativeToModuleDirectory(@"/data/MetadataTest-Regular.ttf");


MetadataTest [](start = 84, length = 12)

nit: make it a const? #WontFix

msft-Jeyaram · 2017-02-23T17:56:44Z

tests/unittests/CoreText/CTFontTests.mm

+    // Don't want to make test too precise so that it may fail should fonts change, but 'cmap' is a required font table
+    // So it should be safe to always test that this value is available
+    EXPECT_TRUE(CFArrayContainsValue(availableTables, { 0, count }, (const void*)kCTFontTableCmap));


(const void*) [](start = 68, length = 13)

nit: c++ cast #WontFix

msft-Jeyaram · 2017-02-23T17:58:34Z

@notes Returns family name

super nit: I feel the note here could be better.
Just saying returns family name doesn't help.
So It doesn't return the full name and 'only' returns family name? #WontFix

Refers to: Frameworks/CoreGraphics/CGFont.mm:225 in 031bf91. [](commit_id = 031bf91, deletion_comment = False)

msft-Jeyaram · 2017-02-23T18:01:16Z

Frameworks/CoreText/CTFont.mm

- @Status Stub
- @Notes
+ @Status Interoperable
+ @Notes options is not supported, and is deprecated anyway


anyway [](start = 52, length = 6)

super nit: 'anyway' is not needed #WontFix

msft-Jeyaram · 2017-02-23T18:03:24Z

Frameworks/CoreText/DWriteWrapper_CoreText.h

@@ -62,10 +62,12 @@ CTFontSymbolicTraits _CTFontSymbolicTraitsFromCFNumber(CFNumberRef num);
 HRESULT _DWriteCreateFontFaceWithFontDescriptor(CTFontDescriptorRef fontDescriptor, IDWriteFontFace** fontFace);

 CFDictionaryRef _DWriteFontCreateTraitsDict(const Microsoft::WRL::ComPtr<IDWriteFontFace>& fontFace);
-CFStringRef _DWriteFontCopyName(const Microsoft::WRL::ComPtr<IDWriteFontFace>& fontFace, CFStringRef nameKey);
+CFStringRef _DWriteFontCopyName(const Microsoft::WRL::ComPtr<IDWriteFontFace>& fontFace, CFStringRef nameKey, CFStringRef* actualLanguage);


actualLanguage [](start = 123, length = 14)

same here with the default argument #Resolved

msft-Jeyaram · 2017-02-23T18:07:58Z

Frameworks/CoreText/DWriteWrapper_CTFont.mm

+        void* tableContext;
+        BOOL exists;
+
+        RETURN_NULL_IF_FAILED(fontFace->TryGetFontTable(_CTToDWriteFontTableTag(tag), &tableData, &tableSize, &tableContext, &exists));


tableContext [](start = 111, length = 12)

nit: c++ cast #WontFix

msft-Jeyaram · 2017-02-23T18:10:18Z

tests/unittests/CoreText/CTFontTests.mm

+    auto copyrightName = woc::MakeAutoCF<CFStringRef>(CTFontCopyLocalizedName(font, kCTFontCopyrightNameKey, &actualLanguage));
+    EXPECT_OBJCEQ(c_copyrightName, (__bridge NSString*)copyrightName.get());
+    ASSERT_NE(nil, actualLanguage);


actualLanguage [](start = 19, length = 14)

nit: if actualLanguage was nil, this would never run.
Given they are not dependent, They should be able to be evaluated regardless. #Resolved

msft-Jeyaram

:

ms-jihua · 2017-02-23T18:30:15Z

Frameworks/CoreText/DWriteWrapper_CTFont.mm

@@ -284,7 +284,7 @@ static CFDictionaryRef _DWriteFontCreateTraitsDict(const ComPtr<IDWriteFontFace>
 /**
 * Gets a name/informational string from a DWrite font face corresponding to a CTFont constant
 */
-CFStringRef _DWriteFontCopyName(const ComPtr<IDWriteFontFace>& fontFace, CFStringRef nameKey) {
+CFStringRef _DWriteFontCopyName(const ComPtr<IDWriteFontFace>& fontFace, CFStringRef nameKey, CFStringRef* actualLanuguage) {


actualLanuguage [](start = 107, length = 15)

nit: typo

ms-jihua · 2017-02-23T18:35:18Z

Frameworks/CoreText/DWriteWrapper_CTFont.mm

+        RETURN_NULL_IF_FAILED(fontFace->TryGetFontTable(_CTToDWriteFontTableTag(tag), &tableData, &tableSize, &tableContext, &exists));
+        if (exists) {
+            // Returned array is expected to have unboxed CTFontTableTag values
+            CFArrayAppendValue(ret, (const void*)tag);


surprised this doesn't screw with the memory management on the CFArray... can you run your tests under appverifier and make sure there are no issues?

This array isn't using any memory management callbacks so it shouldn't be leaking anything, but it's worth checking.

Update: no leaks!

rajsesh · 2017-02-23T19:48:35Z

Frameworks/CoreGraphics/DWriteWrapper.mm

@@ -97,8 +97,9 @@ HRESULT UpdateCollection(const ComPtr<IDWriteFactory>& dwriteFactory, const ComP
 }

 // Private helper, wraps around GetUserDefaultLocaleName() and returns a default of "en-us" if it fails
-static inline const wchar_t* __GetUserDefaultLocaleName(wchar_t* buf, size_t bufferSize) {
-    int defaultLocaleSuccess = GetUserDefaultLocaleName(buf, bufferSize);
+template <size_t size>


This will result in an instance of this method for every size. could we just use _countof() where size is needed in the previous version of this?

This is only being used with buffers of size LOCALE_NAME_MAX_LENGTH, so there will only be one instance, but I didn't think it made sense to do any calculation or pass in a parameter when the size is known at compile-time. Would _countof work w/ function arg?

I don't see any issue with that. It is standard practice to pass a buffer and a length. Now you are constrained to passing an array instead.

rajsesh · 2017-02-23T20:08:13Z

Frameworks/CoreText/CTFont.mm

@@ -493,17 +493,16 @@ CFStringRef CTFontCopyDisplayName(CTFontRef font) {
 @Status Interoperable
 */
 CFStringRef CTFontCopyName(CTFontRef font, CFStringRef nameKey) {
-    RETURN_NULL_IF(!font);
-    return _DWriteFontCopyName(font->_dwriteFontFace, nameKey);
+    return CTFontCopyLocalizedName(font, nameKey, nullptr);
 }

 /**
 @Status Stub
 @Notes
 */
 CFStringRef CTFontCopyLocalizedName(CTFontRef font, CFStringRef nameKey, CFStringRef _Nullable* actualLanguage) {


Not a stub anymore.

rajsesh · 2017-02-23T20:08:24Z

Frameworks/CoreText/CTFont.mm

- @Status Stub
- @Notes
+ @Status Interoperable
+ @Notes options is not supported, and is deprecated anyway


Caveat instead.

I was going off of the decision w/ CTFontCopyTable where we're still marking it Interoperable despite not supporting kCTFontTableOptionExcludeSynthetic

I think they should all be caveats :)

ms-jihua

Leave a comment

DHowett-MSFT

:clock1024:

DHowett-MSFT · 2017-02-24T00:20:47Z

Frameworks/CoreText/CTFont.mm

 @Notes
 */
 CFStringRef CTFontCopyLocalizedName(CTFontRef font, CFStringRef nameKey, CFStringRef _Nullable* actualLanguage) {
-    UNIMPLEMENTED();
-    return StubReturn();
+    RETURN_NULL_IF(!font);


why is this a RETURN_NULL_IF but the other functions in this file are return x ? : nullptr?

DHowett-MSFT · 2017-02-24T00:22:28Z

Frameworks/CoreText/DWriteWrapper_CTFont.mm

+                                              kCTFontTableProp, kCTFontTableSbit, kCTFontTableSbix, kCTFontTableTrak, kCTFontTableVhea,
+                                              kCTFontTableVmtx };
+
+CFArrayRef _DWriteCopyAvailableFontTables(const ComPtr<IDWriteFontFace>& fontFace) {


nit: prefer taking ComPtr values by unboxed ptr

I know there's precedent for doing the wrong thing in this code, so it may not be worth fixing. #WontFix

DHowett-MSFT · 2017-02-24T00:23:42Z

Frameworks/include/CoreGraphics/DWriteWrapper.h

+
+inline uint32_t _CTToDWriteFontTableTag(uint32_t tag) {
+    // CT has the opposite byte order of DWrite, so we need 'BASE' -> 'ESAB'


Just like we've done with some CG constants, we can simply force these to have the values we want them to.
That'll reduce stuff like this down to, say, not existing at all.

A dev can pass in a specific tag for CTFontCopyTable though. We'd at least need to keep the function around (though I guess we could reverse the internal constants)

EDIT: Forget I said anything, for some reason I thought the tags were strings (plausible to pass in raw) rather than ints (pretty unlikely)

I can see that case where someone would just use 'BASE' or something. The values are public and the conversion is cheap so I think it's worth it to just do the conversion "Just in case"

DHowett-MSFT · 2017-02-24T00:36:28Z

Ah, I see. I didn’t see that it allowed freeform input.

aballway · 2017-03-01T17:46:35Z

ping @rajsesh-msft @msft-Jeyaram

Implement CTFont CopyLocalizedName and CopyAvailableTables

031bf91

aballway requested review from rajsesh and ms-jihua February 23, 2017 17:38

msftclas added the cla-not-required label Feb 23, 2017

msft-Jeyaram reviewed Feb 23, 2017

View reviewed changes

Default params

bae6476

msft-Jeyaram reviewed Feb 23, 2017

View reviewed changes

msft-Jeyaram suggested changes Feb 23, 2017

View reviewed changes

ms-jihua reviewed Feb 23, 2017

View reviewed changes

Spelling is hard

17f337e

rajsesh suggested changes Feb 23, 2017

View reviewed changes

aballway added 3 commits February 23, 2017 12:42

Comment cleanup

949f076

Remove leftover __debugbreak

80425d3

CR feedback

6d2367d

ms-jihua approved these changes Feb 23, 2017

View reviewed changes

DHowett-MSFT suggested changes Feb 24, 2017

View reviewed changes

DHowett-MSFT approved these changes Feb 24, 2017

View reviewed changes

Consistent nil handling

17cc402

rajsesh approved these changes Mar 1, 2017

View reviewed changes

msft-Jeyaram approved these changes Mar 2, 2017

View reviewed changes

aballway merged commit 5c54820 into microsoft:develop Mar 2, 2017

aballway deleted the ct branch April 25, 2017 16:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement CTFontCopyLocalizedName and CTFontCopyAvailableTables #2058

Implement CTFontCopyLocalizedName and CTFontCopyAvailableTables #2058

aballway commented Feb 23, 2017

msft-Jeyaram Feb 23, 2017 •

edited by aballway

Loading

msft-Jeyaram Feb 23, 2017 •

edited by aballway

Loading

msft-Jeyaram Feb 23, 2017 •

edited by aballway

Loading

msft-Jeyaram Feb 23, 2017

msft-Jeyaram Feb 23, 2017 •

edited by aballway

Loading

msft-Jeyaram Feb 23, 2017 •

edited by aballway

Loading

msft-Jeyaram Feb 23, 2017 •

edited by aballway

Loading

msft-Jeyaram commented Feb 23, 2017 •

edited by aballway

Loading

msft-Jeyaram Feb 23, 2017 •

edited by aballway

Loading

msft-Jeyaram Feb 23, 2017 •

edited by aballway

Loading

msft-Jeyaram Feb 23, 2017 •

edited by aballway

Loading

msft-Jeyaram Feb 23, 2017 •

edited by aballway

Loading

msft-Jeyaram left a comment

ms-jihua Feb 23, 2017

ms-jihua Feb 23, 2017

aballway Feb 23, 2017 •

edited

Loading

rajsesh Feb 23, 2017

aballway Feb 23, 2017

rajsesh Feb 23, 2017

rajsesh Feb 23, 2017

rajsesh Feb 23, 2017

aballway Feb 23, 2017

rajsesh Feb 23, 2017

ms-jihua left a comment

DHowett-MSFT left a comment

DHowett-MSFT Feb 24, 2017

DHowett-MSFT Feb 24, 2017 •

edited by aballway

Loading

DHowett-MSFT Feb 24, 2017

DHowett-MSFT Feb 24, 2017

ms-jihua Feb 24, 2017 •

edited

Loading

aballway Feb 24, 2017

DHowett-MSFT commented Feb 24, 2017 via email

aballway commented Mar 1, 2017

		@@ -65,7 +65,8 @@ COREGRAPHICS_EXPORT HRESULT _DWriteCreateTextFormatWithFontNameAndSize(CFStringR

		// DWriteFont getters that convert to a CF/CG object or struct
		COREGRAPHICS_EXPORT CFStringRef _DWriteFontCopyInformationalString(const Microsoft::WRL::ComPtr<IDWriteFontFace>& fontFace,


		inline uint32_t _CTToDWriteFontTableTag(uint32_t tag) {
		// CT has the opposite byte order of DWrite, so we need 'BASE' -> 'ESAB'

Implement CTFontCopyLocalizedName and CTFontCopyAvailableTables #2058

Implement CTFontCopyLocalizedName and CTFontCopyAvailableTables #2058

Conversation

aballway commented Feb 23, 2017

msft-Jeyaram Feb 23, 2017 • edited by aballway Loading

Choose a reason for hiding this comment

msft-Jeyaram Feb 23, 2017 • edited by aballway Loading

Choose a reason for hiding this comment

msft-Jeyaram Feb 23, 2017 • edited by aballway Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msft-Jeyaram Feb 23, 2017 • edited by aballway Loading

Choose a reason for hiding this comment

msft-Jeyaram Feb 23, 2017 • edited by aballway Loading

Choose a reason for hiding this comment

msft-Jeyaram Feb 23, 2017 • edited by aballway Loading

Choose a reason for hiding this comment

msft-Jeyaram commented Feb 23, 2017 • edited by aballway Loading

msft-Jeyaram Feb 23, 2017 • edited by aballway Loading

Choose a reason for hiding this comment

msft-Jeyaram Feb 23, 2017 • edited by aballway Loading

Choose a reason for hiding this comment

msft-Jeyaram Feb 23, 2017 • edited by aballway Loading

Choose a reason for hiding this comment

msft-Jeyaram Feb 23, 2017 • edited by aballway Loading

Choose a reason for hiding this comment

msft-Jeyaram left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aballway Feb 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ms-jihua left a comment

Choose a reason for hiding this comment

DHowett-MSFT left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett-MSFT Feb 24, 2017 • edited by aballway Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ms-jihua Feb 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett-MSFT commented Feb 24, 2017 via email

aballway commented Mar 1, 2017

msft-Jeyaram Feb 23, 2017 •

edited by aballway

Loading

msft-Jeyaram Feb 23, 2017 •

edited by aballway

Loading

msft-Jeyaram Feb 23, 2017 •

edited by aballway

Loading

msft-Jeyaram Feb 23, 2017 •

edited by aballway

Loading

msft-Jeyaram Feb 23, 2017 •

edited by aballway

Loading

msft-Jeyaram Feb 23, 2017 •

edited by aballway

Loading

msft-Jeyaram commented Feb 23, 2017 •

edited by aballway

Loading

msft-Jeyaram Feb 23, 2017 •

edited by aballway

Loading

msft-Jeyaram Feb 23, 2017 •

edited by aballway

Loading

msft-Jeyaram Feb 23, 2017 •

edited by aballway

Loading

msft-Jeyaram Feb 23, 2017 •

edited by aballway

Loading

aballway Feb 23, 2017 •

edited

Loading

DHowett-MSFT Feb 24, 2017 •

edited by aballway

Loading

ms-jihua Feb 24, 2017 •

edited

Loading