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

Android route overview #345

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

Android route overview #345

wants to merge 20 commits into from

Conversation

ianthetechie
Copy link
Contributor

@ianthetechie ianthetechie commented Nov 4, 2024

trim.95AB815E-C784-4239-B898-120176CE11C8.MOV

Closes #158

Copy link
Contributor Author

@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.

Few comments from the discussion but have a good path forward.

Copy link
Contributor Author

@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.

This should be ready for review now! I've updated the video attached to the PR description showing the updated UX.

data class VisualNavigationViewConfig(
var showMute: Boolean = false,
var showZoom: Boolean = false,
var buttonSize: DpSize = DpSize(56.dp, 56.dp)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was previously an informal constant that was copied everywhere. This at least gives it a place to live, and opens the path for button scaling enhancements if needed.

}
}

// NOTE: Some controls hidden when the camera is not following the user
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me know if my memory of this is incorrect, but I believe your request on the call was to hide the mute button but leave zoom (if present), progress, and instructions.

@@ -79,6 +79,8 @@ data class NavigationUiState(
currentStepRoadName = coreState.tripState.currentRoadName(),
remainingSteps = coreState.tripState.remainingSteps())
}

fun isNavigating(): Boolean = progress != null
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved this to the UI state class, since 1) it was always derived from this anyways, and 2) moving it here means we can pass only the UI state in some cases rather than a full view model.

Comment on lines +53 to 55
onMapReadyCallback: ((Style) -> Unit)? = null,
content: @Composable @MapLibreComposable ((NavigationUiState) -> Unit)? = null
) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another notable change here and throughout: we don't actually need to be passing around State<T> values directly and inspecting their values in most cases. Recomposition will already handle this at the relevant point, and this is a much cleaner API.

Comment on lines +80 to +81
onMapReadyCallback =
onMapReadyCallback ?: { if (isNavigating) camera.value = navigationCamera },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This changed to be a nullable parameter rather than a capturing closure for two reasons.

  1. The old implementation relied on direct view model access, which we are no longer doing for good reasons.
  2. It avoids capturing state in parameters in ways that may be non-obvious. This is cleaner IMO and will still recompose properly off uiState when needed.

Comment on lines +38 to +39
// Bottom padding must take the recenter button into account
val bottomPadding = (progressViewHeight + this.buttonSize.height.value + 50) * scale
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let it be known that I really don't like that we have to do this as a result of moving the button, further reducing the amount of space available for the map. I still think the UI would make more sense if we left the recenter button in the top end grid cell.

import com.stadiamaps.ferrostar.maplibreui.NavigationViewMetrics

@Composable
fun VisualNavigationViewConfig.cameraControlState(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. This is as good a place as any to drop this I think... I think an extension is better than a free function, but there's no place we colud put this that would remove a substantial number of parameters.
  2. Note that this can't live in the same file as the declaration in the composeui module, since almost everything about it is MapLibre specific.

@@ -36,9 +43,10 @@ import kotlinx.coroutines.flow.asStateFlow
fun LandscapeNavigationOverlayView(
modifier: Modifier,
camera: MutableState<MapViewCamera>,
navigationCamera: MapViewCamera = navigationMapViewCamera(),
navigationCamera: MapViewCamera,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe that making this a non-optional parameter was a net negative for code quality for not much benefit. It's way too easy to forget to pass it with this many layers, and in fact it was not actually passed down previously. I haven't changed the progressViewSize parameter in this PR, but I also strongly suspect that the same is true of that as well and we should require it at compile itme.

@@ -74,11 +93,17 @@ fun LandscapeNavigationOverlayView(
showMute = config.showMute,
isMuted = uiState.isMuted,
onClickMute = { viewModel.toggleMute() },
buttonSize = config.buttonSize,
cameraControlState =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's the (IMO fairly elegant) solution I ended up with. We can avoid 1) repeating a lot of code, and 2) leaking out the instruction view size (propagating more boilerplate) than if we moved it up a level (which was my first instinct. I'm pretty happy with this since it's minimal code, with all parameters readily available, and users of the overlay just get it "for free" (fewer composable parameters).

@ianthetechie ianthetechie marked this pull request as ready for review November 7, 2024 15:19
Copy link
Contributor Author

@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.

Ended up discovering some bugs in real world testing, which led me to realize the view model swopping antipattern in the demo app HAD to go ;)

Comment on lines +24 to +27
class DemoNavigationViewModel(
// This is a simple example, but these would typically be dependency injected
private val ferrostarCore: FerrostarCore = AppModule.ferrostarCore,
) : ViewModel(), NavigationViewModel {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have revamped the view model so it is no longer crap.

Comment on lines -126 to +116
viewModel.stopNavigation()
val vm = DemoNavigationViewModel()
viewModel = vm

vm.startLocationUpdates(locationProvider)
},
onTapExit = { viewModel.stopNavigation(stopLocationUpdates = false) },
userContent = { modifier ->
if (!viewModel.isNavigating()) {
if (!vmState.isNavigating()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These evil bits are gone!

Comment on lines +304 to +307
fun stopNavigation(stopLocationUpdates: Boolean = true) {
foregroundServiceManager?.stopService()
locationProvider.removeListener(this)
if (stopLocationUpdates) {
locationProvider.removeListener(this)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New argument that doesn't break source compatibility; lets us keep the location provider running, which makes life easier for single map view apps.

route: Route,
config: NavigationControllerConfig? = null
): DefaultNavigationViewModel {
fun startNavigation(route: Route, config: NavigationControllerConfig? = null) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Major change 🚨

The state flow was already exposed. And @Archdoog was telling me for some time that this was a bit of an antipattern 😅 So this resolves long-standing tech debt. We no longer return view models here, which actually encourages people to write their own to purpose if needed, and the "default" is still quite usable for modal activities.

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.

[Android] Route overview
1 participant