Repository navigation
Layoutable and SizeCalculable protocols - #148
Conversation
| // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN | ||
| // THE SOFTWARE. | ||
|
|
||
| /* |
There was a problem hiding this comment.
Temporarily commented out, should be removed
There was a problem hiding this comment.
Should probably copy/paste comments before deleting the file 🤔
There was a problem hiding this comment.
Keep the file for now, we could do that in another PR
| @@ -23,7 +23,7 @@ import Foundation | |||
| import UIKit | |||
|
|
|||
| extension UIView { | |||
There was a problem hiding this comment.
Should probably be moved in the extension where protocol conformance is declared
There was a problem hiding this comment.
I agree, this should be moved in UIView+PinLayout.swift and NSView+PinLayout.swift
|
|
||
| @available(OSX 10.10, *) | ||
| private func computeLegacyFitSize(size: Size) -> Size { | ||
| guard let sizeCalculableView = view as? SizeCalculable else { |
There was a problem hiding this comment.
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.
|
|
||
| @available(OSX 10.10, *) | ||
| private func computeSizeToFit(adjustSizeType: AdjustSizeType, size: Size) -> Size { | ||
| guard let sizeCalculableView = view as? SizeCalculable else { |
| // Currently we only support 1 and 2 levels. | ||
| } else { | ||
| return referenceSuperview.convert(point, to: layoutSuperview) | ||
| return referenceSuperview.convert(point, toView: layoutSuperview as! View.View) |
There was a problem hiding this comment.
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?
|
|
||
| displayText += ", Tag: \(view.tag))\n" | ||
| // Removed, layoutable tag should probably not be exposed | ||
| //displayText += ", Tag: \(view.tag))\n" |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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, CustomDebugStringConvertibleThere was a problem hiding this comment.
I didn't know this one, excellent idea.
|
|
||
| import Foundation | ||
|
|
||
| extension Layoutable { |
There was a problem hiding this comment.
Yeah, really nice to add this as an extension of the Layoutable protocol 👍
| } | ||
|
|
||
| @available(OSX 10.10, *) | ||
| extension NSControl: SizeCalculable { |
|
|
||
| import Foundation | ||
|
|
||
| @available(OSX 10.10, *) |
There was a problem hiding this comment.
The @available is not really required in the protocol declaration.
| extension UIView: Layoutable, SizeCalculable { | ||
| public typealias View = UIView | ||
|
|
||
| public func convert(_ point: CGPoint, toView view: UIView) -> CGPoint { |
There was a problem hiding this comment.
I'm not sure to understand why does this method is required? Why not simply use directly the method convert(:CGPoint, to: UIView?) 🤔
There was a problem hiding this comment.
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.
|
|
||
| extension UIView { | ||
| func getRect(keepTransform: Bool) -> CGRect { | ||
| public func getRect(keepTransform: Bool) -> CGRect { |
There was a problem hiding this comment.
To be cleaner, I would move immediately getRect and setRect in UIView+PinLayout.swift
|
|
||
| extension NSView { | ||
| func getRect(keepTransform: Bool) -> CGRect { | ||
| public func getRect(keepTransform: Bool) -> CGRect { |
There was a problem hiding this comment.
Same here, I would move immediately getRect and setRect in NSView+PinLayout.swift
| } | ||
|
|
||
| displayText += ", Tag: \(view.tag))\n" | ||
| displayText += ", Debug description: \(view.debugDescription))\n" |
There was a problem hiding this comment.
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 {
}
There was a problem hiding this comment.
@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 ?
90754c4 to
f4f55f7
Compare
|
Thanks @antoinelamy for this amazing PR 👍 😄 |
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.