Add support for UISplitViewController

[alfonzzz]

This pull request adds support for UISplitViewController on all devices, OS versions & orientations. It supports size classes directly so that on an iPad the settings are displayed with a split view, on iPhone as a standard tableview (collapsed split view) and on the iPhone 6+ as a tableview in portrait (compact) and split in landscape (large).
The behavior of UISplitViewController was changed in iOS 8 as it was made available on the iPhone. Therefore, wiring it up correctly to support all configurations is tricky. For this reason I added an IASKAppSettingsSplitViewController and updated the sample app to use it where relevant. For this reason I removed the settings tab from the storyboard and added it in code so that we use the correct view controller depending on the device & os version we're running on. This is intentionally in a separate commit so that we can revert it if you'd like to keep the storyboard example but in that case it won't support split view (as before).

There are some limitations:

• A UISplitViewController can't be displayed modally pre iOS 8 so I used a workaround that adds it to a container view controller - works nicely.
• A UISplitViewController can't be pushed onto a UINavigationController. The previous workaround can be used here as well but the external UINavigationController doesn't play nicely with the ones inside the UISplitViewController so this isn't recommended. Therefore in the sample app I don't use IASKAppSettingsSplitViewController for the push example.

I've added documentation where relevant & updated the readme.

Tested on all of the following configurations in the simulator:

• iPhone 4s, iPhone 5, iPhone 6, iPhone 6+, iPad Air
• iOS 7.1, iOS 8.3
• Portrait & Landscape
• Tab, Push, Modal, Popover

I don't have all of the above configurations so I tested on a selection of physical devices.

I believe this fix should also support the new split view (side by side apps) on iPads running iOS 9 but I haven't verified this because InAppSettingsKit doesn't compile under Xcode 7. I plan to submit a separate pull request for that after which this can be tested.

[futuretap]

Thanks for your great work. I just found a minor glitch: when a multi value specifier is updated in the detail view controller, the title in the master view controller is not updated to reflect the change. Could you fix this? Then I'm more than happy to pull!

[alfonzzz]

Nice catch! Check it out now - should work :-)

[alfonzzz]

Wait, I messed up this branch with a bad rebase. I'll fix it up and comment when it's ready for review properly....

[alfonzzz]

Ok so what happened here is that I was rebased above the branch in this pull request:
#312
So those changes got pushed as well by accident. I think that it's better to leave these separately so I'll wait for you to review that pull request and then I'll fix this (essentially I'll rebase on master after you accept/reject it and push again).

[alfonzzz]

Ok so this is ready for final review. I made a few changes since your last comment:

• Fixed the bug you found!
• Added an option for the delegate to supply an initial view controller to be used as the details view controller. This is for when the split view is first shown - to prevent the details view section to be left blank. I updated the sample app to use this feature.

I also tested in split view mode on the iOS 9 iPad simulator and it works like a charm! I resize the app, flip orientations and the settings updates itself as expected, changing from collapsed to split view, really nice!

Let me know if there are any issues left.

[alfonzzz]

Hey there, wondering if you plan on merging this in the end?

[futuretap]

Absolutely, I'm just terribly busy right now.

Am 21.09.2015 um 10:43 schrieb alfonzzz notifications@github.com:

Hey there, wondering if you plan on merging this in the end?

—
Reply to this email directly or view it on GitHub #311 (comment).

[alfonzzz]

Ok OK, awesome, no rush. Just hadn't heard back for a few days.

[alfonzzz]

Ping?

[futuretap]

Hi, just looked into your code. One thing came to my mind: Is there a compelling reason why we shouldn't remove -initialDetailViewControllerForSettingsViewController: and just keep -initialSelectedIndexPathForSettingsViewController:? Would be much cleaner imo.

[futuretap]

Thinking twice, I think it would make even more sense to convert it to a simple property selectedIndexPath whose setter simply selects the given index path. That would even allow to make selections later on, e.g. based on some other settings the user changed. The property would also be set internally by -tableView:didSelectRowAtIndexPath: so it always reflects the current selection which might be convenient to have as well.

Making it a property instead of a delegate callback would also emphasize that it should only be set on the root settings VC.

What do you think?

[alfonzzz]

I'll take a look and get back to you. I'll try and remember if there was a specific reason I did it differently or maybe it just evolved that way.

On 26 בנוב׳ 2015, at 12:27, Ortwin Gentz, FutureTap notifications@github.com wrote:

Thinking twice, I think it would make even more sense to convert it to a simple property selectedIndexPath whose setter simply selects the given index path. That would even allow to make selections later on, e.g. based on some other settings the user changed. The property would also be set internally by -tableView:didSelectRowAtIndexPath: so it always reflects the current selection which might be convenient to have as well.

Making it a property instead of a delegate callback would also emphasize that it should only be set on the root settings VC.

What do you think?

—
Reply to this email directly or view it on GitHub.

[futuretap]

I noticed another problem with your branch. This:

- (void)settingDidChange:(NSNotification*)notification {
    [_masterSettingsViewController.tableView reloadData];
}

causes the dynamic cell hiding animation to break / look ugly on iPhone.

[alfonzzz]

Ok there are actually a few problems that I've noticed so I'm going to rethink this. I'm closing the pull request for now.
Thanks for the feedback!