mapbox-navigation-android icon indicating copy to clipboard operation
mapbox-navigation-android copied to clipboard

[Drop-In UI] Moved RecenterButtonComponent outside Drop-In UI

Open tomaszrybakiewicz opened this issue 3 years ago • 1 comments

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

TL;DR

This PR adds new ComponentInstaller for a component that re-centers the map to device location on button click.

MapboxNavigationApp.installComponents(/* LifecycleOwner */ this) {
    recenterButton(mapView, recenterButton)
}

Description

  • Moved RecenterButtonComponent from libnavui-dropin -> libnavui-maps
  • Introduced RecenterButtonComponentContract that defines RecenterButtonComponent controller logic.
  • Added default RecenterButtonComponentContract implementation (MapboxRecenterButtonComponentContract) that when attached to MapboxNavigation instance, registers itself as LocationObserver and recenters the map to last know location on button click.
  • Introduced ComponentInstaller.recenterButton(..)
  • Introduced ComponentInstaller.components(...) that allows installation of multiple components at once.
  • Updated MapboxNavigationObserverChain.removeAndDetach(...) to allow removal and detaching of multiple components.
  • Added new ComponentInstaller API example to qa-test-app.

tomaszrybakiewicz avatar Aug 12 '22 13:08 tomaszrybakiewicz

Codecov Report

Merging #6158 (78c3ddf) into main (65a5b7b) will increase coverage by 0.02%. The diff coverage is 53.42%.

:exclamation: Current head 78c3ddf differs from pull request most recent head 5da6ce6. Consider uploading reports for the commit 5da6ce6 to get more accurate results

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #6158      +/-   ##
============================================
+ Coverage     68.83%   68.86%   +0.02%     
+ Complexity     4262     4255       -7     
============================================
  Files           639      640       +1     
  Lines         25690    25685       -5     
  Branches       3008     3003       -5     
============================================
+ Hits          17684    17687       +3     
+ Misses         6863     6849      -14     
- Partials       1143     1149       +6     
Impacted Files Coverage Δ
...box/navigation/dropin/binder/ActionButtonBinder.kt 62.50% <0.00%> (ø)
...nt/recenter/RecenterButtonComponentContractImpl.kt 0.00% <0.00%> (ø)
...navigation/ui/maps/installer/ComponentInstaller.kt 19.35% <40.00%> (+19.35%) :arrow_up:
.../internal/extensions/MapboxNavigationObserverEx.kt 57.69% <66.66%> (+3.52%) :arrow_up:
...ion/ui/maps/internal/ui/RecenterButtonComponent.kt 81.81% <81.81%> (ø)
...navigation/ui/base/installer/ComponentInstaller.kt 100.00% <100.00%> (ø)
...igation/dropin/coordinator/InfoPanelCoordinator.kt 76.05% <0.00%> (-1.73%) :arrow_down:
...ui/maps/route/line/MapboxRouteLineApiExtensions.kt 81.11% <0.00%> (-1.12%) :arrow_down:
...mapbox/navigation/dropin/ViewStyleCustomization.kt 100.00% <0.00%> (ø)
...apbox/navigation/ui/maneuver/ComponentInstaller.kt 0.00% <0.00%> (ø)
... and 21 more

codecov[bot] avatar Aug 12 '22 14:08 codecov[bot]

In the example activities, we are showing recenter button always, but it doesn't work unless we start navigation. We should hide the recenter button and make it visible only when the navigation starts.

Are you referring to this particular QA App example or MapboxRecenterButtonComponentContract implementation? Since applicaiton creates and styles the button, shouldn't it be responsible for its visibility?

tomaszrybakiewicz avatar Aug 16 '22 16:08 tomaszrybakiewicz

In the example activities, we are showing recenter button always, but it doesn't work unless we start navigation. We should hide the recenter button and make it visible only when the navigation starts.

Are you referring to this particular QA App example or MapboxRecenterButtonComponentContract implementation? Since applicaiton creates and styles the button, shouldn't it be responsible for its visibility?

I am referring to the 2 example activities that are modified in this PR. It's kind of annoying that the recenter button is there outside active navigation and it doesn't work. We should hide the button when the activity is started and show it when click on startNavigation

abhishek1508 avatar Aug 16 '22 17:08 abhishek1508

In the example activities, we are showing recenter button always, but it doesn't work unless we start navigation. We should hide the recenter button and make it visible only when the navigation starts.

Are you referring to this particular QA App example or MapboxRecenterButtonComponentContract implementation? Since applicaiton creates and styles the button, shouldn't it be responsible for its visibility?

I am referring to the 2 example activities that are modified in this PR. It's kind of annoying that the recenter button is there outside active navigation and it doesn't work. We should hide the button when the activity is started and show it when click on startNavigation

Done. Let me know if anything else needs changing.

tomaszrybakiewicz avatar Aug 16 '22 18:08 tomaszrybakiewicz

Looking at this further

MapboxNavigationApp.installComponents(this) {
    audioGuidanceButton(binding.soundButton)
    routeLine(binding.mapView)
    routeArrow(binding.mapView)
    recenterButton(binding.mapView, binding.recenterButton)

    // custom components
    component(DropInStartReplayButton(binding.startNavigation))

the problem is that we are starting trip session only when clicked on start navigation. Since trip session doesn't start when activity is started, clicking recenter doesn't work. While hiding recenter initially and showing only on active navigation works, this is kind of a hack. The correct fix should actually modify the code above to start the trip session right away when the activity starts.

This is out of the scope of this PR and should be followed up in a different one. @tomaszrybakiewicz let's revert the last commit as this is not the correct solution for the problem I mentioned here. Once the last commit is reverted, I will ✅ this PR as rest everything else looks good.

abhishek1508 avatar Aug 17 '22 01:08 abhishek1508

This is out of the scope of this PR and should be followed up in a different one. @tomaszrybakiewicz let's revert the last commit as this is not the correct solution for the problem I mentioned here. Once the last commit is reverted, I will ✅ this PR as rest everything else looks good.

@abhishek1508 Done

tomaszrybakiewicz avatar Aug 17 '22 14:08 tomaszrybakiewicz