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

Fixed bug where 'haveCoreMotionPermission' incorrectly returns true #60

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

0xTomDaniel
Copy link

At least on iOS 12.2, 'startDeviceMotionUpdates' provides motion data even while Core Motion permissions are denied. 'startDeviceMotionUpdates' provides valid motion data before and after the permissions alert is denied by the user. It also provides valid motion data if the app is terminated and started again without motion data permissions.

Given this behavior, 'coreMotionPermission' should not be updated to 'true' within the 'startDeviceMotionUpdates' callback.

@sobri909
Copy link
Owner

sobri909 commented Jun 6, 2019

Hm. Curious. So this has changed again.

Thanks for the PR! I'll do some testing of my own, to make sure I can reproduce your findings here.

@0xTomDaniel
Copy link
Author

0xTomDaniel commented Jun 6, 2019

Apple did an absolutely TERRIBLE job with Core Motion. I spent all day yesterday trying to get my Motion & Fitness permissions screen to work correctly. There's no provided way to determine if the system alert has ever been prompted to the user below iOS 11. I had to resort to UIApplicationWillResignActive and UIApplicationDidBecomeActive to guess if the alert is being shown to the user. Why couldn't they provide delegate methods like with Core Location!? It's so frustrating!

@sobri909
Copy link
Owner

sobri909 commented Jun 7, 2019

I agree. It's diabolical. And it keeps changing every year, but with no proper documentation (or is given ambiguous and misleading documentation).

I had to resort to UIApplicationWillResignActive and UIApplicationDidBecomeActive to guess if the alert is being shown to the user.

I've done exactly the same thing in Arc App's onboarding views. It's a dirty and unreliable hack, but we're really not given any better options.

@0xTomDaniel
Copy link
Author

@sobri909 Any word on this? There have been many commits since creating this.

@sobri909
Copy link
Owner

Sorry for the very, very late reply to this!

I haven't run into any problems with the current code, so haven't treated it as high priority. But I'm looking into it now, because something has changed in iOS 13.6 beta.

'startDeviceMotionUpdates' provides motion data even while Core Motion permissions are denied. 'startDeviceMotionUpdates' provides valid motion data before and after the permissions alert is denied by the user. It also provides valid motion data if the app is terminated and started again without motion data permissions.

This makes it sound like it's technically okay to return true, even though it's not true, because Core Motion data can be received. Although it sounds like a very odd situation - CM data being successfully received even though permission hasn't been granted?!

Anyway, I'm getting different results now on iOS 13.6, so I can't keep ignoring this 😂 I'll do some more testing now, and figure out what's going on with 13.6. Your PR looks correct to me, but the above mentioned situation where false is kind of true makes it ambiguous. So I'll at least figure out whether there's less ambiguity in 13.6, or ... if not ... yeah, anyway, I'll figure something out and get back to you shortly!

Sorry again for the very late reply 🙏🏼

@0xTomDaniel
Copy link
Author

No worries and thanks for replying. I hope all is well!

So the behavior is still changing with iOS updates!? 😵

I believe that a principle for a good API is to be as unambiguous as possible and maintain veracity of expectation. If the purpose of 'haveCoreMotionPermission' is to answer if motion data will be provided, then it should probably be named something else like 'canReceiveCoreMotionData'. Maybe even create one property for permissions and one for the ability to receive core motion data.

@sobri909
Copy link
Owner

sobri909 commented Jun 14, 2020

If the purpose of 'haveCoreMotionPermission' is to answer if motion data will be provided, then it should probably be named something else like 'canReceiveCoreMotionData'. Maybe even create one property for permissions and one for the ability to receive core motion data.

Yeah I agree with this.

It just seems a very weird situation to be in, that data will be provided without permission granted.

There's no API changes in iOS 13.6, so the situation in terms of API hasn't changed. It's still the same situation of "try to start recording, then make assumptions based on whether you get data or not".

I had to jump over to something else before I could test the proper combinations, so I'm not sure where it's really at yet. But in principle I agree with what you said above - that it looks like it's worth either renaming the property or having two properties. Although the property of "have we really got permission or not?" seems potentially still impossible to do? Needs more testing. Sigh.

@0xTomDaniel
Copy link
Author

Although the property of "have we really got permission or not?" seems potentially still impossible to do?

It seems that you already cover if the permissions have been granted or not explicitly, (line 428)

if #available(iOS 11.0, *) {
    coreMotionPermission = CMMotionActivityManager.authorizationStatus() == .authorized
} else {
    coreMotionPermission = CMSensorRecorder.isAuthorizedForRecording()
}

but it's mixed with the implicit rule that my pull request removes (line 705)
self.coreMotionPermission = true.

Given that iOS may provide motion data without user granted permissions, I guess permissions themselves may only matter conditionally based on the ability to receive the motion data without permissions. In the end, all that anyone really cares about is being able to receive the motion data itself. Why bother the user and ask for permissions if you don't need to?

I do like the idea you've implemented of knowing whether or not motion data can be received, but rather than using that to implicitly infer permissions, it can explicitly stand on it's own as 'canReceiveCoreMotionData' or whatever. Then the actual permissions themselves can stand on their own as a secondary tool to the developer.

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

Successfully merging this pull request may close these issues.

2 participants