Skip to content

use built-in widgets in android auto#6076

Merged
Zayankovsky merged 1 commit intomainfrom
vz-maps-widgets
Jul 21, 2022
Merged

use built-in widgets in android auto#6076
Zayankovsky merged 1 commit intomainfrom
vz-maps-widgets

Conversation

@Zayankovsky
Copy link
Copy Markdown
Contributor

@Zayankovsky Zayankovsky commented Jul 19, 2022

Description

Closes #5793.
I decided to go ahead with this ticket, because I noticed that this also fixes an issue in 1TAP.

@Zayankovsky Zayankovsky added skip changelog Should not be added into version changelog. Android Auto Bugs, improvements and feature requests on Android Auto. labels Jul 19, 2022
@Zayankovsky Zayankovsky requested a review from a team as a code owner July 19, 2022 18:18
@Zayankovsky Zayankovsky self-assigned this Jul 19, 2022
@Zayankovsky
Copy link
Copy Markdown
Contributor Author

For some reason all widgets disappear when you open and close feedback screen.

@Zayankovsky Zayankovsky marked this pull request as draft July 19, 2022 19:04
@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 19, 2022

Codecov Report

Merging #6076 (5b88547) into main (478e656) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##               main    #6076   +/-   ##
=========================================
  Coverage     68.05%   68.05%           
  Complexity     4086     4086           
=========================================
  Files           617      617           
  Lines         24972    24972           
  Branches       2951     2951           
=========================================
  Hits          16995    16995           
  Misses         6852     6852           
  Partials       1125     1125           

@Zayankovsky Zayankovsky removed the skip changelog Should not be added into version changelog. label Jul 20, 2022
@Zayankovsky
Copy link
Copy Markdown
Contributor Author

For some reason all widgets disappear when you open and close feedback screen.

Turned out the problem was caused by keeping and reusing the same SpeedLimitWidget instance, but we have to create a new one.

@Zayankovsky Zayankovsky marked this pull request as ready for review July 20, 2022 09:15
@Zayankovsky Zayankovsky requested a review from kmadsen July 20, 2022 16:40
Comment on lines +19 to +20
WidgetPosition(WidgetPosition.Horizontal.RIGHT, WidgetPosition.Vertical.BOTTOM),
marginX = 26f, marginY = 10f,
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.

Thinking about cleaning this up a little, considering this will be a more permanent class. What do you think about adding the logo widget to the constructor, and adding the default to a companion object?

We could remove private var logoWidget: LogoWidget? = null and make a default in the constructor.

class CarLogoSurfaceRenderer(
  val logoWidget: LogoWidget = defaultLogoWidget
) : MapboxCarMapObserver {

  companion object {
    val defaultLogoWidget = LogoWidget(...
  }
}

Copy link
Copy Markdown
Contributor

@kmadsen kmadsen Jul 20, 2022

Choose a reason for hiding this comment

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

Oh, it is constructed when onAttached is called so that idea is not great. Unless it has a Provider wrapper

Smaller change, would be to move the constants to a companion object so the magic numbers are a little more visible. These margin values are going to need to change in order to support different screen resolutions

class CarLogoSurfaceRenderer(
) : MapboxCarMapObserver {

  private companion object {
    private val defaultWidgetPosition = WidgetPosition(...
    private const val defaultMarginX = 26f
    private const val defaultMarginY = 10f
  }
}

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.

Not sure about the naming, default suggests that these values can be customized, however they aren't. I think we can get back to it, when we add support for more screen resolutions.

* The horizontal margin of the widget relative to the map.
*/
private val marginLeft: Float = 10f,
marginX: Float = 26f,
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 this specific to screen resolutions?

What happens when you run this on a different resolution mapbox/mapbox-navigation-android-examples#81 (comment)

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.

If there is an issue there, the issue would be existing. So if you test it please take some screenshots and post them on a new issue

Copy link
Copy Markdown
Contributor

@kmadsen kmadsen Jul 20, 2022

Choose a reason for hiding this comment

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

Turns out, testing other resolutions still takes a few steps. am posting the steps just because we're talking about it

  1. Update to a preview android studio https://developer.android.com/studio/preview/install-preview
  2. Update Android Auto Desktop Head Unit Emulator from Android Studio SDK downloads
  3. Configure desktop head unit https://developer.android.com/training/cars/testing#configure-dhu
  4. cd $(ANDROID_HOME)/extras/google/auto/ && ./desktop-head-unit -c config/default_1080p.ini

Copy link
Copy Markdown
Contributor Author

@Zayankovsky Zayankovsky Jul 20, 2022

Choose a reason for hiding this comment

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

By the way, I changed these margins, so that the widgets are better aligned with the rest of the buttons. Like in the screenshot below you can see that compass widget now has the same margin as the buttons on the right side.
Screenshot 2022-07-20 at 21 58 30
Also Maps examples use the same values: https://github.com/mapbox/mapbox-maps-android/blob/main/android-auto-app/src/main/java/com/mapbox/maps/testapp/auto/car/CarMapWidgets.kt.

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.

Awesome! Thanks for doing this 🪂

@Zayankovsky Zayankovsky enabled auto-merge (rebase) July 21, 2022 07:58
@Zayankovsky Zayankovsky merged commit 857b96e into main Jul 21, 2022
@Zayankovsky Zayankovsky deleted the vz-maps-widgets branch July 21, 2022 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Android Auto Bugs, improvements and feature requests on Android Auto.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Android Auto] Integrate maps widgets for logo compass

2 participants