Skip to content
This repository was archived by the owner on Feb 2, 2023. It is now read-only.

[ASTextNode] Fix ASTextNode shadow is not rendering - #2042

Merged
Adlai-Holler merged 2 commits into
facebookarchive:masterfrom
maicki:MSFixASTextNodeShadow
Aug 9, 2016
Merged

[ASTextNode] Fix ASTextNode shadow is not rendering #2042
Adlai-Holler merged 2 commits into
facebookarchive:masterfrom
maicki:MSFixASTextNodeShadow

Conversation

@maicki

@maicki maicki commented Aug 7, 2016

Copy link
Copy Markdown
Contributor

We didn't pass through shadow properties via the renderer attributes. That meant that the renderer shadower didn't get those and so setting a shadow on the text node did not have any effect.

@ghost ghost added the CLA Signed label Aug 7, 2016
@@ -1044,6 +1049,7 @@ - (void)setShadowColor:(CGColorRef)shadowColor
CGColorRetain(shadowColor);
}
_shadowColor = shadowColor;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you're already in this code, could you add if (_shadowColor != NULL) { CGColorRelease(_shadowColor); } so we stop leaking them 🚰

@maicki maicki Aug 9, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On it 👷

@Adlai-Holler

Copy link
Copy Markdown
Contributor

Love this, let's land it ASAP. Thoughts about comment @maicki ?

@ghost ghost added the CLA Signed label Aug 9, 2016
@maicki

maicki commented Aug 9, 2016

Copy link
Copy Markdown
Contributor Author

@Adlai-Holler Fixed the memory leak ready to go

if (_shadowColor != shadowColor) {
if (shadowColor != NULL) {
CGColorRetain(shadowColor);
CGColorRelease(_shadowColor);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm... I think we need to move this out of the if statement and wrap it in a new one: if (_shadowColor != NULL) because no matter what the new value is, if we have an old value we must release it

@Adlai-Holler

Copy link
Copy Markdown
Contributor

Sweet!

@Adlai-Holler
Adlai-Holler merged commit d9db780 into facebookarchive:master Aug 9, 2016
hannahmbanana pushed a commit that referenced this pull request Aug 9, 2016
* Passing through shadow in renderer attribute

* Fix memory leak setting shadow color
maxsz pushed a commit to equinux/AsyncDisplayKit that referenced this pull request Aug 11, 2016
…2042)

* Passing through shadow in renderer attribute

* Fix memory leak setting shadow color
hannahmbanana pushed a commit that referenced this pull request Aug 15, 2016
* Passing through shadow in renderer attribute

* Fix memory leak setting shadow color
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants