Merge stackable#7
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces "Stackable", a declarative UIStackView builder for iOS that utilizes result builders to streamline layout construction. Key features include smart spacing, hairlines, visibility binding, and SwiftUI-style convenience builders (VStack, HStack, Spacer). The library has also been updated to Swift 6.0 with extensive @mainactor annotations for concurrency safety. Reviewers pointed out critical technical issues with Objective-C associated objects, specifically the inability to store closures directly and the incorrect use of struct metatypes as keys. Additional feedback suggests removing redundant result builder methods, deleting commented-out code, and replacing deprecated system colors.
| get { return objc_getAssociatedObject(base, &UIStackView.AssociatedKeys.hairlineProvider) as? StackableHairlineProvider } | ||
| set { objc_setAssociatedObject(base, &UIStackView.AssociatedKeys.hairlineProvider, newValue, .OBJC_ASSOCIATION_RETAIN_NONATOMIC) } |
There was a problem hiding this comment.
Swift closures (like StackableHairlineProvider) are not class instances and cannot be directly stored as associated objects using objc_setAssociatedObject. This will likely result in a crash or failure to store/retrieve the value correctly. You should wrap the closure in a class instance (a 'box') before storing it.
| get { return objc_getAssociatedObject(self, &UIStackView.AssociatedKeys.hairlineColor) as? UIColor } | ||
| set { objc_setAssociatedObject(self, &UIStackView.AssociatedKeys.hairlineColor, newValue, .OBJC_ASSOCIATION_RETAIN_NONATOMIC) } |
There was a problem hiding this comment.
In a static property of a struct extension, self refers to the struct's metatype. Since ZDTinyLayoutNamespace is a struct, its metatype is not an Objective-C object and cannot be used with objc_setAssociatedObject. You should use Base.self (the UIStackView class metatype) as the target for these global configuration associated objects.
static var hairlineColor: UIColor? {
get { return objc_getAssociatedObject(Base.self, &UIStackView.AssociatedKeys.hairlineColor) as? UIColor }
set { objc_setAssociatedObject(Base.self, &UIStackView.AssociatedKeys.hairlineColor, newValue, .OBJC_ASSOCIATION_RETAIN_NONATOMIC) }| @MainActor private var strongKey: Void? | ||
| @MainActor private var weakKey: Void? |
There was a problem hiding this comment.
Using Void? as a key for associated objects is unconventional and potentially unsafe. While it technically has an address, it is standard practice to use a static var of type UInt8 or Int to ensure a stable, unique pointer for the Objective-C runtime's association mechanism.
| @MainActor private var strongKey: Void? | |
| @MainActor private var weakKey: Void? | |
| @MainActor private var strongKey: UInt8 = 0 | |
| @MainActor private var weakKey: UInt8 = 0 |
| var debug: DebugExtension { | ||
| get { return DebugExtension(base) } | ||
| set {} | ||
| } |
There was a problem hiding this comment.
The debug property should be read-only since it has an empty setter. This makes the API intent clearer and avoids confusion.
var debug: DebugExtension {
return DebugExtension(base)
}| shape: .outline, | ||
| lineStyle: .solid, | ||
| color: .groupTableViewBackground, | ||
| lineWidth: 2 |
There was a problem hiding this comment.
.groupTableViewBackground is deprecated. Since the deployment target is now 12.0, you should consider using a non-deprecated alternative or a conditional check for systemGroupedBackground (available from iOS 13.0). For a debug outline, a simple lightGray might be a suitable replacement.
| lineWidth: 2 | |
| color: .lightGray, |
| static var hairlineColor: Void? | ||
| static var hairlineThickness: Void? | ||
| static var hairlineProvider: Void? |
There was a problem hiding this comment.
Using Void? for associated object keys is non-standard. It is recommended to use UInt8 for unique pointers.
| static var hairlineColor: Void? | |
| static var hairlineThickness: Void? | |
| static var hairlineProvider: Void? | |
| static var hairlineColor: UInt8 = 0 | |
| static var hairlineThickness: UInt8 = 0 | |
| static var hairlineProvider: UInt8 = 0 |
| /* | ||
| public static func buildExpression(_ expression: Expression?) -> Component { | ||
| guard let expression else { | ||
| return [] | ||
| } | ||
| return [expression] | ||
| } | ||
| */ |
There was a problem hiding this comment.
Commented-out code should be removed to maintain a clean codebase.
| public static func buildBlock(_ components: Component...) -> Component { | ||
| components.flatMap { $0 } | ||
| } |
There was a problem hiding this comment.
The buildBlock method is redundant when buildPartialBlock is implemented in Swift 5.7+. Removing it simplifies the result builder implementation.
…ementConvertible' has no effect
Replace internal KVO observations with Combine publishers and use MainActor.assumeIsolated for synchronous UI updates. BREAKING CHANGE: Minimum supported platforms are now iOS 13, macOS 10.15, tvOS 13, and watchOS 6.
This reverts commit 0ded242.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces 'Stackable', a declarative UIStackView builder for iOS using result builders, and updates the library to Swift 6.0 with enhanced concurrency safety and updated platform requirements. Review feedback identifies critical safety issues regarding the direct storage of Swift closures in associated objects and potential thread-safety violations in Combine-based UI updates. Additionally, the review suggests optimizing layout logic to prevent redundant constraints and removing unnecessary @mainactor annotations to reduce code noise.
| var hairlineProvider: StackableHairlineProvider? { | ||
| get { return objc_getAssociatedObject(base, &UIStackView.AssociatedKeys.hairlineProvider) as? StackableHairlineProvider } | ||
| set { objc_setAssociatedObject(base, &UIStackView.AssociatedKeys.hairlineProvider, newValue, .OBJC_ASSOCIATION_RETAIN_NONATOMIC) } | ||
| } |
There was a problem hiding this comment.
Storing a Swift closure directly as an associated object with objc_setAssociatedObject is unsafe. Swift closures are not guaranteed to be Objective-C objects and this can lead to crashes, especially if the closure captures context. The changelog mentions that issues with associated objects were fixed, but this usage remains.
The recommended way to store a closure is to wrap it in a class instance. For example:
private final class ClosureWrapper<T> {
let closure: T
init(_ closure: T) {
self.closure = closure
}
}
// ... inside the extension ...
var hairlineProvider: StackableHairlineProvider? {
get {
let wrapper = objc_getAssociatedObject(base, &UIStackView.AssociatedKeys.hairlineProvider) as? ClosureWrapper<StackableHairlineProvider>
return wrapper?.closure
}
set {
let wrapper = newValue.map { ClosureWrapper($0) }
objc_setAssociatedObject(base, &UIStackView.AssociatedKeys.hairlineProvider, wrapper, .OBJC_ASSOCIATION_RETAIN_NONATOMIC)
}
}This pattern should be applied to all closure-based associated objects, including the static hairlineProvider.
| let observation = ancestor.publisher(for: \.frame, options: [.initial, .new]) | ||
| .sink { [weak view, weak stackView, weak ancestor] _ in | ||
| MainActor.assumeIsolated { | ||
| guard let view = view, | ||
| let stackView = stackView, | ||
| let ancestor = ancestor | ||
| else { return } | ||
|
|
||
| let bounds = view.bounds | ||
| let ancestorBounds = view.convert(ancestor.bounds, to: view) | ||
|
|
||
| switch stackView.axis { | ||
| case .horizontal: | ||
| let top = (ancestorBounds.minY - bounds.minY) + ancestor.layoutMargins.top | ||
| let bottom = (bounds.maxY - ancestorBounds.maxY) + ancestor.layoutMargins.bottom | ||
| view.layoutMargins.top = top | ||
| view.layoutMargins.bottom = bottom | ||
|
|
||
| case .vertical: | ||
| let left = (ancestorBounds.minX - bounds.minX) + ancestor.layoutMargins.left | ||
| let right = (bounds.maxX - ancestorBounds.maxX) + ancestor.layoutMargins.right | ||
| view.layoutMargins.left = left | ||
| view.layoutMargins.right = right | ||
|
|
||
| @unknown default: | ||
| debugPrint("Unsupported stackView axis: \(stackView.axis)") | ||
| } | ||
| } | ||
| } | ||
| observation.attach(to: view) |
There was a problem hiding this comment.
The KVO-based Combine publisher for \.frame does not guarantee that it will publish values on the main thread. UI updates, such as changing view.layoutMargins, must be performed on the main thread. Using MainActor.assumeIsolated is unsafe here as it doesn't enforce execution on the main actor; it only silences compiler warnings. A property change on a background thread could lead to a crash.
To ensure thread safety, you should explicitly receive updates on the main run loop.
let observation = ancestor.publisher(for: \.frame, options: [.initial, .new])
.receive(on: RunLoop.main)
.sink { [weak view, weak stackView, weak ancestor] _ in
guard let view = view,
let stackView = stackView,
let ancestor = ancestor
else { return }
let bounds = view.bounds
let ancestorBounds = view.convert(ancestor.bounds, to: view)
switch stackView.axis {
case .horizontal:
let top = (ancestorBounds.minY - bounds.minY) + ancestor.layoutMargins.top
let bottom = (bounds.maxY - ancestorBounds.maxY) + ancestor.layoutMargins.bottom
view.layoutMargins.top = top
view.layoutMargins.bottom = bottom
case .vertical:
let left = (ancestorBounds.minX - bounds.minX) + ancestor.layoutMargins.left
let right = (bounds.maxX - ancestorBounds.maxX) + ancestor.layoutMargins.right
view.layoutMargins.left = left
view.layoutMargins.right = right
@unknown default:
debugPrint("Unsupported stackView axis: \(stackView.axis)")
}
}
observation.attach(to: view)| if alignment.contains(.leading) { wrapped.leadingAnchor.constraint(equalTo: layoutMarginsGuide.leadingAnchor).isActive = true } | ||
| if alignment.contains(.left) { wrapped.leftAnchor.constraint(equalTo: layoutMarginsGuide.leftAnchor).isActive = true } | ||
| if alignment.contains(.centerX) { wrapped.centerXAnchor.constraint(equalTo: layoutMarginsGuide.centerXAnchor).isActive = true } | ||
| if alignment.contains(.right) { wrapped.rightAnchor.constraint(equalTo: layoutMarginsGuide.rightAnchor).isActive = true } | ||
| if alignment.contains(.trailing) { wrapped.trailingAnchor.constraint(equalTo: layoutMarginsGuide.trailingAnchor).isActive = true } | ||
| if alignment.contains(.fillHorizontal) { | ||
| wrapped.leadingAnchor.constraint(equalTo: layoutMarginsGuide.leadingAnchor).isActive = true | ||
| wrapped.trailingAnchor.constraint(equalTo: layoutMarginsGuide.trailingAnchor).isActive = true | ||
| } |
There was a problem hiding this comment.
The current implementation for applying horizontal alignment constraints can lead to redundant constraints being created. For example, if alignment contains both .fillHorizontal and .leading, the constraint for the leading anchor will be created twice. This is because the if alignment.contains(.fillHorizontal) block adds a leading anchor constraint, and so does the if alignment.contains(.leading) block.
A similar issue exists for vertical alignment on lines 75-81.
To avoid this, you could restructure the logic to handle the fill cases exclusively.
| if alignment.contains(.leading) { wrapped.leadingAnchor.constraint(equalTo: layoutMarginsGuide.leadingAnchor).isActive = true } | |
| if alignment.contains(.left) { wrapped.leftAnchor.constraint(equalTo: layoutMarginsGuide.leftAnchor).isActive = true } | |
| if alignment.contains(.centerX) { wrapped.centerXAnchor.constraint(equalTo: layoutMarginsGuide.centerXAnchor).isActive = true } | |
| if alignment.contains(.right) { wrapped.rightAnchor.constraint(equalTo: layoutMarginsGuide.rightAnchor).isActive = true } | |
| if alignment.contains(.trailing) { wrapped.trailingAnchor.constraint(equalTo: layoutMarginsGuide.trailingAnchor).isActive = true } | |
| if alignment.contains(.fillHorizontal) { | |
| wrapped.leadingAnchor.constraint(equalTo: layoutMarginsGuide.leadingAnchor).isActive = true | |
| wrapped.trailingAnchor.constraint(equalTo: layoutMarginsGuide.trailingAnchor).isActive = true | |
| } | |
| if alignment.contains(.fillHorizontal) { | |
| wrapped.leadingAnchor.constraint(equalTo: layoutMarginsGuide.leadingAnchor).isActive = true | |
| wrapped.trailingAnchor.constraint(equalTo: layoutMarginsGuide.trailingAnchor).isActive = true | |
| } else { | |
| if alignment.contains(.leading) { wrapped.leadingAnchor.constraint(equalTo: layoutMarginsGuide.leadingAnchor).isActive = true } | |
| if alignment.contains(.left) { wrapped.leftAnchor.constraint(equalTo: layoutMarginsGuide.leftAnchor).isActive = true } | |
| if alignment.contains(.centerX) { wrapped.centerXAnchor.constraint(equalTo: layoutMarginsGuide.centerXAnchor).isActive = true } | |
| if alignment.contains(.right) { wrapped.rightAnchor.constraint(equalTo: layoutMarginsGuide.rightAnchor).isActive = true } | |
| if alignment.contains(.trailing) { wrapped.trailingAnchor.constraint(equalTo: layoutMarginsGuide.trailingAnchor).isActive = true } | |
| } |
| } | ||
|
|
||
| extension View: AnchorGroupProvider { | ||
| extension View: @MainActor AnchorGroupProvider { |
There was a problem hiding this comment.
The @MainActor annotation on this protocol conformance is redundant because the AnchorGroupProvider protocol itself is already annotated with @MainActor. When a protocol is marked with a global actor, all conforming types must satisfy its requirements on that actor. Adding the annotation to the conformance as well is unnecessary and adds noise to the code. This pattern appears in other places as well (e.g., VisualLayout.swift).
| extension View: @MainActor AnchorGroupProvider { | |
| extension View: AnchorGroupProvider { |
…ith edge alignment in AlignmentView Previously, if alignment contained both .fillHorizontal and .leading (or .trailing), the leading/trailing anchor constraint would be created twice — once in the individual edge check and once in the fill block. Same for vertical. Restructured the logic to use if-else, making fill cases exclusive with edge alignments so each anchor constraint is only created once.
1 participant
No description provided.