Redo how debug "reflow" works #691
Conversation
This was never updated after we transitioned away from `CKComponentLifecycleManager`, so it was mostly broken. Now instead of attempting to use the view hierarchy to trigger reflow, we simply message all interested parties and tell them to update themselves. Crucially this means that offscreen components are recomputed too, not just onscreen ones. Neat.
| } | ||
|
|
||
| static CK::StaticMutex l = CK_MUTEX_INITIALIZER; | ||
| CK::StaticMutexLocker lock(l); |
There was a problem hiding this comment.
unnecessary; mount is only performed on the main thread.
There was a problem hiding this comment.
Do we want to assert this is called on the main thread? Or do you think that's overkill in this case?
There was a problem hiding this comment.
I think it's overkill in this particular case.
| if (view) { | ||
| CKRecursiveComponentReflow(view); | ||
| for (id<CKComponentDebugReflowListener> listener in copiedListeners) { | ||
| [listener didReceiveReflowComponentsRequest]; |
There was a problem hiding this comment.
This is so much better than traversing the view hierarchy to find the component lifecycle manager or explicitly telling component hosting views to layout.
| if (self = [super init]) { | ||
| _componentProvider = componentProvider; | ||
| _sizeRangeProvider = sizeRangeProvider; | ||
| [CKComponentDebugController registerReflowListener:self]; |
There was a problem hiding this comment.
Though it's a bit of a shame to have CKComponentLifecycleManager aware of debug code I think it's totally worthwhile considering how much it simplifies CKComponentDebugController.
There was a problem hiding this comment.
It is a bit weird, definitely. How about having a separate registry of lifecycle managers (exposed in an internal header) that the debug controller can then access? At least that would get the debug controller code out of CKComponentLifecycleManager.
| } | ||
| std::lock_guard<std::mutex> l(reflowMutex); | ||
| if (reflowListeners == nil) { | ||
| reflowListeners = [NSHashTable weakObjectsHashTable]; |
There was a problem hiding this comment.
Nice. So we weakly hold the reflow listeners to naturally keep the collection bounded.
| } | ||
|
|
||
| static CK::StaticMutex l = CK_MUTEX_INITIALIZER; | ||
| CK::StaticMutexLocker lock(l); |
There was a problem hiding this comment.
Do we want to assert this is called on the main thread? Or do you think that's overkill in this case?
|
Looks good to merge to me. I like how much this simplifies things. |
|
Looks good to me. |
natansh
left a comment
natansh
left a comment
There was a problem hiding this comment.
It's definitely much better than before, but I am a bit concerned about introducing debugging code into the core internals - the footprint of debug code in these internals is still pretty small, though, so its cool! 👍
| if (self = [super init]) { | ||
| _componentProvider = componentProvider; | ||
| _sizeRangeProvider = sizeRangeProvider; | ||
| [CKComponentDebugController registerReflowListener:self]; |
There was a problem hiding this comment.
It is a bit weird, definitely. How about having a separate registry of lifecycle managers (exposed in an internal header) that the debug controller can then access? At least that would get the debug controller code out of CKComponentLifecycleManager.
5 participants
This was never updated after we transitioned away from
CKComponentLifecycleManager, so it was mostly broken. Now instead of attempting to use the view hierarchy to trigger reflow, we simply message all interested parties and tell them to update themselves.Crucially this means that offscreen components are recomputed too, not just onscreen ones. Neat.