Skip to content

MapboxCarMapSurface observe map style#5853

Merged
korshaknn merged 1 commit intomainfrom
knn_observe_style
Jun 7, 2022
Merged

MapboxCarMapSurface observe map style#5853
korshaknn merged 1 commit intomainfrom
knn_observe_style

Conversation

@korshaknn
Copy link
Copy Markdown
Contributor

Description

observe map style when MapboxCarMapSurface is attached

Screenshots or Gifs

@korshaknn korshaknn added the improvement Improvement for an existing feature. label May 24, 2022
@korshaknn korshaknn requested review from Zayankovsky and kmadsen May 24, 2022 11:28
@korshaknn korshaknn requested a review from a team as a code owner May 24, 2022 11:28
@korshaknn korshaknn self-assigned this May 24, 2022
@korshaknn korshaknn force-pushed the knn_observe_style branch from 73a82e9 to 706cd2c Compare May 24, 2022 11:30
@codecov
Copy link
Copy Markdown

codecov Bot commented May 24, 2022

Codecov Report

Merging #5853 (ed945e3) into main (77ca26c) will increase coverage by 0.00%.
The diff coverage is n/a.

❗ Current head ed945e3 differs from pull request most recent head a7035c8. Consider uploading reports for the commit a7035c8 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##               main    #5853    +/-   ##
==========================================
  Coverage     67.55%   67.56%            
+ Complexity     3877     3834    -43     
==========================================
  Files           591      586     -5     
  Lines         23844    23621   -223     
  Branches       2747     2725    -22     
==========================================
- Hits          16108    15959   -149     
+ Misses         6709     6644    -65     
+ Partials       1027     1018     -9     
Impacted Files Coverage Δ
...gation/core/routerefresh/RouteRefreshController.kt 93.22% <0.00%> (-1.47%) ⬇️
...ava/com/mapbox/navigation/core/MapboxNavigation.kt 62.92% <0.00%> (-0.66%) ⬇️
...n/ui/maps/route/line/model/RouteLineDynamicData.kt 53.65% <0.00%> (-0.35%) ⬇️
...tion/ui/maps/route/line/api/MapboxRouteLineView.kt 95.91% <0.00%> (-0.07%) ⬇️
...ation/ui/maps/route/line/api/MapboxRouteLineApi.kt 81.05% <0.00%> (-0.06%) ⬇️
...ava/com/mapbox/navigation/dropin/NavigationView.kt 0.00% <0.00%> (ø)
...navigation/dropin/internal/extensions/ContextEx.kt 0.00% <0.00%> (ø)
...ils/internal/datastore/NavigationDataStoreOwner.kt
.../ui/voice/internal/impl/MapboxAudioGuidanceImpl.kt
...on/ui/maps/route/line/model/RouteLineTrimOffset.kt
... and 15 more

mapboxMap.getStyle { style ->
style.addPersistentStyleCustomLayer(
styleLoadedListener = mapboxCarMapSurface.handleStyleOnAttached {
it.addPersistentStyleCustomLayer(
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.

What if style is updated without detach - won't you try to add persistent style several times?

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.

is it an issue to add a persistent style custom layer multiple times?

another note is that, we want to delete this class and adopt the new map widgets #5793.

The solution here will be temporary, but the solution would translate to other persistent styles

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.

added a flag

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.

It won't crash but will return Expected.error from
Expected<String, com.mapbox.bindgen.None> addPersistentStyleCustomLayer(@NonNull String layerId, @NonNull CustomLayerHost layerHost, @Nullable LayerPosition layerPosition);.
It's not a big issue but I assume still better to fix to avoid possible issues.

@korshaknn korshaknn force-pushed the knn_observe_style branch 2 times, most recently from 25ceef9 to ed945e3 Compare May 25, 2022 10:50
@korshaknn
Copy link
Copy Markdown
Contributor Author

korshaknn commented May 25, 2022

@kmadsen btw, do we really need to observe style changes? can it be changed somehow at runtime? do we have any usecases?

it can't be added if we observe style dynamically depending on a head uint / phone settings

cc @Zayankovsky

@korshaknn korshaknn force-pushed the knn_observe_style branch 2 times, most recently from 50dfff5 to 686bdb2 Compare May 26, 2022 10:20
@korshaknn korshaknn force-pushed the knn_observe_style branch from 686bdb2 to b2cc801 Compare May 26, 2022 12:20
@korshaknn korshaknn force-pushed the knn_observe_style branch from b2cc801 to 32cdfa6 Compare May 27, 2022 07:28
@Guardiola31337
Copy link
Copy Markdown
Contributor

Noting that ktlint is failing 👀

> Task :libnavui-androidauto:ktlint
/root/code/libnavui-androidauto/src/main/java/com/******/androidauto/car/navigation/roadlabel/RoadLabelSurfaceLayer.kt:19:1: Unused import (no-unused-imports)

> Task :libnavui-androidauto:ktlint FAILED

FAILURE: Build failed with an exception.

@korshaknn korshaknn force-pushed the knn_observe_style branch 2 times, most recently from 7af179d to a1c4ee0 Compare June 2, 2022 15:24
@korshaknn
Copy link
Copy Markdown
Contributor Author

@kmadsen any concerns here?

import com.mapbox.maps.extension.androidauto.MapboxCarMapSurface
import com.mapbox.maps.plugin.delegates.listeners.OnStyleLoadedListener

internal fun MapboxCarMapSurface.handleStyleOnAttached(
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.

This is quite nice. Nice work!

Thinking about this more, and I think we can move it up to the maps-androidauto extension
https://github.com/mapbox/mapbox-maps-android/tree/main/extension-androidauto

Happy to merge this here, and then we can propose it to maps. And eventually delete it from here

@kmadsen
Copy link
Copy Markdown
Contributor

kmadsen commented Jun 2, 2022

The current road label is not changing when updating map styles (switch from day/night)

You can test day and night changes like this. This triggers a style change and anything designed to update day/night should update their color. Some components manage it internally (location puck for example, is updated internally.. but the night color is only in 1tap not in the sdk)

➜ mapbox-navigation-android git:(knn_observe_style) make car

day
night
day

Actual Expected
Screen Shot 2022-06-02 at 12 54 14 PM Screen Shot 2022-06-02 at 12 55 28 PM

@kmadsen
Copy link
Copy Markdown
Contributor

kmadsen commented Jun 2, 2022

The current road label is not showing up on the MainCarScreen anymore 🤔

Screen Shot 2022-06-02 at 9 20 08 AM

compassWidget.host,
layerPosition
)
styleLoadedListener = mapboxCarMapSurface.handleStyleOnAttached {
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.

This class does not do anything when the style changes. So let's use the getStyle extension here and remove the styleLoadedListener handle and persistentStyleAdded flag

Suggested change
styleLoadedListener = mapboxCarMapSurface.handleStyleOnAttached {
mapboxCarMapSurface.getStyle {

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.

can't use a sync getStyle extension, because style might be null when onAttached is called, so added a new
getStyleAsync extension

@korshaknn korshaknn force-pushed the knn_observe_style branch from a1c4ee0 to f2720a4 Compare June 6, 2022 13:39
@kmadsen
Copy link
Copy Markdown
Contributor

kmadsen commented Jun 6, 2022

Just pulled the branch and the day/night style is no longer updating

Screen Shot 2022-06-06 at 12 21 03 PM

@korshaknn
Copy link
Copy Markdown
Contributor Author

Just pulled the branch and the day/night style is no longer updating

@kmadsen I've tested this commands on 1Tap and they do nothing (tested with AA 0.2.0 and AA with current PR changes)
@Zayankovsky also tested it and had the same results

but style change works with AndroidAuto app settings, so I change it on my phone:

Screen.Recording.2022-06-07.at.12.31.30.online-video-cutter.com.mp4

@korshaknn korshaknn force-pushed the knn_observe_style branch from f2720a4 to a7035c8 Compare June 7, 2022 10:37
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.

Nice work! Video looks great #5853 (comment)

@korshaknn korshaknn merged commit 0da32be into main Jun 7, 2022
@korshaknn korshaknn deleted the knn_observe_style branch June 7, 2022 18:23
abhishek1508 pushed a commit that referenced this pull request Jun 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

improvement Improvement for an existing feature.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants