Repository navigation
fix(widgets): preserve fractional scale bar distances - #1045
Conversation
dkhawk
left a comment
dkhawk
left a comment
There was a problem hiding this comment.
Thank you so much for this contribution, @premsinghsodha7! We really appreciate the care you put into fixing the scale bar truncation issue (#233), adding unit and rendering tests, and catching the swapped x/y screen coordinate bug (Point(0, widthInPixels) -> Point(widthInPixels, 0)) in ScaleBarTests.kt!
We are actively working on making android-maps-compose Kotlin Multiplatform (KMP) friendly, so our main feedback is around avoiding java.* imports (java.text.NumberFormat, java.util.Locale, java.io.File) in favor of pure Kotlin stdlib / kotlin.math, along with a few small layout and test cleanups.
I've left a few inline comments with verified code suggestions below. Thanks again for helping improve the library, and please let us know if you have any questions!
(Note: This review and its code suggestions were prepared with the assistance of AI and verified locally against the test suite.)
| // Here, we determine the appropriate units (meters/kilometers and feet/miles) | ||
| // based on the calculated distance in meters. |
There was a problem hiding this comment.
Nit: Now that the unit selection logic has been extracted into formatImperialDistance and formatMetricDistance, this comment (// Here, we determine the appropriate units...) is slightly orphaned right above the ScaleText calls. Feel free to either remove it or move it into KDoc on the new helper functions!
There was a problem hiding this comment.
Removed the orphaned unit-selection comment. The formatting helpers now own that logic.
| } else { | ||
| "$meters m" | ||
| } | ||
|
|
||
| internal fun formatImperialDistance(meters: Int): String { | ||
| val feet = meters.toDouble().toFeet() | ||
| return if (feet >= FEET_IN_MILE) { | ||
| "${formatScaleDistance(feet.toMiles())} mi" | ||
| } else { | ||
| "${feet.toInt()} ft" | ||
| } | ||
| } | ||
|
|
||
| private fun formatScaleDistance(distance: Double): String = NumberFormat.getNumberInstance().apply { | ||
| maximumFractionDigits = 2 | ||
| isGroupingUsed = false | ||
| }.format(distance) | ||
|
|
||
| /** |
There was a problem hiding this comment.
Since we are working toward making this codebase Kotlin Multiplatform (KMP) friendly, we'd love to avoid introducing java.text.NumberFormat (and java.* imports in general) into :maps-compose-widgets, which currently has zero java.* imports.
Also, when zoomed far out (e.g., continental/world zoom levels where the bar spans hundreds or thousands of kilometers/miles), formatting with 2 decimal places produces long labels like "3456.78 km" (10 characters) that can wrap or clip inside the default 65.dp width. For distances >= 10 km/mi, rounding to a whole number keeps the label compact and readable.
We can handle both cleanly in pure Kotlin using kotlin.math.roundToInt with zero JVM dependencies:
import kotlin.math.roundToInt
internal fun formatMetricDistance(meters: Int): String = if (meters >= METERS_IN_KILOMETER) {
"${formatScaleDistance(meters / METERS_IN_KILOMETER)} km"
} else {
"$meters m"
}
internal fun formatImperialDistance(meters: Int): String {
val feet = meters.toDouble().toFeet()
return if (feet >= FEET_IN_MILE) {
"${formatScaleDistance(feet.toMiles())} mi"
} else {
"${feet.roundToInt()} ft"
}
}
private fun formatScaleDistance(distance: Double): String {
if (distance >= 10.0) {
return distance.roundToInt().toString()
}
val scaled = (distance * 100.0).roundToInt()
val whole = scaled / 100
val remainder = scaled % 100
return when {
remainder == 0 -> "$whole"
remainder % 10 == 0 -> "$whole.${remainder / 10}"
else -> "$whole.${remainder.toString().padStart(2, '0')}"
}
}There was a problem hiding this comment.
This now uses pure Kotlin math and strings, with no java.* imports in the widget module. Values of 10 km/mi and above round to whole units; smaller values use one decimal after the precision follow-up.
| package com.google.maps.android.compose.widgets | ||
|
|
||
| import org.junit.After | ||
| import org.junit.Assert.assertEquals | ||
| import org.junit.Before | ||
| import org.junit.Test | ||
| import java.util.Locale | ||
|
|
||
| internal class ScaleBarDistanceTest { | ||
| private val originalLocale = Locale.getDefault() | ||
|
|
||
| @Before | ||
| fun setUp() { | ||
| Locale.setDefault(Locale.US) | ||
| } | ||
|
|
||
| @After | ||
| fun tearDown() { | ||
| Locale.setDefault(originalLocale) | ||
| } | ||
|
|
||
| @Test | ||
| fun metricDistancePreservesFractionalKilometers() { | ||
| assertEquals("1.9 km", formatMetricDistance(1900)) | ||
| assertEquals("1.01 km", formatMetricDistance(1010)) | ||
| } | ||
|
|
||
| @Test | ||
| fun imperialDistancePreservesFractionalMiles() { | ||
| assertEquals("1.24 mi", formatImperialDistance(2000)) | ||
| assertEquals("1.99 mi", formatImperialDistance(3200)) | ||
| } | ||
|
|
||
| @Test | ||
| fun metricDistanceUsesKilometersAtUnitBoundary() { | ||
| assertEquals("999 m", formatMetricDistance(999)) | ||
| assertEquals("1 km", formatMetricDistance(1000)) | ||
| assertEquals("2 km", formatMetricDistance(2000)) | ||
| } | ||
|
|
||
| @Test | ||
| fun smallDistancesKeepWholeMetersAndFeet() { | ||
| assertEquals("0 m", formatMetricDistance(0)) | ||
| assertEquals("0 ft", formatImperialDistance(0)) | ||
| assertEquals("100 m", formatMetricDistance(100)) | ||
| assertEquals("328 ft", formatImperialDistance(100)) | ||
| } | ||
|
|
||
| @Test | ||
| fun fractionalDistancesUseLocaleDecimalSeparator() { | ||
| Locale.setDefault(Locale.GERMANY) | ||
| assertEquals("1,9 km", formatMetricDistance(1900)) | ||
| assertEquals("1,24 mi", formatImperialDistance(2000)) | ||
| } | ||
| } |
There was a problem hiding this comment.
Thank you for adding unit tests for maps-compose-widgets!
Following up on the KMP comment in ScaleBar.kt, switching formatScaleDistance to pure Kotlin math means we can remove import java.util.Locale (and @Before / @After Locale.setDefault(...) mutation) here as well, and add a test case for larger distances (>= 10 km/mi):
package com.google.maps.android.compose.widgets
import org.junit.Assert.assertEquals
import org.junit.Test
internal class ScaleBarDistanceTest {
@Test
fun metricDistancePreservesFractionalKilometers() {
assertEquals("1.9 km", formatMetricDistance(1900))
assertEquals("1.01 km", formatMetricDistance(1010))
}
@Test
fun imperialDistancePreservesFractionalMiles() {
assertEquals("1.24 mi", formatImperialDistance(2000))
assertEquals("1.99 mi", formatImperialDistance(3200))
}
@Test
fun metricDistanceUsesKilometersAtUnitBoundary() {
assertEquals("999 m", formatMetricDistance(999))
assertEquals("1 km", formatMetricDistance(1000))
assertEquals("2 km", formatMetricDistance(2000))
}
@Test
fun largeDistancesRoundToWholeUnits() {
assertEquals("10 km", formatMetricDistance(10_400))
assertEquals("12 km", formatMetricDistance(11_600))
assertEquals("10 mi", formatImperialDistance(16_100))
}
@Test
fun smallDistancesKeepWholeMetersAndFeet() {
assertEquals("0 m", formatMetricDistance(0))
assertEquals("0 ft", formatImperialDistance(0))
assertEquals("100 m", formatMetricDistance(100))
assertEquals("328 ft", formatImperialDistance(100))
}
}There was a problem hiding this comment.
Removed the Locale setup and teardown, and added boundary, long-distance, rounding and trailing-zero cases. All six widget unit tests pass.
| assertFractionalLabels(dark = true) | ||
| } | ||
|
|
||
| private fun assertFractionalLabels(dark: Boolean) { | ||
| val originalLocale = Locale.getDefault() | ||
| Locale.setDefault(Locale.US) | ||
| try { | ||
| val projection = mockk<Projection>() | ||
| every { projection.fromScreenLocation(any()) } answers { | ||
| // At the equator this spans about 2,224 m, or 1,976 m for the 8/9-width bar. | ||
| if (firstArg<Point>().x == 0) LatLng(0.0, 0.0) else LatLng(0.0, 0.02) | ||
| } | ||
| val camera = mockk<CameraPositionState>() | ||
| every { camera.position } returns CameraPosition.fromLatLngZoom(LatLng(0.0, 0.0), 12f) | ||
| every { camera.projection } returns projection | ||
| val background = if (dark) Color.Black else Color.White | ||
| val foreground = if (dark) Color.White else Color.Black | ||
| composeTestRule.setContent { | ||
| MaterialTheme { | ||
| Box(Modifier.testTag("scale-card").background(background).padding(16.dp)) { | ||
| ScaleBar( | ||
| cameraPositionState = camera, | ||
| textColor = foreground, | ||
| lineColor = foreground, | ||
| shadowColor = background, | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| composeTestRule.onNodeWithText("1.98 km").assertIsDisplayed() | ||
| composeTestRule.onNodeWithText("1.23 mi").assertIsDisplayed() | ||
|
|
||
| val context = InstrumentationRegistry.getInstrumentation().targetContext | ||
| val image = composeTestRule.onNodeWithTag("scale-card").captureToImage().asAndroidBitmap() | ||
| val output = File(context.getExternalFilesDir(null), "scale-bar-${if (dark) "dark" else "light"}.png") | ||
| output.outputStream().use { image.compress(Bitmap.CompressFormat.PNG, 100, it) } | ||
| } finally { | ||
| Locale.setDefault(originalLocale) | ||
| } | ||
| } |
There was a problem hiding this comment.
Calling captureToImage() and writing PNGs to getExternalFilesDir(null) was a great way to visually verify the rendering during development! For the checked-in test suite, could we remove the bitmap capture and file write (along with the java.io.File and java.util.Locale imports) so the test stays fast, self-contained, and avoids unasserted disk I/O in CI?
private fun assertFractionalLabels(dark: Boolean) {
val projection = mockk<Projection>()
every { projection.fromScreenLocation(any()) } answers {
// At the equator this spans about 2,224 m, or 1,976 m for the 8/9-width bar.
if (firstArg<Point>().x == 0) LatLng(0.0, 0.0) else LatLng(0.0, 0.02)
}
val camera = mockk<CameraPositionState>()
every { camera.position } returns CameraPosition.fromLatLngZoom(LatLng(0.0, 0.0), 12f)
every { camera.projection } returns projection
val background = if (dark) Color.Black else Color.White
val foreground = if (dark) Color.White else Color.Black
composeTestRule.setContent {
MaterialTheme {
Box(Modifier.background(background).padding(16.dp)) {
ScaleBar(
cameraPositionState = camera,
textColor = foreground,
lineColor = foreground,
shadowColor = background,
)
}
}
}
composeTestRule.onNodeWithText("1.98 km").assertIsDisplayed()
composeTestRule.onNodeWithText("1.23 mi").assertIsDisplayed()
}There was a problem hiding this comment.
Removed bitmap capture, file writes and locale mutation. In ee57e4c I moved the controlled-projection cases to Robolectric on API 29/35 because MockK’s Android agent crashes during API 29 activity cleanup. All eight cases pass. The device rendering checks now use a real, unbound camera state and pass on API 29.
| val upperRightLatLng = proj.fromScreenLocation(Point(widthInPixels, 0)) | ||
| val canvasWidthMeters = upperLeftLatLng.sphericalDistance(upperRightLatLng) | ||
| val horizontalLineWidthMeters = (canvasWidthMeters * 8 / 9).toInt() | ||
|
|
||
| var metricUnits = "m" | ||
| var metricDistance = horizontalLineWidthMeters | ||
| if (horizontalLineWidthMeters > METERS_IN_KILOMETER) { | ||
| var metricDistance = horizontalLineWidthMeters.toDouble() | ||
| if (horizontalLineWidthMeters >= METERS_IN_KILOMETER) { | ||
| metricUnits = "km" | ||
| metricDistance /= METERS_IN_KILOMETER.toInt() | ||
| metricDistance /= METERS_IN_KILOMETER | ||
| } | ||
|
|
||
| var imperialUnits = "ft" | ||
| var imperialDistance = horizontalLineWidthMeters.toDouble().toFeet() | ||
| if (imperialDistance > FEET_IN_MILE) { | ||
| if (imperialDistance >= FEET_IN_MILE) { | ||
| imperialUnits = "mi" | ||
| imperialDistance = imperialDistance.toMiles() | ||
| } | ||
| imperialText = "${imperialDistance.toInt()} $imperialUnits" | ||
| metricText = "$metricDistance $metricUnits" | ||
| val numberFormat = NumberFormat.getNumberInstance().apply { | ||
| maximumFractionDigits = 2 | ||
| isGroupingUsed = false | ||
| } | ||
| val imperialValue = if (imperialUnits == "ft") { | ||
| imperialDistance.toInt().toString() | ||
| } else { | ||
| numberFormat.format(imperialDistance) | ||
| } | ||
| val metricValue = if (metricUnits == "m") { | ||
| metricDistance.toInt().toString() | ||
| } else { | ||
| numberFormat.format(metricDistance) | ||
| } | ||
| imperialText = "$imperialValue $imperialUnits" | ||
| metricText = "$metricValue $metricUnits" |
There was a problem hiding this comment.
Awesome catch fixing Point(0, widthInPixels) to Point(widthInPixels, 0) here!
Similar to ScaleBar.kt, let's avoid import java.text.NumberFormat here as well so we stay free of new java.text.* imports:
val upperLeftLatLng = proj.fromScreenLocation(Point(0, 0))
val upperRightLatLng = proj.fromScreenLocation(Point(widthInPixels, 0))
val canvasWidthMeters = upperLeftLatLng.sphericalDistance(upperRightLatLng)
val horizontalLineWidthMeters = (canvasWidthMeters * 8 / 9).toInt()
metricText = if (horizontalLineWidthMeters >= METERS_IN_KILOMETER) {
"${formatTestDistance(horizontalLineWidthMeters / METERS_IN_KILOMETER)} km"
} else {
"$horizontalLineWidthMeters m"
}
val feet = horizontalLineWidthMeters.toDouble().toFeet()
imperialText = if (feet >= FEET_IN_MILE) {
"${formatTestDistance(feet.toMiles())} mi"
} else {
"${feet.roundToInt()} ft"
}There was a problem hiding this comment.
Kept the Point(widthInPixels, 0) correction and replaced the Java formatting with pure Kotlin expected labels. The live-map assertions still use the existing API-key assumption.
|
@dkhawk Addressed all five comments in 064e3b5:
Validation: 6 widget unit tests and 4 API-35 emulator rendering tests pass; widget lint has zero issues, and app lint, instrumentation APK assembly, spotlessCheck, and git diff --check complete successfully. Live-map integration still needs a Maps API key. Ready for another review. Thank you! |
|
I find this PR very helpful for addressing the factual problem with the scale bar error, so it definitely is a valuable improvement. I would not say that it completely fixes the issue, which also is about UX and usability. From my perspective a decent map has a scale bar showing lengths of 1 mile or 1 km, or 100ft, 100m, etc. Not 1.76 miles, because I as a human have trouble estimating distances based on fractional units. An optimal solution would size the scale bar to some of these more comprehensible lengths. As far as reducing the error goes, I would not round to more than one decimal. I don't think 2 decimals are beneficial to human map readers. |
Hi @bubenheimer ! Agreed: this PR fixes the wrong numbers, but sizing the bar to round lengths (1 km, 500 m, 1 mi, 100 ft) like Google Maps does is the real fix for #233, so I would keep the issue open for that after this one lands. We are also adding a Units API to android-maps-utils (googlemaps/android-maps-utils#1832) that should make that follow-up easier.
That makes sense, I have suggested the same in my review. |
kikoso
left a comment
kikoso
left a comment
There was a problem hiding this comment.
Thanks for the contribution, @premsinghsodha7 , this is a nice fix for the wrong labels! Minor comment about the precision below. Also, since the variable-length bar from #233 is still pending, could we change Fixes #233 to Refs #233 in the description, so the issue stays open? Happy to approve after that.
| private fun formatScaleDistance(distance: Double): String { | ||
| if (distance >= 10.0) { | ||
| return distance.roundToInt().toString() | ||
| } | ||
| val scaled = (distance * 100.0).roundToInt() | ||
| val whole = scaled / 100 | ||
| val remainder = scaled % 100 | ||
| return when { | ||
| remainder == 0 -> "$whole" | ||
| remainder % 10 == 0 -> "$whole.${remainder / 10}" | ||
| else -> "$whole.${remainder.toString().padStart(2, '0')}" | ||
| } | ||
| } |
There was a problem hiding this comment.
As @bubenheimer mentioned, two decimals are a bit too much for someone reading a map ("1.76 mi"). What about rounding to one decimal, which also matches what we are doing in the utils Units API? The tests would need an update (e.g. 1.01 km becomes 1 km, 1.24 mi becomes 1.2 mi).
| private fun formatScaleDistance(distance: Double): String { | |
| if (distance >= 10.0) { | |
| return distance.roundToInt().toString() | |
| } | |
| val scaled = (distance * 100.0).roundToInt() | |
| val whole = scaled / 100 | |
| val remainder = scaled % 100 | |
| return when { | |
| remainder == 0 -> "$whole" | |
| remainder % 10 == 0 -> "$whole.${remainder / 10}" | |
| else -> "$whole.${remainder.toString().padStart(2, '0')}" | |
| } | |
| } | |
| private fun formatScaleDistance(distance: Double): String { | |
| if (distance >= 10.0) { | |
| return distance.roundToInt().toString() | |
| } | |
| val scaled = (distance * 10.0).roundToInt() | |
| return if (scaled % 10 == 0) "${scaled / 10}" else "${scaled / 10}.${scaled % 10}" | |
| } |
There was a problem hiding this comment.
Applied the one-decimal suggestion and updated the tests: 1.01 km becomes 1 km, and 1.24 mi becomes 1.2 mi. The description now uses Refs #233 so the variable-length bar work stays open.
|
Thanks @kikoso and @bubenheimer. I've reduced the precision to one decimal below 10 km/mi and updated the unit and rendering tests in ba81967. I've also changed |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
@dkhawk Fixed the API 29 MockK crash and the MaterialTheme lint warning in ee57e4c. The 57 unit tests, full API 29 instrumentation/coverage command, build, lint, Spotless and screenshot validation passed locally. I’ve replied in each review thread and merged the latest main. The current head is adb78fd. GitHub is waiting for maintainer approval before it can run the new workflows. Could you approve those runs? |
…esholds for ScaleBar - Add `trimTrailingZeros: Boolean = false` and `NumberFormatSymbols.COMPACT` (`groupingSize = 0`, `trimTrailingZeros = true`) for compact widgets like `ScaleBar` - Add `ImperialUnitsConverter(milesThreshold = ...)` / `ImperialUnitsConverter.withMilesThreshold(...)` and `MetricUnitsConverter(kilometersThreshold = ...)` / `MetricUnitsConverter.withKilometersThreshold(...)` - Add `ScaleBar` distance formatting tests matching `googlemaps/android-maps-compose#1045` (`ScaleBarDistanceTest` and `ScaleBarRenderingTests`) - Update `library.api` and documentation examples
…ing (#1832) * feat: add Units API for distance and elevation conversion and formatting * fix(demo): read LocalLocale.current.platformLocale in UnitsControlPanel * feat(units): expose clean Java conversion methods and fix UseKtx lint * docs(skill): document Units API with Kotlin and Java examples in SKILL.md * feat(units): address PR #1832 review feedback and support hybrid GB units - Default `groupingSize` to `0` in `NumberFormatSymbols.fromLocale` when `!decimalFormat.isGroupingUsed` - Fix threshold boundary rounding in `MetricUnitsConverter` (`999.6 m` -> `"1.0 km"`) and `ImperialUnitsConverter` (`1,319.6 ft` -> `"0.3 mi"`) - Replace `translatable="false"` numeric templates with 4 translatable `%1$s` unit templates (`amu_in_feet`, `amu_in_miles`, `amu_in_meters`, `amu_in_kilometers`), add 24 localized `values-<locale>/strings.xml` translations, and unify `Locale` and `Resources` formatting onto a single `NumberFormatSymbols` code path - Add `@get:JvmName("getMilesThreshold")` and `@get:JvmName("getKilometersThreshold")` for Java interop - Remove short `Number.m`, `.km`, `.ft`, and `.mi` extensions and rename `Meters.to*` properties to `inMeters`, `inKilometers`, `inFeet`, and `inMiles` - Default `"GB"` (`Locale.UK`) out-of-the-box to Imperial road distances (`ft`/`mi`) + Metric elevations (`m`), and add `withDistanceUnits`, `withElevationUnits`, `withDistanceConverter`, `withElevationConverter`, `withDistanceFormatter`, `withElevationFormatter`, and `UnitsConverter(...)` / `UnitsConverter.create(...)` overloads - Update `README.md`, `llm-integration-prompt.md`, `.gemini/skills/android-maps-utils/SKILL.md`, and unit tests (`UnitsTest`, `UnitsJavaTest`, `UnitsDemoTest`) * feat(units): add compact formatting symbols and configurable unit thresholds for ScaleBar - Add `trimTrailingZeros: Boolean = false` and `NumberFormatSymbols.COMPACT` (`groupingSize = 0`, `trimTrailingZeros = true`) for compact widgets like `ScaleBar` - Add `ImperialUnitsConverter(milesThreshold = ...)` / `ImperialUnitsConverter.withMilesThreshold(...)` and `MetricUnitsConverter(kilometersThreshold = ...)` / `MetricUnitsConverter.withKilometersThreshold(...)` - Add `ScaleBar` distance formatting tests matching `googlemaps/android-maps-compose#1045` (`ScaleBarDistanceTest` and `ScaleBarRenderingTests`) - Update `library.api` and documentation examples * feat(units): improve Java API ergonomics and expand Java test coverage * fix(units): align localized unit strings with Navigation SDK and CLDR across all 24 locales
Refs #233.
ScaleBar truncates kilometre and mile labels: 1,900 m becomes
1 km, and 3,200 m becomes1 mi. Preserve up to one decimal place below 10 km/mi, and round larger distances to whole units to keep labels compact at the default 65.dp width. Exactly 1,000 m uses1 km; sub-mile feet round to whole feet.This corrects the displayed labels. Sizing the bar to round lengths remains a follow-up under #233, so that issue should stay open. Formatting uses pure Kotlin math and strings, without Java formatting or locale mutation. No new library dependencies or public API changes. Production drawing and projection calculation are unchanged.
Controlled-projection rendering tests run with Robolectric on API 29 and 35. Device rendering tests use a real, unbound CameraPositionState, avoiding the MockK Android agent reflection crash on API 29 (
PictureInPictureUiStateis unavailable there). Both test files use Material 3 themes and perform no bitmap capture, file writes, or locale mutation. Host-test resources and the Compose test activity are configured in maps-app; a placeholder key fallback supports manifest merging while configured app keys retain Secrets-plugin precedence.Validation:
createDebugCoverageReport :maps-app:createLibraryCoverageReports, passed on an Android 10/API 29 emulator, including the two key-free device rendering tests. Live-key cases use the existing assumptions.An extra API 29 demo-smoke run hits an AdvancedMarkersActivity / BitmapDescriptorFactory.fromPinConfig failure, also reproduced on unchanged commit 58ea02c. The demo-smoke CI workflow runs on API 36.