Correct linePositionModifier behavior #1192

maicki · 2018-10-25T16:52:31Z

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.

Adlai-Holler

Nice, just some nits

Adlai-Holler · 2018-10-25T18:17:56Z

Source/ASTextNode.h

+ */
+@interface ASTextNode (Unsupported)
+
+@property (nonatomic) id _Nullable textContainerLinePositionModifier;


Suggested change

@property (nonatomic) id _Nullable textContainerLinePositionModifier;

@property (nullable, nonatomic) id textContainerLinePositionModifier;

Adlai-Holler · 2018-10-25T18:18:19Z

Source/ASTextNode2.h

@@ -207,6 +209,10 @@ NS_ASSUME_NONNULL_BEGIN

 + (void)enableDebugging;

+#pragma mark - Layout and Sizing
+
+@property (nonatomic) id<ASTextLinePositionModifier> _Nullable textContainerLinePositionModifier;


Suggested change

@property (nonatomic) id<ASTextLinePositionModifier> _Nullable textContainerLinePositionModifier;

@property (nonatomic, nullable) id<ASTextLinePositionModifier> textContainerLinePositionModifier;

Adlai-Holler · 2018-10-25T18:18:45Z

Source/ASTextNode2.mm

+{
+  ASLockScopeSelf();
+  if (_textContainer.linePositionModifier != modifier) {
+    _textContainer.linePositionModifier = modifier;


Suggested change

_textContainer.linePositionModifier = modifier;

ASLockedSelfCompareAssignObjects(_textContainer.linePositionModifier, modifier);

I think delete the ASLockScopeSelf() as well if you take accept this suggestion (as it's built into the macro)

Yeah not need for ASLockScopeSelf, see updated version.

Adlai-Holler · 2018-10-25T18:19:49Z

Source/Private/TextExperiment/Component/ASTextLayout.m

+
+  // Give user a chance to modify the line's position.
+  if (container.linePositionModifier) {
+    [container.linePositionModifier modifyLines:lines fromText:text inContainer:container];


No need to check since nil will no-op.

Adlai-Holler · 2018-10-25T18:24:07Z

Source/ASTextNode2.h

@@ -207,6 +209,10 @@ NS_ASSUME_NONNULL_BEGIN

 + (void)enableDebugging;

+#pragma mark - Layout and Sizing
+
+@property (nonatomic) id<ASTextLinePositionModifier> _Nullable textContainerLinePositionModifier;


The text container is an implementation detail, so I'd just go with linePositionModifier.

Suggested change

@property (nonatomic) id<ASTextLinePositionModifier> _Nullable textContainerLinePositionModifier;

@property (nonatomic, nullable) id<ASTextLinePositionModifier> textContainerLinePositionModifier;

Adlai-Holler · 2018-10-25T18:24:40Z

Source/ASTextNode.h

+ */
+@interface ASTextNode (Unsupported)
+
+@property (nonatomic) id _Nullable textContainerLinePositionModifier;


Same, use nullable property attribute.

Adlai-Holler · 2018-10-25T18:25:27Z

Source/Private/TextExperiment/Component/ASTextLayout.m

+
+  // Give user a chance to modify the line's position.
+  if (container.linePositionModifier) {
+    [container.linePositionModifier modifyLines:lines fromText:text inContainer:container];


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).

Good point, there used to be more in the code block, but all the rest of it melted away.

wiseoldduck · 2018-10-25T18:46:00Z

omg this is amazingly inspired code! I shudder at its inconceivable genius, and think any changes would be almost sacrilegious.

wiseoldduck · 2018-10-25T18:50:54Z

CHANGELOG.md

@@ -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)


We need to reword this since it's not at all what is happening from the Texture perspective

Suggested change

- 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)

wiseoldduck · 2018-10-25T18:56:36Z

Source/Private/TextExperiment/Component/ASTextLayout.m

+
+  // Give user a chance to modify the line's position.
+  if (container.linePositionModifier) {
+    [container.linePositionModifier modifyLines:lines fromText:text inContainer:container];


Good point, there used to be more in the code block, but all the rest of it melted away.

wiseoldduck · 2018-10-25T19:07:14Z

Source/ASTextNode2.mm

+{
+  ASLockScopeSelf();
+  if (_textContainer.linePositionModifier != modifier) {
+    _textContainer.linePositionModifier = modifier;


I think delete the ASLockScopeSelf() as well if you take accept this suggestion (as it's built into the macro)

nguyenhuy · 2018-10-25T19:21:00Z

Source/ASTextNode2.mm

+{
+  ASLockScopeSelf();
+  if (_textContainer.linePositionModifier != modifier) {
+    _textContainer.linePositionModifier = modifier;


maicki requested review from Adlai-Holler and nguyenhuy October 25, 2018 16:53

Adlai-Holler approved these changes Oct 25, 2018

View reviewed changes

wiseoldduck reviewed Oct 25, 2018

View reviewed changes

nguyenhuy approved these changes Oct 25, 2018

View reviewed changes

maicki force-pushed the MSCapToFontHeightOption branch from cf0ca97 to 3414d9c Compare October 25, 2018 19:40

maicki changed the title ~~Implement cap-to-font-height option~~ Correct linePositionModifier behavior Oct 25, 2018

Correct linePositionModifier behavior

990b240

maicki force-pushed the MSCapToFontHeightOption branch from 3414d9c to 990b240 Compare October 25, 2018 19:41

maicki merged commit 25a3d33 into master Oct 26, 2018

maicki deleted the MSCapToFontHeightOption branch October 27, 2018 00:21

mikezucc pushed a commit to mikezucc/Texture that referenced this pull request Nov 7, 2018

Correct linePositionModifier behavior (TextureGroup#1192)

986814d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correct linePositionModifier behavior #1192

Correct linePositionModifier behavior #1192

maicki commented Oct 25, 2018

Adlai-Holler left a comment

Adlai-Holler Oct 25, 2018

Adlai-Holler Oct 25, 2018

Adlai-Holler Oct 25, 2018

wiseoldduck Oct 25, 2018

nguyenhuy Oct 25, 2018

maicki Oct 25, 2018

Adlai-Holler Oct 25, 2018

Adlai-Holler Oct 25, 2018

Adlai-Holler Oct 25, 2018

Adlai-Holler Oct 25, 2018

wiseoldduck Oct 25, 2018

wiseoldduck commented Oct 25, 2018

wiseoldduck Oct 25, 2018

wiseoldduck Oct 25, 2018

wiseoldduck Oct 25, 2018

wiseoldduck Oct 25, 2018

nguyenhuy Oct 25, 2018

	@property (nonatomic) id _Nullable textContainerLinePositionModifier;
	@property (nullable, nonatomic) id textContainerLinePositionModifier;

	@property (nonatomic) id<ASTextLinePositionModifier> _Nullable textContainerLinePositionModifier;
	@property (nonatomic, nullable) id<ASTextLinePositionModifier> textContainerLinePositionModifier;

	_textContainer.linePositionModifier = modifier;
	ASLockedSelfCompareAssignObjects(_textContainer.linePositionModifier, modifier);

	- 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)

Correct linePositionModifier behavior #1192

Correct linePositionModifier behavior #1192

Conversation

maicki commented Oct 25, 2018

Adlai-Holler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wiseoldduck commented Oct 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment