Add Experimental Text Node Implementation #259

Adlai-Holler · 2017-05-11T01:35:36Z

https://paper.dropbox.com/doc/ASTextNode-Experimental-Replacement-bswIlZJjmjDj1fJjwegzH

I'd like to get this reviewed and merged so that we can start experimenting with it in Pinterest and YouTube, even though the experimental implementation is missing some features.

@garrettmoon @nguyenhuy @maicki @appleguy

cc #221 & #227

ghost · 2017-05-11T01:57:57Z

🚫 CI failed with log

nguyenhuy · 2017-05-11T10:17:50Z

Not sure if GitHub is messing with me, or this diff actually has 10k lines :P

Can we somehow break it into chunks? And/or link against YYText (if you haven't done so)?

maicki · 2017-05-11T14:15:12Z

@nguyenhuy My understanding is that this diff is really 10k lines :) We don't fully link against YYText as we don't need all of the components, furthermore it seems like the projects is kind of a dead state :/

Adlai-Holler · 2017-05-11T15:24:49Z

Lol yep the diff is 10k lines. I think we shouldn't link to YYText since it's dead, since we don't need most of it, since we will need to fork it to make modifications (I've already found one, around the shadowing of truncated lines), and also because it's a hassle to our users.

nguyenhuy · 2017-05-11T15:56:50Z

OK. So that's 10k lines we're gonna closely review now and maintain later. However, if the code quality and architecture is better than our current implementation, and if in the future we'll completely switch to it, it might actually turn out to be a net gain!

…sed.

garrettmoon

Jesus I hope I reviewed the right parts.

garrettmoon · 2017-05-13T01:05:54Z

Source/ASTextNode.mm

+
+    // They must call this before allocating any text nodes.
+    ASDisplayNodeAssertFalse(_hasAllocatedNode);
+    ASDisplayNodeAssert(_experimentOptions == 0, @"Can't call +setExperimentOptions: more than once.");


You sure can't it's in a dispatch_once

garrettmoon · 2017-05-13T01:11:18Z

Source/Private/ASTextNode2.mm

+{
+  NSString *plainString = [[self.attributedText string] stringByTrimmingCharactersInSet:[NSCharacterSet newlineCharacterSet]];
+  if (plainString.length > 50) {
+    plainString = [[plainString substringToIndex:50] stringByAppendingString:@"\u2026"];


Super super nit, I assume this evaluates to …, maybe just use the character?

garrettmoon · 2017-05-13T01:11:52Z

Source/Private/ASTextNode2.mm

+  // We discard the backing store and renderer to prevent the very large
+  // memory overhead of maintaining these for all text nodes.  They can be
+  // regenerated when layout is necessary.
+  [super clearContents];      // ASDisplayNode will set layer.contents = nil


Is this method necessary?

garrettmoon · 2017-05-13T01:13:55Z

Source/Private/ASTextNode2.mm

+  return layout.textBoundingSize;
+
+
+  //	ASTextKitRenderer *renderer = [self _rendererWithBoundsSlow:{.size = constrainedSize}];


garrettmoon · 2017-05-13T01:15:25Z

Source/Private/ASTextNode2.mm

+
+  // Accessiblity
+  self.accessibilityLabel = attributedText.string;
+  self.isAccessibilityElement = (length != 0); // We're an accessibility element by default if there is a string.


Probably not, but is there any way attributedText.length could be different than attributedText.string.length?

No I don't think so.

garrettmoon · 2017-05-13T01:25:54Z

Source/Private/ASTextNode2.mm

+    textLayoutCache = [[NSCache alloc] init];
+  });
+
+  ASTextCacheValue *cacheValue = ({


What happens below when a bunch of nodes with the same text all go through this path? The first one sets an empty cache value and the rest of them get it. Then they all 'cache miss' below and layout?

Yep. It's an acceptable tradeoff, since (1) we will mostly be doing sequential passes on the same attributed string, not parallel ones and (2) the alternative would either be complex or amount to having all other text-layout work wait.

garrettmoon · 2017-05-13T01:27:56Z

Source/Private/ASTextNode2.mm

+  {
+    std::lock_guard<std::mutex> lock(cacheValue->_m);
+    cacheValue->_layouts.push_front(std::make_tuple(container.size, layout));
+    if (cacheValue->_layouts.size() > 3) {


Is 3 a magic number? I wonder if there's value in having control of this. My guess is specific strings have layouts that would be worth caching and most have 0

Is 3 a magic number?

Yes.

I wonder if there's value in having control of this. My guess is specific strings have layouts that would be worth caching and most have 0

No probably not. This cache is primarily intended to serve between layout passes and displays of the same text node. Any benefit that we get across the app is negligible (but great!).

garrettmoon · 2017-05-13T01:29:06Z

Source/Private/ASTextNode2.mm

+    std::lock_guard<std::mutex> lock(cacheValue->_m);
+    cacheValue->_layouts.push_front(std::make_tuple(container.size, layout));
+    if (cacheValue->_layouts.size() > 3) {
+      cacheValue->_layouts.pop_back();


Eh, not sure how much it's worth optimizing, but it may be worth making this LRU as opposed to first layout loses.

Yep, a couple lines up there is a TODO for this.

garrettmoon · 2017-05-13T01:30:16Z

Source/Private/ASTextNode2.mm

+  return _shadowColor;
+}
+
+- (void)setShadowColor:(CGColorRef)shadowColor


We should take this opportunity to make it a UIColor?

No because this property is inherited from ASDisplayNode.

garrettmoon · 2017-05-13T01:31:45Z

Source/Private/ASTextNode2.mm

+  ASTextTruncationType yyType;
+  switch (truncationMode) {
+    case NSLineBreakByTruncatingHead:
+      yyType = ASTextTruncationTypeStart;


Looks like the license at the top of this needs to give credit to yy?

No this file uses code from YY but it's all our code.

Adlai-Holler · 2017-05-13T22:32:56Z

@garrettmoon Thanks for reviewing this. What should we do about the license text? Two issues we need to deal with:

KittenNode.mm: This is just the fact that our example apps never got that initial header-update pass. We should open a new PR that updates them.
The files from YYText: We need to figure out a way to get danger to accept the YYText-modified version of the header.

Or we can remove the header danger rule. I don't think it's adding much value right now.

ghost · 2017-05-14T18:58:51Z

	21 Warnings
⚠️	This is a big PR, please consider splitting it up to ease code review.
⚠️	Please ensure license is correct for ASTextDebugOption.h: `// // ASTextDebugOption.h // Texture // // Copyright (c) 2017-present, Pinterest, Inc. All rights reserved. // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. // You may obtain a copy of the License at // // http://www.apache.org/licenses/LICENSE-2.0 //`
⚠️	Please ensure license is correct for NSParagraphStyle+ASText.h: `// // NSParagraphStyle+ASText.h // Texture // // Copyright (c) 2017-present, Pinterest, Inc. All rights reserved. // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. // You may obtain a copy of the License at // // http://www.apache.org/licenses/LICENSE-2.0 //`
⚠️	Please ensure license is correct for ASTextDebugOption.m: `// // ASTextDebugOption.m // Texture // // Copyright (c) 2017-present, Pinterest, Inc. All rights reserved. // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. // You may obtain a copy of the License at // // http://www.apache.org/licenses/LICENSE-2.0 //`
⚠️	Please ensure license is correct for ASTextUtilities.h: `// // ASTextUtilities.h // Texture // // Copyright (c) 2017-present, Pinterest, Inc. All rights reserved. // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. // You may obtain a copy of the License at // // http://www.apache.org/licenses/LICENSE-2.0 //`
⚠️	Please ensure license is correct for ASTextInput.h: `// // ASTextInput.h // Texture // // Copyright (c) 2017-present, Pinterest, Inc. All rights reserved. // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. // You may obtain a copy of the License at // // http://www.apache.org/licenses/LICENSE-2.0 //`
⚠️	Please ensure license is correct for ASTextRunDelegate.h: `// // ASTextRunDelegate.h // Texture // // Copyright (c) 2017-present, Pinterest, Inc. All rights reserved. // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. // You may obtain a copy of the License at // // http://www.apache.org/licenses/LICENSE-2.0 //`
⚠️	Please ensure license is correct for ASTextInput.m: `// // ASTextInput.m // Texture // // Copyright (c) 2017-present, Pinterest, Inc. All rights reserved. // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. // You may obtain a copy of the License at // // http://www.apache.org/licenses/LICENSE-2.0 //`
⚠️	Please ensure license is correct for NSAttributedString+ASText.h: `// // NSAttributedString+ASText.h // Texture // // Copyright (c) 2017-present, Pinterest, Inc. All rights reserved. // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. // You may obtain a copy of the License at // // http://www.apache.org/licenses/LICENSE-2.0 //`
⚠️	Please ensure license is correct for ASTextLayout.h: `// // ASTextLayout.h // Texture // // Copyright (c) 2017-present, Pinterest, Inc. All rights reserved. // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. // You may obtain a copy of the License at // // http://www.apache.org/licenses/LICENSE-2.0 //`
⚠️	Please ensure license is correct for AppDelegate.m: // // AppDelegate.m // Texture // // Copyright (c) 2014-present, Facebook, Inc. All rights reserved. // This source code is licensed under the BSD-style license found in the // LICENSE file in the /ASDK-Licenses directory of this source tree. An additional // grant of patent rights can be found in the PATENTS file in the same directory. // // Modifications to this file made after 4/13/2017 are: Copyright (c) 2017-present, // Pinterest, Inc. Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. // You may obtain a copy of the License at // // http://www.apache.org/licenses/LICENSE-2.0 //
⚠️	Please ensure license is correct for ASTextLayout.m: `// // ASTextLayout.m // Texture // // Copyright (c) 2017-present, Pinterest, Inc. All rights reserved. // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. // You may obtain a copy of the License at // // http://www.apache.org/licenses/LICENSE-2.0 //`
⚠️	Please ensure license is correct for ASTextRunDelegate.m: `// // ASTextRunDelegate.m // Texture // // Copyright (c) 2017-present, Pinterest, Inc. All rights reserved. // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. // You may obtain a copy of the License at // // http://www.apache.org/licenses/LICENSE-2.0 //`
⚠️	Please ensure license is correct for ASTextLine.h: `// // ASTextLine.h // Texture // // Copyright (c) 2017-present, Pinterest, Inc. All rights reserved. // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. // You may obtain a copy of the License at // // http://www.apache.org/licenses/LICENSE-2.0 //`
⚠️	Please ensure license is correct for ASTextUtilities.m: `// // ASTextUtilities.m // Texture // // Copyright (c) 2017-present, Pinterest, Inc. All rights reserved. // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. // You may obtain a copy of the License at // // http://www.apache.org/licenses/LICENSE-2.0 //`
⚠️	Please ensure license is correct for ASTextLine.m: `// // ASTextLine.m // Texture // // Copyright (c) 2017-present, Pinterest, Inc. All rights reserved. // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. // You may obtain a copy of the License at // // http://www.apache.org/licenses/LICENSE-2.0 //`
⚠️	Please ensure license is correct for NSAttributedString+ASText.m: `// // NSAttributedString+ASText.m // Texture // // Copyright (c) 2017-present, Pinterest, Inc. All rights reserved. // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. // You may obtain a copy of the License at // // http://www.apache.org/licenses/LICENSE-2.0 //`
⚠️	Please ensure license is correct for ASTextAttribute.h: `// // ASTextAttribute.h // Texture // // Copyright (c) 2017-present, Pinterest, Inc. All rights reserved. // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. // You may obtain a copy of the License at // // http://www.apache.org/licenses/LICENSE-2.0 //`
⚠️	Please ensure license is correct for NSParagraphStyle+ASText.m: `// // NSParagraphStyle+ASText.m // Texture // // Copyright (c) 2017-present, Pinterest, Inc. All rights reserved. // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. // You may obtain a copy of the License at // // http://www.apache.org/licenses/LICENSE-2.0 //`
⚠️	Please ensure license is correct for ASTextAttribute.m: `// // ASTextAttribute.m // Texture // // Copyright (c) 2017-present, Pinterest, Inc. All rights reserved. // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. // You may obtain a copy of the License at // // http://www.apache.org/licenses/LICENSE-2.0 //`
⚠️	Please ensure license is correct for KittenNode.mm: // // KittenNode.mm // Texture // // Copyright (c) 2014-present, Facebook, Inc. All rights reserved. // This source code is licensed under the BSD-style license found in the // LICENSE file in the /ASDK-Licenses directory of this source tree. An additional // grant of patent rights can be found in the PATENTS file in the same directory. // // Modifications to this file made after 4/13/2017 are: Copyright (c) 2017-present, // Pinterest, Inc. Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. // You may obtain a copy of the License at // // http://www.apache.org/licenses/LICENSE-2.0 //

Generated by 🚫 Danger

Adlai-Holler · 2017-05-14T19:02:01Z

In order to keep moving, I'm going to land this with the YYText-header warnings and the one from KittenNode.mm intact. @garrettmoon Let's run a pass updating the license headers in the sample code, or update or disable the license header rule altogether.

ghost · 2017-05-14T19:16:55Z

🚫 CI failed with log

ghost · 2017-05-14T19:21:18Z

🚫 CI failed with log

Add experimental text node implementation, based on YYText

2abbc49

Adlai-Holler added ASTextNode enhancement labels May 11, 2017

Fix warnings and alert when unimplemented experimental features are u…

9d590b0

…sed.

garrettmoon approved these changes May 13, 2017

View reviewed changes

Address feedback from review

2c4ca11

Adlai-Holler and others added 3 commits May 14, 2017 11:56

Extend the cthulog

2ae8a5e

Merge branch 'master' into AHTextImprovements

8bdebd3

Update license headers

ab8b7c6

Adlai-Holler merged commit d4725a5 into master May 14, 2017

Adlai-Holler deleted the AHTextImprovements branch May 14, 2017 19:02

bolsinga mentioned this pull request May 27, 2020

Ship Decision on ASExperimentalTextNode #1843

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Experimental Text Node Implementation #259

Add Experimental Text Node Implementation #259

Adlai-Holler commented May 11, 2017

ghost commented May 11, 2017

nguyenhuy commented May 11, 2017 •

edited

Loading

maicki commented May 11, 2017 •

edited

Loading

Adlai-Holler commented May 11, 2017

nguyenhuy commented May 11, 2017

garrettmoon left a comment

garrettmoon May 13, 2017

garrettmoon May 13, 2017

garrettmoon May 13, 2017

garrettmoon May 13, 2017

garrettmoon May 13, 2017

Adlai-Holler May 13, 2017

garrettmoon May 13, 2017

Adlai-Holler May 13, 2017

garrettmoon May 13, 2017

Adlai-Holler May 13, 2017 •

edited

Loading

garrettmoon May 13, 2017

Adlai-Holler May 13, 2017 •

edited

Loading

garrettmoon May 13, 2017

Adlai-Holler May 13, 2017

garrettmoon May 13, 2017

Adlai-Holler May 13, 2017

Adlai-Holler commented May 13, 2017

ghost commented May 14, 2017

Adlai-Holler commented May 14, 2017

ghost commented May 14, 2017

ghost commented May 14, 2017

		return layout.textBoundingSize;


		// ASTextKitRenderer *renderer = [self _rendererWithBoundsSlow:{.size = constrainedSize}];

Add Experimental Text Node Implementation #259

Add Experimental Text Node Implementation #259

Conversation

Adlai-Holler commented May 11, 2017

ghost commented May 11, 2017

nguyenhuy commented May 11, 2017 • edited Loading

maicki commented May 11, 2017 • edited Loading

Adlai-Holler commented May 11, 2017

nguyenhuy commented May 11, 2017

garrettmoon 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

Adlai-Holler May 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Adlai-Holler May 13, 2017 • edited Loading

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

Adlai-Holler commented May 13, 2017

ghost commented May 14, 2017

Adlai-Holler commented May 14, 2017

ghost commented May 14, 2017

ghost commented May 14, 2017

nguyenhuy commented May 11, 2017 •

edited

Loading

maicki commented May 11, 2017 •

edited

Loading

Adlai-Holler May 13, 2017 •

edited

Loading

Adlai-Holler May 13, 2017 •

edited

Loading