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

Fix scaling and translation text drawing issues#1254

Merged
rajsesh merged 3 commits into
microsoft:developfrom
aballway:coretext
Oct 29, 2016
Merged

Fix scaling and translation text drawing issues#1254
rajsesh merged 3 commits into
microsoft:developfrom
aballway:coretext

Conversation

@aballway

@aballway aballway commented Oct 27, 2016

Copy link
Copy Markdown
Contributor

CALayers were being scaled twice, and NSString draw methods were being translated twice, causing issues in internal test apps


This change is Reviewable


@implementation NSString (UIKitAdditions)

static CTTextAlignment __UITextAlignmentToCTTextAlignment(UITextAlignment alignment) {

@ms-jihua ms-jihua Oct 27, 2016

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.

not like this - look at what I did in UIKitTypes.h for NS, UI, CT LineBreakMode. Then you can change it back to a static_cast below. #ByDesign

@aballway aballway Oct 28, 2016

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.

Double checked - the values between CT/UI are different, and we can't change the values of NS/UI because our internal test app (and presumably others) are depending on those values staying the same

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.

eesh


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

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.

add a comment here about the non-agreeing order and why we need this


In reply to: 85579206 [](ancestors = 85579206,85569934)

@ms-jihua

ms-jihua commented Oct 27, 2016

Copy link
Copy Markdown
Contributor

:shipit:

}
if (priv->contentsScale != 1.0f) {
CGContextScaleCTM(drawContext, priv->contentsScale, priv->contentsScale);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This scale is for Cairo's sake and needs to be done.
Perhaps we should keep it and apply an inverse scale based on the DPI to the D2D context instead of eliminating it here.

@DHowett-MSFT

Copy link
Copy Markdown

🕐

@msft-Jeyaram

Copy link
Copy Markdown
int j = 0;

uh I just noticed this, although this is in. i,j might not of been the best choice to denote what they actually represent.


Refers to: Frameworks/CoreText/DWriteWrapper_CoreText.mm:397 in 90d1c31. [](commit_id = 90d1c31, deletion_comment = False)

enum {
UITextAlignmentLeft,
UITextAlignmentLeft = 0,
UITextAlignmentCenter,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

make this an NE_ENUM, such as above?

@aballway aballway Oct 28, 2016

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.

This makes UI/NS unable to be coerced, which I don't believe is intended behavior

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Perhaps NS should just be implemented in terms of UI?


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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

or the other way around actually.. yeah


In reply to: 85629574 [](ancestors = 85629574,85553544)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We had this as NS_ENUM before; it's somewhat silly but it makes it unable to be used itnerchangeably with NSTextAlignment.


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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Look in the history for this file for potentially more info!


In reply to: 85629604 [](ancestors = 85629604,85448628)

CGFloat ascent;
CTLineGetTypographicBounds(static_cast<CTLineRef>(lines[i]), &ascent, nullptr, nullptr);
CGContextSetTextPosition(context, origins[i].x + rect.origin.x, origins[i].y + rect.origin.y - ascent / 2.0);
// Need to set text position so each line will be drawn in the correct position relative to eachother

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

eachother [](start = 100, length = 9)

each other


// TODO 1077:: Remove once D2D render target is implemented
_CGContextSetScaleFactor(drawContext, priv->contentsScale);
CGContextScaleCTM(drawContext, priv->contentsScale, priv->contentsScale);

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.

simply revert all changes in this file?

@rajsesh

rajsesh commented Oct 28, 2016

Copy link
Copy Markdown
Contributor

:shipit:

@aballway

Copy link
Copy Markdown
Contributor Author

New iteration correctly addresses drawing problems on high dpi devices


CGFloat ascent, leading;
CTLineGetTypographicBounds(line, &ascent, nullptr, &leading);
CGContextSetTextPosition(curCtx, _lineOrigins[curLine].x, -(_lineOrigins[curLine].y + ascent + leading));

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.

descent missing here intentionally?

@rajsesh rajsesh merged commit 971bf6e into microsoft:develop Oct 29, 2016
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.

6 participants