Navigation UI by frederoni · Pull Request #43 · mapbox/mapbox-navigation-ios

frederoni · 2017-02-03T17:22:45Z

First draft of the Nav UI

bsudekum · 2017-02-03T17:30:15Z

@frederoni can we merge the UI example with the Example in place now? Also, let's see if we can remove the duplicate Geomtry.swift.

bsudekum · 2017-02-06T17:10:04Z

Does looks like this is a duplicate?

bsudekum · 2017-02-06T17:10:53Z

Deleted these plists because they should be included in the app/example and not the SDK.

bsudekum · 2017-02-06T17:12:20Z

Should we include any translations?

I removed them temporarily while cleaning up but I think we should keep the translations in here.

bsudekum · 2017-02-13T09:49:56Z

@frederoni what do you think about just including this in core instead of requiring an additional pod to install?

frederoni · 2017-02-13T10:08:51Z

@bsudekum It'd be convenient not having to maintain the libs separately and we can only version one of them as long as they live in the same repo, unless we publish.

That being said, I still think we should keep them separate. Navigation UI has dependencies we don't want to enforce on all navigation users. Navigation.swift can technically be used on ⌚️ while Navigation UI is📱 only.

bsudekum · 2017-02-13T10:18:08Z

@frederoni ah that makes sense, I'll review this today.

bsudekum · 2017-02-16T10:21:06Z

+    var currentManeuverArrowPolylines: [ArrowFillPolyline] = []
+    var currentManeuverArrowStrokePolylines: [ArrowFillPolyline] = []
+    let distanceFormatter = DistanceFormatter(approximate: true)
+    let secondsBeforeResetTrackingMode:TimeInterval = 25.0


We should pull this out into a constant

bsudekum · 2017-02-16T10:22:15Z

+
+        if let camera = pendingCamera {
+            camera.altitude = 1_000
+            camera.pitch = 45


If a developer sends through a camera with a custom pitch and altitude, we should honor these values.

Default camera with pitch 45 and 1000m but not overriding if pendingCamera != nil.

bsudekum · 2017-02-16T10:23:10Z

+        }
+
+        let imageName = imageNamePattern.replacingOccurrences(of: " ", with: "_").replacingOccurrences(of: "{ref}", with: number)
+        let apiURL = URL(string: "https://commons.wikimedia.org/w/api.php?action=query&format=json&maxage=86400&prop=imageinfo&titles=File%3A\(imageName)&iiprop=url%7Csize&iiurlheight=\(Int(round(height)))")!


@1ec5 you think it's safe to ship this?

This is part of the public Wikimedia Commons API. The maxage parameter should keep repeated requests from being expensive, but we should ensure that we’re calling the API only as frequently as necessary.

bsudekum · 2017-02-16T10:23:42Z

+    }
+
+    func giveLocalNotification(_ step: RouteStep) {
+        if UIApplication.shared.applicationState == .background {


We should make notifications an option.

Added sendNotifications: Bool

bsudekum · 2017-02-16T10:25:24Z

+//  Generated by PaintCode (www.paintcodeapp.com)
+//
+
+


Should we check in the paint code project?

bsudekum · 2017-02-16T12:17:41Z

+class ArrowStrokePolyline: ArrowFillPolyline {}
+
+struct RouteControllerNotification {
+    static let didReceiveNewRoute = Notification.Name("RouteControllerDidReceiveNewRoute")


This should either be removed or added where other notifications are defined

1ec5

Not having taken a look at this PR until now, it’s a bit overwhelming to review the code closely. So I started by reviewing the example view controllers for symptoms of architectural and ergonomic problems, then did a spot check over some of the remaining files. A more thorough review will take a bit more time.

Aside from the comments below, the main README.md should refer to the UI/ folder, so developers know to look there.

1ec5 · 2017-02-17T06:42:43Z

+| --- | --- |
+
+### Examples
+We provide examples in Swift and Objective-C. Run `pod install` from the Example folder and open `Example.xcworkspace` to try it out.


FYI it’s now possible to obtain the map SDK via Carthage: mapbox/mapbox-gl-native#1623 (comment).

1ec5 · 2017-02-17T06:54:09Z

+        // Pass through a
+        // 1. the route the user will take
+        // 2. A `Directions` class, used for rerouting.
+        let controller = NavigationUI.instantiate(route: route, directions: directions)


Can you explain what “a navigation UI” is and how this object fits into an MVC architecture? It seems to be a factory for RouteViewController objects. Why not initialize a RouteViewController directly?

I first added the NavigationUI to handle the theme only, but because of the way Pulley is set up with designated initializer which I can't override conveniently, a factory method was the most appropriate I could come up with. The terminology is inherited from

UIStoryboard.instantiateInitialViewController() UIStoryboard.instantiateViewController(withIdentifier:)

That being said, I agree with you, I'd prefer an init method on RouteViewController. The problem is supporting initializing it programmatically and through storyboard without using a factory method. I guess I could fork Pulley although the intention is to replace it eventually.

UIStoryboard performs a very different role than NavigationUI. The initial view controller is essentially a singleton, whereas RouteViewController is not. If we stick with a factory method, we should name it something like routeViewController(for:).

1ec5 · 2017-02-17T06:54:26Z

+        // Pass through a
+        // 1. the route the user will take
+        // 2. A `Directions` class, used for rerouting.
+        let controller = NavigationUI.instantiate(route: route, directions: directions)


controller is a local variable that isn’t referenced anywhere else – does that mean the object gets deinitialized immediately after creation? If not, ViewController should hang onto it.

controller is actually a view controller that gets pushed into the navigation stack which hangs onto it.

1ec5 · 2017-02-17T06:55:56Z

+        // Pass through a
+        // 1. the route the user will take
+        // 2. A `Directions` class, used for rerouting.
+        let controller = NavigationUI.instantiate(route: route, directions: directions)


Swift and Objective-C favor the term “init” over the term “instantiate”.

See #43 (comment)

1ec5 · 2017-02-17T07:08:47Z

+        let controller = NavigationUI.instantiate(route: route, directions: directions)
+        present(controller, animated: true, completion: nil)
+
+        controller.willDismissNavigationHandler = {


Forcing RouteViewController to own callbacks makes it all too easy for a developer to write a reference cycle that leaks memory. The two most common patterns for “handlers” are:

Subclassing. If the application wants to customize the dismissal experience, it can subclass RouteViewController to override dismiss(animated:completion:) and viewDidDisappear(_:).

Delegation. Add a RouteViewControllerDelegate protocol that ViewController can conform to by implementing routeViewControllerWillDismiss(_:) and routeViewControllerDidDismiss(_:).

Also, is there anything we can do to facilitate the use of an unwind segue to dismiss RouteViewController all the way to a starting point?

1ec5 · 2017-02-17T07:29:20Z

+
+let distanceFormatter = DistanceFormatter()
+
+extension MapboxGeocoder.GeocodedPlacemark {


Is there a conflict that would force us to qualify GeocodedPlacemark with the MapboxGeocoder module here?

1ec5 · 2017-02-17T07:30:18Z

+                    // TODO: Show country if not the current country.
+                    // Cut off country and postal code and add abbreviated state/region code at the end.
+
+                    let stitle = lines.prefix(upTo: 2).joined(separator: NSLocalizedString("ADDRESS_LINE_SEPARATOR", value: ", ", comment: "Delimiter between lines in an address when displayed inline"))


There are multiple references to localized strings, but this PR doesn’t include a .strings file.

1ec5 · 2017-02-17T07:34:01Z

+        return string(for: obj, markUpWithSSML: false)
+    }
+
+    func string(for obj: Any?, markUpWithSSML: Bool) -> String? {


This SSML markup code is unused. I take it it’s for the Polly integration being added in #42.

1ec5 · 2017-02-17T07:35:50Z

+    }
+}
+
+class OSRMInstructionFormatter: Formatter {


As I pointed out in #42 (review), I think it’s very important for the Swift port of OSRMTextInstructions to live independently of the Mapbox navigation SDK. The instructions included in this PR are also out of date compared to what’s in Transifex that could be transformed via the existing script to plist format.

1ec5 · 2017-02-17T07:42:02Z

+                let number = components[1]
+                if var imageName = ShieldImageNamesByPrefix[network] {
+                    imageName = imageName.replacingOccurrences(of: " ", with: "_").replacingOccurrences(of: "{ref}", with: number)
+                    let url = URL(string: "https://commons.wikimedia.org/w/thumb.php?f=\(imageName)&w=\(imageView.bounds.width * UIScreen.main.scale)")


Frankly, our use of thumb.php is more problematic than our use of the Wikimedia Commons API. One problem is that thumb.php has no maxage parameter like the API does. Does SDWebImage do a good job of avoiding duplicate requests for the same thumbnail image as the user scrolls a particular row in and out?

Yes. It will only request once per url in this case. Even after restarting the process, it will not make the request again but instead fetch it from disk. maxage defaults to one week.

stale

Rewrote the library to more closely parallel mapbox/MapboxGeocoder.swift#41 and mapbox/MapboxStatic.swift#19. Removed the class prefix from Swift but kept it for Objective-C. The goal is no longer to be a (poor) drop-in replacement for MapKit’s MKDirections API but rather to be as idiomatic as possible in Swift and Objective-C, with a priority on Swift. Symbols conform to platform naming conventions but in some cases eschew MapKit terminology in favor of Geocoding API terminology, so that api-documentation makes sense in the context of this library. Removed the dependency on RequestKit. This library chooses reasonable default networking behavior; if you need anything more sophisticated, you can access the URL request information to use with a custom NSURLSession. Directions no longer needs to be strongly held in order for the request to finish. Instead, the request is made against the shared URL session; to use a custom URL session, make the request yourself using the URL returned by the URLForCalculatingDirections(options:) method. Removed the cancel() method; instead, directly cancel the NSURLSessionDataTask returned by calculateDirections(options:completionHandler:). Added a shared (singleton) Directions object. Use the shared object if you’ve set your Mapbox access token in the MGLMapboxAccessToken key of your application’s Info.plist file. Otherwise, create a Directions object with the access token explicitly. A single directions object can handle multiple requests concurrently (since it just passes the requests off to the shared URL session). Eliminated the separate APIs for calculating ETAs. Instead, all the options supported by the Directions API, including flags for including steps and the overview geometry, are now exposed through a new RouteOptions class. Effectively, this change adds support for the geometries, overview, radiuses, steps, and continue_straight query parameters and allows the heading accuracy of any waypoint to be customized. However, note that steps are no longer returned by default, and the overview geometry is simplified by default. The access_token parameter is now the last URL parameter instead of the first. Broke out MBDirectionsRequest.TransportType into three profile identifier constants. You always specify a profile identifier, not a transport type. That makes the desired mode of transportation more open-ended. Removed the MBDirectionsResponse class in favor of passing the waypoints and routes from the response directly into the completion handler. Renamed Route.geometry to Route.coordinates. For Objective-C compatibility, all enums are backed by integer types instead of String, but they’re CustomStringConvertible to allow for the same expressiveness we’re used to in Swift. Error handling is much more robust now. Various error conditions returned by the API cause the localized failure reason and recovery suggestion to be set in the NSError object that is passed into the completion handler. Updated test fixtures and expectations for the v5 server-side API and the revamped client-side API, and updated examples. Fixes mapbox#41, fixes mapbox#43, fixes mapbox#44.

frederoni self-assigned this Feb 3, 2017

bsudekum reviewed Feb 6, 2017

View reviewed changes

Comment thread UI/README.md Outdated

Copy link
Copy Markdown

Contributor

bsudekum Feb 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does looks like this is a duplicate?

frederoni reacted with thumbs up emoji

bsudekum reviewed Feb 6, 2017

View reviewed changes

frederoni force-pushed the fred-ui branch 9 times, most recently from aeb4131 to 09c7d9f Compare February 10, 2017 15:59

frederoni force-pushed the fred-ui branch from a2d330a to 94e8ca2 Compare February 13, 2017 17:29

bsudekum reviewed Feb 16, 2017

View reviewed changes

bsudekum previously requested changes Feb 16, 2017

View reviewed changes

1ec5 suggested changes Feb 17, 2017

View reviewed changes

frederoni force-pushed the fred-ui branch from 66b8ae1 to 8631279 Compare February 20, 2017 21:24

frederoni mentioned this pull request Feb 21, 2017

MapboxDirections.swift, iOS SDK v3.4.2 #44

Closed

frederoni force-pushed the fred-ui branch 3 times, most recently from 32fad0d to 34ce2cf Compare February 22, 2017 23:41

frederoni added 22 commits February 23, 2017 18:14

Dev team

1409316

Update deployment target

0dc7274

Remove bundle

e13d15f

Refactored style arrow

fdc8a45

Use Carthage and project dependency for Example

f50a4fc

Update directions bridge

8a0adc6

Dummy test to please CI

c3d96a3

Rename method

87d6ad3

Remove dead code and unused class

20e239b

Merge readme

e6034a6

Bring in OSRMTextInstructions

062e64d

Monoproject

344d1db

Use the official osrm-text-instructions.swift

36283ed

Reorganized project structure

1701e9b

Merge and flatten examples

ce8ae7f

Documentation fix and bogus token

85bf3ce

Separated examples. Correct bundle identifiers and storyboard refs

0e95934

Pin versions for dependencies

927de0a

Street v10

bfcc5b9

NavUI depends on OSRMTI

e43b02f

Shared geometry.swift

5040472

Updated docs

04126f0

frederoni force-pushed the fred-ui branch from 97cb23b to 04126f0 Compare February 24, 2017 02:15

frederoni merged commit d7138c0 into master Feb 24, 2017

friedbunny deleted the fred-ui branch March 1, 2017 02:39

1ec5 mentioned this pull request Apr 7, 2017

RouteViewController should use an initializer rather than a static function on NavigationUI #124

Closed

donaldmanuel mentioned this pull request Aug 17, 2023

[Bug]: Crash when creating MapboxNavigationService #4528

Closed


		let distanceFormatter = DistanceFormatter()

		extension MapboxGeocoder.GeocodedPlacemark {

Conversation

frederoni commented Feb 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bsudekum commented Feb 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frederoni Feb 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsudekum commented Feb 13, 2017

Uh oh!

frederoni commented Feb 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bsudekum commented Feb 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1ec5 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frederoni Feb 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

frederoni commented Feb 3, 2017 •

edited

Loading

frederoni Feb 6, 2017 •

edited

Loading

frederoni commented Feb 13, 2017 •

edited

Loading

frederoni Feb 17, 2017 •

edited

Loading