Layoutable and SizeCalculable protocols #148

antoinelamy · 2018-06-12T23:37:40Z

Refactoring to avoid having to deal directly with view types making it easier to extend layouting to other APIs (ie: CALayer).

I avoided moving/adding files for now to avoid potential conflicts but I feel like some things should be renamed/moved. For exemple, getRect / setRect / isLTR() functions should probably be moved where protocol conformance is declared.

antoinelamy · 2018-06-12T23:48:01Z

Sources/PinLayout.swift

@@ -16,7 +16,7 @@
 //  LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
 //  OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
 //  THE SOFTWARE.
-
+/*


Temporarily commented out, should be removed

Should probably copy/paste comments before deleting the file 🤔

Keep the file for now, we could do that in another PR

antoinelamy · 2018-06-12T23:48:58Z

Sources/Impl/View+LTR.swift

@@ -23,7 +23,7 @@ import Foundation
 import UIKit

 extension UIView {


Should probably be moved in the extension where protocol conformance is declared

I agree, this should be moved in UIView+PinLayout.swift and NSView+PinLayout.swift

lucdion · 2018-06-13T10:59:03Z

Sources/Impl/PinLayoutImpl+Layouting.swift

    private func computeLegacyFitSize(size: Size) -> Size {
+        guard let sizeCalculableView = view as? SizeCalculable else {


It would be better to check if the View implement SizeCalculable in the method setAdjustSizeType() for types .fitTypeWidth, .fitTypeHeight, .fitTypeWidthFlexible, .fitTypeHeightFlexible and .fitSizeLegacy. And here we should simply asserts in the guard, since it should never occurs.

lucdion · 2018-06-13T10:59:21Z

Sources/Impl/PinLayoutImpl+Layouting.swift

    private func computeSizeToFit(adjustSizeType: AdjustSizeType, size: Size) -> Size {
+        guard let sizeCalculableView = view as? SizeCalculable else {


lucdion · 2018-06-13T11:03:39Z

Sources/Impl/PinLayoutImpl+Coordinates.swift

            let layoutSuperviewRect = layoutSuperview.getRect(keepTransform: keepTransform)
            return CGPoint(x: point.x - layoutSuperviewRect.origin.x,
                           y: point.y - layoutSuperviewRect.origin.y)
        // TOOD: Handle all cases. computeCoordinates should compute coordinates using only untransformed
        //       coordinates, but UIView.convert(...) below use transformed coordinates!
        //       Currently we only support 1 and 2 levels.
        } else {
-            return referenceSuperview.convert(point, to: layoutSuperview)
+            return referenceSuperview.convert(point, toView: layoutSuperview as! View.View)


It could be layoutSuperview as? View.View.
Also, View.View is a little bit weird 🙂, in my branch I was using TLayoutable instead of View for the generic type name. What do you think?

lucdion · 2018-06-13T11:12:51Z

Sources/Impl/PinLayoutImpl+Warning.swift

@@ -152,7 +152,8 @@ extension PinLayoutImpl {
            #endif
        }

-        displayText += ", Tag: \(view.tag))\n"
+        // Removed, layoutable tag should probably not be exposed
+        //displayText += ", Tag: \(view.tag))\n"


Maybe the Layoutable should expose a property warningContext, that would expose a String returning this block content:

var hierarchy: [String] = [] .... displayText += ", Tag: \(view.tag))\n"

The idea here is to give as much info as possible of the View being layouted

It would probably feel weird to have it directly in Layoutable as it have nothing to do with layout. If the goal is to print as much info as possible, why not use the built-in CustomDebugStringConvertible protocol that already serve that purpose and print the tag if any and many more properties. We could add it as a requirement to Layoutable, what do you think?

protocol Layoutable: AnyObject, Equatable, CustomDebugStringConvertible

I didn't know this one, excellent idea.

lucdion · 2018-06-13T11:16:40Z

Sources/Layoutable+PinLayout.swift

+
+import Foundation
+
+extension Layoutable {


Yeah, really nice to add this as an extension of the Layoutable protocol 👍

lucdion · 2018-06-13T11:19:58Z

Sources/NSView+PinLayout.swift

@@ -45,4 +35,9 @@ public extension NSView {
    }
 }

+@available(OSX 10.10, *)
+extension NSControl: SizeCalculable {


lucdion · 2018-06-13T11:22:25Z

Sources/SizeCalculable.swift

+
+import Foundation
+
+@available(OSX 10.10, *)


The @available is not really required in the protocol declaration.

lucdion · 2018-06-13T11:25:29Z

Sources/UIView+PinLayout.swift

+extension UIView: Layoutable, SizeCalculable {
+    public typealias View = UIView
+
+    public func convert(_ point: CGPoint, toView view: UIView) -> CGPoint {


I'm not sure to understand why does this method is required? Why not simply use directly the method convert(:CGPoint, to: UIView?) 🤔

I initially thought it was a collision with the function taking UISpaceCoordinates as a parameter but after investigation, it was because the view parameter is optional. Fixed.

lucdion · 2018-06-13T11:27:22Z

Sources/View+Rect.swift

@@ -12,7 +12,7 @@ import Foundation
 import UIKit

 extension UIView {
-    func getRect(keepTransform: Bool) -> CGRect {
+    public func getRect(keepTransform: Bool) -> CGRect {


To be cleaner, I would move immediately getRect and setRect in UIView+PinLayout.swift

lucdion · 2018-06-13T11:27:43Z

Sources/View+Rect.swift

@@ -57,7 +57,7 @@ extension UIView {
 import AppKit

 extension NSView {
-    func getRect(keepTransform: Bool) -> CGRect {
+    public func getRect(keepTransform: Bool) -> CGRect {


Same here, I would move immediately getRect and setRect in NSView+PinLayout.swift

lucdion · 2018-06-13T22:02:42Z

Sources/Impl/PinLayoutImpl+Warning.swift

@@ -152,7 +152,7 @@ extension PinLayoutImpl {
            #endif
        }

-        displayText += ", Tag: \(view.tag))\n"
+        displayText += ", Debug description: \(view.debugDescription))\n"


In fact @antoinelamy, I would have put everything in the debugDescription property, anyway we will need to do that for other Layoutable.

I would include all this:

var hierarchy: [String] = [] ... if hierarchy.count > 0 { }

@lucdion not sure about that one, debugDescription is already implemented on NSView/UIView and it's already pretty verbose. Furthermore, we cannot override an existing function from an extension so I would leave the responsibility to the caller with special debugging needs to override debugDescription in their subclasses. What do you think @lucdion ?

lucdion · 2018-06-15T00:21:32Z

Thanks @antoinelamy for this amazing PR 👍 😄

antoinelamy commented Jun 12, 2018

View reviewed changes

lucdion reviewed Jun 13, 2018

View reviewed changes

antoinelamy added 10 commits June 14, 2018 08:45

Make PinLayout implementation public and remove interface

06a91e4

Extract protocol Layoutable

b2d76e6

Extract sizing in a separate SizeCalculable protocol

8597168

Make test class conform to SizeCalculable

369cfb8

Move functions alongside protocol conformance declaration

3a2c48f

Code review comments

addad7d

Fix ObjC support

e055676

Check for SizeCalculable before assigning adjustSizeType

7ddb2bb

Add missing imports

46c7d94

Bring back comments from PinLayout commented interface

f4f55f7

antoinelamy force-pushed the refactor/layoutable branch from 90754c4 to f4f55f7 Compare June 14, 2018 12:45

Drop support for macOS 10.9 and bump deployment target to 10.10

331757a

lucdion merged commit 230e533 into layoutBox:master Jun 15, 2018

antoinelamy deleted the refactor/layoutable branch June 27, 2018 13:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Layoutable and SizeCalculable protocols #148

Layoutable and SizeCalculable protocols #148

antoinelamy commented Jun 12, 2018

antoinelamy Jun 12, 2018

antoinelamy Jun 13, 2018

lucdion Jun 13, 2018

antoinelamy Jun 12, 2018

lucdion Jun 13, 2018

lucdion Jun 13, 2018

antoinelamy Jun 13, 2018

lucdion Jun 13, 2018

antoinelamy Jun 13, 2018

lucdion Jun 13, 2018

lucdion Jun 13, 2018

antoinelamy Jun 13, 2018

lucdion Jun 13, 2018

lucdion Jun 13, 2018

lucdion Jun 13, 2018

lucdion Jun 13, 2018

antoinelamy Jun 13, 2018

lucdion Jun 13, 2018

antoinelamy Jun 13, 2018

lucdion Jun 13, 2018

antoinelamy Jun 13, 2018

lucdion Jun 13, 2018

antoinelamy Jun 13, 2018

lucdion Jun 13, 2018

antoinelamy Jun 13, 2018

lucdion commented Jun 15, 2018

		@@ -23,7 +23,7 @@ import Foundation
		import UIKit

		extension UIView {

		private func computeLegacyFitSize(size: Size) -> Size {
		guard let sizeCalculableView = view as? SizeCalculable else {

		private func computeSizeToFit(adjustSizeType: AdjustSizeType, size: Size) -> Size {
		guard let sizeCalculableView = view as? SizeCalculable else {

Layoutable and SizeCalculable protocols #148

Layoutable and SizeCalculable protocols #148

Conversation

antoinelamy commented Jun 12, 2018

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

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

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 commented Jun 15, 2018