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

Logical mistake #23

Open
k-o-d-e-n opened this issue Aug 10, 2017 · 5 comments
Open

Logical mistake #23

k-o-d-e-n opened this issue Aug 10, 2017 · 5 comments

Comments

@k-o-d-e-n
Copy link

I think, you have logical mistake in will/didFinishLaunching methods.

Now application will be launched if one service has true result. Though execution should be ended if one service has false result.

Probably you need create optional/required flag for service, to have control app launching.

@rdadkins
Copy link

rdadkins commented Sep 8, 2017

This is true. It should follow at least a pattern as below at the bare minimum.

result && service.application...

This can be seen in

  • willFinishLaunchingWithOptions
  • didFinishLaunchingWithOptions
  • handleOpen url
  • public url, sourceApplication
  • public url, options
  • shouldAllowExtensionPointIdentifier
  • shouldSaveApplicationState
  • shouldRestoreApplicationState
  • willContinueUserActivityWithType
  • continue userActivity

Some of these can vary well just pass with the current implementation (e.g. shouldSaveApplicationState) but others should rely on previous results (e.g. will/did finish)

@basememara
Copy link

I agree, maybe something like this?

open func application(_ application: UIApplication, didFinishLaunchingWithOptions launchOptions: [UIApplicationLaunchOptionsKey: Any]?) -> Bool {
    return __services.reduce(true) {
        $0 && $1.application?(application, didFinishLaunchingWithOptions: launchOptions) ?? true
    }
}

@vinceis1337
Copy link

vinceis1337 commented Feb 27, 2018

There is an additional issue with the way this is written.

If there are no services implementing a function, i.e. willFinishLaunchingWithOptions

    open func application(_ application: UIApplication, willFinishLaunchingWithOptions launchOptions: [UIApplicationLaunchOptionsKey : Any]? = nil) -> Bool {
        var result = false
        for service in __services {
            if service.application?(application, willFinishLaunchingWithOptions: launchOptions) ?? false {
                result = true
            }
        }
        return result
    }

Please note that the result returned will be false, this then prevents continueUserActivity from ever occurring.

"This method is not called if either application(:willFinishLaunchingWithOptions:) or application(:didFinishLaunchingWithOptions:) returns false."

Source:
https://developer.apple.com/documentation/uikit/uiapplicationdelegate/1623072-application

@vinceis1337
Copy link

vinceis1337 commented Feb 28, 2018

I agree, maybe something like this?

open func application(_ application: UIApplication, didFinishLaunchingWithOptions launchOptions: [UIApplicationLaunchOptionsKey: Any]?) -> Bool {
    return __services.reduce(true) {
        $0 && $1.application?(application, didFinishLaunchingWithOptions: launchOptions) ?? true
    }
}

@basememara This assumes incorrectly that the initialResult should always be true, which is not the case:
application(:handleOpen:)
application(
:open:sourceApplication:annotation:)
application(:open:options:)
application(
:willContinueUserActivityWithType:)
application(_:continue:restorationHandler:)

All of these expect initialResult of false

As such, a refactor to something like this is in order:

func iterate(expecting typicalResult: Bool, functionToPerform: (ApplicationService) -> Bool?) -> Bool {
    return __services.map {
        return functionToPerform($0) ?? typicalResult
    }.contains(!typicalResult) ? !typicalResult : typicalResult
}

You may be wondering why I didn't just use .reduce(typicalResult) { $0 && functionToPerform($1) ?? typicalResult }. Reduce appears to work for an initial value of true but some of these functions should have expected values of false, in which case an initial value of false will be preoptimized out by the compiler to not run the functions at all.

Edit: Additionally, we need to favor the !typicalResult so && will not work (since if typicalResult == false and we get true we need to return true, not false && true)

Edit 2: This should also be refactored to

func iterate(expecting typicalResult: Bool, functionToPerform: (ApplicationService) -> Bool?) -> Bool {
    return __services.compactMap {
        return functionToPerform($0)
    }.contains(!typicalResult) ? !typicalResult : typicalResult
}

After Swift 4.1 is supported.

Edit 3: This iterate function does not support executing completionHandlers of agnostic return types so I am currently trying to figure out a way to do that.

@basememara
Copy link

Thx for the feedback. IMHO, I don't think some of the AppDelegate events should be designed to split up across several service instances. For example, why would we need to spread deep linking logic across several services? Really one DeepLinkApplicationService should suffice to handle all routes.

Instead of worrying about deriving the boolean results for these complex cases, which reduce wouldn't work like it does in didFinishLaunchingWithOptions, willFinishLaunchingWithOptions, etc events, I have only one dedicated service for those events and removed it from the ApplicationServicesManager class (notice performActionFor and userActivity events):

@UIApplicationMain
class AppDelegate: PluggableApplicationDelegate {

    private(set) open override lazy var services: [ApplicationService] = {
        [
            CoreApplicationService(),
            BootApplicationService(with: window),
            LoggerApplicationService(),
            DataApplicationService(),
            AnalyticsApplicationService(),
            ShortcutApplicationService(),
            NotificationApplicationService(),
            ThemeApplicationService()
        ]
    }()
}

extension AppDelegate {
    
    func application(_ application: UIApplication, performActionFor shortcutItem: UIApplicationShortcutItem, completionHandler: @escaping (Bool) -> Void) {
        ShortcutApplicationService()
            .application(application, performActionFor: shortcutItem, completionHandler: completionHandler)
    }
    
    func application(_ application: UIApplication, continue userActivity: NSUserActivity, restorationHandler: @escaping ([Any]?) -> Void) -> Bool {
        return DeepLinkApplicationService()
            .application(application, continue: userActivity, restorationHandler: restorationHandler)
    }
}

My PluggableApplicationDelegate class doesn't implement everything under the sun since that gets extremely messy and a nightmare to maintain, only commonly used ones are implemented (as well as ignoring deprecated events as of iOS 10):

public protocol ApplicationService {
    func application(_ application: UIApplication, willFinishLaunchingWithOptions launchOptions: [UIApplicationLaunchOptionsKey: Any]?) -> Bool
    func application(_ application: UIApplication, didFinishLaunchingWithOptions launchOptions: [UIApplicationLaunchOptionsKey: Any]?) -> Bool
    
    func applicationWillEnterForeground(_ application: UIApplication)
    func applicationDidEnterBackground(_ application: UIApplication)
    func applicationDidBecomeActive(_ application: UIApplication)
    func applicationWillResignActive(_ application: UIApplication)
    
    func applicationProtectedDataWillBecomeUnavailable(_ application: UIApplication)
    func applicationProtectedDataDidBecomeAvailable(_ application: UIApplication)
    
    func applicationWillTerminate(_ application: UIApplication)
    func applicationDidReceiveMemoryWarning(_ application: UIApplication)
    
    func application(_ application: UIApplication, didRegisterForRemoteNotificationsWithDeviceToken deviceToken: Data)
    func application(_ application: UIApplication, didFailToRegisterForRemoteNotificationsWithError error: Error)
}

// MARK: - Optionals

public extension ApplicationService {
    func application(_ application: UIApplication, willFinishLaunchingWithOptions launchOptions: [UIApplicationLaunchOptionsKey: Any]?) -> Bool { return true }
    func application(_ application: UIApplication, didFinishLaunchingWithOptions launchOptions: [UIApplicationLaunchOptionsKey: Any]?) -> Bool { return true }
    
    func applicationWillEnterForeground(_ application: UIApplication) {}
    func applicationDidEnterBackground(_ application: UIApplication) {}
    func applicationDidBecomeActive(_ application: UIApplication) {}
    func applicationWillResignActive(_ application: UIApplication) {}
    
    func applicationProtectedDataWillBecomeUnavailable(_ application: UIApplication) {}
    func applicationProtectedDataDidBecomeAvailable(_ application: UIApplication) {}
    
    func applicationWillTerminate(_ application: UIApplication) {}
    func applicationDidReceiveMemoryWarning(_ application: UIApplication) {}
    
    func application(_ application: UIApplication, didRegisterForRemoteNotificationsWithDeviceToken deviceToken: Data) {}
    func application(_ application: UIApplication, didFailToRegisterForRemoteNotificationsWithError error: Error) {}
}

open class PluggableApplicationDelegate: UIResponder, UIApplicationDelegate {
    
    public var window: UIWindow?
    
    private(set) open lazy var services: [ApplicationService] = {
        [ /* Populated from sub-class */ ]
    }()
}

public extension PluggableApplicationDelegate {
    
    func application(_ application: UIApplication, willFinishLaunchingWithOptions launchOptions: [UIApplicationLaunchOptionsKey: Any]? = nil) -> Bool {
        return services.reduce(true) {
            $0 && $1.application(application, willFinishLaunchingWithOptions: launchOptions)
        }
    }
    
    func application(_ application: UIApplication, didFinishLaunchingWithOptions launchOptions: [UIApplicationLaunchOptionsKey: Any]?) -> Bool {
        return services.reduce(true) {
            $0 && $1.application(application, didFinishLaunchingWithOptions: launchOptions)
        }
    }
}

public extension PluggableApplicationDelegate {
    
    func applicationWillEnterForeground(_ application: UIApplication) {
        services.forEach { $0.applicationWillEnterForeground(application) }
    }
    
    func applicationDidEnterBackground(_ application: UIApplication) {
        services.forEach { $0.applicationDidEnterBackground(application) }
    }
    
    func applicationDidBecomeActive(_ application: UIApplication) {
        services.forEach { $0.applicationDidBecomeActive(application) }
    }
    
    func applicationWillResignActive(_ application: UIApplication) {
        services.forEach { $0.applicationWillResignActive(application) }
    }
}

public extension PluggableApplicationDelegate {
    
    func applicationProtectedDataWillBecomeUnavailable(_ application: UIApplication) {
        services.forEach { $0.applicationProtectedDataWillBecomeUnavailable(application) }
    }
    
    func applicationProtectedDataDidBecomeAvailable(_ application: UIApplication) {
        services.forEach { $0.applicationProtectedDataDidBecomeAvailable(application) }
    }
}

public extension PluggableApplicationDelegate {
    
    func applicationWillTerminate(_ application: UIApplication) {
        services.forEach { $0.applicationWillTerminate(application) }
    }
    
    func applicationDidReceiveMemoryWarning(_ application: UIApplication) {
        services.forEach { $0.applicationDidReceiveMemoryWarning(application) }
    }
}

public extension PluggableApplicationDelegate {
    
    func application(_ application: UIApplication, didRegisterForRemoteNotificationsWithDeviceToken deviceToken: Data) {
        services.forEach { $0.application(application, didRegisterForRemoteNotificationsWithDeviceToken: deviceToken) }
    }
    
    func application(_ application: UIApplication, didFailToRegisterForRemoteNotificationsWithError error: Error) {
        services.forEach { $0.application(application, didFailToRegisterForRemoteNotificationsWithError: error) }
    }
}

The core concept still works beautifully and keeps the AppDelegate clean and pleasant to work with.

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

No branches or pull requests

4 participants