-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Correct linePositionModifier behavior #1192
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, just some nits
Source/ASTextNode.h
Outdated
*/ | ||
@interface ASTextNode (Unsupported) | ||
|
||
@property (nonatomic) id _Nullable textContainerLinePositionModifier; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@property (nonatomic) id _Nullable textContainerLinePositionModifier; | |
@property (nullable, nonatomic) id textContainerLinePositionModifier; |
Source/ASTextNode2.h
Outdated
@@ -207,6 +209,10 @@ NS_ASSUME_NONNULL_BEGIN | |||
|
|||
+ (void)enableDebugging; | |||
|
|||
#pragma mark - Layout and Sizing | |||
|
|||
@property (nonatomic) id<ASTextLinePositionModifier> _Nullable textContainerLinePositionModifier; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@property (nonatomic) id<ASTextLinePositionModifier> _Nullable textContainerLinePositionModifier; | |
@property (nonatomic, nullable) id<ASTextLinePositionModifier> textContainerLinePositionModifier; |
Source/ASTextNode2.mm
Outdated
{ | ||
ASLockScopeSelf(); | ||
if (_textContainer.linePositionModifier != modifier) { | ||
_textContainer.linePositionModifier = modifier; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_textContainer.linePositionModifier = modifier; | |
ASLockedSelfCompareAssignObjects(_textContainer.linePositionModifier, modifier); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think delete the ASLockScopeSelf()
as well if you take accept this suggestion (as it's built into the macro)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah not need for ASLockScopeSelf
, see updated version.
|
||
// Give user a chance to modify the line's position. | ||
if (container.linePositionModifier) { | ||
[container.linePositionModifier modifyLines:lines fromText:text inContainer:container]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to check since nil will no-op.
Source/ASTextNode2.h
Outdated
@@ -207,6 +209,10 @@ NS_ASSUME_NONNULL_BEGIN | |||
|
|||
+ (void)enableDebugging; | |||
|
|||
#pragma mark - Layout and Sizing | |||
|
|||
@property (nonatomic) id<ASTextLinePositionModifier> _Nullable textContainerLinePositionModifier; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The text container is an implementation detail, so I'd just go with linePositionModifier
.
@property (nonatomic) id<ASTextLinePositionModifier> _Nullable textContainerLinePositionModifier; | |
@property (nonatomic, nullable) id<ASTextLinePositionModifier> textContainerLinePositionModifier; |
Source/ASTextNode.h
Outdated
*/ | ||
@interface ASTextNode (Unsupported) | ||
|
||
@property (nonatomic) id _Nullable textContainerLinePositionModifier; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same, use nullable property attribute.
|
||
// Give user a chance to modify the line's position. | ||
if (container.linePositionModifier) { | ||
[container.linePositionModifier modifyLines:lines fromText:text inContainer:container]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to read the property twice – nil will no-op. If you want to keep the if-statement for readability then assign to a local variable (too bad this file isn't mm or you could if (auto modifier = container.linePositionModifier)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, there used to be more in the code block, but all the rest of it melted away.
omg this is amazingly inspired code! I shudder at its inconceivable genius, and think any changes would be almost sacrilegious. |
CHANGELOG.md
Outdated
@@ -65,6 +65,7 @@ | |||
- Add NSLocking conformance to ASNodeController [Michael Schneider](https://github.com/maicki)[#1179] (https://github.com/TextureGroup/Texture/pull/1179) | |||
- Don’t handle touches on additional attributed message if passthrough is enabled [Michael Schneider](https://github.com/maicki)[#1184] (https://github.com/TextureGroup/Texture/pull/1184) | |||
- Yoga integration improvements [Michael Schneider](https://github.com/maicki)[#1187] (https://github.com/TextureGroup/Texture/pull/1187) | |||
- Implement cap-to-font-height option [Michael Schneider](https://github.com/maicki)[#1192] (https://github.com/TextureGroup/Texture/pull/1192) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to reword this since it's not at all what is happening from the Texture perspective
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Implement cap-to-font-height option [Michael Schneider](https://github.com/maicki)[#1192] (https://github.com/TextureGroup/Texture/pull/1192) | |
- Correct linePositionModifier behavior [Michael Schneider](https://github.com/maicki)[#1192] (https://github.com/TextureGroup/Texture/pull/1192) |
|
||
// Give user a chance to modify the line's position. | ||
if (container.linePositionModifier) { | ||
[container.linePositionModifier modifyLines:lines fromText:text inContainer:container]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, there used to be more in the code block, but all the rest of it melted away.
Source/ASTextNode2.mm
Outdated
{ | ||
ASLockScopeSelf(); | ||
if (_textContainer.linePositionModifier != modifier) { | ||
_textContainer.linePositionModifier = modifier; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think delete the ASLockScopeSelf()
as well if you take accept this suggestion (as it's built into the macro)
Source/ASTextNode2.mm
Outdated
{ | ||
ASLockScopeSelf(); | ||
if (_textContainer.linePositionModifier != modifier) { | ||
_textContainer.linePositionModifier = modifier; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
cf0ca97
to
3414d9c
Compare
3414d9c
to
990b240
Compare
iOS will adjust line spacing if it pulls in out-of-font glyphs (ie emojis).
The cap-to-font-height option will override this adjustment, keeping line spacing (baseline-to-baseline) consistent but possibly crowding the emoji (or other out-of-font glyphs).
Implementing required ASTextNode2 to expose the linePositionModifier delegate used by ASTextLayout.