[ASTextNode2] Add initial implementation for link handling. #396
Conversation
This is a fairly basic first step to achieving feature parity between ASTextNode2 and ASTextNode. It does not yet do the 9-box detection of links, and there is other code from ASTextNode that could be shared to improve this. However, in the interest of getting a shippable implementation running as soon as possible, I'm hoping to quickly refine this. Then we can continue improving it based on the original ASTextNode with the benefit of testing UI against it.
Source/Private/ASTextNode2.mm
Outdated
| NSRange effectiveRange = NSMakeRange(0, 0); | ||
| for (NSString *attributeName in self.linkAttributeNames) { | ||
| id value = [self.attributedText attribute:attributeName atIndex:range.start.offset longestEffectiveRange:&effectiveRange inRange:clampedRange]; | ||
| NSString *name = attributeName; |
There was a problem hiding this comment.
Nit: remove extra local variable or make the difference between name and attributeName more clear.
There was a problem hiding this comment.
Done. Doing this required adding a semi-ugly __strong to the iteration variable declaration, as the compiler does generate an error for writing to that variable without it.
Source/Private/ASTextNode2.mm
Outdated
|
|
||
| ASTextContainer *containerCopy = [_textContainer copy]; | ||
| containerCopy.size = self.calculatedSize; | ||
| ASTextLayout *layout = [ASTextNode2 compatibleLayoutWithContainer:containerCopy text:_attributedText]; |
There was a problem hiding this comment.
I don't think it's required for us to copy the container here (or below at line 740). Except in extreme edge cases, _textContainer will be at the displayed size. Is this just extra safety, or was this necessary to make it work?
There was a problem hiding this comment.
@Adlai-Holler unfortunately, without these, link handling does not work. I could try removing just one or the other, but instead I think it is going to require a more detailed look at what is going on.
There was a problem hiding this comment.
OK would you be willing to add a comment linking to this discussion?
Source/Private/ASTextNode2.mm
Outdated
| if (value != nil || name != nil) { | ||
| *rangeOut = effectiveRange; | ||
| if (NSMaxRange(*rangeOut) > NSMaxRange(visibleRange)) { | ||
| (*rangeOut).length = MAX(NSMaxRange(visibleRange) - (*rangeOut).location, 0); |
There was a problem hiding this comment.
Is it reasonable for this just to be *rangeOut = NSIntersectionRange(effectiveRange, visibleRange) or am I missing something?
Source/Private/ASTextNode2.mm
Outdated
|
|
||
| ASTextContainer *containerCopy = [_textContainer copy]; | ||
| containerCopy.size = self.calculatedSize; | ||
| ASTextLayout *layout = [ASTextNode2 compatibleLayoutWithContainer:containerCopy text:_attributedText]; |
There was a problem hiding this comment.
OK would you be willing to add a comment linking to this discussion?
…roup#396) * [ASTextNode2] Add initial implementation for link handling. This is a fairly basic first step to achieving feature parity between ASTextNode2 and ASTextNode. It does not yet do the 9-box detection of links, and there is other code from ASTextNode that could be shared to improve this. However, in the interest of getting a shippable implementation running as soon as possible, I'm hoping to quickly refine this. Then we can continue improving it based on the original ASTextNode with the benefit of testing UI against it. * [ASTextNode2] Refine implementation of link handling.
This is a fairly basic first step to achieving feature parity between ASTextNode2
and ASTextNode. It does not yet do the 9-box detection of links, and there is
other code from ASTextNode that could be shared to improve this.
However, in the interest of getting a shippable implementation running as soon
as possible, I'm hoping to quickly refine this. Then we can continue improving
it based on the original ASTextNode with the benefit of testing UI against it.