791 waypoint removal #806

JThramer · 2017-11-07T17:01:25Z

Implementing #791. This is based off of @bsudekum's #789, so I'll change bases to master once that gets merged in.

This is definitely still a WIP... it's a working implementation, but buggy, as the two gesture recognizers like to interfere with one another. Still some work to do.

✅ :

/cc @bsudekum @frederoni @ericrwolfe

…rom route, but it's a buggy implementation, will rework by combining recognizers so they don't step on each-other

bsudekum · 2017-11-07T17:17:06Z

MapboxNavigation/NavigationMapView.swift

+
+        //take the best one, if existant, and prompt to delete
+        guard let best = candidates.1.first, let multipoint = candidates.0.first else { return }
+        requestNew(route: multipoint, without: best)


This might be a good place to slip in the delegate method. If the user implements it, give them the waypoint they selected. Otherwise, call provided boilerplate functionality.

Should 'boilerplate functionality' just be a navigationViewController(_:didSelectWaypoint:)delegate method or something? Perhaps the implementer doesn't want to delete a waypoint when the user selects one...

Right, so if they do not want this to happen, they implement navigationViewController(_:didSelectWaypoint:) and do something custom. Otherwise, we show the action sheet, and re-request the route.

bsudekum · 2017-11-07T17:19:27Z

MapboxNavigation/NavigationMapView.swift

+        options.waypoints.remove(at: index)
+
+        removeRoutes()
+        removeWaypoints()


No need to remove these; if you call showRoute/waypoint, we update routes/waypoints if they are already on the map.

bsudekum · 2017-11-07T17:22:47Z

MapboxNavigation/NavigationMapView.swift

+        _ = Directions.shared.calculate(options) { (waypoints, routes, error) in
+            guard let routes = routes else { return }
+            self.routes = routes
+            self.showRoutes(routes)


In the event of an error, we should keep the previous right and gently notify the user the request failed.

What if the view controller hosting the map view already has a way of notifying the user of a routing failure?

# Conflicts: # MapboxNavigation/NavigationMapView.swift

…h cases, however bug where an alternative route disappears is still persisting

JThramer · 2017-11-08T00:17:16Z

MapboxNavigation.xcodeproj/project.pbxproj

@@ -913,6 +916,7 @@
 				C5D9800E1EFBCDAD006DBF2E /* Date.swift */,
 				C51DF8651F38C31C006C6A15 /* Locale.swift */,
 				C578DA071EFD0FFF0052079F /* ProcessInfo.swift */,
+				8DF49B2F1FB2778A00B2F932 /* Route.swift */,


This shouldn't be here. Investigate.

…blown away at inappropriate times.

…ion annotation to not be added to the map.

1ec5

We’re trying too hard to cram view controller logic into this view class. The separations of responsibility in MVC exist for a reason. Inevitably, we’re going to find that the map view doesn’t have enough context for these features, and we’ll end up having to pull more and more view controller logic into this view, which will reduce its versatility and reusability.

Think of NavigationMapView as a renderer of data. Yes, some gesture functionality can be built into it, but anything that could possibly depend on the surrounding application’s state (such as whether we’re in a preview or not) should go in the delegate.

1ec5 · 2017-11-08T19:38:07Z

MapboxNavigation/NavigationMapView.swift

+
+        removeRoutes()
+        removeWaypoints()
+        _ = Directions.shared.calculate(options) { (waypoints, routes, error) in


It’s contrary to the MVC pattern for a view to kick off network requests. It should be the responsibility of the map view’s delegate (typically a view controller) to issue this request.

Agreed. @bsudekum's idea of just putting this burden on the implementation side will suffice for now, but yeah, it would be nice if the NavigationMapView got its own controller (which would become the parent of all our other NavigationMapView-related controllers) that could provide default implementations for some of this behavior. It's time will come :)

1ec5 · 2017-11-08T19:40:01Z

MapboxNavigation/NavigationMapView.swift

+        return (multipointRoutes, candidates)
+    }
+
+    private func routes(closeTo tap: UITapGestureRecognizer) -> [Route]? {


This method should take a point (in screen coordinates), rather than a gesture recognizer. That’ll make it useful for other purposes too.

The problem with this is that we do our distance calculation based upon CGPoint distance, not CLLocationDistance. I need the recognizer so I can convert coordinates back into screen geometry, and then test if the tap was within 50 points of the closest route coordinate.

Agreed, my suggestion is to have the method take a CGPoint, not a CLLocationCoordinate2D.

Apologies, I misread. Will Do.

1ec5 · 2017-11-08T19:48:05Z

MapboxNavigation/NavigationMapView.swift

+            return requestNew(route: activeRoute, without: waypointToRemove)
+        } else if let routes = self.routes(closeTo: sender) {
+            guard let selectedRoute = routes.first else { return }
+            select(route: selectedRoute)


This logic should be moved to the delegate implementation. We can make the helper methods public to facilitate that implementation. After all, the developer may want to programmatically select a route, or they may want to determine the route closest to a view that’s floating above the map view.

If implementing this logic represents an undue burden on the developer, we should vend a prefabricated view controller (#808). Alternatively, we could explore supplementing the existing Objective-C delegate protocol with a Swift protocol that can come with its own default implementations.

1ec5 · 2017-11-08T19:49:21Z

MapboxNavigation/NavigationMapView.swift

+        navigationMapDelegate?.navigationMapView?(self, didTap: route)
+    }
+
+    private func requestNew(route: Route, without waypoint: Waypoint) {


Extend the RouteOptions class with a method deleting(_:) that takes a waypoint and returns a new RouteOptions object.

Good idea. Will implement.

1ec5 · 2017-11-08T19:50:11Z

MapboxNavigation/NavigationMapView.swift

+        _ = Directions.shared.calculate(options) { (waypoints, routes, error) in
+            guard let routes = routes else { return }
+            self.routes = routes
+            self.showRoutes(routes)


What if the view controller hosting the map view already has a way of notifying the user of a routing failure?

bsudekum · 2017-11-08T20:06:01Z

@JThramer I agree with @1ec5 here. Let's expose the delegate method and leave it up to the developer to do the rest. Anything related to making a new request can be removed.

bsudekum · 2017-11-09T16:23:59Z

Examples/Swift/ViewController.swift

@@ -241,7 +258,14 @@ class ViewController: UIViewController, MGLMapViewDelegate, CLLocationManagerDel
        return false
    }

-    func navigationMapView(_ mapView: NavigationMapView, didTap route: Route) {
+    func navigationMapView(_ mapView: NavigationMapView, didSelect waypoint: Waypoint) {
+        guard let opts = currentRoute?.routeOptions else { return }


Can you call this routeOptions? Slightly unclear what it is at first glance.

…tation side of things

Requested changes implemented.

frederoni · 2017-11-09T21:27:28Z

MapboxCoreNavigation/RouteOptions.swift

@@ -1,12 +1,19 @@
 import MapboxDirections

-extension RouteOptions {
-    var activityType: CLActivityType {
+public extension RouteOptions {


Nit: avoid setting access level for the entire scope of an extension. It's safer to publicize methods individually. Especially when using Swift < 3.2.

Agree. Willfix.

frederoni · 2017-11-09T21:29:43Z

Examples/Swift/ViewController.swift

+            return routes?.first
+        }
+        set {
+            self.startButton.isEnabled = (newValue != nil)


nit: drop self ×4

Agree. Willfix.

As it turns out, the last two are required, but the first two are redundant and have been removed.

bsudekum · 2017-11-11T17:42:35Z

@JThramer now that #828 is in, it might make sense to rebase this PR since this needs MapboxDirections.swift >= v0.12. This will help with reviewing the pr locally.

bsudekum · 2017-11-11T17:45:43Z

MapboxCoreNavigation/RouteOptions.swift

        switch self.profileIdentifier {
        case MBDirectionsProfileIdentifier.cycling, MBDirectionsProfileIdentifier.walking:
            return .fitness
        default:
            return .automotiveNavigation
        }
    }
+    public func without(waypoint: Waypoint) -> RouteOptions {


Can you document this function now that it is public?

bsudekum · 2017-11-11T17:55:22Z

In the example app, I'm getting a prompt to remove a waypoint after selecting it, it removes the waypoint from the map, but the route is not recalculated.

Updated- huh, it seems to have only happened once, on the first load.

Update 2 - Can't reproduce now.

bsudekum · 2017-11-13T04:19:54Z

Examples/Swift/ViewController.swift

+            return routes?.first
+        }
+        set {
+            startButton.isEnabled = (newValue != nil)


No need for parens here.

bsudekum

Great work, I think this ready to go after a few minor comments are addressed.

JThramer · 2017-11-13T16:18:09Z

@bsudekum The unreproducible issue you were seeing is a Race Condition, with an observed probability of about 1/20 of repro-ing. I've got a fix ready, just want to test it a bit more to make sure.

…ute` (both changing the `routes` property), can cause a race condition.

Bobby Sudekum and others added 12 commits November 6, 2017 11:39

Add alternate routes

f7807f1

wider

bc375d2

Dont use run time styling

6fc4437

Change func

51e4a8e

fix

6ebd251

improve

1706c41

remove

853c413

make styleable

4ea0da5

rename func, simplify

56efc11

Use order

67c0f13

Constant

6348def

WIP -- Added gesture recognizer to detect taps and remove waypoints f…

d7d54bc

…rom route, but it's a buggy implementation, will rework by combining recognizers so they don't step on each-other

JThramer added feature New feature request. wip ⚠️ DO NOT MERGE PR should not be merged! labels Nov 7, 2017

bsudekum reviewed Nov 7, 2017

View reviewed changes

JThramer changed the base branch from select-alts to master November 7, 2017 19:29

Jerrad Thramer added 2 commits November 7, 2017 12:32

Merge branch 'master' into jerrad/791-waypoint-removal

e56c42f

# Conflicts: # MapboxNavigation/NavigationMapView.swift

WIP. Combined gesture recognizers into a single instance handling bot…

341573e

…h cases, however bug where an alternative route disappears is still persisting

JThramer commented Nov 8, 2017

View reviewed changes

Oops.

a035859

JThramer mentioned this pull request Nov 8, 2017

Rename navigationMapView(_:didTap:) #809

Closed

Jerrad Thramer added 2 commits November 8, 2017 11:42

Fixing issue where showWaypoints was causing routes model to get …

5b215cd

…blown away at inappropriate times.

Fixing regression where improperly placed conditional caused destinat…

0820770

…ion annotation to not be added to the map.

1ec5 previously requested changes Nov 8, 2017

View reviewed changes

1ec5 mentioned this pull request Nov 8, 2017

Route preview view controller #808

Closed

JThramer mentioned this pull request Nov 8, 2017

Adding support for NSCopying protocol. mapbox/mapbox-directions-swift#200

Merged

bsudekum reviewed Nov 9, 2017

View reviewed changes

Jerrad Thramer added 9 commits November 9, 2017 12:53

Moving route selection logic and relevant models over to the implemen…

14c4fd7

…tation side of things

Cleanup: Better Nomenclature for routeOptions

27a7cd1

Cleanup: turning selectedRoute into a bidirectional computed property

317bc97

Cleanup: better constant for NavigationMapView

a79b45b

Cleanup: Organizing methods, adding pragma marks

58d6053

Cleanup: typo in extension function name

e1d1e8f

Cleanup: Organizing methods, adding pragma marks for ViewController

15c3d0c

Fixing issue where waypoint feature was not removed when route changed

d208ebd

Adding confirmation action sheet to ViewController. Ready for review.

3d42329

JThramer added ✓ ready for review and removed wip ⚠️ DO NOT MERGE PR should not be merged! labels Nov 9, 2017

frederoni reviewed Nov 9, 2017

View reviewed changes

Jerrad Thramer added 2 commits November 9, 2017 15:48

drop public from RouteOptions extension

0e6c8e2

Removing redundant self calls

8324261

bsudekum mentioned this pull request Nov 10, 2017

Release v0.10.0 #834

Merged

bsudekum reviewed Nov 11, 2017

View reviewed changes

bsudekum reviewed Nov 13, 2017

View reviewed changes

bsudekum approved these changes Nov 13, 2017

View reviewed changes

Jerrad Thramer added 2 commits November 13, 2017 09:57

Fixing issue where setting self.routes right before `self.currentRo…

d9d7a8f

…ute` (both changing the `routes` property), can cause a race condition.

Adding documentation for extension function.

a756feb

JThramer merged commit 2bfaa59 into master Nov 13, 2017

JThramer deleted the jerrad/791-waypoint-removal branch November 13, 2017 17:38

JThramer mentioned this pull request Nov 13, 2017

Add delegate method for selecting a waypoint #791

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

791 waypoint removal #806

791 waypoint removal #806

JThramer commented Nov 7, 2017 •

edited

Loading

bsudekum Nov 7, 2017

JThramer Nov 7, 2017

bsudekum Nov 7, 2017

bsudekum Nov 7, 2017

bsudekum Nov 7, 2017

1ec5 Nov 8, 2017

JThramer Nov 8, 2017

1ec5 left a comment

1ec5 Nov 8, 2017

JThramer Nov 8, 2017 •

edited

Loading

1ec5 Nov 8, 2017

JThramer Nov 8, 2017

1ec5 Nov 8, 2017 •

edited

Loading

JThramer Nov 8, 2017

1ec5 Nov 8, 2017

1ec5 Nov 8, 2017

JThramer Nov 8, 2017

1ec5 Nov 8, 2017

bsudekum commented Nov 8, 2017

bsudekum Nov 9, 2017

frederoni Nov 9, 2017

JThramer Nov 9, 2017

frederoni Nov 9, 2017

JThramer Nov 9, 2017

JThramer Nov 9, 2017

bsudekum commented Nov 11, 2017 •

edited

Loading

bsudekum Nov 11, 2017

bsudekum commented Nov 11, 2017 •

edited

Loading

bsudekum Nov 13, 2017

bsudekum left a comment

JThramer commented Nov 13, 2017

791 waypoint removal #806

791 waypoint removal #806

Conversation

JThramer commented Nov 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JThramer Nov 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 Nov 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsudekum commented Nov 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsudekum commented Nov 11, 2017 • edited Loading

Choose a reason for hiding this comment

bsudekum commented Nov 11, 2017 • edited Loading

Choose a reason for hiding this comment

bsudekum left a comment

Choose a reason for hiding this comment

JThramer commented Nov 13, 2017

JThramer commented Nov 7, 2017 •

edited

Loading

JThramer Nov 8, 2017 •

edited

Loading

1ec5 Nov 8, 2017 •

edited

Loading

bsudekum commented Nov 11, 2017 •

edited

Loading

bsudekum commented Nov 11, 2017 •

edited

Loading