Implement CTFontCreatePathForGlyph by ms-jihua · Pull Request #1330 · microsoft/WinObjC

ms-jihua · 2016-11-07T22:37:08Z

Minor style changes to CTFont.mm

Fixes #922

This change is

- Minor style changes to CTFont.mm Fixes microsoft#922

ms-jihua · 2016-11-07T22:38:12Z

+    fontTransformWithoutTranslate.ty = 0;
+    CGAffineTransform totalTransform = CGAffineTransformConcat(fontTransformWithoutTranslate, scaleByFontSize);
+
+    // Compare against a transformed version of expectedElements


a transformed version of expectedElements [](start = 23, length = 41)

i wanted to use CGPathCreateCopyByTransformingPath, but we don't have that yet :( #Resolved

DHowett-MSFT · 2016-11-07T22:49:09Z

+}
+
+HRESULT CGPathGeometrySink::RuntimeClassInitialize() {
+    return S_OK;


nit: These two could be in the body of the class, right? #Resolved

yeah, I copied the template from mnithish's code above

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

DHowett-MSFT · 2016-11-07T22:53:59Z

+                break;
+
+            default:
+                EXPECT_TRUE_MSG(false, @"An invalid CGPathElementType was returned from CTFontCreatePathForGlyph");


EXPECT_TRUE_MSG [](start = 16, length = 15)

you could FAIL() << "message" for this to escape the weird EXPECT_TRUE(false) #Resolved

Or, ADD_FAILURE_AT() << "msg" to make it a non-fatal failure.

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

huh, TIL

In reply to: 86888869 [](ancestors = 86888869,86888788)

aballway · 2016-11-07T23:10:41Z

+            CGPathAddCurveToPoint(_cgPath.get(),
+                                  _transform,
+                                  beziers[i].point1.x,
+                                  -beziers[i].point1.y,


-beziers[i].point1.y [](start = 34, length = 20)

nit: maybe add parens around this so the unary - sticks out more, right now it's super sneaky #WontFix

Mm, tried it, it looks funky to me, and I called these out in the class comment anyway. Going to leave as-is.

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

aballway · 2016-11-07T23:12:13Z

+    }
+
+    void STDMETHODCALLTYPE AddLines(_In_ const D2D1_POINT_2F* points, unsigned int pointsCount) {
+        // Could have used CGPathAddLines instead,


// Could have used CGPathAddLines instead, [](start = 8, length = 42)

nit: feels more like a commit comment than in-code #Resolved

Hmmm...

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

ms-jihua · 2016-11-07T23:14:44Z

+
+/**
+ * Custom IDWriteGeometrySink class that captures the glyph runs generated by
+ * DWrite for the given TextLayout and TextRenderer constraints.


oops #Resolved

aballway · 2016-11-07T23:17:02Z

+ * IDWriteFontFace::GetGlyphRunOutline uses this class in a single-threaded manner (dumping the draw instructions from the font linearly)
+ * As such, this class is deliberately left thread-unsafe.
+ */
+class CGPathGeometrySink : public RuntimeClass<RuntimeClassFlags<WinRtClassicComMix>, IDWriteGeometrySink> {


CGPathGeometrySink [](start = 6, length = 18)

nit: class name should follow framework naming convention #Resolved

? what did you have in mind?

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

__CTPathGeometrySink if it's only going to be used here #Resolved

aballway · 2016-11-07T23:18:41Z

+
+CGPathGeometrySink::CGPathGeometrySink(const CGAffineTransform* transform) {
+    _cgPath.reset(CGPathCreateMutable());
+    _transform = transform;


_transform [](start = 4, length = 10)

leaking? #ByDesign

? CGAffineTransforms are not ref-counted o.o

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

aballway · 2016-11-07T23:21:15Z

+                EXPECT_TRUE_MSG(false, @"An invalid CGPathElementType was returned from CTFontCreatePathForGlyph");
+                break;
+        }
+        ++comparePathContext->count;


comparePathContext [](start = 10, length = 18)

nit: parens for clear operator precedence #Resolved

aballway · 2016-11-07T23:23:15Z

aballway · 2016-11-07T23:26:37Z

+        switch (element->type) {
+            case kCGPathElementMoveToPoint:
+            case kCGPathElementAddLineToPoint:
+                EXPECT_NEAR(comparePathContext->expectedElements[comparePathContext->count].point.x, element->points[0].x, 0.002f);


0.002f [](start = 123, length = 6)

nit: make this a constant #Resolved

rajsesh · 2016-11-07T23:20:05Z

+
+CGPathGeometrySink::CGPathGeometrySink(const CGAffineTransform* transform) {
+    _cgPath.reset(CGPathCreateMutable());
+    _transform = transform;


Transform is stored by pointer, thereby requiring the caller manages its lifetime. could you make a copy here instead, so this class can be free-standing? #Resolved

rajsesh · 2016-11-07T23:21:31Z

 static const CFStringRef c_courierNewBoldName = CFSTR("Courier New Bold");
 static const CFStringRef c_courierNewBoldItalicName = CFSTR("Courier New Bold Italic");
 static const CFStringRef c_trebuchetMSItalicName = CFSTR("Trebuchet MS Italic");
+static const CFStringRef c_timesNewRomanName = CFSTR("Times New Roman");


Are we sure these fonts exist on the build server/phone? #ByDesign

yup!

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

rajsesh · 2016-11-07T23:23:35Z

+
+    // CGPath has support for both orders of Bezier curve,
+    // but DWrite's GeometrySink only supports cubic Bezier curves, and approximates any quadratic curves in terms of a cubic curve
+    // Eg: Reference platform quadratic curve:  (previous endpoint),              (512, 632),              (480, 632)


The comments until this point are immensely useful in the context of the implementation code as well, would help to duplicate it there. #Resolved

rajsesh · 2016-11-07T23:28:07Z

+            _invalidState = true;
+        }
+
+        return _invalidState ? E_INVALIDARG : S_OK;


E_UNEXPECTED is probably more appropriate given there are no args to this method. #Resolved

isn't E_UNEXPECTED generally kind of scary? I was trying to go for something a bit lighter.

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

It is essentially saying that the object wasn't expecting the call at this point. E_FAIL is scarier. #Resolved

rajsesh

Per offline discussion, lets add a visual verification test as well, just to be complete. I am changing my status back to request changes so we get the sample work as well.

aballway · 2016-11-08T17:05:10Z

+ * IDWriteFontFace::GetGlyphRunOutline uses this class in a single-threaded manner (dumping the draw instructions from the font linearly)
+ * As such, this class is deliberately left thread-unsafe.
+ */
+class CGPathGeometrySink : public RuntimeClass<RuntimeClassFlags<WinRtClassicComMix>, IDWriteGeometrySink> {


__CTPathGeometrySink if it's only going to be used here #Resolved

aballway · 2016-11-08T17:07:12Z

+private:
+    woc::unique_cf<CGMutablePathRef> _cgPath;
+    const CGAffineTransform* _transform;
+    bool _figureInProgress = false; // Keeps track of whether this class is currently between a BeginFigure and EndFigure call


nit: maybe just an enum to hold the state rather than two bools #WontFix

ms-jihua · 2016-11-08T22:55:06Z

 			isa = PBXGroup;
 			children = (
+				1ED8201D1DD28D4D00DF2DD7 /* CTCPathForGlyphTestViewController.h */,
+				1ED8201A1DD28D2700DF2DD7 /* CTCPathForGlyphTestViewController.h */,


oops I might've added it twice apparently? let me check #Resolved

ms-jihua · 2016-11-08T22:56:25Z

+// - (void)drawRect:(CGRect)rect {
+// }
+
+// @end


oops #Resolved

ms-jihua · 2016-11-09T16:54:08Z

ping!

Implement CTFontCreatePathForGlyph

3c7dada

- Minor style changes to CTFont.mm Fixes microsoft#922

msftclas added the cla-not-required label Nov 7, 2016

ms-jihua commented Nov 7, 2016

View reviewed changes

DHowett-MSFT reviewed Nov 7, 2016

View reviewed changes

aballway reviewed Nov 7, 2016

View reviewed changes

ms-jihua commented Nov 7, 2016

View reviewed changes

aballway reviewed Nov 7, 2016

View reviewed changes

cr feedback

00e02da

aballway reviewed Nov 7, 2016

View reviewed changes

rajsesh approved these changes Nov 7, 2016

View reviewed changes

rajsesh suggested changes Nov 7, 2016

View reviewed changes

aballway reviewed Nov 8, 2016

View reviewed changes

CR feedback, added a test app page for visual verification

781c4de

ms-jihua commented Nov 8, 2016

View reviewed changes

fix minor mistakes in test app changes

012bf8d

rajsesh approved these changes Nov 9, 2016

View reviewed changes

aballway approved these changes Nov 9, 2016

View reviewed changes

ms-jihua merged commit b52a219 into microsoft:develop Nov 9, 2016

Conversation

ms-jihua commented Nov 7, 2016 • edited by rajsesh Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ms-jihua Nov 7, 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!

DHowett-MSFT Nov 7, 2016 • edited by ms-jihua 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 Nov 7, 2016 • edited by ms-jihua 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!

aballway Nov 7, 2016 • edited by ms-jihua 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 Nov 7, 2016 • edited by ms-jihua 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!

ms-jihua Nov 7, 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!

aballway Nov 7, 2016 • edited by ms-jihua Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ms-jihua Nov 7, 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!

aballway Nov 8, 2016 • edited by ms-jihua 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 Nov 7, 2016 • edited by ms-jihua 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 Nov 7, 2016 • edited by ms-jihua 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 commented Nov 7, 2016

Uh oh!

aballway Nov 7, 2016 • edited by ms-jihua Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajsesh Nov 7, 2016 • edited by ms-jihua Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajsesh Nov 7, 2016 • edited by ms-jihua 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 Nov 7, 2016 • edited by ms-jihua Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ms-jihua commented Nov 7, 2016 •

edited by rajsesh

Loading

ms-jihua Nov 7, 2016 •

edited

Loading

DHowett-MSFT Nov 7, 2016 •

edited by ms-jihua

Loading

DHowett-MSFT Nov 7, 2016 •

edited by ms-jihua

Loading

aballway Nov 7, 2016 •

edited by ms-jihua

Loading

aballway Nov 7, 2016 •

edited by ms-jihua

Loading

ms-jihua Nov 7, 2016 •

edited

Loading

aballway Nov 7, 2016 •

edited by ms-jihua

Loading

ms-jihua Nov 7, 2016 •

edited

Loading

aballway Nov 8, 2016 •

edited by ms-jihua

Loading

aballway Nov 7, 2016 •

edited by ms-jihua

Loading

aballway Nov 7, 2016 •

edited by ms-jihua

Loading

aballway Nov 7, 2016 •

edited by ms-jihua

Loading

rajsesh Nov 7, 2016 •

edited by ms-jihua

Loading

rajsesh Nov 7, 2016 •

edited by ms-jihua

Loading

rajsesh Nov 7, 2016 •

edited by ms-jihua

Loading

rajsesh Nov 7, 2016 •

edited by ms-jihua

Loading

rajsesh Nov 7, 2016 •

edited by ms-jihua

Loading

aballway Nov 8, 2016 •

edited by ms-jihua

Loading

aballway Nov 8, 2016 •

edited by ms-jihua

Loading

ms-jihua Nov 8, 2016 •

edited

Loading

ms-jihua Nov 8, 2016 •

edited

Loading