Fix wrap content calculation by filter hidden views #280

ruslandzhafarov · 2024-03-28T15:53:44Z

No description provided.

Sources/PinLayout+WrapContent.swift

MontakOleg · 2024-03-28T20:43:27Z

Sources/PinLayout+WrapContent.swift

+        let subviews: [PinView.PinView]
+        switch viewFilter {
+        case .visibleOnly:
+            subviews = view.subviews.filter { !$0.isHidden }


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 })

, it may lead to confusion if name "view filter" will behave differently here.

Good point, you could use the visible() here to filter views.

MontakOleg · 2024-03-28T20:45:10Z

Sources/PinLayout+WrapContent.swift

    }

-    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 {


do we need default argument value (viewFilter = .none) in private function?

Not needed here, the parameter should always be specified by callers

lucdion

Excellent. Could you update the documentation of wrapContent (README.md)? 🙏

lucdion · 2024-03-28T20:53:47Z

Sources/PinLayout+WrapContent.swift

+        let subviews: [PinView.PinView]
+        switch viewFilter {
+        case .visibleOnly:
+            subviews = view.subviews.filter { !$0.isHidden }


Good point, you could use the visible() here to filter views.

lucdion · 2024-03-28T20:54:45Z

Sources/Types.swift

+@objc public enum ViewFilter: Int {
+    /// No filter, use all views
+    case none
+    /// Consider only views where `hidden` is false


Suggested change

/// Consider only views where `hidden` is false

/// Consider only visible views (isHidden is false or alpha is > 0)

I believe

isHidden is false or alpha is > 0

should be:

isHidden is false and alpha is > 0

MontakOleg · 2024-03-28T22:31:33Z

Sources/Layoutable.swift

@@ -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 }


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 } } } ...

Another option to avoid duplicating implementations:

protocol Layoutable { var isConsideredVisibleForViewFilters: Bool { get } ... } extension Layoutable { var isConsideredVisibleForViewFilters: Bool { return true } // default impl }

lucdion · 2024-04-05T12:38:27Z

Sources/Extensions/CALayer+PinLayout.swift

@@ -30,6 +30,10 @@ extension CALayer: Layoutable {
        return sublayers ?? []
    }

+    public var isVisible: Bool {
+        !isHidden && opacity > 0


Suggested change

!isHidden && opacity > 0

return !isHidden && opacity > 0

lucdion · 2024-04-05T12:38:55Z

Sources/Extensions/NSView+PinLayout.swift

@@ -33,6 +33,10 @@ extension NSView: Layoutable {
        return PinLayout(view: self, keepTransform: false)
    }

+    public var isVisible: Bool {
+        !isHidden && alphaValue > 0


Suggested change

!isHidden && alphaValue > 0

return !isHidden && alphaValue > 0

lucdion · 2024-04-05T12:39:11Z

Sources/Extensions/UIView+PinLayout.swift

@@ -37,6 +37,10 @@ extension UIView: Layoutable, SizeCalculable {
        return PinLayoutObjCImpl(view: self, keepTransform: true)
    }

+    public var isVisible: Bool {
+        !isHidden && alpha > 0


Suggested change

!isHidden && alpha > 0

return !isHidden && alpha > 0

lucdion · 2024-04-05T12:40:57Z

Sources/Types.swift

+    /// No filter, use all views
+    case none


After more thinking, I think it would be clearer if we rename none for all

Suggested change

/// No filter, use all views

case none

/// Consider all views

case all

lucdion · 2024-04-05T12:46:29Z

Sources/Layoutable.swift

@@ -29,6 +29,8 @@ public protocol Layoutable: AnyObject, Equatable, CustomDebugStringConvertible {
    var superview: PinView? { get }
    var subviews: [PinView] { get }

+    var isVisible: Bool { get }


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

Fix wrap content calculation by filter hidden views

e20a081

ruslandzhafarov closed this Mar 28, 2024

ruslandzhafarov deleted the patch-1 branch March 28, 2024 16:55

ruslandzhafarov restored the patch-1 branch March 28, 2024 16:56

lucdion requested changes Mar 28, 2024

View reviewed changes

Sources/PinLayout+WrapContent.swift Outdated Show resolved Hide resolved

ruslandzhafarov reopened this Mar 28, 2024

Ruslan Dzhafarov added 3 commits March 28, 2024 19:41

Fix wrap content calculation by filter hidden views

98258f8

Add comments

fb3a375

minor fixes

7d756bc

ruslandzhafarov requested a review from lucdion March 28, 2024 18:22

MontakOleg reviewed Mar 28, 2024

View reviewed changes

lucdion reviewed Mar 28, 2024

View reviewed changes

minor fixes

6d81204

MontakOleg reviewed Mar 28, 2024

View reviewed changes

Ruslan Dzhafarov added 2 commits March 29, 2024 00:54

minor fixes

69903d5

update README.md

46e93fa

ruslandzhafarov requested a review from lucdion March 28, 2024 23:07

lucdion reviewed Apr 5, 2024

View reviewed changes

Ruslan Dzhafarov added 2 commits April 5, 2024 20:23

minor fixes

44697d4

update README.md

de0868e

ruslandzhafarov requested review from lucdion and MontakOleg April 5, 2024 17:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix wrap content calculation by filter hidden views #280

Fix wrap content calculation by filter hidden views #280

ruslandzhafarov commented Mar 28, 2024

MontakOleg Mar 28, 2024

lucdion Mar 28, 2024

MontakOleg Mar 28, 2024

lucdion Mar 28, 2024

lucdion left a comment

lucdion Mar 28, 2024

lucdion Mar 28, 2024

MontakOleg Mar 28, 2024

MontakOleg Mar 28, 2024

MontakOleg Mar 28, 2024

lucdion Apr 5, 2024

lucdion Apr 5, 2024

lucdion Apr 5, 2024

lucdion Apr 5, 2024

lucdion Apr 5, 2024 •

edited

	/// Consider only views where `hidden` is false
	/// Consider only visible views (isHidden is false or alpha is > 0)

	!isHidden && alphaValue > 0
	return !isHidden && alphaValue > 0

Fix wrap content calculation by filter hidden views #280

Are you sure you want to change the base?

Fix wrap content calculation by filter hidden views #280

Conversation

ruslandzhafarov commented Mar 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucdion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucdion Apr 5, 2024 • edited

Choose a reason for hiding this comment

lucdion Apr 5, 2024 •

edited