Save location by GPS #6 #14
Conversation
* added CLLocationManager * added "when in use" authorization * lookup the returned location * added a label to display location (though perhaps it should just be an alert controller for user to confirm they want to use this address) * still need to handle persistence and the zipcode retrieval logic
mostly finished. need to clean up the fetching, and general cleanup
* cleanup unneeded init in `EditLocationViewController` * `IssuesContainerViewController` * refactored setTitle * refactored `updateWith` for zipCode and for lat/long * updated `IssuesManager` to handle zip or latLong * updated `FetchIssuesOperation to handle zip or latLong * updated execute to append zip or lat long -- confirmed both work * added LocationInfoKeys
* removed leftover `print`
|
Just a point of feedback on commit messages, I don't think it's valuable to reference the branch name as the subject of each commit. Here's a good article on why: http://chris.beams.io/posts/git-commit/ |
| @@ -8,16 +8,25 @@ | |||
|
|
|||
| import UIKit | |||
| import CoreLocation | |||
| //import GLKit | |||
There was a problem hiding this comment.
Oh please tell us we're getting some cool 3D effects? ;)
There was a problem hiding this comment.
I'm sure we can find a reason that we(I) need to learn how to write shaders in order to complete this app.
| } | ||
|
|
||
| @IBAction func cancelTapped(_ sender: Any) { | ||
| delegate?.editLocationViewControllerDidCancel(self) | ||
| } | ||
|
|
||
| @IBAction func submitZipCodeTapped(_ sender: Any) { | ||
| //TODO: delete location if present |
There was a problem hiding this comment.
Does this comment still make sense if we are deleting the defaults on updateWith below? Or did you mean in the controller itself?
| @@ -65,13 +62,56 @@ class IssuesContainerViewController : UIViewController, EditLocationViewControll | |||
| } | |||
|
|
|||
| func editLocationViewController(_ vc: EditLocationViewController, didSelectZipCode zip: String) { | |||
| dismiss(animated: true, completion: { //`updateWith` must be in completion block so that VC is listening for notification | |||
| self.updateWith(zipCode: zip) | |||
There was a problem hiding this comment.
Should use [weak self] on completion blocks that reference self. (A few of them here below as well)
There was a problem hiding this comment.
Thanks. I actually hadn't fully considered the [weak self] issue. I've been making an unexamined assumption that Swift (3) was magically taking care of that for me.
| locationButton.setTitle(zip, for: .normal) | ||
| } else if let locationInfo = UserDefaults.standard.value(forKey: UserDefaultsKeys.locationInfo.rawValue) as? [String: Any] { | ||
| let displayName = (locationInfo["displayName"] as? String) ?? "Selected Location" | ||
| self.locationButton.setTitle(displayName, for: .normal) |
There was a problem hiding this comment.
In this case no need to use self. outside of completion blocks.
| var urlString = "https://5calls.org/issues/" | ||
| let urlQueryString: String? = zipCode ?? latLongString | ||
| if let query = urlQueryString { | ||
| urlString = urlString + "?address=" + query |
There was a problem hiding this comment.
NSURLComponents + queryItems might be a bit cleaner here.
There was a problem hiding this comment.
Yeah, though perhaps it would be better to break that into a separate ticket though with a thorough clarification of the fetch operation. @subdigital, what do you think?
* removed old comments from `EditLocationViewController` * removed unneeded `self` reference in `IssuesContainerViewController` * made `self` references in VC dismissal closures `[weak self]
| @@ -8,16 +8,25 @@ | |||
|
|
|||
| import UIKit | |||
| import CoreLocation | |||
| //import GLKit | |||
| } | ||
|
|
||
| @IBAction func cancelTapped(_ sender: Any) { | ||
| delegate?.editLocationViewControllerDidCancel(self) | ||
| } | ||
|
|
||
| @IBAction func submitZipCodeTapped(_ sender: Any) { | ||
| //TODO: delete location if present |
There was a problem hiding this comment.
In my branch issue-detail I moved the location persistence behind IssuesManager because we have to do this from multiple places. I'm thinking of leaving this VC to be oblivious to storage & fetching and simply notify its delegate of whatever happened.
| locationInfo["zipcode"] = placemarks?.first?.postalCode ?? "" | ||
| completion(locationInfo) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Do you think this title setting code would be cleaner if we did the reverse geocode in EditLocationViewController and just used an activity indicator there before calling the delegate?
That way you'd just have the string and wouldn't have to have a flash of two values.
Alternatively we could just read the string from issuesManager.issuesList.normalizedLocation since the API returns it.
There was a problem hiding this comment.
sounds great. that was my initial approach. no particular reason I didn't stick with it.
There was a problem hiding this comment.
ok let's get this ready for merge and we can make that change later. Can you merge in master / resolve that conflict?
#6
The primary logic that I have employed regarding whether to fetch with zip or latLong is that when a user sets zip or latLong, the other is deleted. So when a fetch is requested, the only non-nil value is passed along to the query. Additionally if both are nil, then the non-query url is passed along. This felt natural to do and less complicated than setting an order of preference or setting another key to indicate which to use. But definitely open to a different approach. I have confirmed that switching between location/zip as a user correctly returns the correct results based on last selected location/zip.
EditLocationViewControllerIssuesContainerViewControllerupdateWithfor zipCode and for lat/longIssuesManagerto handle zip or latLongLastly, there are a few dinky cleanup commits. I would recommend the "squash and merge" option, or I can squash locally and push it up fresh, but i added the commits after I noticed small things sequentially after making the PR.