Repository navigation
fix(widgets): preserve fractional scale bar distances - #1045
premsinghsodha7 wants to merge 7 commits into
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!
| } 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')}"
}
}| 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))
}
}| 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()
}| 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"
}
|
@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}" | |
| } |
|
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}
| import androidx.compose.foundation.background | ||
| import androidx.compose.foundation.layout.Box | ||
| import androidx.compose.foundation.layout.padding | ||
| import androidx.compose.material.MaterialTheme |
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 with a decimal point, without Java formatting or locale mutation. No new dependencies or public API changes. Production drawing and projection calculation are unchanged.
Validation:
Code coverage report was not run.