Implement annotation publisher on Android

[ahmedre]

This patch introduces an AnnotationPublisher to publish custom annotations for Android. Fixes #316.

[ahmedre]

Opening this as a draft because it strays a bit from the iOS implementation. On Android, that implementation didn't feel right for a few reasons:

1. due to having separate StateFlows, it's entirely possible for the annotation state to be for a stale NavigationState, especially if the work for publishing annotations happens on a different dispatcher and/or takes time.
2. exposing AnnotationPublisher within FerrostarCore and being able to get a typed annotation from it would require making FerrostarCore accept a generic type, which is a bit unfortunate.
3. the NavigationViewModel is a more natural place to do this, since we'd read the states there and act on them accordingly. i.e. to add the speed, we'd just read the wrapped speed and use it. If we want to allow people to get the actual typed annotation out in the future, we could do this by making the view model generic, which is less of a hassle than doing the same for FerrostarCore.
4. the wrapper type model makes it easy to have a SimpleAnnotationParser that just parses the current item, or a CurrentStepAnnotationParser (just for the current step), or a WholeRouteAnnotationParser - and people can plug in whatever they want - or a single parser can be there with the computations for "current step" and "whole route" being lazily computed on demand.

Wanted to get some thoughts on this idea. Things left if we decide to move forward:

• test the parsing of the actual speed with value (the route i tested with has data that comes back as unknown, so i need to test in a place where speed limits are known).
• documentation
• tests

then, in additional PRs:

• wiring up the speed to the ui
• wiring up the annotation parsing for the current step and/or whole route
• replacing Moshi reflection with Moshi codegen and simplifying the parsing logic for the speed adapter

[ianthetechie]

I think @Archdoog is more qualified to give a deep technical review on this, but here some quick thoughts:

First, it totally makes sense that the Android implementation will necessarily differ. Your points sound valid to me.

If you need a route fixture, here's one: https://gist.github.com/ianthetechie/ff84527c3e90a775805aff42f422418. This is the actual route traveled by the iOS demo app.

[Archdoog]

I think this approach looks good given the differences on kotlin 👍.

Added a couple things. @ianthetechie you might also have an opinion on them.

[Archdoog]

I think we either need to:

1. Make this the full NavigationStateWrapper<AnnotationType> which includes everything related to navigation (including annotation stuff).
2. Remove val state: NavigationState and publish annotations separately to navigation state on the ViewModel(s).

This decision can be made here, then it'll be applied to the AnnotationPublishers

[ahmedre]

Agreed, and wanted to share some thoughts:

• the first solution would imply that NavigationViewModel now is typed as well, returning whatever AnnotationWrapper is there.
• the second solution could, in theory, cause issues of being out of sync (since you have 2 sources of truth).

I mentioned to Ian before that our current solution is to skip the NavigationViewModel entirely and just apply something similar to wrap the state with parsed annotations before emitting it in a single Flow on our side. The first solution would allow usages that use FerrostarCore directly without using NavigationViewModel to benefit from this by just mapping the FerrostarCore state to the wrapped state.

Other thoughts:

• performance for people who don't need annotations - they can use some sort of NoAnnotationPublisher that just sets them to null automagically. We can provide that here if so.
• publishing annotations for the entire route (as per the bonus part of Android Annotation Flow Publisher #316) - we can have DefaultAnnotationPublisher (faster since it'd only parse the current annotation, as the PR does today), and DefaultFullRouteAnnotationPublisher to allow choosing whether or not to also publish annotations for the entire route.

[ahmedre]

Took another look at this today and am torn between two different approaches and their implications:

Returning a NavigationStateWrapper<AnnotationType> from the DefaultNavigationViewModel (instead of a NavigationUiState directly as is the case today) has some cons:

• it's not intuitive - a ViewModel should return the view model ready for rendering, and this is just wrapping it with annotation stuff that cannot (directly) be rendered (well, minus speed)
• it encourages bad practice of business logic in the ui layer (if I have the annotations and need to do something to get a specific subset to get congestion, for example, I'll be tempted to do this in the ui, which is not good).

It seems like it'd be better to continue returning NavigationUiState, enriched with whatever annotation data from the parsed annotations that we'd like to use.

On the flip side, doing this makes it really difficult to make NavigationViewModel and DefaultNavigationViewModel customizable, since the mapping of NavigationState plus annotations from the annotation publisher all happen inside of the DefaultNavigationViewModel. I thought of having a lambda parameter to specify how to map NavigationStateWrapper into a NavigationUiState, but that requires parameterized types to bubble up to FerrostarCore, (due to the startNavigation method calls).

I think it makes sense to do one of 2 things:

1. make DefaultNavigationViewModel the opinionated / not customizable default. using this, you can't modify annotations beyond what we put in for the app demo to support the features we want to support (speed, lanes, traffic, etc). People who want to customize can either implement their own implementation of NavigationViewModel (though they'll need to skip FerrostarCore.startNavigation() call in favor of their own), or just rely on FerrostarCore.state directly and do whatever they want with it.

2. if we want to make it customizable, we can add a lambda parameter (with a default value) of how to map a NavigationStateWrapper<AnnotationType> into a NavigationUiState. This would imply making NavigationViewModel a parameterized type, and would bubble to FerrostarCore, due to startNavigation() needing to know what type to make without losing type info. in that case though, why even force NavigationUiState if we go with this, instead of a NavigationViewModel<ANNOTATION_TYPE, STATE_TYPE>?

In my opinion, i'd vote for option 1 because:

• DefaultNavigationViewModel is small - 158 lines including the state object itself (which is ~35 lines), plus imports (~20 lines) - meaning it's less than 100 lines.
• DefaultNavigationViewModel itself is an Android ViewModel, and while many use them, many opt to not using them. Making it super customizable would still let people who don't want to use Android ViewModels roll their own solution on top of FerrostarCore directly.

Finally, should we consider moving startNavigation out of FerrostarCore?

[ahmedre]

Side note - the implementation of this patch is the implementation of point 1 above. So when we come to add speed, we'd not change the signature of NavigationViewModel, and we'd only update the NavigationUiState to have a field for speed which we'd wire in the ferrostarCore.map that we have there. Consequently marking this as Ready for Review but happy to revisit it accordingly.

[ianthetechie]

Some relevant bits over here: #345 (review).

@Archdoog and I have been discussing that the current situation is a bit of a mess on Android. I am fairly convinced now that returning a viewmodel from startNavigation was a very bad idea (and it was thoroughly mine to be clear :P). Instead, every app will create their own view model anyways that reflects their way of looking at things.

That said, we should include a "default" view model so that it is still possible to drop Ferrostar into a minimalist "modal" activity triggered by an application which is not primarily map-centric. This ironically no longer applies to our demo app, but I think we should still include it.

So TL;DR I'm endorsing @ahmedre's first suggestion with the commits I made last night (not even noticing the thread haha). That both of us came to the same conclusion independently seems to indicate that it's the right one. My current code it #345 gets its state from the state property of FerrostarCore and a few other flows, coalescing into a single UI state.

Finally, should we consider moving startNavigation out of FerrostarCore?

Can you elaborate on this?

[ahmedre]

Can you elaborate on this?

What you did in #345 :)

[ahmedre]

@ianthetechie after rebasing, am now am wondering whether my changes to NavigationViewModel should go into DemoNavigationViewModel instead, since no one is using DefaultNavigationViewModel anymore anyway? it seems confusing to me for both of these to be there as recommendations though?

[ianthetechie]

I'll look again in the morning with a clearer head, but re: why both view models exist, the point of the default on is to be functional for apps that are just doing the "modal navigation" style of fetching a route somewhere in app code based on user action, and then switching to a navigation view that has no other purpose.

I think it still makes sense to have this, but I'm far from the resident expert on view models 😅 Does this make sense? Or do you think we sholud drop that one too?

cc @Archdoog

[ahmedre]

I didn't wire this up into the DemoNavigationViewModel for now, but can do so in the next PR where we connect the speed limit.

[Archdoog]

Is this double still needed? I think this was initial pass note, but probably not relevant now that Speed exists.

[ahmedre]

I was checking, on iOS there is:

    /// The speed limit of the segment.
    public let speedLimit: MaxSpeed?

    /// The estimated speed of travel for the segment, in meters per second.
    public let speed: Double?

should I comment them as such and leave it instead?

[ahmedre]

I added the comments.