[ASTextNode2] Integrating Google’s changes #2005
Conversation
I isolated the changes from ASTextNode2 from the large google diff. I got things building, but some snapshot tests are failing for ASTextNode2. The tests that are failing: ``` -[ASTextNode2SnapshotTests testTextContainerInsetHighlight_ASTextNode2] -[ASTextNode2SnapshotTests testTextTintColor_ASTextNode2] -[ASTextNode2SnapshotTests testTextTruncationModes_ASTextNode2] -[ASTextNode2SnapshotTests testTextTruncationModes_ASTextNode2] -[ASTextNode2SnapshotTests testThatDefaultTruncationTokenAttributesAreInheritedFromTextWhenTruncated_ASTextNode2] -[ASTextNode2SnapshotTests testTintColorHierarchyChange] ``` I also noticed that several tests were removed from ASTextNode2Tests.mm. Some notes on my merge: * I revert the `ASDN` namespace back to `AS` until we can change everything at once * I called `DISABLED_ASAssertLocked` unstead of `ASAssertLocked`. Again thinking if this changed, we could do them all in one diff * Brought back `AS_TN2_CLASSNAME` since the text experiement `ASTextNode` is still the default on master I’d like to discuss the snapshot failures and if they are expected/how to fix them. I’m also curious why some other tests were removed.
|
I ran these tests locally via |
|
Do you guys think you want the accessibility changes from Google? We're not sure if they are excluded because you don't need/want them (in which case we should just delete them) or if you just thought they belonged in a different PR |
| @@ -131,8 +137,6 @@ @implementation ASTextCacheValue | |||
| return layout; | |||
There was a problem hiding this comment.
I like our version of the comment on line 120
There was a problem hiding this comment.
I'm not sure I see a different comment
| static NSCache<NSAttributedString *, ASTextCacheValue *> *textLayoutCache; | ||
| static dispatch_once_t onceToken; |
There was a problem hiding this comment.
We haven't observed the issue described here #136 but it's not clear how it would come up or how this change corrects it. Can you clarify?
There was a problem hiding this comment.
This is overall missing several of the google changes, like for text attachments and highlighting. Is this a subset of the Unified PR we put up, or does it include additional changes? Do you think it would be easiest as far as integrating the Google work to essentially break that PR into smaller topical pieces, perhaps with modifications?
I isolated the changes from
ASTextNode2from the large google diff. I got things building, but some snapshot tests are failing forASTextNode2. The tests that are failing are:I also noticed that several tests were removed from ASTextNode2Tests.mm.
Some notes on my merge:
ASDNnamespace back toASuntil we can change everything at onceDISABLED_ASAssertLockedinstead ofASAssertLocked. Again thinking if this changed, we could do them all in one diffAS_TN2_CLASSNAMEsince the text experiment still exists andASTextNodeis the default text node on masterI’d like to discuss the snapshot failures and if they are expected/how to fix them. I’m also curious why some other tests were removed.