Fixup EndOfRouteViewController #58

michaelkirk · 2024-06-13T00:49:56Z

~~DO NOT MERGE: This currently exposes a crashes due to another issue: see #57~~ Fixed with latest commit.

~~I think we'll eventually want to merge this and it might be helpful if you're trying to debug #57, which I'm currently stuck on.~~

Fixes #57
Fixes #62

Previously, having voice instructions was a requirement of ending the route.

Now we'll end the route when banner instructions are done and only consider voice instructions if there were any to begin with.

I also fell down the rabbit hole of fixing one bug after another once the EndOfRouteViewController (EORVC) was presented, otherwise, it's arguably better not to show it at all.

In particular, I also removed the "feedback" UI from the EORVC since the feedback mechanism was removed. See #62

michaelkirk · 2024-06-13T00:51:25Z

MapboxCoreNavigation/RouteController.swift

@@ -424,19 +424,23 @@ extension RouteController: CLLocationManagerDelegate {

    func updateRouteLegProgress(for location: CLLocation) {
        let currentDestination = self.routeProgress.currentLeg.destination
-        guard let remainingVoiceInstructions = routeProgress.currentLegProgress.currentStepProgress.remainingSpokenInstructions else { return }


This guard is what was throwing us off. We'd unconditionally return without calling didArriveAt: if not using spoken instructions.

michaelkirk · 2024-06-13T20:42:59Z

I've fixed the crash, though as I've documented in my commit message, I don't really understand what's going on.

I don't know very much about storyboards nor swift package manager, but my guess is that there's some problem happening between the interaction of storyboards and SPM.

Here's what I've observed:

Before this change, here's the storyboard UI for the RatingControl element:

I see this error when presenting the EndOfRouteViewController:

[Storyboard] Unknown class _TtC40maplibre-navigation-ios_MapboxNavigation13RatingControl in Interface Builder file.

So the ViewController is successfully loaded from the bundle, but RatingControl has some prefix applied to the name, presumably that is some dynamic prefix thing that the compiler adds to namespace modules.

If I uncheck "Inherit Module from Target", the "Module" text field becomes empty:

Re-running my route, upon completion of the route, it crashes at the same point, but with a slightly different error:

[Storyboard] Unknown class RatingControl in Interface Builder file.

Note that the RatingControl class is simple now, lacking the generated prefix from the first error.

If I then manually type "MapboxNavigation" into the "Module" field like this:

The end of route controller now presents as expected. This is the change I'm proposing.

michaelkirk · 2024-06-14T00:29:47Z

I just keep going down the rabbit hole here. 🐇

Marking as draft again until #62 is resolved.

michaelkirk · 2024-06-14T02:55:21Z

I think I've arrived at something worthwhile.

Ultimately I've:

fixed the logic that was preventing the EORVC from presenting Fixup EndOfRouteViewController #58
fixed the crash that occurred when the EORVC finally was presented crash when route ends: [Storyboard] Unknown class _TtC40maplibre-navigation-ios_MapboxNavigation13RatingControl in Interface Builder file. #57
fixed the crash that happened when you tried to interact with the feedback text field (I never opened an issue, but it's 2bf7799)
fixed a layout bug with the keyboard animation: (I never opened an issue: 1402368)
removed all the feedback fields per Feedback in EndOfRouteViewController goes nowhere. #62

I regret that it ballooned into something as big as it did, but I think it's kind of the smallest reasonably coherent set of work. I could upstream the above changes individually, or you might find reviewing the individual commits helpful.

I did take some liberty in my implementation in that I replaced the EORVC storyboard with programatic view construction. If this is viewed as distasteful, I can rework my changes to keep the storyboard in play.

My motivation for getting rid of the storyboard was:

I don't know very much about storyboards and they seem complicated and somewhat magical. For example, I really don't understand how I fixed the crash in crash when route ends: [Storyboard] Unknown class _TtC40maplibre-navigation-ios_MapboxNavigation13RatingControl in Interface Builder file. #57.
It was the only storyboard view in the framework. It seems nice to reduce paradigms.

Here's a waypoint with a pathologically long name so you can see how the layout grows:

michaelkirk · 2024-06-14T02:55:42Z

scripts/file_conversion.sh

@@ -1,4 +1,4 @@
-#!/usr/bin/env/bash
+#!/usr/bin/env bash


michaelkirk · 2024-06-14T02:57:36Z

scripts/extract_localizable.sh


    # Extract localizable strings from .swift files
    find ${CORE} -name "*.swift" -print0 | xargs -0 xcrun extractLocStrings -o "${CORE}/Resources/${lang}.lproj"
    STRINGS_FILE="${CORE}/Resources/${lang}.lproj/Localizable.strings"
-


I removed some strings, so I wanted to regenerate them. Unfortunately the script was broken, so I fixed that too.

It seems like maybe there were some other stale strings that hadn't been removed by earlier work.

michaelkirk · 2024-06-14T02:58:12Z

MapboxNavigation/UIView.swift

@@ -70,7 +70,10 @@ extension UIView {
    }

    func pinInSuperview(respectingMargins margins: Bool = false) {
-        guard let superview else { return }
+        guard let superview else {
+            assertionFailure("superview was unexpectedly nil")


I added this while debugging. It seems like a good check, but it's ancillary to my change. I can remove it if anyone is offended.

michaelkirk · 2024-06-14T02:58:41Z

MapboxNavigation/RouteMapViewController.swift

@@ -414,11 +416,6 @@ class RouteMapViewController: UIViewController {
        self.navigationView.endOfRouteView = endOfRoute.view
        self.navigationView.constrainEndOfRoute()
        endOfRoute.didMove(toParent: self)
-
-        endOfRoute.dismissHandler = { [weak self] _, _ in


Now that we're building programmatically, not from the storyboard, this moved to the initializer.

michaelkirk · 2024-06-14T03:04:22Z

MapboxNavigation/EndOfRouteViewController.swift

    }

    override func viewWillAppear(_ animated: Bool) {
-        super.viewWillDisappear(animated)
-        view.roundCorners([.topLeft, .topRight])


After converting to a programmatic view, the view was not visible unless I commented out view.roundCorners here.

view.roundCorners depends on view.bounds, which are not necessarily at their final state in viewWillAppear, though I guess they "happened to be" with the Storyboard. I've moved it to viewDidLayoutSubiews, which guarantees the views bounds are set.

Also this means we'll correctly re-compute the path if the bounds change (e.g. on device rotation).

michaelkirk · 2024-06-14T03:05:50Z

MapboxNavigation/EndOfRouteViewController.swift

-        self.stars.rating = 0
-    }
-
-    private func styleForUnnamedDestination() {


I inlined this because it was short, and easier to understand the parallel logic of the "else" case.

For example, we now set self.staticYouHaveArrived.alpha back to 1.0 in the case that a new destination has a name after a previous one doesn't.

michaelkirk · 2024-06-14T03:10:03Z

MapboxNavigation/RouteMapViewController.swift

-        case .linear:
-            self = .curveLinear
-        @unknown default:
-            fatalError("Unknown curve")


There was a crash here, fixed in 2bf7799, but then since it was only used for the feedback views, I deleted it all anyway.

michaelkirk · 2024-06-14T03:17:35Z

MapboxNavigation/EndOfRouteViewController.swift

    }

    override func viewWillAppear(_ animated: Bool) {
-        super.viewWillDisappear(animated)


I think this was a typo?: Disappear

Previously, having voice instructions was a requirement of ending the route. Now we'll end the route when banner instructions are done *and* only consider voice instructions if there were any to begin with. Note: This currently crashes do to another issue with the end of route view controller: see maplibre#57

I have very little experience with Storyboards. Here's what I've observed: Before this change, I see this error when presenting the EndOfRouteViewController: [Storyboard] Unknown class _TtC40maplibre-navigation-ios_MapboxNavigation13RatingControl in Interface Builder file. So the ViewController is successfully loaded from the bundle, but `RatingControl` has some prefix applied to the name, presumably that is some dynamic prefix thing that the compiler adds to namespace modules. If I disable the "Inherit Module from Target" I get: [Storyboard] Unknown class RatingControl in Interface Builder file. Note the simpler class name.

…sh though

…c view

boldtrn · 2024-06-15T08:47:34Z

MapboxCoreNavigation/RouteController.swift


-            self.routeProgress.currentLegProgress.userHasArrivedAtWaypoint = true
+        if let remainingVoiceInstructions = routeProgress.currentLegProgress.currentStepProgress.remainingSpokenInstructions {


If I understand this right, if we have no voiceInstructions, then we will progress the legIndex on the second to last step, which might be very long.

To be honest I don't like the previous code here as well.

How about we rewrite this code to:

Check if user is on the second to last step

If the delegate is implemented, we ask the delegate: self.delegate?.routeController?(self, didArriveAt: currentDestination)

If the delegate is not implemented, I would propose something like (distance remaining to next WP < 50m or duration remaining < 5s or something like this and then advance to next leg.

Unless I am missing anything, we should also only set

self.previousArrivalWaypoint = currentDestination self.routeProgress.currentLegProgress.userHasArrivedAtWaypoint = true

If the delegate returns true.

If I understand this right, if we have no voiceInstructions, then we will progress the legIndex on the second to last step, which might be very long.

Yes, that's the pre-existing logic. If you're saying it's weird, I agree with you. Maybe it was never relevant before because mapbox always included voice instructions? I honestly don't know.

If the delegate is implemented, we ask the delegate: self.delegate?.routeController?(self, didArriveAt: currentDestination)

Just in case there is confusion here, on iOS, the did in a method name connotes a hook called after something has already happened.

e.g. viewDidAppear is a hook called after the view appears, application(_:didReceiveRemoteNotification:) is a hook called after the application receives a remote notification. Contrast it with will which is called just before the thing happens.

viewWillAppear <- user hook

(view appears) <- system does this

viewDidAppear <- user hook

re: didArriveAt:

returns: True to advance to the next leg, if any, or false to remain on the completed leg.

So the existing boolean returned by didArriveAt does not answer the question "did I arrive?". It says "you have arrived. Should I continue navigating you to the next leg?".

I guess it's only relevant to multi-leg journeys. TBH I haven't used it.

I agree that the current logic for determining when you're considered to have have arrived is weird. I think your approach, to compare remaining distance/time, makes much more sense than what's there now. I will update that logic.

I don't think it's worth exposing a user over-ridable delegate method to customize this behavior. Though, I don't feel strongly about that if you do. I do feel strongly that iff we create and expose such a delegate method, it should be called something like shouldArriveAt: (as opposed to did...)

Maybe it was never relevant before because mapbox always included voice instructions?

Yes, I think so.

So the existing boolean returned by didArriveAt does not answer the question "did I arrive?". It says "you have arrived. Should I continue navigating you to the next leg?".

Ah ok got it, yeah I still trown off by iOS conventions, thanks.

I don't think it's worth exposing a user over-ridable delegate method to customize this behavior

Ok, maybe we just make this whole function overwritable? As, to be honest we are thinking about overwriting this with some more specialized code in our app ;)

to compare remaining distance/time, makes much more sense than what's there now. I will update that logic.

I've pushed up a commit that implements this. It turns out there was already some "slop" built in. See a165167

I don't think it's worth exposing a user over-ridable delegate method to customize this behavior

Ok, maybe we just make this whole function overwritable? As, to be honest we are thinking about overwriting this with some more specialized code in our app ;)

Heh, I didn't think it was worthwhile because I was having a hard time imagining that someone would have a substantially different definition of what "arrival" means, but you have already proven me wrong! 😆 I'm happy to add a delegate method in that case.

Can you share what your definition of arrival would look like? I'm curious.

We mostly have multi leg routes, so we might want to do something different for the last waypoint compared to the others, or something like that :)

boldtrn · 2024-06-15T08:53:05Z

Thanks a lot for looking into this @michaelkirk. I would really appreciate if you could split this into two PRs.

IMHO, the RouteController change is very critical, so I'd appreciate that in a separate PR, ideally with some tests that make sure everything is working as expected.

Since we are not using the RouteMapViewController in our app, I am 100% sure about these changes and maybe @hactar might @Patrick-Kladek would also like to review these.

Patrick-Kladek · 2024-06-16T17:54:58Z

Regarding this, it would probably be a good idea to release this as 3.1 or 3.0.1 as this is needed for the old navigation. Once we release #54 this is no longer needed and would be the responsibility of the app to present such a screen.

boldtrn · 2024-06-17T07:02:58Z

Yes makes sense, so you are fine if we don't merge #54 today, first get this fix through, release a new version and thne merge #54?

Patrick-Kladek · 2024-06-17T13:42:59Z

@boldtrn yes, I don't mind if something takes a day longer to make it right. So lets try to get this done first.

Patrick-Kladek · 2024-06-18T18:47:00Z

@michaelkirk any update on this?

michaelkirk · 2024-06-18T19:04:23Z

IMHO, the RouteController change is very critical, so I'd appreciate that in a separate PR, ideally with some tests that make sure everything is working as expected.

The RouteController changes by themselves make sure that the EORVC is presented.

But separate bugs (also fixed in this PR) will cause your app to crash when that EORVC is presented.

The result of fixing the first part without fixing the second part means more apps will crash than without any changes — that's why I coupled them in this big PR.

Am I correct that you want me to make a PR that will fix only the presentation logic, resulting in more apps crashing until further changes to the EORVC are also merged?

boldtrn · 2024-06-18T20:45:17Z

Am I correct that you want me to make a PR that will fix only the presentation logic, resulting in more apps crashing until further changes to the EORVC are also merged?

My idea was to fix the bug in the EORVC first, as this bug seems to be concerning all users that use it?

The change in the RouteController could be independent, but doesn't have to be, I was just a bit concerned that I might block this too long as I really care about these few lines, as they are a crucial core of the navigation experience and need to work well :)

The previous logic would terminate early, at the penultimate step. I don't understand why you would ever want this, but perhaps as an explanation, the logic for audible instructions was reasonable. So provided you had audio instructions, the early visual banner instructions termination wasn't relevant. Note, there is some slop built into this via `updateRouteStepProgress` which advances to the next step when within the user is within `RouteControllerManeuverZoneRadius` (40m) ``` /** Radius in meters the user must enter to count as completing a step. One of two heuristics used to know when a user completes a step, see `RouteControllerMaximumAllowedDegreeOffsetForTurnCompletion`. */ public var RouteControllerManeuverZoneRadius: CLLocationDistance = 40 ```

boldtrn · 2024-06-20T07:59:11Z

MapboxCoreNavigation/RouteController.swift

@@ -425,7 +425,9 @@ extension RouteController: CLLocationManagerDelegate {
    func updateRouteLegProgress(for location: CLLocation) {
        let currentDestination = self.routeProgress.currentLeg.destination

-        guard self.routeProgress.currentLegProgress.remainingSteps.count <= 1 else { return }
+        guard self.routeProgress.currentLegProgress.remainingSteps.count == 0 else {


We should not do this :), can we use distance or time remaining? For this to become true, the arrival needs to be exactly triggered, which is especially an issue for multi leg routes.

Ah, sorry, I just saw your comment. To be honest, I would prefer if we could actually create another variable like the RouteControllerManeuverZoneRadius, but for leg progress distance.

RouteControllerManeuverZoneRadius is used 5 times in the code base, and it seems to be used pretty consistently to say:

How close do we need to be to a thing to be considered "at" that thing?

TBH, that seems exactly in line with how we're using it here. Why would we want it to be different ? Do you actually have this use case? What value would you choose for it?

Examples pulled from the code:

Are we "at" the beginning of a leg:

let isWithinDepartureStep = distanceToFirstCoordinateOnLeg < RouteControllerManeuverZoneRadius

Are we "at" the intersection:

if absoluteDistanceToIntersection <= RouteControllerManeuverZoneRadius {

Are we at/along the current step?

// If user is close to an intersection, the radius will be lower, so reroutes will fire easier. let radius = max(reroutingTolerance, RouteControllerManeuverZoneRadius)

Are we "at" the end of a step?

if userSnapToStepDistanceFromManeuver <= RouteControllerManeuverZoneRadius,

Are we not "at" the start of the route?

lastKnownUserAbsoluteDistance > RouteControllerManeuverZoneRadius

michaelkirk · 2024-06-20T22:02:36Z

I'm going to close this. #54 was a really wide change, so a simple rebase is not feasible.

michaelkirk marked this pull request as draft June 13, 2024 00:50

michaelkirk commented Jun 13, 2024

View reviewed changes

michaelkirk mentioned this pull request Jun 13, 2024

crash when route ends: [Storyboard] Unknown class _TtC40maplibre-navigation-ios_MapboxNavigation13RatingControl in Interface Builder file. #57

Closed

michaelkirk marked this pull request as ready for review June 13, 2024 20:34

michaelkirk marked this pull request as draft June 13, 2024 23:54

michaelkirk mentioned this pull request Jun 14, 2024

Feedback in EndOfRouteViewController goes nowhere. #62

Closed

michaelkirk force-pushed the mkirk/show-end-of-route branch 2 times, most recently from 18d451e to 7006eda Compare June 14, 2024 02:42

michaelkirk commented Jun 14, 2024

View reviewed changes

scripts/file_conversion.sh

@@ -1,4 +1,4 @@

#!/usr/bin/env/bash

#!/usr/bin/env bash

Copy link

Collaborator Author

michaelkirk Jun 14, 2024

Choose a reason for hiding this comment

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

typo?

michaelkirk commented Jun 14, 2024

View reviewed changes

michaelkirk force-pushed the mkirk/show-end-of-route branch 2 times, most recently from 46a289b to 25aeefc Compare June 14, 2024 03:08

michaelkirk commented Jun 14, 2024

View reviewed changes

michaelkirk marked this pull request as ready for review June 14, 2024 03:13

michaelkirk commented Jun 14, 2024

View reviewed changes

michaelkirk changed the title ~~Show end of route even if remainingVoiceInstructions is nil~~ Fixup EndOfRouteViewController Jun 14, 2024

michaelkirk mentioned this pull request Jun 14, 2024

Rename Mapbox packages to Maplibre #55

Open

michaelkirk added 6 commits June 14, 2024 16:31

Keyboard animation curve 7 doesnt match handled cases. Lets not cra…

176af1d

…sh though

none of this is needed to keep the view resting on the keyboard

d50e9a5

Remove "Route Feedback" and replace EORVC storyboard with programmati…

7ff076b

…c view

fix and re-run extract_localizable.sh

09c8fe9

michaelkirk force-pushed the mkirk/show-end-of-route branch from 25aeefc to 09c8fe9 Compare June 14, 2024 23:31

style tweaks for EORVC

73d1d87

boldtrn reviewed Jun 15, 2024

View reviewed changes

Patrick-Kladek mentioned this pull request Jun 20, 2024

Start & Stop Navigation in existing Map #54

Merged

5 tasks

boldtrn reviewed Jun 20, 2024

View reviewed changes

michaelkirk closed this Jun 20, 2024

This was referenced Jun 24, 2024

Fix crash in EndOfRouteViewController and restore its presentation #71

Merged

Fix: waypoint didArrive: is never called if audio instructions are not enabled. #72

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixup EndOfRouteViewController #58

Fixup EndOfRouteViewController #58

michaelkirk commented Jun 13, 2024 •

edited

Loading

michaelkirk Jun 13, 2024

michaelkirk commented Jun 13, 2024

michaelkirk commented Jun 14, 2024 •

edited

Loading

michaelkirk commented Jun 14, 2024

michaelkirk Jun 14, 2024

michaelkirk Jun 14, 2024

michaelkirk Jun 14, 2024

michaelkirk Jun 14, 2024

michaelkirk Jun 14, 2024 •

edited

Loading

michaelkirk Jun 14, 2024

michaelkirk Jun 14, 2024

michaelkirk Jun 14, 2024

boldtrn Jun 15, 2024

michaelkirk Jun 18, 2024

boldtrn Jun 18, 2024

michaelkirk Jun 18, 2024 •

edited

Loading

boldtrn Jun 20, 2024

boldtrn commented Jun 15, 2024

Patrick-Kladek commented Jun 16, 2024

boldtrn commented Jun 17, 2024

Patrick-Kladek commented Jun 17, 2024

Patrick-Kladek commented Jun 18, 2024

michaelkirk commented Jun 18, 2024

boldtrn commented Jun 18, 2024

boldtrn Jun 20, 2024

boldtrn Jun 20, 2024

michaelkirk Jun 20, 2024

michaelkirk commented Jun 20, 2024


		self.routeProgress.currentLegProgress.userHasArrivedAtWaypoint = true
		if let remainingVoiceInstructions = routeProgress.currentLegProgress.currentStepProgress.remainingSpokenInstructions {

Fixup EndOfRouteViewController #58

Fixup EndOfRouteViewController #58

Conversation

michaelkirk commented Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

michaelkirk commented Jun 13, 2024

michaelkirk commented Jun 14, 2024 • edited Loading

michaelkirk commented Jun 14, 2024

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

michaelkirk Jun 14, 2024 • 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

michaelkirk Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boldtrn commented Jun 15, 2024

Patrick-Kladek commented Jun 16, 2024

boldtrn commented Jun 17, 2024

Patrick-Kladek commented Jun 17, 2024

Patrick-Kladek commented Jun 18, 2024

michaelkirk commented Jun 18, 2024

boldtrn commented Jun 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Examples pulled from the code:

michaelkirk commented Jun 20, 2024

michaelkirk commented Jun 13, 2024 •

edited

Loading

michaelkirk commented Jun 14, 2024 •

edited

Loading

michaelkirk Jun 14, 2024 •

edited

Loading

michaelkirk Jun 18, 2024 •

edited

Loading