Avoid reset of containers when anchor element is replaced in ItemsRepeater #926
Conversation
jevansaks
added
the
release note
|
🎉 Handy links: |
| { | ||
| if (m_flowAlgorithm.GetElementIfRealized(0)) | ||
| if (m_cachedFirstElement != nullptr && m_flowAlgorithm.GetElementIfRealized(0)) |
There was a problem hiding this comment.
Can this check ever pass? On this line if m_cachedElement is successfully added to m_flowAlgorithm then m_cachedElement is set to nullptr.
So the way I read it, is that m_cachedFirstElement != nullptr OR m_flowAlgorithm.GetElementIfRealized(0) can be true, but not both together. Or am I missing something?
There was a problem hiding this comment.
We do not want to recycle if just m_cachedFirstElement is not nullptr. That would mean that every layout pass we will throw away the element only to realize it in the next pass. This cache is trying to avoid it. This is making sure that if we created element0 in the state, but the algorithm created one too, then we can let the one state created go away. Otherwise we just keep the cached element. This is a bit twisted unfortunately because we are reusing the algorithm for stack/grid and flow layouts and grid has the special behavior based on the first element.
There was a problem hiding this comment.
Thanks @ranjeshj. That makes sense, I think what confused me was the comment "flowlayout algorithm took ownership", which suggested to me that this was supposed to tidy up the element transferred to the flow layout on line 62. 👍
| { | ||
| // We created the element, but then flowlayout algorithm took ownership, so we can clear it and | ||
| // let flowlayout algorithm do its thing. | ||
| context.RecycleElement(m_cachedFirstElement); |
There was a problem hiding this comment.
This doesn't handle the case where we created the element, but then flowlayout didn't take ownership, in which case the element still needs to be recycled IIUC?
There was a problem hiding this comment.
We want to keep it cached (and not recycle) in that case to use for the next pass.
Currently replace is modeled as a remove and an add, but unfortunately if the remove ends up removing the anchor element, then during next measure we think we are disconnected and throw all the containers away and start from scratch. This causes a flash of all the images which is undesirable. With this change, if we are doing a replace in the realized range, then we can just clear those items and set the containers as null (sentinels). We already look for sentinels during the measure pass and realize them as needed. This will ensure that only the replaced containers will get cleared/prepared again.
Fixes #915
Fixes #935