iOS 13 modal sheets (issue #41) #43

nataliq · 2019-07-17T16:19:07Z

What's new:
On iOS 13 we have new default modal presentation called Sheet. The modal Sheets can be dismissed by swiping them down, and in doing so, Presentation framework wasn't disposing the bag of its presentation.
This PR makes Presentation's view controller to conform to UIAdaptivePresentationDelegate that will handle actions for when the view attempt's, will and did dismiss and disposing the bag appropriately when the view dismisses.

In addition, it adds functionality for the developer to pass a block of code when the view attempts to dismiss through a signal in CustomAdaptivePresentationDelegate like this

vc.customAdaptivePresentationDelegate.didAttemptToDismissSignal.onValue { ... }

The didAttemptToDismissSignal will fire only if the view controller that's presented has .isModalInPresentation = true and then the developer can execute a block with what they want to happen when the user attempt to dismiss (optional)

This PR also includes a temp solution for #40

More info: #41

Presentation/PresentationOptions.swift

Presentation/PresentationStyle.swift

niil-ohlin

Nice. When I was looking into this issue previously I thought about doing the same solution but did not get as far! Nice work!

iZNunev · 2019-07-19T12:56:14Z

I've added a new file to presentation and it the test-iOS on Circle-CI is failing due to

Build input file cannot be found: '/System/Volumes/Data/Users/vasil-nunev/Presentation/Presentation/CustomAdaptivePresentationDelegate.swift' (in target 'Presentation')

Anyone knows how to fix this?

moglistree · 2019-07-22T09:54:55Z

Presentation/CustomAdaptivePresentationDelegate.swift

+    }
+}
+
+private var customAdaptivePresentationDelegateKey = false


Can this property be modified?
If not can it be made a let?

moglistree

Seems nice.
Is it possible to squash some of the progress commits?

Presentation/CustomAdaptivePresentationDelegate.swift

mansbernhardt · 2019-07-23T09:21:24Z

Presentation/CustomAdaptivePresentationDelegate.swift

+        get { return associatedValue(forKey: &customAdaptivePresentationDelegateKey, initial: CustomAdaptivePresentationDelegate()) }
+        set { setAssociatedValue(newValue, forKey: &customAdaptivePresentationDelegateKey) }
+    }
+}


Not sure why we want to expose this like this?

I like this kind of pattern:

extension AVCaptureDataOutputSynchronizer { var dataCollectionSignal: Signal<AVCaptureSynchronizedDataCollection> { class Delegate: NSObject, AVCaptureDataOutputSynchronizerDelegate { let callbacker = Callbacker<AVCaptureSynchronizedDataCollection>() func dataOutputSynchronizer(_ synchronizer: AVCaptureDataOutputSynchronizer, didOutput synchronizedDataCollection: AVCaptureSynchronizedDataCollection) { callbacker.callAll(with: synchronizedDataCollection) } } return Signal { callback in let bag = DisposeBag() let delegate = (self.delegate as? Delegate) ?? Delegate() bag.hold(delegate) bag.hold(self) self.setDelegate(delegate, queue: .outputSynchronizer) bag += delegate.callbacker.addCallback(callback) return bag } } }

So instead of exposing CustomAdaptivePresentationDelegate and customAdaptivePresentationDelegate you just expose didAttemptToDismissSignal on UIViewController.

Presentation/PresentationStyle.swift

mansbernhardt · 2019-07-23T16:41:50Z

Presentation/PresentationStyle.swift

@@ -274,3 +289,82 @@ private class PopoverPresentationControllerDelegate: NSObject, UIPopoverPresenta
 }

 private var isIpad: Bool { return UIDevice.current.userInterfaceIdiom == .pad }
+
+private class AdaptiveProxyPresentationDelegate: NSObject, UIAdaptivePresentationControllerDelegate {
+    private let didDismissCallbacker = Callbacker<()>()


After some more reading of the code and hopefully a better understanding I wonder if it is not cleaner to make AdaptiveProxyPresentationDelegate public with support for all the delegates signals and delegates similar to tableViewDelegate and friends. Then we can check when modally presenting if the delegate is nil or of type AdaptiveProxyPresentationDelegate and use the delegate to handle dismiss, or else not handle dismiss (as someone else is doing that instead) and potentially log a warning.

@mansbernhardt we did some changes based on this feedback. Unfortunately, we can't allow the user's to set directly the delegate to the presentationController of the vc because it causes memory leak problems when trying to access the presentationController before it gets presented on the screen. Instead we keep the custom delegate on the VC and the user needs to set that one instead so that Presentation can set it properly when we are presenting the vc

Not sure I follow? Who is leaking? The one who is setting the delegate needs to hold on to the delegate as well using e.g. bag.hold(delegate)

mansbernhardt · 2019-07-24T16:25:51Z

Presentation/CustomAdaptivePresentationDelegate.swift

+    /// will be able to be swiped down.
+    ///
+    /// By default on an navigation stack only the first view is allowed to be dismissed by swiping down.
+    func presentationControllerShouldDismiss(_ presentationController: UIPresentationController) -> Bool {


Can't this be handled else where by using a Delegate<(), Bool> instead?

And the below including allowSwipeDismissAlways could be moved else-where?

mansbernhardt · 2019-07-24T16:30:07Z

Presentation/PresentationStyle.swift

@@ -274,3 +289,82 @@ private class PopoverPresentationControllerDelegate: NSObject, UIPopoverPresenta
 }

 private var isIpad: Bool { return UIDevice.current.userInterfaceIdiom == .pad }
+
+private class AdaptiveProxyPresentationDelegate: NSObject, UIAdaptivePresentationControllerDelegate {
+    private let didDismissCallbacker = Callbacker<()>()


Not sure I follow? Who is leaking? The one who is setting the delegate needs to hold on to the delegate as well using e.g. bag.hold(delegate)

mansbernhardt · 2019-07-24T16:35:48Z

Presentation/PresentationStyle.swift

+                        }
+
+                        customPresentationDelegate.allowSwipeDismissAlways = options.contains(.allowSwipeDismissAlways)
+                        vc.presentationController?.delegate = customPresentationDelegate


Not sure I understand the memory leak issue. I was thinking something like this instead:

let delegate = (vc.presentationController?.delegate as? CustomAdaptivePresentationDelegate) ?? CustomAdaptivePresentationDelegate() vc.presentationController?.delegate = delegate bag.hold(delegate) bag += delagate.presentationControllerShouldDismiss.set { return Bool.... based on .allowSwipeDismissAlways etc } bag += delagate.didDismissSignal.onValue { completion(.failure(PresentError.dismissed)) }

It's very weird leak, will try to understand more about it now. Basically seeing a memory leak alert (and a check in the memory graph confirms it) when running the UI tests when accessing viewController.presentationController (even when the rest of the logic is commented out). I think the problem is coming from the fact that If you have not yet presented the current view controller, accessing presentationController creates a presentation controller. In our case viewController is not presented but if vc and viewController are different (when embedding in a navigation controller for example) then the one that will be presented and needs a presentation controller is vc and the presentation controller we just created for viewController leaks. Maybe related to http://www.openradar.appspot.com/36256721 . Will test this assumption now.

Yep, that's what's happening. Here is a branch where you can reproduce it: https://github.com/iZettle/Presentation/tree/ios13-modal-sheets-leak. Not sure if there is a better workaround for a leak in UIKit. We can ofc add a comment about it...

mansbernhardt · 2019-07-24T16:39:49Z

Presentation/CustomAdaptivePresentationDelegate.swift

+import UIKit
+import Flow
+
+public final class CustomAdaptivePresentationDelegate: NSObject, UIAdaptivePresentationControllerDelegate {


Do we want this to be available for < iOS 13.0?

Should we move the conformance to UIAdaptivePresentationControllerDelegate to the extension where we implement it?

The protocol has few iOS 8 methods, added them too. Prefer to keep the iOS 13 only signals here as well, they won't signal before iOS 13 but no need of a check on the call side too.

mansbernhardt

Great addition!

mansbernhardt · 2019-07-26T06:02:13Z

Presentation/CustomAdaptivePresentationDelegate.swift

+    public var didDismissSignal: Signal<UIPresentationController> {
+        return Signal(callbacker: didDismissCallbacker)
+    }
+


Should these three signals be exposed for < iOS 13 users? Will they ever signal?

I think that's nicer than adding iOS 13 check everywhere they are used. I specified it in the docs.

mansbernhardt · 2019-07-26T06:03:01Z

Presentation/CustomAdaptivePresentationDelegate.swift

+}
+
+public extension UIViewController {
+    /// A custom delegate  that allows you to listen to a Signals for adaptive presentations.


Grammar and spacing.

mansbernhardt · 2019-07-26T06:09:56Z

Presentation/PresentationStyle.swift

+                    if !(vc is UIAlertController) {
+                        let customPresentationDelegate: CustomAdaptivePresentationDelegate
+                        if let givenDelegate = viewController.customAdaptivePresentationDelegate {
+                            customPresentationDelegate = givenDelegate


Should we rely on someone else holding a reference for us? If not, potential simplification:

let delegate = viewController.customAdaptivePresentationDelegate ?? CustomAdaptivePresentationDelegate()
bag.hold(delegate)

iZNunev force-pushed the ios13-modal-sheets branch from 74fd2b1 to b5a8a4b Compare July 18, 2019 09:21

nataliq commented Jul 18, 2019

View reviewed changes

Presentation/PresentationOptions.swift Outdated Show resolved Hide resolved

nataliq commented Jul 18, 2019

View reviewed changes

Presentation/PresentationStyle.swift Show resolved Hide resolved

nataliq commented Jul 18, 2019

View reviewed changes

Presentation/PresentationStyle.swift Outdated Show resolved Hide resolved

nataliq commented Jul 18, 2019

View reviewed changes

Presentation/PresentationStyle.swift Outdated Show resolved Hide resolved

nataliq commented Jul 18, 2019

View reviewed changes

Presentation/PresentationStyle.swift Outdated Show resolved Hide resolved

niil-ohlin reviewed Jul 18, 2019

View reviewed changes

iZNunev marked this pull request as ready for review July 22, 2019 07:59

iZNunev requested a review from niil-ohlin July 22, 2019 08:00

nataliq changed the title ~~[WIP] iOS 13 modal sheets (issue #41)~~ iOS 13 modal sheets (issue #41) Jul 22, 2019

iZNunev requested review from saidsikira, mansbernhardt, moglistree and CarlEkman July 22, 2019 08:21

moglistree reviewed Jul 22, 2019

View reviewed changes

moglistree previously approved these changes Jul 22, 2019

View reviewed changes

mansbernhardt reviewed Jul 23, 2019

View reviewed changes

iZNunev dismissed moglistree’s stale review via ebdb4b3 July 23, 2019 14:25

mansbernhardt reviewed Jul 23, 2019

View reviewed changes

mansbernhardt reviewed Jul 24, 2019

View reviewed changes

iOS 13 modal sheets with custom presentation delegate

e9ad1e8

nataliq force-pushed the ios13-modal-sheets branch from 220caaf to e9ad1e8 Compare July 25, 2019 21:33

nataliq requested review from iZNunev and mansbernhardt July 25, 2019 21:38

mansbernhardt previously approved these changes Jul 26, 2019

View reviewed changes

nataliq-pp added 2 commits July 26, 2019 11:02

Fix documentation and compiling on Xcode 10

3f37b28

Bump version to 1.7.0

94789c3

nataliq dismissed mansbernhardt’s stale review via 94789c3 July 26, 2019 09:23

iZNunev approved these changes Jul 26, 2019

View reviewed changes

nataliq merged commit c6e906b into master Jul 26, 2019

nataliq deleted the ios13-modal-sheets branch July 26, 2019 09:41

This was referenced Jul 26, 2019

Swiping down a new modal sheet (iOS 13 beta sdk) doesn't cancel the presentation #41

Closed

Delayed presentation of UISplitViewController's content breaks on iPhone with the iOS 13 beta sdk #40

Open

Flaky testNavigationBarVisibilityPreference #36

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

iOS 13 modal sheets (issue #41) #43

iOS 13 modal sheets (issue #41) #43

nataliq commented Jul 17, 2019 •

edited by iZNunev

Loading

niil-ohlin left a comment

iZNunev commented Jul 19, 2019

moglistree Jul 22, 2019

moglistree left a comment

mansbernhardt Jul 23, 2019

mansbernhardt Jul 23, 2019

iZNunev Jul 24, 2019

mansbernhardt Jul 24, 2019

mansbernhardt Jul 24, 2019

nataliq Jul 25, 2019

mansbernhardt Jul 24, 2019

mansbernhardt Jul 24, 2019

nataliq Jul 25, 2019

nataliq Jul 25, 2019

mansbernhardt Jul 24, 2019

nataliq Jul 25, 2019

mansbernhardt left a comment

mansbernhardt Jul 26, 2019

nataliq Jul 26, 2019

mansbernhardt Jul 26, 2019

mansbernhardt Jul 26, 2019

iOS 13 modal sheets (issue #41) #43

iOS 13 modal sheets (issue #41) #43

Conversation

nataliq commented Jul 17, 2019 • edited by iZNunev Loading

niil-ohlin left a comment

Choose a reason for hiding this comment

iZNunev commented Jul 19, 2019

Choose a reason for hiding this comment

moglistree left a 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

mansbernhardt left a 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

nataliq commented Jul 17, 2019 •

edited by iZNunev

Loading