Don’t handle touches on additional attributed message if passthrough is enabled #1184
Conversation
Source/ASTextNode.mm
Outdated
| @@ -974,7 +974,7 @@ - (BOOL)pointInside:(CGPoint)point withEvent:(UIEvent *)event | |||
| BOOL linkCrossesVisibleRange = (lastCharIndex > range.location) && (lastCharIndex < NSMaxRange(range) - 1); | |||
|
|
|||
| if (inAdditionalTruncationMessage) { | |||
| return YES; | |||
| return !_passthroughNonlinkTouches; | |||
There was a problem hiding this comment.
Right above here we short-circuit if (!_passthroughNonlinkTouches). So this is equivalent to return NO. We should ideally remove this branch entirely, and simply return YES if we're in a link and NO otherwise. Whether we're in the truncation message or not doesn't matter for this method.
There was a problem hiding this comment.
@Adlai-Holler Agree and addressed
|
🚫 CI failed with log |
83846ac to
c9cd1b5
Compare
Source/ASTextNode.mm
Outdated
| if (inAdditionalTruncationMessage) { | ||
| return YES; | ||
| } else if (range.length && !linkCrossesVisibleRange && linkAttributeValue != nil && linkAttributeName != nil) { | ||
| if (range.length && !linkCrossesVisibleRange && linkAttributeValue != nil && linkAttributeName != nil) { |
There was a problem hiding this comment.
The range.length check should probably have > 0 just to be a bit more formal than implicit cast to BOOL. If you decide to change it, there's another instance of this check below in ASTextNode2.
Source/ASTextNode2.mm
Outdated
| @@ -194,6 +194,7 @@ - (instancetype)init | |||
| self.userInteractionEnabled = NO; | |||
| self.needsDisplayOnBoundsChange = YES; | |||
|
|
|||
| // The default truncation type is end for truncation | |||
There was a problem hiding this comment.
Probably don't need a comment here, but one in the header file (if not there already) would be useful.
| BOOL inAdditionalTruncationMessage = NO; | ||
|
|
||
| CTLineRef truncatedCTLine = layout.truncatedLine.CTLine; | ||
| if (truncatedCTLine != NULL && _additionalTruncationMessage != nil) { |
There was a problem hiding this comment.
Should we check _additionalTruncationMessage.length > 0 in these various cases? Probably rare for the value to be set to @"" I guess, so it may not matter.
There was a problem hiding this comment.
Across the text nodes where are checking _additionalTruncationMessage != nil if we determine if the an _additionalTruncationMessage is set. I would suggest we keep it as _additionalTruncationMessage != nil
Source/ASTextNode2.mm
Outdated
| CFIndex truncatedCTLineGlyphCount = CTLineGetGlyphCount(truncatedCTLine); | ||
|
|
||
| CTLineRef truncationTokenLine = | ||
| CTLineCreateWithAttributedString((CFAttributedStringRef)_truncationAttributedText); |
There was a problem hiding this comment.
Bump this to the line above, and on the several lines below. No 100 char line limits in Texture, thank goodness!
Source/ASTextNode2.mm
Outdated
| switch (_textContainer.truncationType) { | ||
| case ASTextTruncationTypeStart: { | ||
| if (stringIndexForPosition > truncationTokenLineGlyphCount && | ||
| stringIndexForPosition < (additionalTruncationTokenLineGlyphCount + truncationTokenLineGlyphCount)) { |
There was a problem hiding this comment.
This expression is used here and on line 648, though with the variables added together in the opposite order. Is there a descriptive name we can give for this value and put it in a variable for better readability?
| + (ASConfiguration *)textureConfiguration | ||
| { | ||
| ASConfiguration *configuration = [ASConfiguration new]; | ||
| // configuration.experimentalFeatures = ASExperimentalTextNode; |
There was a problem hiding this comment.
Possibly worth landing it with this enabled :). Otherwise probably delete the commented line.
… is enabled (TextureGroup#1184) * Don’t handle touches on additional attributed message if passthrough is enabled * Cleanup * Don't handle extra inAdditionalTruncationMessage within pointInside:withEvent: * Update changelog * Address comments
This diff will change the behavior handling tapping on the additional attributed message:
If
passthroughNonlinkTouchesis set to YES, taps on the additional attributed message will not call thetextNodeTappedTruncationToken:delegate method anymore. Previously tapping on the additional attributed message always calledtextNodeTappedTruncationToken:without any dependence onpassthroughNonlinkTouches.Another approach could be to add a new property like:
handleAdditionalTruncationStringthat if set to YES will calltextNodeTappedTruncationToken:otherwise not. This would not depend onpassthroughNonlinkTouches.Furthermore this diff fixes taps within the additional attributed message within
ASTextNode2what currently didn't really work. Before this the tap was not detected if tapped directly on the additional attributed text, it was only detected if tapped a bit outside of the additional attributed text but still within theASTextNode2bounds.