Fixup EndOfRouteViewController

michaelkirk commented 3 months ago

~~DO NOT MERGE: This currently exposes a crashes due to another issue: see https://github.com/maplibre/maplibre-navigation-ios/issues/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 commented 3 months ago

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 commented 3 months ago

I just keep going down the rabbit hole here. 🐇

Marking as draft again until https://github.com/maplibre/maplibre-navigation-ios/issues/62 is resolved.

michaelkirk commented 3 months ago

I think I've arrived at something worthwhile.

Ultimately I've:

fixed the logic that was preventing the EORVC from presenting #58
fixed the crash that occurred when the EORVC finally was presented #57
fixed the crash that happened when you tried to interact with the feedback text field (I never opened an issue, but it's https://github.com/maplibre/maplibre-navigation-ios/pull/58/commits/2bf7799f7a0c356cb57039b84f2781fddbc3b931)
fixed a layout bug with the keyboard animation: (I never opened an issue: https://github.com/maplibre/maplibre-navigation-ios/pull/58/commits/140236852c4807d71f0b5c61d54d3858eec58cc5)
removed all the feedback fields per #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 #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:

boldtrn commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

@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 commented 3 months ago

@michaelkirk any update on this?

michaelkirk commented 3 months ago

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 commented 3 months ago

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 :)

michaelkirk commented 3 months ago

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

maplibre / maplibre-navigation-ios

Fixup EndOfRouteViewController #58