SwiftUI Optimisation

[Patrick-Kladek]

Description

We are wrapping MapLibre-navigation in a UIViewControllerRepresentable. The updateUIViewController function is run multiple times leading to setting the same variable over and over.

Open Tasks

• prevent code from running again after setting the same property again
• only call navigationViewControllerDidFinishRouting when confirmed via button

Infos for Reviewer

[Patrick-Kladek]

I've verified that this is called regardless if showsEndOfRouteFeedback is set to true or false.

[Patrick-Kladek]

Resets LocationManager in case a SimulatedLocationManager was used

[michaelkirk]

Hi @Patrick-Kladek -
I just want to make sure that it stays focused on one issue, so that we can avoid having a far reaching PR that changes a lot of things at once. It makes it hard to test and evaluate. I see this is currently a draft. Why is that? What's the scope of this PR?

[Patrick-Kladek]

@michaelkirk Sure, this PR is about fixing small things we've missed in #54. It has 3 small changes, I hope you don't want me to submit 3 PRs 😅, that's why I included it here. I've already got the approval from my team, so it's ready to review here too.

[michaelkirk]

It has 3 small changes, I hope you don't want me to submit 3 PRs 😅

Well when you phrase it like that it makes me feel crazy 😆, but to be honest, it certainly would be my preference if they are separate! It makes it easier to talk about separate concerns.

For example, I have no concerns with:

b0c1397

But 4111671 seems like the wrong fix. Since location manger is per-route, not per-view-controller, it probably makes more sense to set it in startRoute rather than in init, right?

And as for 427721a, In my own application, I was calling hideEndOfRoute in my application logic, but I think you're right that we should probably include this in the library. However, I don't think it makes sense to call hideEndOfRoute unless showEndOfRoute was called, so probably here is the right place to include it?

maplibre-navigation-ios/MapboxNavigation/RouteMapViewController.swift

Line 450 in fa4d5f4

self.showEndOfRoute(duration: presentationDuration, onDismiss: onDismiss, presentationCompletion: presentationCompletion)

What do you think?

Some good questions by Michael, lets wait for Patrick K's response.

[Patrick-Kladek]

Good point that the LocationManager is per Route, I fixed that.

hideEndOfRoute has no side effects, it will simply remove the built-in feedback view if it's in the view hierarchy. If you want to provide your own, you do it differently anyway by listening to the delegate method navigationViewControllerDidFinishRouting().

I checked that the navigationViewControllerDidFinishRouting() is called regardless of the showsEndOfRouteFeedback property.

[michaelkirk]

Good point that the LocationManager is per Route, I fixed that.

👍

hideEndOfRoute has no side effects, it will simply remove the built-in feedback view if it's in the view hierarchy. If you want to provide your own, you do it differently anyway by listening to the delegate method navigationViewControllerDidFinishRouting().

Ok, I don't feel that strongly about this one.