Skip to content
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

Improve Layout and Constraint Implementation (so that the control works well in Interface Builder.) #118

Closed
MosheBerman opened this issue Aug 3, 2017 · 6 comments

Comments

@MosheBerman
Copy link
Owner

MosheBerman commented Aug 3, 2017

Summary

Right now CKCalendarView on the 5.0.0 branch works pretty decently with auto layout when instantiated with code. The problem is that the way layout is defined, we do all of the work in updateConstraints. Views are installed, constraints are created, and constraints are updated. This is far from ideal, for a number of reasons. (See the sources at the bottom for more on this.)

Expected Behavior

The control should work correctly when instantiated from code and from Interface Builder.

Actual Behavior

The control works almost correctly in code (except for initial layout) and completely incorrectly in Interface Builder.

Notes

I noticed that when I moved my subview installation logic from layoutSubviews to updateConstraints, constraint based cell animations became much smoother. However, when I tried to add Interface Builder support, I noticed that things weren't going right.

Instances of the calendar view that were created in code initially were a little too short. Calendars created in Interface Builder had too much space between the cells until the user forced a layout pass.

A screenshot of the iOS Simulator with a buggy Calendar View

Interface Builder itself showed a skewed version of the calendar, with a similar problem. Here, though, the bottom border rendered fine, but the calendar itself occupies the top left corner of the control, instead of the full size. The same cell spacing problem occurs as when the code is run.

A screenshot of Interface Builder with a buggy Calendar View

Digging in a little more deeply, I realized that my layout setup was probably incorrect. So it turns out, that even though it made sense to me to have a single method for installing each view, (_installTable, _installHeader, etc) this design ignores the structure of UIView layout. I need to rethink this.

The fix here is probably to go through the view lifecycle and properly set up all of the parts at the appropriate times.


Sources:

  1. Auto Layout Best Practices for Minimum Pain (Omar Abdelhafith, Medium)
  2. Demistifying iOS Layout (Ami Kumar, GC Tech Blog)
  3. How to Use updateConstraints (Ole Begemann)
  4. Advanced Autolayout Toolbox (objc.io)
  5. Render and Inspect Your Custom Views (Apple)
  6. Modifying Constraints (Apple)
  7. layoutSubviews() documentation (Apple)
  8. updateConstraints() documentation (Apple)
  9. Layout Guide: Understanding Auto Layout (Apple)
@MosheBerman
Copy link
Owner Author

This article offered a red herring but ultimately led me closer to my solution:

Advanced iOS Animations with Autolayout

@MosheBerman
Copy link
Owner Author

MosheBerman commented Aug 10, 2017

What Went Wrong

tl;dr By using auto layout for the everything but the positions of calendar cells, and using manual layout for the cells themselves, I was creating complexity that I couldn't solve for. When I tried to fix it by naively adding constraints, the visuals looked great, but performance suffered.

So it turns out™ that the cells were incorrectly spaced because the layout engine was doing cell arithmetic instead of properly pinning the views to each other. In a pre-autolayout world, this was accurate and fast. Once autolayout came into play, I made changes to account for updateConstraints and the intrinsic content size of the calendar view.

This increasingly complex layout environment meant that somewhere, the dimensions of each cell, which almost all of the old layout code was based on, ended up wrong in one of the layout passes.
Rather than track down exactly what was causing this, I looked for another layout approach. (I still don't know why the calendar view had the wrong spacing, but the right size for the cells.)

Trying on Autolayout Constraints for Size

My first solution was to pin the cells' trailing and leading edges using constraints. With this approach, I'm still using manually recycled UIView subclasses for calendar cells, and managing the recycling and positioning myself.

Using constraints fixed the layout inconsistencies by leaning on auto layout to guarantee correct pinning, instead of basing it on the width of the calendar view. It also had the added benefit of correct rendering in localized right-to-left environments.

The tradeoff here was an unacceptable performance hit. Running in the simulator on a quad core machine, the demo app performed fine, but an iPod touch running iOS 9 took well over a minute to switch between months. (Instruments verified this.)

A screenshot of Instruments Profile of MBCalendarKit's auto layout struggles

Layout Constraints, Memory Constraints, Performance Constraints

I tried iterating on this by caching constraints on the cells, using NSMapTable properties to store vertical and horizontal pinning constraints, by the new cell to pin against. The idea here was that each cell has its constraints deactivated when it animates off-screen. If each cell kept copies of its layout constraints around that could be referenced by keying into the cache with the other item in the constraint, we could activate them based on the paired view, perhaps alleviating the bottleneck.

This wasn't how it worked out. Caching constraints reduces the time spent creating new constraints on subsequent layout passes (by allocating memory for them only once) but it doesn't solve for the first time a cell is pinned to another cell. So if Cell A is pinned to Cell B the first time around, and then is pinned to Cell C, the constraint is created a second time.

This is even worse from a memory standpoint, because even though we have a cache with pretty good access, memory usage is factorial - we have one constraint on each cell for every other cell. A grid requires pinning in both the horizontal and the vertical direction, so, not that it makes a huge difference, but the performance becomes O(2 * n!). Ouch!

More importantly, creating the constraints isn't the expensive part of the layout operation - activating the constraints is. "Activating" a constraint essentially causes the auto layout engine to solve the layout again.

Calling [someView addConstraints:@[constraintX, constraintY]] is the same as calling [NSLayoutConstraint activateConstraints:@[constraintX, constraintY] which is in turn the same as contraintX.isActive = YES; constraintY.isActive = YES. (The addConstraints: call requires you to know which view to call the method on, while the others figure this out for you.)

Doing this about 60 times per transition is what was causing the delay. 🤦‍♂️

I also want to call out that the documentation for NSLayoutConstraint activateConstraints:` says that this method is "usually more efficient" than manually activating each constraint, but profiling has shown that it didn't seem to make a difference in this case.

Collecting the Pieces with Collection View

Finally, I began to try laying out the cells with UICollectionView. It turns out that implementing a grid representing a calendar month is absurdly easy with UICollectionView. The develop branch is in an incomplete state right now, but as things progress, this approach seems to offer everything I was looking for: Autolayout support, proper layout in right-to-left environments, and most importantly, buttery smooth performance.

This brings me to the point of this comment, which is that this issue is now blocked by #109 Upgrade to UICollectionView as well. It's a work in progress, but so far I'm happy with it.

Final Thoughts

How UIStackView Stacks Up

I have no idea. I didn't try, and don't plan to if UICollectionView pans out.

Interface Builder Support, Built In

Another added benefit of this journey through cleaning up and optimizing MBCalendarKit's layout is that the IBDesignable rendering of the calendar control began to work once I simplified some of the intrinsic content size and layout calculations of CKCalendarView. 🎉

Improve Your iOS Foundations by Reading This

I found this issue of objc.io on characteristics of foundation classes and it looks absolutely amazing.

@MosheBerman
Copy link
Owner Author

Collection View is a Go 👍

I got this to work nicely with UICollectionView and a custom subclass of UICollectionViewFlowLayout. Animation was really easy, by implementing initialLayoutAttributesForAppearingItemAtIndexPath: and finalLayoutAttributesForDisappearingItemAtIndexPath:, I was able to re-implement cell animation and not have to worry about any of the positioning. UICollectionViewFlowLayout did all of the hard work.

Benefits

This actually enabled a large number of enhancements to the framework by itself, including proper right-to-left support, and week view animation.

What Actually Went Wrong! 🤦‍♂️

While I was working on this, though, I came across something really interesting. The reason the why the cell spacing was wrong initially was because views that are instantiated in a storyboard have a default bounds size of 600 pixels by 600 pixels.

The size doesn't change until the view moves to the window, by which time the cells have already been laid out. The simplest fix would have been to reload the cells in didMoveToWindow.

Known Bugs

There is one slightly noticeable issue here, which is that when transitioning between months that have a different number of weeks visible, there are gaps in the animation. This is less notificable if the background color and the cell color is the same. (which it is by default, so I'm not worrying about it for 5.0.0)

@MosheBerman
Copy link
Owner Author

Known Bugs Redux

The issue with the cells being slightly off in transitions from month to month with different week lengths is because we're using the contentSize of the collection view before transitioning, and not the height for the new set of rows to determine offset.

@MosheBerman MosheBerman mentioned this issue Aug 17, 2017
@MosheBerman
Copy link
Owner Author

Going to open an issue for that transition bug, and then close this.

@MosheBerman
Copy link
Owner Author

Opened as #125

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant