Remove Foundation from CTRun by msft-Jeyaram · Pull Request #2511 · microsoft/WinObjC

msft-Jeyaram · 2017-04-13T18:14:40Z

Implement CTRun via runtimebase
Implement CTLine's internal holder of CTRun via CoreFoundation
General code improvements

msft-Jeyaram · 2017-04-13T18:15:04Z

*Note: The NSAttribute changes will be part of the CTLine code review #ByDesign

DHowett-MSFT · 2017-04-13T18:16:19Z


        for (size_t j = 0; j < [line->_runs count]; ++j) {
-            _CTRun* curRun = [line->_runs objectAtIndex:j];
+			__CTRun* curRun = static_cast<__CTRun*>([line->_runs objectAtIndex:j]);


since you've done the conversion of line->_runs to a CFArray, you can make these two changes to CFArray as well! #ByDesign

It's already changed as part of CTLine, that will be out soon.

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

DHowett-MSFT · 2017-04-13T18:17:45Z

+// struct __CTRun has to be made public via CoreTextInternal.h (Frameworks\include). Therefore __CTRun(const CTRunRef& other)'s
+// implementation needs to be moved here.
+__CTRun::__CTRun(const CTRunRef& other)
+    : _attributes(CFDictionaryCreateMutableCopy(kCFAllocatorSystemDefault, CFDictionaryGetCount(other->_attributes), other->_attributes)),


do you need to use woc::TakeOwnership here? (is _attributes a StrongCF?) #Resolved

Yep, I forgot that the bug is now fixed :p

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

DHowett-MSFT · 2017-04-13T18:18:12Z

+      _glyphAdvances(other->_glyphAdvances),
+      _relativeXOffset(other->_relativeXOffset),
+      _relativeYOffset(other->_relativeYOffset),
+      _stringFragment(CFStringCreateCopy(kCFAllocatorDefault, other->_stringFragment)) {


ut supra #Resolved

DHowett-MSFT · 2017-04-13T18:18:56Z

 const CGGlyph* CTRunGetGlyphsPtr(CTRunRef run) {
-    _CTRun* runPtr = static_cast<_CTRun*>(run);
-    return (runPtr && runPtr->_dwriteGlyphRun.glyphCount) ? runPtr->_dwriteGlyphRun.glyphIndices : nullptr;
+    RETURN_NULL_IF(!run);


you can fold these into one RETURN_NULL_IF(!x || !y) #Resolved

can do.

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

DHowett-MSFT · 2017-04-13T18:19:33Z


 /**
- @Status NotInPlan
- @Notes this would require us to move to using bridged type implementation, seems of little value at this point


lol
#ByDesign

DHowett-MSFT · 2017-04-13T18:20:34Z

            // Create _CTRun objects and make them part of _CTLine
-            _CTRun* run = [[_CTRun new] autorelease];
+            __CTRun* run = const_cast<__CTRun*>(_CTRunCreate());
+            CFAutorelease(run);


really really don't prefer CFAutorelease; this is getting put into an array that will own the reference! #Resolved

DHowett-MSFT · 2017-04-13T18:21:00Z

+#pragma region CTRun
+struct __CTRun : CoreFoundation::CppBase<__CTRun> {
+    __CTRun()
+        : _attributes(CFDictionaryCreateMutable(kCFAllocatorSystemDefault,


takeownership? #Resolved

DHowett-MSFT · 2017-04-13T18:21:49Z

+        delete[] _dwriteGlyphRun.glyphOffsets;
+    }
+
+    static CFTypeID GetTypeID() {


The point of CppBase is that you do not have to think about this and the only reason to override it is to set different parameters.
Are we doing that? #ByDesign

Oh, I see. We overrode copy.

Copying is ill-defined for CF, and it doesn't look like anything ever calls it... #Resolved

DHowett-MSFT · 2017-04-13T18:24:29Z

 CORETEXT_EXPORT CGAffineTransform CTRunGetTextMatrix(CTRunRef run);
 CORETEXT_EXPORT CFTypeID CTRunGetTypeID() STUB_METHOD;
-CORETEXT_EXPORT CFTypeID CTRunGetTypeID() NOTINPLAN_METHOD;
+CORETEXT_EXPORT CFTypeID CTRunGetTypeID();


there are two of these? l o l #Resolved

aballway · 2017-04-13T18:16:55Z


        for (size_t j = 0; j < [line->_runs count]; ++j) {
-            _CTRun* curRun = [line->_runs objectAtIndex:j];
+			__CTRun* curRun = static_cast<__CTRun*>([line->_runs objectAtIndex:j]);


did clang-format do this? #ByDesign

aballway · 2017-04-13T18:18:49Z

 - (instancetype)init {
    if (self = [super init]) {
-        _runs.attach([NSMutableArray new]);
+        _runs = CFArrayCreateMutable(kCFAllocatorDefault, 0, &kCFTypeArrayCallBacks);


This will over-retain #Resolved

aballway · 2017-04-13T18:19:00Z

    ret->_leading = _leading;
    ret->_glyphCount = _glyphCount;
-    ret->_runs.attach([_runs copy]);
+    ret->_runs = CFArrayCreateMutableCopy(kCFAllocatorDefault, 0, _runs);


same as above #Resolved

aballway · 2017-04-13T18:20:13Z

-        _CTRun* curRun = [line->_runs objectAtIndex:i];
+    CFIndex count = CFArrayGetCount(line->_runs);
+    for (CFIndex i = 0; i < count; ++i) {
+        __CTRun* curRun = const_cast<__CTRun*>(static_cast<CTRunRef>(CFArrayGetValueAtIndex(line->_runs, i)));


nit: just use CTRunRef for these #ByDesign

can't due to the address of usage below.
&curRun->_dwriteGlyphRun

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

aballway · 2017-04-13T18:22:10Z

-    ret->_relativeXOffset = _relativeXOffset;
-    ret->_relativeYOffset = _relativeYOffset;
-    return ret;
+CTRunRef _CTRunCreate() {


nit: is this necessary? #ByDesign

rather not expose the actual creation in another file. Let's say we decide to change how it's being created in the future :p

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

I think this is an unnecessary abstraction, especially since we're calling CreateInstance directly for the other CppBase structs #ByDesign

abstractions make life easier for maintainers.
Just because it's already bad, i don't think it should go on :p
I do realize that i introduced one and that will get fixed up.

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

abstractions don't necessarily make life easier, they just abstract :P
It's not a huge deal, just seems to exist for the sake of existing #ByDesign

aballway · 2017-04-13T18:23:19Z

+// struct __CTRun has to be made public via CoreTextInternal.h (Frameworks\include). Therefore __CTRun(const CTRunRef& other)'s
+// implementation needs to be moved here.
+__CTRun::__CTRun(const CTRunRef& other)
+    : _attributes(CFDictionaryCreateMutableCopy(kCFAllocatorSystemDefault, CFDictionaryGetCount(other->_attributes), other->_attributes)),


does this need to use TakeOwnershipT? #Resolved

aballway · 2017-04-13T18:23:28Z

+      _glyphAdvances(other->_glyphAdvances),
+      _relativeXOffset(other->_relativeXOffset),
+      _relativeYOffset(other->_relativeYOffset),
+      _stringFragment(CFStringCreateCopy(kCFAllocatorDefault, other->_stringFragment)) {


same as above #Resolved

aballway · 2017-04-13T18:24:12Z

 CTRunStatus CTRunGetStatus(CTRunRef runRef) {
+    RETURN_RESULT_IF_NULL(runRef, kCTRunStatusNoStatus);
+
+	__CTRun* run = const_cast<__CTRun*>(runRef);


nit: no need to cast, can use CTRunRef directly #Resolved

aballway · 2017-04-13T18:26:39Z

        for (j = i; (j < numOfGlyphRuns) && (glyphRunDetails._baselineOriginY[j] == baselineOriginYForCurrentLine); ++j) {
            // Create _CTRun objects and make them part of _CTLine
-            _CTRun* run = [[_CTRun new] autorelease];
+            __CTRun* run = const_cast<__CTRun*>(_CTRunCreate());


nit: just use CreateInstance and CTRunRef #ByDesign

if you look below, you are accessing members of the struct, if it's CTRunRef, it would end up being a const ref.
can't assign to a const ref :p

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

aballway · 2017-04-13T18:27:21Z

    CGFloat _width;
    NSUInteger _glyphCount;
-    StrongId<NSMutableArray<_CTRun*>> _runs;
+    woc::StrongCF<CFMutableArrayRef> _runs;


nit: this should be part of the CTLineRef fix. #ByDesign

not really, I don't want to do extra work to make the usage of _runs work and just kill it in CTLine :p

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

ms-jihua · 2017-04-13T18:29:40Z

+          _textMatrix(CGAffineTransformIdentity) {
+    }
+
+    __CTRun(const CTRunRef& other);


isn't CTRunRef already const? is this redundant then? #Resolved

It's also a pointer so no need to take as reference #Resolved

yeah to be a copy ctor this must be const __CTRun& #Resolved

msft-Jeyaram · 2017-04-13T20:46:06Z

@ms-jihua @DHowett-MSFT @aballway knock knock

DHowett-MSFT · 2017-04-13T20:52:38Z

 - (instancetype)init {
    if (self = [super init]) {
-        _runs.attach([NSMutableArray new]);
+        _runs = woc::MakeStrongCF(CFArrayCreateMutable(kCFAllocatorDefault, 0, &kCFTypeArrayCallBacks));


y'could just use .attach :P #Resolved

DHowett-MSFT · 2017-04-13T20:52:42Z

    ret->_leading = _leading;
    ret->_glyphCount = _glyphCount;
-    ret->_runs.attach([_runs copy]);
+    ret->_runs = woc::MakeStrongCF(CFArrayCreateMutableCopy(kCFAllocatorDefault, 0, _runs));


ut supra #Resolved

DHowett-MSFT · 2017-04-13T20:54:33Z

-                                                0,
-                                                &kCFCopyStringDictionaryKeyCallBacks,
-                                                &kCFTypeDictionaryValueCallBacks)),
+        : _attributes(woc::MakeStrongCF(CFDictionaryCreateMutable(kCFAllocatorSystemDefault,


It is slightly more concise and involves fewer temporary objects to simply use woc::TakeOwnership #Resolved

ms-jihua · 2017-04-13T21:16:41Z

-    if (!line || [line->_runs count] == 0) {
-        return 0;
-    }
+    RETURN_RESULT_IF((!line || CFArrayGetCount(line->_runs) == 0), 0);


nit: parenthesize the two sides of || rather than, or in addition to, both in one
as is, my brain keeps wanting to read it as
( (!line || CFArrayGetCount(line->_runs)) == 0 ) #Resolved

ms-jihua · 2017-04-13T21:19:00Z

 CORETEXT_EXPORT CGRect CTRunGetImageBounds(CTRunRef run, CGContextRef context, CFRange range);
 CORETEXT_EXPORT void CTRunDraw(CTRunRef run, CGContextRef context, CFRange range);
 CORETEXT_EXPORT CGAffineTransform CTRunGetTextMatrix(CTRunRef run);
-CORETEXT_EXPORT CFTypeID CTRunGetTypeID() STUB_METHOD;


we had it twice?! #ByDesign

aballway · 2017-04-13T22:41:51Z

 */
 CFDictionaryRef CTRunGetAttributes(CTRunRef run) {
-    return run ? static_cast<CFDictionaryRef>(static_cast<_CTRun*>(run)->_attributes.get()) : nil;
+    RETURN_NULL_IF(!run);


you sure you wouldn't prefer a nice clean ternary here? 😉
return run ? run->_attributes : nil; #ByDesign

aballway · 2017-04-14T16:16:14Z

        for (j = i; (j < numOfGlyphRuns) && (glyphRunDetails._baselineOriginY[j] == baselineOriginYForCurrentLine); ++j) {
            // Create _CTRun objects and make them part of _CTLine
-            _CTRun* run = [[_CTRun new] autorelease];
+            auto runRef = woc::MakeStrongCF<CTRunRef>(_CTRunCreate());


or if you're gonna stick with _CTRunCreate make it return a __CTRun* so you don't need to do this cast #ByDesign

aballway · 2017-04-14T16:16:50Z

-            run->_stringFragment = [static_cast<NSString*>(CFAttributedStringGetString(string))
-                substringWithRange:NSMakeRange(range.location + run->_range.location, run->_range.length)];
+
+            run->_stringFragment = CFStringCreateWithSubstring(nullptr,


this will overretain #Resolved

aballway · 2017-04-14T16:20:13Z

+                                                0,
+                                                &kCFCopyStringDictionaryKeyCallBacks,
+                                                &kCFTypeDictionaryValueCallBacks)),
+          _textMatrix(CGAffineTransformIdentity) {


nit: could this just be moved down into where it's declared? #ByDesign

msft-Jeyaram · 2017-04-14T22:09:06Z

@DHowett-MSFT @aballway @ms-jihua ping

DHowett-MSFT · 2017-04-14T22:10:19Z

-            _CTRun* curRun = [line->_runs objectAtIndex:j];
+        CFIndex len = CFArrayGetCount(line->_runs);
+        for (size_t j = 0; j < len; ++j) {
+            __CTRun* curRun = const_cast<__CTRun*>(static_cast<CTRunRef>(CFArrayGetValueAtIndex(line->_runs, j)));


wait, why does this run need to be non-const? we aren't manipulating it, so it's actually better that it's const.

&curRun->_dwriteGlyphRun
is holding it via a pointer which isn't non-const.
I can downcast it here (GlyphRunData{ &curRun->_dwriteGlyphRun but i'll just be safe and leave it very similar to how it was before.

DHowett-MSFT · 2017-04-14T22:10:42Z

            CFDictionaryRef attribs = CTRunGetAttributes(run);
-            NSAttributedString* string =
-                [[NSAttributedString alloc] initWithString:(static_cast<_CTRun*>(run))->_stringFragment attributes:(NSDictionary*)attribs];
+            NSAttributedString* string = [[NSAttributedString alloc] initWithString:static_cast<NSString*>(run->_stringFragment.get())


nit: get should not be necessary

… DWriteWrapper

msftclas added the cla-not-required label Apr 13, 2017

msft-Jeyaram requested review from DHowett-MSFT, aballway, ms-jihua and rajsesh April 13, 2017 18:15

DHowett-MSFT suggested changes Apr 13, 2017

View reviewed changes

aballway suggested changes Apr 13, 2017

View reviewed changes

ms-jihua reviewed Apr 13, 2017

View reviewed changes

DHowett-MSFT suggested changes Apr 13, 2017

View reviewed changes

ms-jihua reviewed Apr 13, 2017

View reviewed changes

aballway reviewed Apr 13, 2017

View reviewed changes

aballway reviewed Apr 14, 2017

View reviewed changes

DHowett-MSFT approved these changes Apr 14, 2017

View reviewed changes

ms-jihua approved these changes Apr 14, 2017

View reviewed changes

aballway approved these changes Apr 15, 2017

View reviewed changes

Jeyaram Jeyaraj added 5 commits April 18, 2017 17:34

Implement CTLine::CTRun via CoreFoundation

aa82bc0

Implementation of CTRun via runtime base and removal of foundation usage

35aaf2d

Implementation of CTRun::CTRunGetTypeID

1ab3846

Remove Foundation usage associated with CTRun in CRFrame

173f329

Remove Foundation usage associated with CTRun and AttributedString in…

829f9ad

… DWriteWrapper

msft-Jeyaram force-pushed the CTRUN_DEVELOP branch from 1dfd611 to 829f9ad Compare April 19, 2017 01:09

msft-Jeyaram merged commit 832ba7f into microsoft:develop Apr 19, 2017

msft-Jeyaram mentioned this pull request Apr 27, 2017

Break CoreGraphics and CoreText's dependencies on Foundation + UIKit #2026

Closed

Conversation

msft-Jeyaram commented Apr 13, 2017

Uh oh!

msft-Jeyaram commented Apr 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DHowett-MSFT Apr 13, 2017 • edited by msft-Jeyaram 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!

DHowett-MSFT Apr 13, 2017 • edited by msft-Jeyaram 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!

DHowett-MSFT Apr 13, 2017 • edited by msft-Jeyaram Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DHowett-MSFT Apr 13, 2017 • edited by msft-Jeyaram 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!

DHowett-MSFT Apr 13, 2017 • edited by msft-Jeyaram Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DHowett-MSFT Apr 13, 2017 • edited by msft-Jeyaram Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DHowett-MSFT Apr 13, 2017 • edited by msft-Jeyaram Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DHowett-MSFT Apr 13, 2017 • edited by msft-Jeyaram Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DHowett-MSFT Apr 13, 2017 • edited by msft-Jeyaram Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DHowett-MSFT Apr 13, 2017 • edited by msft-Jeyaram Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aballway Apr 13, 2017 • edited by msft-Jeyaram Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aballway Apr 13, 2017 • edited by msft-Jeyaram Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aballway Apr 13, 2017 • edited by msft-Jeyaram Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aballway Apr 13, 2017 • edited by msft-Jeyaram 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!

aballway Apr 13, 2017 • edited by msft-Jeyaram 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!

aballway Apr 13, 2017 • edited by msft-Jeyaram Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msft-Jeyaram commented Apr 13, 2017 •

edited

Loading

DHowett-MSFT Apr 13, 2017 •

edited by msft-Jeyaram

Loading

DHowett-MSFT Apr 13, 2017 •

edited by msft-Jeyaram

Loading

DHowett-MSFT Apr 13, 2017 •

edited by msft-Jeyaram

Loading

DHowett-MSFT Apr 13, 2017 •

edited by msft-Jeyaram

Loading

DHowett-MSFT Apr 13, 2017 •

edited by msft-Jeyaram

Loading

DHowett-MSFT Apr 13, 2017 •

edited by msft-Jeyaram

Loading

DHowett-MSFT Apr 13, 2017 •

edited by msft-Jeyaram

Loading

DHowett-MSFT Apr 13, 2017 •

edited by msft-Jeyaram

Loading

DHowett-MSFT Apr 13, 2017 •

edited by msft-Jeyaram

Loading

DHowett-MSFT Apr 13, 2017 •

edited by msft-Jeyaram

Loading

aballway Apr 13, 2017 •

edited by msft-Jeyaram

Loading

aballway Apr 13, 2017 •

edited by msft-Jeyaram

Loading

aballway Apr 13, 2017 •

edited by msft-Jeyaram

Loading

aballway Apr 13, 2017 •

edited by msft-Jeyaram

Loading

aballway Apr 13, 2017 •

edited by msft-Jeyaram

Loading

aballway Apr 13, 2017 •

edited by msft-Jeyaram

Loading

aballway Apr 14, 2017 •

edited by msft-Jeyaram

Loading

aballway Apr 13, 2017 •

edited by msft-Jeyaram

Loading

aballway Apr 13, 2017 •

edited by msft-Jeyaram

Loading

aballway Apr 13, 2017 •

edited by msft-Jeyaram

Loading

aballway Apr 13, 2017 •

edited by msft-Jeyaram

Loading

aballway Apr 13, 2017 •

edited by msft-Jeyaram

Loading

ms-jihua Apr 13, 2017 •

edited by msft-Jeyaram

Loading

aballway Apr 13, 2017 •

edited by msft-Jeyaram

Loading

DHowett-MSFT Apr 13, 2017 •

edited by msft-Jeyaram

Loading

DHowett-MSFT Apr 13, 2017 •

edited by msft-Jeyaram

Loading

DHowett-MSFT Apr 13, 2017 •

edited by msft-Jeyaram

Loading

DHowett-MSFT Apr 13, 2017 •

edited by msft-Jeyaram

Loading