Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix/android/view builder and speed json #360

Merged
merged 12 commits into from
Nov 19, 2024

Conversation

Archdoog
Copy link
Collaborator

  • Converts Demo's ViewModel to extend DefaultNavigationViewModel
  • Fixes "speed" vs "value" key in SpeedUnit
  • Internalizes view builder params so that you can't accidentally try to customize them.
  • Adds a public get for custom view (used in maplibreui).

@@ -18,7 +18,7 @@ class SpeedSerializationAdapter : JsonAdapter<Speed>() {
is Speed.NoLimit -> writer.name("none").value(true)
is Speed.Unknown -> writer.name("unknown").value(true)
is Speed.Value ->
writer.name("value").value(speed.value).name("unit").value(speed.unit.text)
writer.name("speed").value(speed.value).name("unit").value(speed.unit.text)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ahmedre I believe the actual key should be "speed" when there's a value. E.g.

{
  "speed": 56,
  "unit": "km/h"
},

Let me know if there are additional cases. Definitely a bit of an obnoxious json object 😄.

Also, do we want to try catch the annotations parsing? On iOS we just convert a json parsing error to nil/null for annotations and an onError callback so that developers can log the failure. We figured better to let navigation succeed even in the case of a degraded/malformed annotations. See

do {
return try decoder.decode(Annotation.self, from: data)
} catch {
onError(error)
return nil
}

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, you are right - my mistake! agree re: try/catching the parsing with an error callback 👍

@@ -24,7 +25,8 @@ import uniffi.ferrostar.UserLocation
class DemoNavigationViewModel(
// This is a simple example, but these would typically be dependency injected
private val ferrostarCore: FerrostarCore = AppModule.ferrostarCore,
) : ViewModel(), LocationUpdateListener, NavigationViewModel {
annotationPublisher: AnnotationPublisher<*> = valhallaExtendedOSRMAnnotationPublisher()
) : DefaultNavigationViewModel(ferrostarCore, annotationPublisher), LocationUpdateListener {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ianthetechie now that this view model uses the DefaultNavigationViewModel. We should be able to refactor it to use the existing uiState from ferrostar and implement its own additional state for things like route loading, etc. Just to highlight how easy it is to customize without interrupting the default navigation behavior.

Copy link
Contributor

@ianthetechie ianthetechie left a comment

Choose a reason for hiding this comment

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

Looks good to me but I'll wait for @ahmedre to comment since he may have an opinion based on what they're building 👍

@ahmedre
Copy link
Contributor

ahmedre commented Nov 15, 2024

On the DemoNavigationViewModel usage of DefaultNavigationViewModel - I think it is a step forward, but I think it is not as flexible as we have with just using FerrostarCore directly, because in this case, for example, both need to emit a NavigationUiState object.

We can swap NavigationUiState for a sealed class to solve this problem specifically for the demo use case of additional states like "not navigating" (which is what we also did in our implementation, but using FerrostarCore directly), but if we want to add lane support or speed limit support later, we'll need to either add these to NavigationUiState for everyone, or have our own ui state, or have two flows (in which case are we sure they're in sync, or would someone just need to combine them together to get a combination).

If we want to go this route, should we consider making NavigationUiState an interface with some required fields and let the return type be any instance? The downside of this approach though is that people would need an instance of check, and/or we'd need to do something with generics to avoid that. Alternatively, should we consider a single ViewModel for the demo app demoing the types of things you can do using FerrostarCore - it'd be opinionated and you could use it or use the formula from the ViewModel for building your own?

) : ViewModel(), NavigationViewModel {

private val muteState: StateFlow<Boolean?> =
ferrostarCore.spokenInstructionObserver?.muteState ?: MutableStateFlow(null)

override val uiState =
override val navigationUiState =
Copy link
Collaborator Author

@Archdoog Archdoog Nov 19, 2024

Choose a reason for hiding this comment

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

@ahmedre based on your comment, I've renamed this to navigationUiState. My thought was most of the time a developer will just pass navigationUiState into whatever NavigationView they're using from ferrostar. If they want additional custom ui state, they now have access to common name and can even merge in navigationUiState into their custom state. A custom view model may look something like this:

class NavViewModel(
  val ferrostarCore: FerrostarCore,
  val annotationPublisher: AnnotationPublisher<ValhallaOSRMExtendedAnnotation>
) : DefaultNavigationViewModel(ferrostarCore, annotationPublisher) {

  // your own custom uiState can also be added to use alongside or merged with the navigationUiState.

  val speed = navigationUiState
    .map { navigationUiState ->
      val metersPerSecond = navigationUiState.location?.speed?.value
      metersPerSecond?.let {
        return@map MeasurementSpeed(it, SpeedUnit.MetersPerSecond)
      }
      return@map null
    }

  fun addStop(latitude: Double, longitude: Double, name: String) {
    viewModelScope.launch {
      val userLocation = uiState.value.location ?: return@launch

      val nextWaypoint = Waypoint(
        coordinate = GeographicCoordinate(latitude, longitude),
        kind = WaypointKind.BREAK
      )
      val routes = ferrostarCore.getRoutes(userLocation, listOf(nextWaypoint))
      ferrostarCore.startNavigation(routes.first())
    }
  }
}

if (json != null) {
adapter.fromJson(json)
} else {
try {
Copy link
Collaborator Author

@Archdoog Archdoog Nov 19, 2024

Choose a reason for hiding this comment

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

We now try catch and pass null when the serialization fails.

@Archdoog Archdoog requested a review from ahmedre November 19, 2024 01:24
@ianthetechie
Copy link
Contributor

This looks good to me; going to merge now but happy to discuss other ways to make things clearer on today's call @ahmedre

@ianthetechie ianthetechie merged commit e1b6235 into main Nov 19, 2024
14 checks passed
@ianthetechie ianthetechie deleted the fix/android/view-builder-and-speed-json branch November 19, 2024 02:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants