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

Conversation

@aballway
Copy link
Contributor

@aballway aballway commented Oct 31, 2016

Fixes incorrect rotation introduced by my recent math changes and adds descent to nslayoutmanager line drawing to make exclusion zones more accurate

Fixes #1275


This change is Reviewable


CGFloat ascent, leading;
CGFloat ascent, descent, leading;
CTLineGetTypographicBounds(line, &ascent, nullptr, &leading);
Copy link
Contributor

Choose a reason for hiding this comment

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

nullptr, [](start = 50, length = 8)

you're not currently getting the descent though, so it doesn't look like this would've changed anything?

Copy link
Contributor

Choose a reason for hiding this comment

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

(i think ascent + leading would've been correct here in any case)


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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, added a comment and reverted change

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I would probably just make the comment be about what the SetTextPosition is actually doing? "because CG origin is the opposite vertically from UIKit origin, flip the y line origin from upper left corner to lower left corner (baseline)", except use better wording than i did


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


// Perform anti-clockwise rotation required to match the reference platform.
imgRenderTarget->SetTransform(D2D1::Matrix3x2F(transform.a, -transform.b, transform.c, transform.d, transform.tx, -transform.ty));
imgRenderTarget->SetTransform(D2D1::Matrix3x2F(transform.a, -transform.b, -transform.c, transform.d, transform.tx, -transform.ty));
Copy link
Contributor

Choose a reason for hiding this comment

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

-transform.c [](start = 78, length = 12)

i'd been wondering for a while why this was missing...

Copy link
Contributor Author

@aballway aballway Oct 31, 2016

Choose a reason for hiding this comment

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

I'm not quite sure how it worked before

@ms-jihua
Copy link
Contributor

:shipit:

Copy link
Contributor

@rajsesh rajsesh left a comment

Choose a reason for hiding this comment

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

Good catch. Please add a page to CTCatalog for rotate. Also I'd like to start adding automated tests that validate the contents of bitmap context to catch regressions that would be easy to miss with untrained eye.

<AppxManifest Include="Package.appxmanifest">
<SubType>Designer</SubType>
</AppxManifest>
<ClangCompile Include="..\..\CTCatalog\Tests\CTCAffineTransformationTestViewController.mm" />
Copy link
Contributor

Choose a reason for hiding this comment

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

These files also need to be added to xcodeproject and should compile and run on reference platform.

Copy link
Contributor Author

@aballway aballway Oct 31, 2016

Choose a reason for hiding this comment

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

Already done and tested

[_translateYSlider setNeedsDisplay];
}

- (void)didReceiveMemoryWarning {
Copy link
Contributor

Choose a reason for hiding this comment

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

these functions can be deleted.


#import "CTCAffineTransformationTestViewController.h"

static const NSString* text = @"The quick brown dog jumps over the lazy fox. THE QUICK BROWN DOG JUMPS OVER THE LAZY FOX.";
Copy link
Contributor

Choose a reason for hiding this comment

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

I would change this text to read what the expected test result should be (changing rotate should cause the text to rotate in which direction?).


setting.spec = kCTParagraphStyleSpecifierAlignment;
setting.valueSize = sizeof(CTTextAlignment);
CTTextAlignment alignment = kCTCenterTextAlignment;
Copy link
Contributor

Choose a reason for hiding this comment

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

left alignment here to easily see the pivot point?

Copy link
Contributor

@ms-jihua ms-jihua left a comment

Choose a reason for hiding this comment

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

nit: it'd be nice if the sliders had labels, but it's fine as-is


// Creates outline
CGContextSetLineWidth(context, 2.0);
CGColorSpaceRef colorspace = CGColorSpaceCreateDeviceRGB();

Choose a reason for hiding this comment

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

This colorspace does not seem to be used.

@aballway aballway merged commit 85ccfb8 into microsoft:develop Nov 1, 2016
rajsesh pushed a commit to rajsesh/WinObjC that referenced this pull request Nov 3, 2016
* Fix rotation and nslayout line drawing

* Remove descent and add comment

* Add CTCatalog page for testing CTM transformation

* Add new test page to xcodeproj

* Address CR feedback

* Remove vestigial colorspace

Fixes incorrect rotation introduced by my recent math changes and adds descent to nslayoutmanager line drawing to make exclusion zones more accurate

Fixes microsoft#1275
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