Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix main thread assertion for tint color on text nodes, re-render tint-able nodes on hierarchy changes #1670

Merged
merged 3 commits into from
Sep 16, 2019

Conversation

rahul-malik
Copy link
Contributor

@rahul-malik rahul-malik commented Sep 12, 2019

  • Avoid accessing self.tintColor off-main since this will trigger an
    assertion and might be undefined behavior
  • Trigger setNeedsDisplay in cases of hierarchy change and
    tintColorDidChange in ASButtonNode, ASTextNode{1,2}
  • Remove overwriting the tintColor of _ASTableViewCell. This actually breaks the typical inheritance behavior since setting it's tint color to the ASCellNode tintColor will always return a value which will be the systemBlueColor if it is unable to be determined

- Avoid accessing `self.tintColor` off-main since this will trigger an
assertion and might be undefined behavior
- Trigger setNeedsDisplay in cases of hierarchy change and
tintColorDidChange in ASButtonNode, ASTextNode{1,2}
- Fix bug in _ASTableViewCell where we were overwriting the tint color
instead of inheriting the correct value
- Add tests for new logic in changing hierarchy
@rahul-malik
Copy link
Contributor Author

@bolsinga - PTAL, this should be good to land now

Copy link
Contributor

@bolsinga bolsinga left a comment

Choose a reason for hiding this comment

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

This all looks reasonable; I assume the added tests will fail without the code changes? Also please be sure to update the main title; this is not only about text node tint color.

@rahul-malik
Copy link
Contributor Author

Will definitely update the title / commit message. I did verify the tests fail without these changes

@bolsinga
Copy link
Contributor

Will definitely update the title / commit message. I did verify the tests fail without these changes

Thanks for confirming!

@rahul-malik rahul-malik changed the title Address various issues with tint color on text nodes Fix main thread assertion for tint color on text nodes, re-render tint-able nodes on hierarchy changes Sep 16, 2019
@rahul-malik rahul-malik merged commit 618f21e into master Sep 16, 2019
@rahul-malik rahul-malik deleted the rmalik/textnode-tint-fixes branch September 16, 2019 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants