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

Record location with CLLocationManager if permission given by user #107

Merged
merged 14 commits into from
Mar 16, 2016

Conversation

fnakstad
Copy link
Contributor

Here's a proposed fix for #102 which I posted the other day. Some notes about this solution:

  • The NSLocationWhenInUseUsageDescription key in Info.plist must be set to whatever message should be displayed when requesting Location Services access from the user. If this key is not set, CLLocationManager will not attempt to fetch new locations.
  • This solution doesn't write the GPS tag to the actual image file, but rather is stored via the PHPhotoLibrary API (in a separate DB I'm guessing). This means that accessing the image via the asset library always gives you access to the location, but if you only look at the image data itself it's not available. I had a solution actually writing the GPS tag too, but it was quite ugly since converting to UIImage drops any associated metadata. This led to a lot of marshalling between UIImage CGImage and NSData. Additionally, a separate fix was needed for iOS 8 which doesn't support creating image assets directly from data via PHAssetCreationRequest. This solution, however, is very straightforward and should work for both iOS 8 and 9.
  • startUpdatingLocation() and stopUpdatingLocation() were used rather than requestLocationUpdate() in order to get a location fix more quickly.

As always, thanks for maintaining this project, and hoping this is helpful :)

@zenangst
Copy link
Contributor

giphy-17

@zenangst
Copy link
Contributor

@vadymmarkov @RamonGilabert what do you guys think?

@RamonGilabert
Copy link
Contributor

Hey @fnakstad! Thank's for your contribution. Have a question about this. Could we make it optional to use?

One of the problems that we have with ImagePicker, or with any photos app really is that iOS asks you two permissions to grab images, the access to the camera and access to the library. I personally am very skeptical about those pop ups that appear, they are very aggressive to the user. Putting one more seems a bit harsh, if we put this as an optional thing, then it would be perfect, what do you think?

@fnakstad
Copy link
Contributor Author

Yea, that's a point I was considering myself, and it actually is optional the way it works right now. If the NSLocationWhenInUseUsageDescription key is not given in the Info.plist file, the user will not be prompted for permission to Location Services, and the app will function normally albeit not record location. So, to opt in to location support the user needs to set this key, which seems easier than juggling around a configuration setting in the library itself. What do you think?

@zenangst
Copy link
Contributor

@fnakstad what if you use location for other things in your application, like showing maps etc. This would mean that your application would be configured to use it but you don't have the fine-grained control that you might want. Hope I didn't misread your previous messages.

You could make it a configuration that defaults to true so that you don't have to turn it on, just configure your application like you would with this PR. And to handle the edge-case of not including geo data in images would also be an option as you could disable it by setting the property to false.

So that would actually fulfill both needs but handling edge-cases before they appear might not be what we want.

torn

Regardless on what we land on, I think there should be a paragraph about this in the README.

What do you guys think? @vadymmarkov @RamonGilabert

@fnakstad
Copy link
Contributor Author

That's a very good point @zenangst! If the user already has authorized your app to use Location Services some other place in your app (e.g. map), then it won't prompt you again for permission to use it for the camera. But I guess we're talking about the possible case where someone wants to use Location Services for a map or something, but actively does not want the camera to fetch location, in which case we need a configuration setting in the library. It seems like an uncommon scenario, but I'd be happy to make the changes if you'd rather have it work this way.

@zenangst
Copy link
Contributor

@fnakstad mind fixing the merge conflicts? ^^

@fnakstad
Copy link
Contributor Author

Should be fixed now!

@fnakstad
Copy link
Contributor Author

The more I thought about it the more sense it makes to have an explicit configuration option, so I went ahead and added it. Sorry it took so long, but it's been busy the last few days :(
I also refactored the CLLocationManager implementation into a wrapper class, so that we can use optional chaining in CameraView rather than strewing if statements everywhere to check the config value.

@fnakstad
Copy link
Contributor Author

Thanks for the feedback! Went ahead an implemented the changes.

zenangst added a commit that referenced this pull request Mar 16, 2016
Record location with CLLocationManager if permission given by user
@zenangst zenangst merged commit f9f96bf into hyperoslo:master Mar 16, 2016
@zenangst
Copy link
Contributor

@fnakstad this is great! thanks a lot for all the hard work you have put into this PR 😎

@fnakstad
Copy link
Contributor Author

Awesome :) Already using it in my own app, so I'm hoping it proves useful to others as well.
Props to you guys for maintaining and open-sourcing this project!

@v-ken
Copy link

v-ken commented Apr 3, 2018

Hi guys, thanks for this awesome library and PR. Was just wondering what the best way is to extract the location data in the doneButtonDidPress delegate callback?

I tried using the sample code here:
#102

But the fullImage?.properties field does not contain the GPS data.

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.

None yet

5 participants