Skip to content
This repository has been archived by the owner on Feb 27, 2019. It is now read-only.

Add CloudKit discoverability permissions. #58

Closed
evermeer opened this issue Aug 7, 2015 · 26 comments
Closed

Add CloudKit discoverability permissions. #58

evermeer opened this issue Aug 7, 2015 · 26 comments
Milestone

Comments

@evermeer
Copy link
Contributor

evermeer commented Aug 7, 2015

For this you can use the following 2 Async calls:

let container = CKContainer.defaultContainer()
container.statusForApplicationPermission(CKApplicationPermissions.PermissionUserDiscoverability,
    completionHandler: { applicationPermissionStatus, error in
        // handle applicationPermissionStatus for statuses like
        // CKApplicationPermissionStatus.Granted, .Denied, .CouldNotComplete, .InitialState
})


let container = CKContainer.defaultContainer()
container.accountStatusWithCompletionHandler({status, error in
    switch status {
        case .Available, .Restricted:
        container.requestApplicationPermission(CKApplicationPermissions.PermissionUserDiscoverability,
            completionHandler: { applicationPermissionStatus, error in
            // handle applicationPermissionStatus for statuses like CKApplicationPermissionStatus.Granted, .Denied, .CouldNotComplete, .InitialState
        })
        case .CouldNotDetermine, .NoAccount:
            // Ask user to login to iCloud
    }
})

Edit: Formatted code.

@nickoneill
Copy link
Owner

Thanks @evermeer, I'll look into adding this in.

@nickoneill nickoneill mentioned this issue Aug 21, 2015
6 tasks
@bre7
Copy link
Collaborator

bre7 commented Aug 21, 2015

Status should be an async call then :P

@bre7 bre7 added this to the 1.0 milestone Aug 21, 2015
@nickoneill
Copy link
Owner

Yeah, that was about as far as I got into the implementation too :)

@bre7
Copy link
Collaborator

bre7 commented Aug 21, 2015

What should we do when CouldNotComplete is returned ? Docs say that you should investigate the NSError returned so...Use throws ? Print the error ?

Quick-dirty version done...Can someone test it ? Don't use the example app, but rather a new one since it requires entitlements on your App ID

Should we add an observer for CKAccountChangedNotification ? Or is it the job of the dev to add it and use PScope's API to request if necessary ?

Posted when the status of the signed in iCloud account may have changed.

The notification is sent by an instance of the CKContainer class. If there are no instances of the class, notifications are not sent. The notification may be sent on any thread or work queue. Use the accountStatusWithCompletionHandler: method to obtain the current account status.

@evermeer
Copy link
Contributor Author

Hi, I tried adding the Quick-dirty version to a project of mine using:

pod 'PermissionScope', :git => 'https://github.com/bre7/PermissionScope.git', :branch => 'wip/cloudkit'

I just did a pull request to make it work in beta 6

@evermeer
Copy link
Contributor Author

I'm currently adding it to the demo at https://github.com/evermeer/EVCloudKitDao/tree/Swift2
I will push it to GitHub when it works ok with beta 6.

@bre7
Copy link
Collaborator

bre7 commented Aug 25, 2015

Awesome, thanks for that @evermeer

@evermeer
Copy link
Contributor Author

I have been thinking about the CKAccountChangedNotification. I think it should be implemented by the developer. This because he should probably also have to reset the interface to an initial state. (exit user specific screens) and there is a chance that it should be done before PermissionScope screen is shown. So the current callback functions might be too late.

Or... Maybe PermissionScope could have an extra optional callback function that will be executed when it's detected that permissions could have been changed or should be reevaluated. that function could have a return value Bool that will control if PermissionScope should show again or not.

@evermeer
Copy link
Contributor Author

I found some issues with this version. I will do some more tests.
My test case:

    func showPermissionScope() {
        pscope.headerLabel.text = "Setting permissions"
        pscope.bodyLabel.text = "For optimal usage we need some permissions."

        pscope.addPermission(PermissionConfig(type: .Notifications, demands: .Required, message: "For if you want to receive notifications that people send directly to you"))
        pscope.addPermission(PermissionConfig(type: .CloudKit, demands: .Required, message: "So that other users can find you"))        
        pscope.show({ (finished, results) -> Void in
            print("TODO: results is a PermissionsResult for each config")
            if finished {
                self.getUser()
            }
        }, cancelled: { (results) -> Void in
            print("WARNING: PermissionScope was cancelled")
        })
    }

When running this on a simulator that was just reset:

  1. Click on the notifications button will give a prompt, when clicking on OK the button changes to ‘Denied notifications’. But I pressed OK so it should say that notifications are allowed.
  2. When pressing on CloudKit you get the message ‘please enable access to CloudKit in Setting’ I think it would be nicer/clearer if the text was something like 'Please first login into ICloud in the Settings app’
  3. When going to the settings app and then returning to the app, PermissionScope will trigger the following Assert (L250):
assert(configuredPermissions.filter { $0.type == config.type }.isEmpty, "Permission for \(config.type) already set")

The log will show you this:

assertion failed: Permission for Notifications already set: file PermissionScope.swift, line 250

@bre7
Copy link
Collaborator

bre7 commented Aug 25, 2015

Click on the notifications button will give a prompt, when clicking on OK the button changes to ‘Denied notifications’. But I pressed OK so it should say that notifications are allowed.

Yeah, it's a bug being tracked in #69.

When pressing on CloudKit you get the message ‘please enable access to CloudKit in Setting’ I think it would be nicer/clearer if the text was something like 'Please first login into ICloud in the Settings app’

It's a system alert so there's nothing we can do other than present a new alert first ? Which isn't very nice.

When going to the settings app and then returning to the app, PermissionScope will trigger the following Assert (L250)...

Where is the function showPermissionScope() being called from ? Because it sounds like you are trying to add a .Notifications permission again, maybe you added the call in viewDidAppear ?

@evermeer
Copy link
Contributor Author

Ah, ok, for now I will wait for #69

No, it's not a system setting, it's from this line:

let alert = UIAlertController(title: "\(permission) is currently disabled.",
                    message: "Please enable access to \(permission) in Settings",
                    preferredStyle: .Alert)

Maybe that line is a little to generic?

Another ah, I see, my bad.. The moment NSUbiquityIdentityDidChangeNotification is triggered I tried executing PermissionScope again without resetting it.

I do have another issue. After restarting the app it will immediately execute the callback with finished is true but only 1 element in the results while both are required. The element is for Notifications Authorized. I expect also an entry for CloudKit Authorized. (or a finished = false)

@bre7
Copy link
Collaborator

bre7 commented Aug 26, 2015

No, it's not a system setting, it's from this line:...

Oops, sorry about that. The message might need some rephrasing, @nickoneill will take a look later though imo it's fine. The only fix I can think of is using prettyDescription instead of the name itself.

I do have another issue. After restarting the app it will immediately execute the callback with finished is true but only 1 element in the results while both are required. The element is for Notifications Authorized. I expect also an entry for CloudKit Authorized. (or a finished = false)

Not sure why it's happening...I'll have a look later.

Update: @evermeer I think I've found the problem, it was related to the computed variables (someone deleted the .Required) which is fixed in the healthkit branch. I'll merge it to cloudkit later.

@evermeer
Copy link
Contributor Author

Great!

I now have it working in my demo app and pushed it to GitHub. I did create a workaround for reactivating PermissionScope in the event that the NSUbiquityIdentityDidChangeNotification was called the moment the PermissionScope screen was still open. The workaround is calling the hide and then after 1 second the show. It would be nice if there was a method in PermissionScope to reinitialize everything. Do you want me to create a new issue for this?

See the function reactToiCloudloginChanges in:
https://github.com/evermeer/EVCloudKitDao/blob/Swift2/AppMessage/AppMessage/Controlers/RootViewController.swift

@bre7
Copy link
Collaborator

bre7 commented Aug 26, 2015

The workaround is calling the hide and then after 1 second the show. It would be nice if there was a method in PermissionScope to reinitialize everything. Do you want me to create a new issue for this?

Create a separate issue for it. That's the disadvantage of not having PScope as the observer itself.

Everything else worked as expected using my branch ?

@evermeer
Copy link
Contributor Author

Besides the #69 and #73 issues everything worked as expected.

@bre7 bre7 mentioned this issue Aug 26, 2015
4 tasks
@bre7 bre7 closed this as completed Aug 31, 2015
@bre7 bre7 removed the help wanted label Aug 31, 2015
@nickoneill
Copy link
Owner

Maybe I don't have this set up correctly @evermeer but this looks broken in the example project for me.

I did the basics and let Xcode set up entitlements for me and requested the cloudkit permission, but the request screen is all over the place (see the screenshot below).

Additionally, the results callback is called with the other non-cloudkit permissions immediately, but never called with CloudKit. So if you had requested two other permissions that were allowed but CloudKit had not, the permissions dialog would never show and the API feedback would be incorrect.

I still don't have a real clear idea for how we should handle these sorts of very asynchronous permissions requests. It seems to me that we either get into a situation where the dialog is displayed and jumps away when the cloudkit permission finally returns Authorized or we wait some longer period of time before showing the dialog which defeats the purpose if you require certain permissions in part of your app.

@nickoneill nickoneill reopened this Aug 31, 2015
@bre7
Copy link
Collaborator

bre7 commented Aug 31, 2015

Mmmmh, it's probably a good idea to move CK to a new branch for the time being then.

@nickoneill could you close it and open a new issue ? So we can tag it properly as a bug

@nickoneill
Copy link
Owner

Yeah, I'm feeling a bit of pressure to get the Swift 2 stuff locked down for iOS 9. I'll move it to a cloudkit branch and pull it from this branch so we can keep things relatively stable.

@evermeer
Copy link
Contributor Author

evermeer commented Sep 1, 2015

@nickoneill I have seen that screen 'glitch' before, but it's not consistent. It's always the last texts that is placed at the top of the screen. Maybe create a new issue for that? Or do you think it's Async/CloudKit related?

How I think you should handle async permissions:

  • When the .show is called loop through the permissions to fetch the status. (no matter if it's async or not)
  • Add a PermissionStatus called Pending which will be the default state.
  • When you have the status, then always call the authChange handler where the status of not completed permission checks will be Pending.
  • depending on that status decide if the permissions screen should show. (Pending will be a no show)
  • since the authChange is always called the developer using PermissionScope can decide what to do when the status is still Pending.

p.s. The current swift2 branch has a compile error. CloudKit was removed from a switch, but it's still in the Enum which makes the switch incomplete.

@bre7
Copy link
Collaborator

bre7 commented Sep 1, 2015

p.s. The current swift2 branch has a compile error. CloudKit was removed from a switch, but it's still in the Enum which makes the switch incomplete.

Fixed.

@nickoneill
Copy link
Owner

I remember fixing a similar visual glitch for another permission in the past, I'll have to dig into some commits to remember what the issue was.

I definitely like the .Pending status idea, @evermeer, particularly the repeated callbacks to authchange. I do think we should cater to the folks that just want to call show() and have us do The Right Thing as well. A property such as showsWithPendingItems that defaults to true would let users flip that switch without doing too much extra work.

(Why should this default to true? I would rather people see the dialog and look at the docs to change behavior than try to use pscope and be unable because "nothing shows up", i.e. they have pending items and we defaulted to not showing them)

For discussion:
• Should we hide immediately if a .Pending item turns out to be .Authorized and thus all items are authorized?
• Should we cache this result for future performance? We already do this with notifications.

Thanks @bre7 for that fix, I missed that last part when I ripped cloudkit out.

@bre7
Copy link
Collaborator

bre7 commented Sep 2, 2015

I remember fixing a similar visual glitch for another permission in the past, I'll have to dig into some commits to remember what the issue was.

Old issue: #14. Fix 8a800f3

@owaves
Copy link

owaves commented Nov 9, 2015

I'm kind of new to open source, so not exactly sure how to document this, but there is a problem with the 'getResultsForConfig' function and cloudKit.
You should use dispatch_group_enter / dispatch_group_leave instead of dispatch_group_async.

This is because the request for the CloudKit permission status has another asynchronous call inside of it (unlike the other permissions). Therefore the dispatch_group thinks the block is completed, when it actually has not.

For example, I am requesting Notifications and CloudKit, but as soon as I answer Notifications, it returns.

I think this is what it should be

func getResultsForConfig(completionBlock: resultsForConfigClosure) {
        var results: [PermissionResult] = []
        let group: dispatch_group_t = dispatch_group_create()

        for config in configuredPermissions {
         dispatch_group_enter(group)

            dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0)) {
                    self.statusForPermission(config.type, completion: { status in
                        let result = PermissionResult(type: config.type,
                            status: status)
                        results.append(result)
                     dispatch_group_leave(group)
                    })
            }
        }

        // FIXME: Return after async calls were executed
        dispatch_group_notify(group,
            dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0)) {
               print("returned results \(results)")
                completionBlock(results)
        }
    }

@nickoneill
Copy link
Owner

Hi @owaves, glad to have you contributing.

The cloudkit stuff is still work in progress and probably a bit out of date compared to master at the moment. I think we can get rid of the dispatch_group stuff when we move to a pending status for cloudkit, but I haven't actually worked on it yet.

I'm happy to accept a PR for changes to how the cloudkit branch works at the moment if you'd like to submit one, or you can take a stab at the proposed pending status. It's probably a couple weeks out before I'll have time to work on it myself.

@bre7
Copy link
Collaborator

bre7 commented Oct 11, 2016

Reopen if there's enough user interest

@bre7 bre7 closed this as completed Oct 11, 2016
@evermeer
Copy link
Contributor Author

I already had given up hope that this would ever be implemented.
Currently I use a custom solution for requesting permissions.
It would still be nice if PermissionScope would implement this.
Without it it's not a complete solution for all possible permission questions.

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

No branches or pull requests

4 participants