Skip to content

[Drop-In UI] Moved LocationComponent outside Drop-In UI#6206

Merged
abhishek1508 merged 1 commit intomainfrom
tr-dropin_extract-component-location-puck
Sep 21, 2022
Merged

[Drop-In UI] Moved LocationComponent outside Drop-In UI#6206
abhishek1508 merged 1 commit intomainfrom
tr-dropin_extract-component-location-puck

Conversation

@tomaszrybakiewicz
Copy link
Copy Markdown
Contributor

@tomaszrybakiewicz tomaszrybakiewicz commented Aug 19, 2022

Part of https://github.com/mapbox/navigation-sdks/issues/1780

TL;DR

This PR adds a new ComponentInstaller that simplifies the configuration of a Map LocationPuck.

MapboxNavigationApp.installComponents(/* LifecycleOwner */ this) {
    locationPuck(binding.mapView)
}

Description

  • moved the LocationComponent to libnavui-maps module
  • moved LocationObserver logic from LocationStateController to LocationComponent
  • introduced ComponentInstaller.locationPuck(map)
  • updated QA Test App
  • regen libnavui-maps Metalava file

@tomaszrybakiewicz tomaszrybakiewicz added ⚠️ DO NOT MERGE PR should not be merged! UI Work related to visual components, Android Auto, Camera, 3D, voice, etc. labels Aug 19, 2022
@tomaszrybakiewicz tomaszrybakiewicz requested a review from a team as a code owner August 19, 2022 15:59
@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 19, 2022

Codecov Report

Merging #6206 (9bc938b) into main (59b01e7) will decrease coverage by 0.01%.
The diff coverage is n/a.

❗ Current head 9bc938b differs from pull request most recent head 13dfaed. Consider uploading reports for the commit 13dfaed to get more accurate results

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #6206      +/-   ##
============================================
- Coverage     68.87%   68.85%   -0.02%     
+ Complexity     4478     4453      -25     
============================================
  Files           678      668      -10     
  Lines         26830    26690     -140     
  Branches       3153     3147       -6     
============================================
- Hits          18479    18378     -101     
+ Misses         7142     7104      -38     
+ Partials       1209     1208       -1     
Impacted Files Coverage Δ
...om/mapbox/navigation/dropin/NavigationViewModel.kt 0.00% <0.00%> (-69.24%) ⬇️
...ternal/extensions/NavigationViewContextFlowable.kt 0.00% <0.00%> (-62.50%) ⬇️
.../mapbox/navigation/dropin/NavigationViewContext.kt 54.05% <0.00%> (-8.11%) ⬇️
...in/java/com/mapbox/navigation/dropin/ViewBinder.kt 90.00% <0.00%> (-4.29%) ⬇️
...igation/dropin/internal/MapboxNavigationViewApi.kt 91.54% <0.00%> (-1.41%) ⬇️
...mapbox/navigation/dropin/ViewStyleCustomization.kt 100.00% <0.00%> (ø)
...apbox/navigation/dropin/ViewBinderCustomization.kt 100.00% <0.00%> (ø)
...ion/dropin/component/marker/MapMarkersComponent.kt 100.00% <0.00%> (ø)
...n/dropin/binder/infopanel/InfoPanelHeaderBinder.kt 100.00% <0.00%> (ø)
...n/dropin/internal/extensions/ReloadingComponent.kt 63.63% <0.00%> (ø)
... and 14 more

@tomaszrybakiewicz tomaszrybakiewicz force-pushed the tr-dropin_extract-component-location-puck branch from d6db9b8 to 029fb87 Compare August 19, 2022 20:50
@tomaszrybakiewicz tomaszrybakiewicz changed the base branch from tr-dropin_extract-component-camera-mode to main August 19, 2022 20:50
@tomaszrybakiewicz tomaszrybakiewicz removed the ⚠️ DO NOT MERGE PR should not be merged! label Aug 19, 2022
@tomaszrybakiewicz tomaszrybakiewicz force-pushed the tr-dropin_extract-component-location-puck branch from 029fb87 to dffe76d Compare August 29, 2022 19:48
@abhishek1508
Copy link
Copy Markdown
Contributor

@kmadsen Can you give a brief look and see if this would work for AA, or if this needs some changes?

private val mapView: MapView,
private val locationPuck: LocationPuck,
private val locationProvider: NavigationLocationProvider,
private val enableLocationUpdates: Boolean = true
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These option parameters are not good for java backwards compatibility.

What do you think about moving the option as a function?

class LocationComponent(
...

  fun enableLocationUpdates(enable: Boolean)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Otherwise it would need to go in an Options.Builder

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I wouldn't worry about the "java backwards compatibility" (this is an experimental, internal component).
Sure, we can do the method instead.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wouldn't worry about the "java backwards compatibility

We're addressing java backwards compatiblity #6292. That is part of creating a stable api. If there is no intention to make this LocationComponent public, then sure we don't have to worry about it.

Some version of this is going to become a public api for AA

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Nvm. I found a better way to deal with this.
I've split LocationComponent into two separate components:

  • LocationComponent responsible for registering LocationObserver and updating NavigationLocationProvider
  • LocationPuckComponent for configuring and updating LocationPuck. This component depends on MapboxMap,LocationComponentPlugin, LocationPuck and NavigationLocationProvider, which are available in both AA and DropIn UI.

@tomaszrybakiewicz tomaszrybakiewicz force-pushed the tr-dropin_extract-component-location-puck branch from dffe76d to 86ce56d Compare September 12, 2022 19:06
@tomaszrybakiewicz
Copy link
Copy Markdown
Contributor Author

@kmadsen Thanks for reviewing. I've addressed your feedback. Can you give it another look and let me know if this will work for AA?

*/
@ExperimentalPreviewMapboxNavigationAPI
fun ComponentInstaller.locationPuck(
mapView: MapView,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We spoke of this some, but is there a way to remove the MapView parameter here?

The component installer could be used in android auto if it used something like a MapComponentInstaller 🤔

Copy link
Copy Markdown
Contributor Author

@tomaszrybakiewicz tomaszrybakiewicz Sep 14, 2022

Choose a reason for hiding this comment

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

We need MapView here in order to access LocationComponentPlugin (mapView.location) which is required to create LocationPuckComponent.

Remember, these installers are meant to work with MapView, not MapSurface.
For installers that work with AA, we need to create separate installers that will accept MapSurface as an argument.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I mean, why not use MapPluginProviderDelegate to get the LocationComponentPlugin

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Use MapPluginProviderDelegate instead of MapView? That wouldn't work. We wouldn't be able to acquire MapboxMap, norContext.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, would also prefer a simple MapView over requiring it all to be passed like this

fun ComponentInstaller.locationPuck(
    context: Context,
    mapboxMap: MapboxMap,
    plugins: MapPluginProviderDelegate,
    config: LocationPuckComponentConfig.() -> Unit = {}
): Installation {

Another approach could be to define a MapComponentInstaller

fun MapComponentInstaller.locationPuck(
    config: LocationPuckComponentConfig.() -> Unit = {}
): Installation {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Another approach could be to define a MapComponentInstaller

fun MapComponentInstaller.locationPuck(
    config: LocationPuckComponentConfig.() -> Unit = {}
): Installation {

We could introduce this once we implement dedicated component installers for MapView and SurfaceMap.

MapboxNavigationApp.installComponents(this) {
    map(mapView) {  // this: MapComponentInstaller
       locationPuck()
    }
    // and for surface map
    map(surfaceMap) { // this: SurfaceMapComponentInstaller
       locationPuck()
    }
}

@tomaszrybakiewicz tomaszrybakiewicz force-pushed the tr-dropin_extract-component-location-puck branch 3 times, most recently from fb22cb7 to 84d564a Compare September 16, 2022 13:13
Copy link
Copy Markdown
Contributor

@kmadsen kmadsen left a comment

Choose a reason for hiding this comment

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

LGTM

Split LocationComponent into two separate components. One responsible for observing MapboxNavigation location and updating NavigationLocationProvider, and second for configuring and updating Location Puck.
@tomaszrybakiewicz tomaszrybakiewicz force-pushed the tr-dropin_extract-component-location-puck branch from 84d564a to b82ffde Compare September 20, 2022 20:12
@abhishek1508 abhishek1508 merged commit 14b8460 into main Sep 21, 2022
@abhishek1508 abhishek1508 deleted the tr-dropin_extract-component-location-puck branch September 21, 2022 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

UI Work related to visual components, Android Auto, Camera, 3D, voice, etc.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants