Small changes required by the coming layout debugger #337
Conversation
nguyenhuy
force-pushed
the
HNLayoutDebugger-CoreChanges
branch
from
5adbb8d
to
1c98eaa
Compare
|
🚫 CI failed with log |
|
🚫 CI failed with log |
nguyenhuy
force-pushed
the
HNLayoutDebugger-CoreChanges
branch
from
e42f6c9
to
a8f1bbf
Compare
Source/ASDisplayNode.h
Outdated
| @@ -554,6 +554,19 @@ extern NSInteger const ASDefaultDrawingPriority; | |||
| @interface ASDisplayNode (Debugging) <ASDebugNameProvider> | |||
|
|
|||
| /** | |||
| * Whether ASDisplayNode should store their unflattened layouts. The layout can be accessed via `-unflattenedCalculatedLayout`. | |||
| * Flattened layouts use less memory and are faster to lookup, while unflattened ones are useful for debugging | |||
| * because they reserve original information. | |||
There was a problem hiding this comment.
nit: typo reserve should be preserve
Source/ASDisplayNode.h
Outdated
| * | ||
| * Defaults to NO. | ||
| */ | ||
| + (void)setShouldStoreUnflattenedLayouts:(BOOL)shouldStore; |
There was a problem hiding this comment.
Any reason this isn't also a property?
There was a problem hiding this comment.
Nm, I see now that this is a class method :)
Source/ASDisplayNode.mm
Outdated
| @@ -3286,6 +3294,21 @@ - (UIView *)preferredFocusedView | |||
|
|
|||
| @implementation ASDisplayNode (Debugging) | |||
|
|
|||
| + (void)setShouldStoreUnflattenedLayouts:(BOOL)shouldStore | |||
| { | |||
| storesUnflattenedLayouts = shouldStore; | |||
There was a problem hiding this comment.
These don't appear to be thread safe. Should we use OSAtomic at least here? I wouldn't be surprised if setting BOOLs is actually atomic on most architectures but we probably shouldn't at least have a comment about that or fix it?
Source/ASDisplayNode.mm
Outdated
|
|
||
| - (ASLayout *)unflattenedCalculatedLayout | ||
| { | ||
| return _unflattenedLayout; |
Source/Layout/ASLayout.mm
Outdated
| @@ -76,6 +76,13 @@ @implementation ASLayout | |||
|
|
|||
| @dynamic frame, type; | |||
|
|
|||
| static BOOL static_retainsSublayoutLayoutElements = NO; | |||
There was a problem hiding this comment.
Any reason to have the prepended static here? Seems like a newish convention? Is there an existing one we can use?
There was a problem hiding this comment.
That is because ASLayout also has an instance variable called retainSublayoutLayoutElements here.
Source/Layout/ASLayout.mm
Outdated
|
|
||
| + (void)setShouldRetainSublayoutLayoutElements:(BOOL)shouldRetain | ||
| { | ||
| static_retainsSublayoutLayoutElements = shouldRetain; |
There was a problem hiding this comment.
This doesn't seem thread safe either.
- `ASDisplayNode` can be told to not flatten its layout immediately but later on. The unflattened layout is also stored in a separate property. It's needed for inspecting not only display nodes but also layout specs used to compute a layout tree. - `ASLayout` can be told to always retain its sublayout elements. This is needed especially for layout specs since they are usually not retained after an ASLayout was computed.
nguyenhuy
force-pushed
the
HNLayoutDebugger-CoreChanges
branch
from
061e5ee
to
3dcf125
Compare
|
@garrettmoon Ready for another round of review. Thanks! |
| @@ -76,6 +76,18 @@ @implementation ASLayout | |||
|
|
|||
| @dynamic frame, type; | |||
|
|
|||
| static std::atomic_bool static_retainsSublayoutLayoutElements = ATOMIC_VAR_INIT(NO); | |||
There was a problem hiding this comment.
Added static prefix because ASLayout also has an instance variable called retainSublayoutLayoutElements here.
| @@ -82,6 +82,7 @@ @implementation ASDisplayNode | |||
| @synthesize threadSafeBounds = _threadSafeBounds; | |||
|
|
|||
| static BOOL suppressesInvalidCollectionUpdateExceptions = NO; | |||
| static std::atomic_bool storesUnflattenedLayouts = ATOMIC_VAR_INIT(NO); | |||
There was a problem hiding this comment.
Oh man, this is way nicer than objc!
| @@ -76,6 +76,18 @@ @implementation ASLayout | |||
|
|
|||
| @dynamic frame, type; | |||
|
|
|||
| static std::atomic_bool static_retainsSublayoutLayoutElements = ATOMIC_VAR_INIT(NO); | |||
|
Awesome. Thanks, @garrettmoon! |
* Small changes required by the layout debugger - `ASDisplayNode` can be told to not flatten its layout immediately but later on. The unflattened layout is also stored in a separate property. It's needed for inspecting not only display nodes but also layout specs used to compute a layout tree. - `ASLayout` can be told to always retain its sublayout elements. This is needed especially for layout specs since they are usually not retained after an ASLayout was computed. * Update CHANGELOG * Address comments
ASDisplayNodecan be told to not flatten its layout immediately but later on. The unflattened layout is also stored in a separate property. It's needed for inspecting not only display nodes but also layout specs used to compute a layout tree.ASLayoutcan be told to always retain its sublayout elements. This is needed especially for layout specs since they are usually not retained after anASLayoutis computed.