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 layout] Fix an issue that sometimes causes a node's pending layout to not be applied #792

Merged

Conversation

nguyenhuy
Copy link
Member

  • Since the implementation of layout version (Use a sentinel NSUInteger for node layout data #trivial #428), if a node's pending and calculated layouts have the same current version as well as the same constrained size, the 2 layouts are considered equal and can be used interchangeably. A layout version check between the 2 layouts was added in [ASDisplayNode+Layout] Ensure a pending layout is applied once #695. This PR adds a missing constrained size check.
  • If the pending layout has the same version but a different constrained size compare to the calculated layout's, we can assume that the pending layout is newer and should be preferred over the calculated one. That is because layout operations always register their new layout as pending, which then (immediately or eventually) get applied to the node as calculated layout.

…ut to not be applied

- Since the implementation of layout version (TextureGroup#428), if a node's pending and calculated layouts have the same current version as well as the same constrained size, the 2 layouts are considered equal and can be used interchangeably. A layout version check between the 2 layouts was added in TextureGroup#695. This PR adds a missing constrained size check.
- If the pending layout has the same version but a different constrained size compare to the calculated layout's, we can assume that the pending layout is newer and should be preferred over the calculated one. That is because layout operations always register their new layout as pending, which then (immediately or eventually) get applied to the node as calculated layout.
@nguyenhuy nguyenhuy changed the title [ASDisplayNode layout] Fix an issue that causes a node's pending layout to not be applied [ASDisplayNode layout] Fix an issue that sometimes causes a node's pending layout to not be applied Feb 6, 2018
@nguyenhuy nguyenhuy force-pushed the HNFixValidPendingLayoutBeingIgnored branch from 980c94b to d2dc377 Compare February 6, 2018 20:42
@nguyenhuy nguyenhuy force-pushed the HNFixValidPendingLayoutBeingIgnored branch from d2dc377 to 9958f86 Compare February 6, 2018 20:44
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.

I like this change, but it may discard valid layouts. If the versions are equal, and they're both equal to the current version (_layoutVersion) then we can safely use whichever layout matches the constrained size we're interested in.

So we can look at what constrained size we will use for this pass (down below at line 367):

ASSizeRange constrainedSize = [self _locked_constrainedSizeForLayoutPass];

And then if either layout's version and constrained size are valid (the version matches the node's _layoutVersion and the constrained size matches this method's constrainedSize) then we can skip the measure and use that layout.

Thoughts?

@nguyenhuy
Copy link
Member Author

nguyenhuy commented Feb 12, 2018

@Adlai-Holler You're right that we can use whichever layout fits the constrained size/bounds. However, currently _layoutSublayouts only picks up _calculated layout, so we need to fix that as well.

Another concern is that, such behaviour will break the current implications of _pending and _calculated. We'll end up with 2 (or more) calculated layouts (and 0 pending layout) from which the node can choose and apply. It's a better behaviour that fits our vision of the new layout system. However, since it'll require more time and effort to implement and test, I'd prefer to land this diff as is, because it makes the current system to behave correctly, and follow up later. Thoughts?

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.

I'm down with that!

@nguyenhuy
Copy link
Member Author

Awesome. Thank you!

@nguyenhuy nguyenhuy merged commit e2478fc into TextureGroup:master Feb 12, 2018
@nguyenhuy nguyenhuy deleted the HNFixValidPendingLayoutBeingIgnored branch February 12, 2018 21:05
bernieperez pushed a commit to AtomTickets/Texture that referenced this pull request Apr 25, 2018
…nding layout to not be applied (TextureGroup#792)

* [ASDisplayNode layout] Fix an issue that causes a node's pending layout to not be applied

- Since the implementation of layout version (TextureGroup#428), if a node's pending and calculated layouts have the same current version as well as the same constrained size, the 2 layouts are considered equal and can be used interchangeably. A layout version check between the 2 layouts was added in TextureGroup#695. This PR adds a missing constrained size check.
- If the pending layout has the same version but a different constrained size compare to the calculated layout's, we can assume that the pending layout is newer and should be preferred over the calculated one. That is because layout operations always register their new layout as pending, which then (immediately or eventually) get applied to the node as calculated layout.
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

2 participants