-
Notifications
You must be signed in to change notification settings - Fork 21
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
Autocomplete search for Android Demo App #295
Conversation
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.
Looking pretty sweet 👍. A handful of deeper comments we probably need to discuss and get sorted pretty soon. Though we can merge this before we solve them, as this PR definitely improves things from main.
android/core/src/main/java/com/stadiamaps/ferrostar/core/Location.kt
Outdated
Show resolved
Hide resolved
fun isNavigating(): Boolean = uiState.value.progress != null | ||
} | ||
|
||
class IdleNavigationViewModel(locationProvider: LocationProvider) : |
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.
This behavior probably needs to go directly into the DefaultNavigationViewModel
. Since our android implementation is still a little incorrect. In practice most architectures should only inject a single view model from their AppModule into their navigation scene/view.
@@ -65,11 +69,12 @@ object AppModule { | |||
FerrostarForegroundServiceManager(appContext, DefaultForegroundNotificationBuilder(appContext)) | |||
} | |||
|
|||
// TODO: This is hard-coded for golf cart routing; change to something else before merging |
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.
Probably also "make this configurable" 😄.
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.
ha! Yeah, I'm open to that if we can do it without too much work. There's a fine line between a demo app and recreating Google Maps ;)
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.
We can throw in a segmented button here with walking, cycling, and driving and call it a day: https://developer.android.com/reference/kotlin/androidx/compose/material3/package-summary#SingleChoiceSegmentedButtonRow(androidx.compose.ui.Modifier,androidx.compose.ui.unit.Dp,kotlin.Function1)
android/demo-app/src/main/java/com/stadiamaps/ferrostar/DemoNavigationScene.kt
Outdated
Show resolved
Hide resolved
scope.launch(Dispatchers.IO) { | ||
// TODO: Fail gracefully | ||
val routes = | ||
AppModule.ferrostarCore.getRoutes( | ||
loc, | ||
listOf( | ||
Waypoint( | ||
coordinate = | ||
GeographicCoordinate( | ||
center.latitude, center.longitude), | ||
kind = WaypointKind.BREAK), | ||
)) | ||
|
||
val route = routes.first() | ||
viewModel = AppModule.ferrostarCore.startNavigation(route = route) | ||
|
||
if (locationProvider is SimulatedLocationProvider) { | ||
locationProvider.setSimulatedRoute(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.
With regards to view models, this looks like a prime example of functionality that should exist in one.
I wonder if we use this as an excuse (or put a TODO) to try and extend our default NavigationViewModel
behaviors. It might push us to figure out a pattern that makes the DefaultNavigationViewModel
extendable and overridable.
Happy to put this in a new ticket, but this composable is getting a lot of view model-like behavior in it and highlights something many developers will probably want. That being a default NavigationViewModel with app specific customizations alongside it. Currently, my app is using a second app specific view model alongside the DefaultNavigationViewModel
to work around this issue, not ideal but works.
android/maplibreui/src/main/java/com/stadiamaps/ferrostar/maplibreui/NavigationMapView.kt
Show resolved
Hide resolved
android/maplibreui/src/main/java/com/stadiamaps/ferrostar/maplibreui/NavigationMapView.kt
Show resolved
Hide resolved
android/demo-app/src/main/java/com/stadiamaps/ferrostar/DemoNavigationScene.kt
Outdated
Show resolved
Hide resolved
// AppModule.locationProvider.lastLocation = initialSimulatedLocation | ||
// AppModule.locationProvider.warpFactor = 2u |
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.
The TODO here is to enable switching location providers.
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.
Let's do another segmented control ;)
...rc/main/java/com/stadiamaps/ferrostar/maplibreui/views/DynamicallyOrientingNavigationView.kt
Outdated
Show resolved
Hide resolved
Co-authored-by: Jacob Fielding <[email protected]>
Discussed on today's call. We will merge with some minor cleanup and changing the default profile to something more common than golf carts for now and keep improving things. Stuff we still want to fix in the short term:
We agreed it's best to merge early though since this definitely improves things a lot + lets devs test :) |
* Support arbitrary Valhalla options (formerly we only merged costing options) * Fix argument * Improve docs of some convenience initializers * Align Android with iOS costing options * swiftformat * Don't forget the web ;) * Add Android-specific README * Set up Android to use API keys properly via local.properties * Fix CI * Fix subtle bug where Android location providers never updated lastLocation * Rename U-Turn images to match how Kotlin stringifies the maneuvers * Allow the fused location provider to report an initial fix faster * First pass at an actually usable demo app * Minor build script improvement * Checkpoint commit phase 1 * Checkpoint commit phase 2 highlighting some bad stuff to clean up * Add docs on the StaticLocationEngine * Fix formatting + doc test * Fix typo in Android readme Co-authored-by: Jacob Fielding <[email protected]> * Kotlin let closure idiom * ktfmtFormat + auto-commit * Remove extraneous env? * Try switching to a mix of Apple Silicon Ubuntu runners for Android * Fix runner permissions (I think) * Apply automatic changes * Address open thread re: iOS uturn image name * Add TODO docs * Fix the navigation camera (re-)application logic * Cleanup * Remove old comment * Apply automatic changes --------- Co-authored-by: Jacob Fielding <[email protected]> Co-authored-by: ianthetechie <[email protected]>
* Support for visual sub-maneuvers * Address PR comments * Added and updated tests * Update release version * Fix a typo * Update _typos.toml to ignore extended response * Update common/ferrostar/src/routing_adapters/osrm/models.rs Co-authored-by: Ian Wagner <[email protected]> * Add Prettier (#304) * Update ferrostar version * Add prettier * Run Prettier * Update guide * Rename format to format:fix, add format:check script * Add format:check to GitHub workflow * Autocomplete search for Android Demo App (#295) * Support arbitrary Valhalla options (formerly we only merged costing options) * Fix argument * Improve docs of some convenience initializers * Align Android with iOS costing options * swiftformat * Don't forget the web ;) * Add Android-specific README * Set up Android to use API keys properly via local.properties * Fix CI * Fix subtle bug where Android location providers never updated lastLocation * Rename U-Turn images to match how Kotlin stringifies the maneuvers * Allow the fused location provider to report an initial fix faster * First pass at an actually usable demo app * Minor build script improvement * Checkpoint commit phase 1 * Checkpoint commit phase 2 highlighting some bad stuff to clean up * Add docs on the StaticLocationEngine * Fix formatting + doc test * Fix typo in Android readme Co-authored-by: Jacob Fielding <[email protected]> * Kotlin let closure idiom * ktfmtFormat + auto-commit * Remove extraneous env? * Try switching to a mix of Apple Silicon Ubuntu runners for Android * Fix runner permissions (I think) * Apply automatic changes * Address open thread re: iOS uturn image name * Add TODO docs * Fix the navigation camera (re-)application logic * Cleanup * Remove old comment * Apply automatic changes --------- Co-authored-by: Jacob Fielding <[email protected]> Co-authored-by: ianthetechie <[email protected]> * Fix ManeuverImage * Fix ManeuverImage again * Fix newline ending --------- Co-authored-by: Ian Wagner <[email protected]> Co-authored-by: Mike Hays <[email protected]> Co-authored-by: Ian Wagner <[email protected]> Co-authored-by: Jacob Fielding <[email protected]> Co-authored-by: ianthetechie <[email protected]>
This makes the Android demo app into something more like a "normal" navigation experience with a single map view for everything and an autocomplete overlay. This involved... a few things ;)
Usage as a non-nav map
In the process, I had to address most of the same issues on Android that @hactar ran into with using this as a non-navigation map in #275. I tried a few ideas and ended up with a fairly similar approach (after accounting for Kotlin + Jetpack idioms) as that PR. Since I wasn't intending to do that by any means, I'm now pretty sure that the iOS PR is also going the right direction, so I'll figure out what else we need to do to get that over the line.
NOTE: I have not yet deeply thought through the Jetpack approach to super deep customization like we do with chainable modifiers in SwiftUI, but I think this PR gets Android most of the way there, and it started from a significantly worse state than iOS.
Outstanding tasks
Misc
While doing this, I realized that we can probably move the grid stuff to the compose playground (same thing for SwiftUI). This seems to be generally useful and I think we can move this outside Ferrostar and build on top of it with our "defaults" since it's all composable :)