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

[ASDisplayNode] Fix an issue that causes a node to sometimes return an outdated calculated size or size range #808

Merged
merged 4 commits into from
May 21, 2018

Conversation

nguyenhuy
Copy link
Member

@nguyenhuy nguyenhuy commented Feb 24, 2018

When asked for the calculated size or constrained size for the calculated layout, prefer the pending layout only if its version is up-to-date. This should fix the issue reported by @codeOfRobin (here) and reproducible by this sample project.

@nguyenhuy nguyenhuy changed the title [ASDisplayNode] When asked for calculated size or constrained size for calculated layout, only prefer pending layout if its version is up-to-date [ASDisplayNode] Fix an issue that causes a node to sometimes return an outdated calculated size or size range Feb 24, 2018
@@ -305,24 +305,22 @@ - (void)_u_measureNodeWithBoundsIfNecessary:(CGRect)bounds
}

CGSize boundsSizeForLayout = ASCeilSizeValues(bounds.size);
NSUInteger calculatedVersion = _calculatedDisplayNodeLayout->version;
Copy link
Member Author

Choose a reason for hiding this comment

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

Note to reviewers: No change in the logic of this method (-_u_measureNodeWithBoundsIfNecessary:), just call the new isValid() function to (hopefully) increase readability.

Copy link
Contributor

@maicki maicki left a comment

Choose a reason for hiding this comment

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

Nice! Also great to increase our test base.

@codeOfRobin
Copy link

@nguyenhuy any updates on this?

@appleguy
Copy link
Member

@nguyenhuy should we be moving forward on this? I'm considering (finally) pulling in the _layoutVersion changes, which I avoided for a while...wondering if this is an important fix before doing so. cc @maicki, @Adlai-Holler

Copy link
Member

@Adlai-Holler Adlai-Holler left a comment

Choose a reason for hiding this comment

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

Looks good! I should have had this logic from the beginning.

We should never return any layout whose version is out of date, period

@TextureGroup TextureGroup deleted a comment May 21, 2018
@codeOfRobin
Copy link

Wooohoo 🙌 🎉 ! I depended heavily on this feature working. Thank you so much guys 😄

@nguyenhuy
Copy link
Member Author

@Adlai-Holler Thanks for syncing with master!
@codeOfRobin Thanks for reporting this bug!

@nguyenhuy nguyenhuy merged commit b9ae6c4 into TextureGroup:master May 21, 2018
@nguyenhuy nguyenhuy deleted the HNInvalidCalculatedSize branch May 21, 2018 18:12
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.

5 participants