Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Implement annotation publisher on Android #324
Implement annotation publisher on Android #324
Changes from all commits
3ca1aa6
455a7c7
7c07648
cf14196
4c1d3e2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we either need to:
NavigationStateWrapper<AnnotationType>
which includes everything related to navigation (including annotation stuff).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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, and wanted to share some thoughts:
NavigationViewModel
now is typed as well, returning whateverAnnotationWrapper
is there.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 singleFlow
on our side. The first solution would allow usages that useFerrostarCore
directly without usingNavigationViewModel
to benefit from this by just mapping the FerrostarCore state to the wrapped state.Other thoughts:
NoAnnotationPublisher
that just sets them tonull
automagically. We can provide that here if so.DefaultAnnotationPublisher
(faster since it'd only parse the current annotation, as the PR does today), andDefaultFullRouteAnnotationPublisher
to allow choosing whether or not to also publish annotations for the entire route.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Took another look at this today and am torn between two different approaches and their implications:
Returning a
NavigationStateWrapper<AnnotationType>
from theDefaultNavigationViewModel
(instead of aNavigationUiState
directly as is the case today) has some cons:ViewModel
should return the view model ready for rendering, and this is just wrapping it with annotation stuff that cannot (directly) be rendered (well, minusspeed
)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
andDefaultNavigationViewModel
customizable, since the mapping ofNavigationState
plus annotations from the annotation publisher all happen inside of theDefaultNavigationViewModel
. I thought of having a lambda parameter to specify how to mapNavigationStateWrapper
into aNavigationUiState
, but that requires parameterized types to bubble up toFerrostarCore
, (due to thestartNavigation
method calls).I think it makes sense to do one of 2 things:
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 ofNavigationViewModel
(though they'll need to skipFerrostarCore.startNavigation()
call in favor of their own), or just rely onFerrostarCore.state
directly and do whatever they want with it.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 aNavigationUiState
. This would imply makingNavigationViewModel
a parameterized type, and would bubble toFerrostarCore
, due tostartNavigation()
needing to know what type to make without losing type info. in that case though, why even forceNavigationUiState
if we go with this, instead of aNavigationViewModel<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 AndroidViewModel
, 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 AndroidViewModel
s roll their own solution on top ofFerrostarCore
directly.Finally, should we consider moving
startNavigation
out ofFerrostarCore
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 theNavigationUiState
to have a field forspeed
which we'd wire in theferrostarCore.map
that we have there. Consequently marking this as Ready for Review but happy to revisit it accordingly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ofFerrostarCore
and a few other flows, coalescing into a single UI state.Can you elaborate on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What you did in #345 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ianthetechie after rebasing, am now am wondering whether my changes to
NavigationViewModel
should go intoDemoNavigationViewModel
instead, since no one is usingDefaultNavigationViewModel
anymore anyway? it seems confusing to me for both of these to be there as recommendations though?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this double still needed? I think this was initial pass note, but probably not relevant now that
Speed
exists.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was checking, on iOS there is:
should I comment them as such and leave it instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the comments.