vtourraine / AcknowList

Acknowledgements screen displaying a list of licenses, for example from CocoaPods and Swift Package Manager dependencies.
MIT License
798 stars 60 forks source link

UIApplication avoidance #46

Open KieranHarper opened 6 years ago

KieranHarper commented 6 years ago

Added an optional closure parameter that can be invoked when the user wants to open the cocoapods URL, rather than using UIApplication.

This is needed in situations where the view controller lives in a framework target that doesn't allow UIApplication to be used.

vtourraine commented 6 years ago

Hello Kieran, and thanks for the pull request.

I appreciate that not relying on UIApplication in those situations would be valuable, but that shouldn’t come at the expense of the most-used configuration. Unfortunately, I don’t think there’s a good solution for that, because you can’t really test at build time if the target supports UIApplication. Maybe add a preprocessor check (#if etc)?

If somebody has a better solution, please let us know.

ApolloZhu commented 6 years ago

In Objective-C, you can use NS_EXTENSION_UNAVAILABLE macro, but in Swift, there isn't really an replacement for that.

So what I did is this:

https://github.com/LiulietLee/LLDialog/blob/master/Source/LLDialog.swift#L69-L73

Since https://github.com/apple/swift/pull/12410 got merged, you can just use @available to check if it's app extension.

I don't think there's enough sample to prove this implementation is App Store compatible, but I discussed it with one of the Apple engineers during WWDC17. He said it "should" be fine.

vtourraine commented 6 years ago

That’s super interesting. Thank you very much for pointing that out, @ApolloZhu!

I guess we should update this pull request to replace the closure with the appropriate @available checks, then I‘ll be happy to merge it in.

vtourraine commented 5 years ago

So I’ve tried adding the @available checks on openCocoaPodsWebsite(), as pointed out by @ApolloZhu. It works, but unfortunately that only moves the problem. Now the compilation error is on the UITapGestureRecognizer instantiation, because the selector is not available for Extensions:

'openCocoaPodsWebsite' is unavailable: This method is NS_EXTENSION_UNAVAILABLE.

It seems like this should be easily solvable by testing the availability when creating the gesture recognizer, but as fas as I understand, this type of condition does not exist. Something like if #available(iOSApplicationExtension 1.0, *) {...} wouldn’t help, because the * is mandatory, so we can’t limit just for Extensions.

To reiterate: I want to keep the current behavior in a non-Extension context, and I don’t want to require more code from the app using this library.

Am I missing something, or do we need to wait for more flexible compiler instructions?

ifrins commented 3 years ago

FYI in XCode 13b3 this started causing issues, as the SwiftPM packages are being built with -application-extension flag, so now any app that imports AcknowList package fails to build.

Looking at the discussion in Swift Forums, right now the only solution seems to be annotating either the class or some part of it with @available(iOSApplicationExtension, unavailable).

I see two options here:

@vtourraine Are you open to receiving any PR for this or you'd rather wait and see if there's any fixes in this area on further betas?

vtourraine commented 3 years ago

@ifrins Hello Francesc, and thanks a lot for raising this issue!

It looks like we should do something about it, but maybe there’s an easier solution. I think we can add the @available() statement to the whole AcknowListViewController class, which would silence the compilation error. I’ve never used this library from an app Extension, but maybe I’m missing other use-cases?

@available(iOS 9.0.0, tvOS 9.0.0, *)
@available(iOSApplicationExtension, unavailable)
open class AcknowListViewController: UITableViewController {
ifrins commented 3 years ago

What you suggested works (I created a fork doing exactly that to unblock myself 😊); but it would break if anyone is using this VC in an extension (not really sure if that's a real use case though).

vtourraine commented 3 years ago

Right, so we’re on the same page. I think you should create a pull request for this. We’ll merge it to main, then see if that turns out problematic for some apps. I’d really prefer to avoid adding alternative implementations/methods just to dance around this issue.

vtourraine commented 3 years ago

@ifrins Well... another pull request was already submitted with the @available annotation (#85), so I’ve merged it. I’ve also mentioned you in the changelog as a co-author, I hope that’s OK.

Everyone can now test the Xcode 13 support with the xcode-13 branch. And I’ve opened a dedicated pull request (#86) to discuss any further changes.