Android route overview #345

ianthetechie · 2024-11-04T14:00:51Z

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

Closes #158

ianthetechie

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

...poseui/src/main/java/com/stadiamaps/ferrostar/composeui/config/VisualNavigationViewConfig.kt

.../src/main/java/com/stadiamaps/ferrostar/composeui/views/gridviews/NavigatingInnerGridView.kt

...ain/java/com/stadiamaps/ferrostar/maplibreui/views/overlays/PortraitNavigationOverlayView.kt

…l everywhere and can get more granular recompositions

ianthetechie

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

ianthetechie · 2024-11-07T14:40:38Z

...poseui/src/main/java/com/stadiamaps/ferrostar/composeui/config/VisualNavigationViewConfig.kt

+data class VisualNavigationViewConfig(
+    var showMute: Boolean = false,
+    var showZoom: Boolean = false,
+    var buttonSize: DpSize = DpSize(56.dp, 56.dp)


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.

ianthetechie · 2024-11-07T14:43:00Z

.../src/main/java/com/stadiamaps/ferrostar/composeui/views/gridviews/NavigatingInnerGridView.kt

+            }
+          }
+
+          // NOTE: Some controls hidden when the camera is not following the user


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.

This looks correct (as per matching inspired by Apple Maps):

ianthetechie · 2024-11-07T14:47:03Z

android/core/src/main/java/com/stadiamaps/ferrostar/core/NavigationViewModel.kt

@@ -79,6 +79,8 @@ data class NavigationUiState(
            currentStepRoadName = coreState.tripState.currentRoadName(),
            remainingSteps = coreState.tripState.remainingSteps())
  }
+
+  fun isNavigating(): Boolean = progress != null


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.

ianthetechie · 2024-11-07T14:49:55Z

android/maplibreui/src/main/java/com/stadiamaps/ferrostar/maplibreui/NavigationMapView.kt

+    onMapReadyCallback: ((Style) -> Unit)? = null,
+    content: @Composable @MapLibreComposable ((NavigationUiState) -> Unit)? = null
 ) {


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.

ianthetechie · 2024-11-07T14:52:09Z

android/maplibreui/src/main/java/com/stadiamaps/ferrostar/maplibreui/NavigationMapView.kt

+      onMapReadyCallback =
+          onMapReadyCallback ?: { if (isNavigating) camera.value = navigationCamera },


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

The old implementation relied on direct view model access, which we are no longer doing for good reasons.

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.

ianthetechie · 2024-11-07T15:01:53Z

...i/src/main/java/com/stadiamaps/ferrostar/maplibreui/extensions/VisualNavigationViewConfig.kt

+          // Bottom padding must take the recenter button into account
+          val bottomPadding = (progressViewHeight + this.buttonSize.height.value + 50) * scale


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.

Personally, I'd probably just ignore the button heights in the bounding box calculation. I think just filling the entire rectangle between the instructions and progress view with a reasonable padding probably succeeds in most cases. The chances of a button hiding important route visual are already low & a user can easily adjust the map by pan if need be.

ianthetechie · 2024-11-07T15:04:03Z

...i/src/main/java/com/stadiamaps/ferrostar/maplibreui/extensions/VisualNavigationViewConfig.kt

+import com.stadiamaps.ferrostar.maplibreui.NavigationViewMetrics
+
+@Composable
+fun VisualNavigationViewConfig.cameraControlState(


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.

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.

...in/java/com/stadiamaps/ferrostar/maplibreui/views/overlays/LandscapeNavigationOverlayView.kt

ianthetechie · 2024-11-07T15:08:56Z

...in/java/com/stadiamaps/ferrostar/maplibreui/views/overlays/LandscapeNavigationOverlayView.kt

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


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

…apshots

ianthetechie

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 ;)

ianthetechie · 2024-11-08T18:30:51Z

android/demo-app/src/main/java/com/stadiamaps/ferrostar/DemoNavigationViewModel.kt

+class DemoNavigationViewModel(
+    // This is a simple example, but these would typically be dependency injected
+    private val ferrostarCore: FerrostarCore = AppModule.ferrostarCore,
+) : ViewModel(), NavigationViewModel {


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

ianthetechie · 2024-11-08T18:31:08Z

android/demo-app/src/main/java/com/stadiamaps/ferrostar/DemoNavigationScene.kt

-        viewModel.stopNavigation()
-        val vm = DemoNavigationViewModel()
-        viewModel = vm
-
-        vm.startLocationUpdates(locationProvider)
-      },
+      onTapExit = { viewModel.stopNavigation(stopLocationUpdates = false) },
      userContent = { modifier ->
-        if (!viewModel.isNavigating()) {
+        if (!vmState.isNavigating()) {


These evil bits are gone!

ianthetechie · 2024-11-08T18:32:13Z

android/core/src/main/java/com/stadiamaps/ferrostar/core/FerrostarCore.kt

+  fun stopNavigation(stopLocationUpdates: Boolean = true) {
    foregroundServiceManager?.stopService()
-    locationProvider.removeListener(this)
+    if (stopLocationUpdates) {
+      locationProvider.removeListener(this)


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

ianthetechie · 2024-11-08T18:35:02Z

android/core/src/main/java/com/stadiamaps/ferrostar/core/FerrostarCore.kt

-      route: Route,
-      config: NavigationControllerConfig? = null
-  ): DefaultNavigationViewModel {
+  fun startNavigation(route: Route, config: NavigationControllerConfig? = null) {


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.

Archdoog · 2024-11-11T00:54:09Z

.../src/main/java/com/stadiamaps/ferrostar/composeui/views/gridviews/NavigatingInnerGridView.kt

+            }
+          }
+
+          // NOTE: Some controls hidden when the camera is not following the user


This looks correct (as per matching inspired by Apple Maps):

android/demo-app/src/main/java/com/stadiamaps/ferrostar/DemoNavigationScene.kt

Archdoog · 2024-11-11T01:17:29Z

...i/src/main/java/com/stadiamaps/ferrostar/maplibreui/extensions/VisualNavigationViewConfig.kt

+          // Bottom padding must take the recenter button into account
+          val bottomPadding = (progressViewHeight + this.buttonSize.height.value + 50) * scale


Personally, I'd probably just ignore the button heights in the bounding box calculation. I think just filling the entire rectangle between the instructions and progress view with a reasonable padding probably succeeds in most cases. The chances of a button hiding important route visual are already low & a user can easily adjust the map by pan if need be.

Initial checkpoint commit on Android route overview

7e77c8c

ianthetechie requested a review from Archdoog November 4, 2024 14:00

Apply automatic changes

ce94b79

ianthetechie commented Nov 5, 2024

View reviewed changes

ianthetechie added 13 commits November 6, 2024 00:20

Minor clean-up; version updates

794d2de

Merge branch 'main' into route-overview-android

5063fc7

Merge branch 'main' into route-overview-android

060e839

Cleanup: by delegation

6ab8f7e

State -> raw value

584bb74

Move isNavigating into state so we don't have to pass a full viewmode…

8318660

…l everywhere and can get more granular recompositions

Make onMapReadyCallback optional rather than a capturing default closure

bc096e3

Auto-adjust most insets

3e99c47

Support horizontal padding

dd73e99

Snapshots; hide mute button and move recenter to bottom leading corner

8fc0ce8

Refactor; the layering makes sense now!

cf15dd8

Undo experimental change

742a5ca

Minor tweaks; update macOS CI to Sonoma

cee5ffd

ianthetechie commented Nov 7, 2024

View reviewed changes

ianthetechie marked this pull request as ready for review November 7, 2024 15:19

ianthetechie and others added 4 commits November 8, 2024 00:31

Switch to iOS 18 simulator for snapshots; update nanopixel-perfect sn…

aee4461

…apshots

Ditch the view model swapping!

6819739

Apply automatic changes

c12f3c8

Remove old comments

6545310

ianthetechie commented Nov 8, 2024

View reviewed changes

Fix imports

06843fe

This was referenced Nov 9, 2024

Fix initializing of traversed variable in distance_along #353

Merged

Implement annotation publisher on Android #324

Merged

Allow stopping demo VM location updates

1d8d8db

Archdoog reviewed Nov 11, 2024

View reviewed changes

Move view model to AppModule to better reflect a typical DI use case

df63897

ianthetechie and others added 5 commits November 11, 2024 01:37

Apply automatic changes

5c03f41

Hide zoom controls as well when showing the recenter button

cc83bbd

Update snapshots

28dd2cc

Freshen docs

9cfff25

Release prep: bump version

98effcf

ianthetechie merged commit 0d3c088 into main Nov 11, 2024
14 checks passed

ianthetechie deleted the route-overview-android branch November 11, 2024 03:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Android route overview #345

Android route overview #345

ianthetechie commented Nov 4, 2024 •

edited

Loading

ianthetechie left a comment

ianthetechie left a comment

ianthetechie Nov 7, 2024

ianthetechie Nov 7, 2024

Archdoog Nov 11, 2024

ianthetechie Nov 7, 2024

ianthetechie Nov 7, 2024

ianthetechie Nov 7, 2024

ianthetechie Nov 7, 2024

Archdoog Nov 11, 2024

ianthetechie Nov 7, 2024

ianthetechie Nov 7, 2024

ianthetechie left a comment

ianthetechie Nov 8, 2024

ianthetechie Nov 8, 2024

ianthetechie Nov 8, 2024

ianthetechie Nov 8, 2024

Archdoog Nov 11, 2024

Archdoog Nov 11, 2024

		onMapReadyCallback =
		onMapReadyCallback ?: { if (isNavigating) camera.value = navigationCamera },

		// Bottom padding must take the recenter button into account
		val bottomPadding = (progressViewHeight + this.buttonSize.height.value + 50) * scale

Android route overview #345

Android route overview #345

Conversation

ianthetechie commented Nov 4, 2024 • edited Loading

ianthetechie left a comment

Choose a reason for hiding this comment

ianthetechie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianthetechie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianthetechie commented Nov 4, 2024 •

edited

Loading