ustwo / baseview-swift

UIView subclass to abstract Base functionality for iOS
MIT License
1 stars 0 forks source link

Only call setupConstraints if opted-in to auto layout and during the "measurement pass" #13

Open markuspersson opened 8 years ago

markuspersson commented 8 years ago

Problem 1

The constraints should be constructed during the measurement pass in the layout process to make sure they are added when the system asks for them.

Problem 2

From a developer point of view; if I decide to not use auto layout in my layout I wouldn't expect to get a call to setupConstraints()

Proposed solution

When setting up local constraints, as it is done within the View.swift in the sample, we make this view dependent on auto layout and it cannot be used anymore without auto layout enabled. Because of this it is best to make this dependency explicit by implementing requiresConstraintBasedLayout to return true.

By setting the requiresConstraintBasedLayout we tell the system to call back when it is ready to receive the local constraints, this callback is the method: updateConstraints() and it will be called during the measurement pass. The measurement pass is the first thing that is happening within the layout process. Meaning it will call bottom-up in the view-tree and ask for each view's constraints. Then it prepares the views so that the next step in the layout can set the actual frame of the views.

In a basic layout it almost always works fine to construct the constraints when the view is initialising. But when it comes to a more complex layout with activation/de-activation of constraints, priorities, e.tc during runtime we need to make sure the changes are happening when the system is asking for them, i. e during the measurement pass.

In BaseView.swift I would suggest the following changes:

private(set) var needsSetupOfConstraints = true

override func updateConstraints() {
    if needsSetupOfConstraints {
        self.setupConstraints()
        needsSetupOfConstraints = false
    }

    super.updateConstraints()
}

Then in View.swift I would add this, telling the system we use local constraints to construct our layout:

override class func requiresConstraintBasedLayout() -> Bool {
    return true
}

So basically we move the setupConstraints() to the updateConstraints() callback to make sure we construct the constraints when the system asks for it. We also make sure the setupConstraints() is called when we are opted-in to auto layout. Meaning it will not be called if we expect to create a layout with frames and not auto layout.

I have also added a flag to block the setup from happening more than once. Any call to setNeedsUpdateConstraints will trigger the measurements pass and the updateConstraints() will be called. Also any changes to the system of constraints will automatically trigger updateConstraints(), so we need to make sure we only construct the constraints once.

The changes in View.swift is to tell the system that we are opting-in to auto layout, and will construct the layout with constraints. This is only needed when creating constraints from code. When using NIBs and Storyboards, the system will return this automatically for us.

References

Apple docs:

https://developer.apple.com/library/ios/documentation/UIKit/Reference/UIView_Class/#//apple_ref/occ/instm/UIView/updateConstraints https://developer.apple.com/library/ios/documentation/UIKit/Reference/UIView_Class/#//apple_ref/occ/clm/UIView/requiresConstraintBasedLayout

Pretty good article about the different passes (although a bit old):

https://www.objc.io/issues/3-views/advanced-auto-layout-toolbox/

Please let me know what you think @madhikarma @aamctustwo

ghost commented 8 years ago

Problem 1:

I agree. This should be moved to a layout phase of the view.

Problem 2:

I see this as a non-issue. If someone is using BaseView but is setting up all of the constraints in InterfaceBuilder, then they would just not override the setupConstraints() method and the empty function would be called in BaseView, causing no change to the view.

markuspersson commented 8 years ago

@aamctustwo: Regarding problem 2, let me clarify. What I mean is opting-out of auto layout all-together. If you are setting up constraints in IB, then it's fine to get the setupConstraints() callback, you just don't override it because you already have your constraints. You are still in the world of auto layout, so all good here.

But if you don't have constraints in either IB or in code, and instead go with the setting of frame approach, then this method is not expected to be called, hence my suggestion to move it to updateConstraints().

You don't want choose the desired approach for the developer in a base class, the developer should be able to go with whatever approach he/she like.

danieladias commented 8 years ago

@markuspersson updateConstraints() is called multiple times and it's not to do setup but to change any existing constraints. Using it as setup implies adding code to check if constraints already exist. Correct me if I am wrong on this but the way I see it the setupConstraints() method does nothing. It's just a suggestion, putting it available if you want to use it. You could call it setupLayout if you want even. It's just a suggestion to do more than just setup. You could even do everything inside setup method if you wished.

Maybe it's me but I don't see it as forcing me to use constraints, it is just saying: "if you want to add constraints, do it here".

markuspersson commented 8 years ago

@danieladias It does say that local constraints should be added in this method: "Custom views that set up constraints themselves should do so by overriding this method."

I agree that it adds some extra complexity because this method can get called multiple times. However from a base class perspective you want to ask for the constraints when the system asks for it. Basically just because you don't know the complexity of the layout from the base class and you don't want to end up in a state where constraints can possibly break due to the system not being ready in the layout process.

danieladias commented 8 years ago

@markuspersson I am a bit confused by what you say about adding constraints when the system asks for it. It reminds me of lazy loading and I see that as an option. I usually add constraints after creating my views so I guarantee I only add them once and that is clear when that is happening. Again please feel free to correct me but I had the idea that adding the constraints is not dependent on the system being ready right? By ready I see it as all the views added to the superviews and all relations in constraints are valid because then the system will use this only when it's ready to layout everything on screen right?

markuspersson commented 8 years ago

@danieladias Don't get me wrong, I agree with you that normally we add constraints after the view has been created and everything is fine. This is also how IB treats it because the constraints from IB will be constructed even before any view initialisation has been done.

However adding constraints IS depending on whether the system is ready or not. This is because with auto layout there's 2 new passes in the layout process: updating constraints and layout of the view.

So with auto layout we have 3 different "stages" (or passes) in the layout process: update constraints, layout and then finally display. This is done exactly in that order every time a new layout process is triggered, so each stage is dependent on the previous.

Now the tricky part with auto layout is that this is not a one-way street, meaning that both the layout and the display stage can make changes to the constraints (as an example). Leading to a new layout process being triggered.

This is what I mean when I say "when the system asks for it". Because each stage can ask for the constraints, you need to make sure you give them when you are in the update constraints stage, i.e updateConstraints() method.

Also worth to note is that other views can own your view constraints. This also adds extra complexity to auto layout. There's no guarantee that you are the only owner of the constraints, except for the custom ones you add, so it is even more important that the setup is done when it should be.

Looking at the example in View.swift the titleLabel constraints are depending on the constraints of the View. The View constraints go all the way back to UIView, meaning that if any of these constraints would change (maybe by the UIViewController owning the UIView), they will affect your custom constraints on titleLabel. If these are not added when they are supposed to you would potentially get a conflict.

Hope this clarifies a little bit more. And as I said, I do agree with you that normally we just add them when the view is initialised and everything is fine. But when we start adding a base class for our views then I find it important that we call for the constraints when we are supposed to do it to prevent eventual problems in the future for the ones using the base class.

ghost commented 8 years ago

@markuspersson I like your proposed solution. However, I feel that auto-layout should be a default and not using it to require explicit intervention by the developer.

Autolayout has been around for a long time and has been heavily pushed in recent years by Apple, especially to take easy advantage of newer SDK features. Most the iOS code I have seen written at work in the last year has all used autolayout or was converted to using autolayout.

markuspersson commented 8 years ago

@aamctustwo That is perfectly fine with me. I haven't been using anything other than auto layout myself for a couple of years so :)

Then I would suggest that we add that as a dependency, that auto layout is required. And also to implement requiresConstraintBasedLayout in the BaseView class to force auto layout. To make sure that it is not being used by non-auto layout views.

ghost commented 8 years ago

@markuspersson :+1: Agreed!

madhikarma commented 8 years ago

My TLDR; Agree with you both about adding requiresConstraintBasedLayout to BaseView.

A bit of back history, we used to add constraints in updateConstraints without the flag a few years ago on iOS 6 and iOS 7 only to find when iOS 8 came out that layouts were getting broken due to the multiple constraints. The flag approached always seemed less nice so we installed the constraints first i.e. the convention of a setupConstraints method.

@markuspersson I like the proposed solution but I don't want to give too much behaviour in the abstract base and especially in a system method, what if a developer subclasses, actually wants to use updateConstraints and calls super but the proposed call to setup conflicts with what they're trying to do.

I figured we could keep it open to extension by simply them not using the method if they don't want to.

@markuspersson requiresConstraintBasedLayout is a great knowledge share for me at least I had no idea that existed in Cocoa but its been there since day 1 it seems. Implementing it should remove any doubt and hopefully help if someone tries to put an Auto Layout compatible BaseView subclasses into a non Auto Layout-ed view that auto resizing can't help with. Since it's a class method if we implement it, we are going to go out on a limb here and say all BaseView instances require Auto Layout and I'm ok with this. (although in the past some UIKit classes such a UITableView headers or footers were not working well with Auto Layout). We should test what happens when we actually do this and that there are no unexpected side effects to implementing the property but I'm all for it based on docs and what you explained :+1:

@danieladias Do you agree as well with the final agreed change?

danieladias commented 8 years ago

@markuspersson thank you so much for taking the time to explain it to me :+1:

@madhikarma fine by me! I was just trying to understand what Markus was saying.

markuspersson commented 8 years ago

@danieladias No problem!

@madhikarma

A bit of back history, we used to add constraints in updateConstraints without the flag a few years ago on iOS 6 and iOS 7 only to find when iOS 8 came out that layouts were getting broken due to the multiple constraints. The flag approached always seemed less nice so we installed the constraints first i.e. the convention of a setupConstraints method.

Yes, but that is an implementation error. You should never create the constraints more than once. I get that flagging is less ideal, but there's no API to check if constraints has been created. Given what it says in the documentation and how the layout process is designed I still think it's a good idea to use updateConstraints for the setup.

I like the proposed solution but I don't want to give too much behaviour in the abstract base and especially in a system method, what if a developer subclasses, actually wants to use updateConstraints and calls super but the proposed call to setup conflicts with what they're trying to do.

How can that happen? You never call updateConstraints yourself directly, it is always called by the system at the right time. If any subclass overrides this method and calls super (which they should), it will only setup the constraints if they are not set up already, otherwise update. I don't see how this could be an issue, or am I missing something?

I don't want to drag the discussion, just raising my concerns. I just get a bit worried when we are not utilising the methods from the system and when the documentation states that custom views should add its constraints in an override of updateConstraints.

ghost commented 8 years ago

@madhikarma I believe that @markuspersson is correct. We really should be doing constraint setup in updateConstraints. It follows Apple's guidelines and will likely save us headaches with future versions of iOS.

Just like you should not call drawRect yourself and instead call setNeedsDisplay, you should not call updateConstraints and instead call setNeedsUpdateConstraints (with the exception of super). That would be a violation of Apple's documentation for these functions. So a developer shouldn't be surprised if they get unexpected results.

Even then, you should really only call setNeedsUpdateConstraints this when you are removing constraints in order to force autolayout to recalculate and re-layout all the affected views. This situation too is beginning to disappear from my code bases with the advent of UIStackView and other views that manage their own layout code.

While adding a bool flag to make sure that constraints are only added once, this is a minimal addition to BaseView. It can (and should) be done in a similar manner to how we fixed the awakeFromNib bug (#5).