-
Notifications
You must be signed in to change notification settings - Fork 143
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
Fix wrap content calculation by filter hidden views #280
base: master
Are you sure you want to change the base?
Conversation
Sources/PinLayout+WrapContent.swift
Outdated
let subviews: [PinView.PinView] | ||
switch viewFilter { | ||
case .visibleOnly: | ||
subviews = view.subviews.filter { !$0.isHidden } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about views with alpha = 0
?
There is public visible(_ views: [UIView])
views filter that takes alpha into account -
PinLayout/Sources/Filters.swift
Line 25 in babea9c
return views.filter({ !$0.isHidden && $0.alpha > 0 }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, you could use the visible()
here to filter views.
Sources/PinLayout+WrapContent.swift
Outdated
} | ||
|
||
private func wrapContent(_ type: WrapType, padding: PEdgeInsets, _ context: Context) -> PinLayout { | ||
let subviews = view.subviews | ||
private func wrapContent(_ type: WrapType, padding: PEdgeInsets, viewFilter: ViewFilter = .none, _ context: Context) -> PinLayout { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need default argument value (viewFilter = .none
) in private function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed here, the parameter should always be specified by callers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent. Could you update the documentation of wrapContent
(README.md)? 🙏
Sources/PinLayout+WrapContent.swift
Outdated
let subviews: [PinView.PinView] | ||
switch viewFilter { | ||
case .visibleOnly: | ||
subviews = view.subviews.filter { !$0.isHidden } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, you could use the visible()
here to filter views.
Sources/Types.swift
Outdated
@objc public enum ViewFilter: Int { | ||
/// No filter, use all views | ||
case none | ||
/// Consider only views where `hidden` is false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// Consider only views where `hidden` is false | |
/// Consider only visible views (isHidden is false or alpha is > 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe
isHidden is false or alpha is > 0
should be:
isHidden is false and
alpha is > 0
Sources/Layoutable.swift
Outdated
@@ -29,6 +29,9 @@ public protocol Layoutable: AnyObject, Equatable, CustomDebugStringConvertible { | |||
var superview: PinView? { get } | |||
var subviews: [PinView] { get } | |||
|
|||
var isHidden: Bool { get } | |||
var alpha: CGFloat { get } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not find implementation for NSView
currently (in AppKit this property has different name – alphaValue
, so it should be explicitly proxied).
Also it looks like a breaking change (adding new requirement to public protocol). Maybe it's fine, but if not, we should have some safe default implementation.
I don't think that we can have reasonable default implementations for isHidden
and alpha
because it too general properties, but we can have something like:
protocol Layoutable {
func isVisible(forViewFilter: ViewFilter) -> Bool
...
}
extension Layoutable {
func isVisible(forViewFilter: ViewFilter) { return true } // default implementation
}
And then have explicit implementations for UIView, NSView, CALayer:
extension UIView {
func isVisible(forViewFilter viewFilter: ViewFilter) {
switch viewFilter {
case .none: return true
case .visibleOnly: return !isHidden && alpha > 0
}
}
}
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option to avoid duplicating implementations:
protocol Layoutable {
var isConsideredVisibleForViewFilters: Bool { get }
...
}
extension Layoutable {
var isConsideredVisibleForViewFilters: Bool { return true } // default impl
}
@@ -30,6 +30,10 @@ extension CALayer: Layoutable { | |||
return sublayers ?? [] | |||
} | |||
|
|||
public var isVisible: Bool { | |||
!isHidden && opacity > 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!isHidden && opacity > 0 | |
return !isHidden && opacity > 0 |
@@ -33,6 +33,10 @@ extension NSView: Layoutable { | |||
return PinLayout(view: self, keepTransform: false) | |||
} | |||
|
|||
public var isVisible: Bool { | |||
!isHidden && alphaValue > 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!isHidden && alphaValue > 0 | |
return !isHidden && alphaValue > 0 |
@@ -37,6 +37,10 @@ extension UIView: Layoutable, SizeCalculable { | |||
return PinLayoutObjCImpl(view: self, keepTransform: true) | |||
} | |||
|
|||
public var isVisible: Bool { | |||
!isHidden && alpha > 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!isHidden && alpha > 0 | |
return !isHidden && alpha > 0 |
Sources/Types.swift
Outdated
/// No filter, use all views | ||
case none |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After more thinking, I think it would be clearer if we rename none
for all
/// No filter, use all views | |
case none | |
/// Consider all views | |
case all |
Sources/Layoutable.swift
Outdated
@@ -29,6 +29,8 @@ public protocol Layoutable: AnyObject, Equatable, CustomDebugStringConvertible { | |||
var superview: PinView? { get } | |||
var subviews: [PinView] { get } | |||
|
|||
var isVisible: Bool { get } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I agree with @MontakOleg, we should avoid adding properties that could conflict with user's possible extension (isVisible
has a high risk of conflict). I like the idea of using the name isConsideredVisibleForViewFilters
No description provided.