fix: Return early in rememberComposeBitmapDescriptor for invalid view size #533

ln-12 · 2024-03-15T08:21:00Z

Fixes #518 🦕

Make sure to open a GitHub issue as a bug/feature request before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
Ensure the tests and linter pass
Code coverage does not decrease (if any source code was changed)
Appropriate docs were updated (if necessary)

By returning early in the case of invalid view width/height, we can avoid the IllegalArgumentException described in issue #518.

bubenheimer

The end result of this change is that the map will render the default marker instead of crashing on an empty composable marker. This would seem to hide the problem in a workaround, rather than flag it. Would throwing a more meaningful exception help with troubleshooting here?

Just my 2 cents here: creating an empty ComposeView suggests some kind of problem in user code. I imagine user code may be able to avoid creating the marker entirely by testing whether necessary preconditions are met before attempting to create the marker.

bubenheimer · 2024-03-15T20:11:13Z

maps-compose/src/main/java/com/google/maps/android/compose/RememberComposeBitmapDescriptor.kt

@@ -51,6 +51,11 @@ private fun renderComposableToBitmapDescriptor(
        View.MeasureSpec.makeMeasureSpec(parent.height, View.MeasureSpec.AT_MOST),
    )

+    if(composeView.measuredWidth == 0 || composeView.measuredHeight == 0) {
+        parent.removeView(composeView)


You can avoid the extra removeView() by adding try-finally in the right place here.

Thanks for the feedback! Yeah, unfortunately I am not sure why this happens in the first place. Maybe some race condition while navigating? I can't dig into it as this happens super rarely and I was not able to reproduce it while having the debugger attached.

I don't think that a different error would help here as we already know that the problem is an incorrect view size. The question is just why we get to this state. As you can see from the example code in the issue, the original reporter only uses the GoogleMaps composable and adds some markers (the same as I do in my code). I don't see what we as users of this library could do differently as the code works >99% of the time. Or could we add any meaningful information to a new exception which would help finding the root cause?

For now, not providing a custom icon at all in this edge case was the best solution for me as it at least does not cause crashes in production.

~~What if you used Modifier.requiredSizeIn() on your marker composable content with a minimum of 1.dp or so, as an alternative workaround? I'd be curious if you still crash then.~~

@ln-12 Actually, I suspect that setting a minimum size on the composable, like I suggested above, would not make a difference.

Here is the problem that I think may be at the root of this issue (have not tested it):

android-maps-compose/maps-compose/src/main/java/com/google/maps/android/compose/RememberComposeBitmapDescriptor.kt

Lines 50 to 51 in 0599412

View.MeasureSpec.makeMeasureSpec(parent.width, View.MeasureSpec.AT_MOST),

View.MeasureSpec.makeMeasureSpec(parent.height, View.MeasureSpec.AT_MOST),

A parent view dimension of 0 would cause this crash, no matter how large the Composable wants to be.

I'd think the parent view here would commonly be the Activity's top ComposeView, or supposedly something else in case of navigation, etc. So the maximum marker icon size would commonly be constrained by the size of the screen, which looks weird to me, as that could be quite large.

A better approach, short of workarounds, may be to use View.MeasureSpec.UNSPECIFIED here; restricting marker size by screen size just does not look that useful, it's way too big, creating giant bitmaps. However, I imagine this change would be breaking, as there is no max size anymore. Yet a marker composable relying on something like fillMaxSize is not doing it right, it needs to use something more specific. There may be other concerns with UNSPECIFIED, however, not sure.

You are right. I can force the crash by setting the size of the fragment which contains the compose view to 0 (but I am still not sure when this happens while navigating): https://github.com/ln-12/GoogleMapsCrashReproducer/blob/main/app/src/main/res/layout/activity_main.xml#L11-L12

With that, I also tested View.MeasureSpec.UNSPECIFIED and for me it did not crash anymore. I can change my PR to use this spec, but just like you, I am not sure about other side effects when doing so.

So what would you suggest going forward?

Should this be documented inline with the code or only as a release note in the changelog?

The requirement for the Composable to have a non-null size in both dimensions, and the absence of a parent size need to be documented in public API KDoc documentation on MarkerComposable(). Also in release notes, I believe this would be a task for project maintainers, e.g. @kikoso or @dkhawk.

It could also be flagged prominently in code; personally I'd check for this case after measure() and throw an exception of some sort if measure returns 0 for one or both of the dimensions. This way the problem is identified in a well-defined manner if it happens with the ability to give specific instructions on what to do (set a size in the Composable).

What @bubenheimer describer: the expectation is to have the documentation incode from the PR. The release notes can be manually updated to reflect any other relevant changed.

BTW, I meant "absence of a parent size", not "absence of a parent".

@bubenheimer Unfortunately, we now get crashes containing the exact exception from this PR. We use version 6.1.2 of this library. Do you have another idea how this could be solved? Before we forked this lib and implemented an early return which might have led to undesired results but at least did not crash.

@ln-12 might be best to create a new issue with stacktrace so a maintainer can look at it.

You need to ensure that the content Composable that you are drawing for the Marker always has a non-zero size. Like setting a minimum size of (1,1) via a Modifier. If that does not work for some reason, maybe add some sort of canvas with a single transparent pixel?

el-qq · 2024-05-16T10:11:31Z

@ln-12 hi. Thank you for this contribution

Please add semantic prefixes to commits

maps-compose/src/main/java/com/google/maps/android/compose/RememberComposeBitmapDescriptor.kt

…sable is zero sized

ln-12 · 2024-05-17T09:29:45Z

I added the commit prefixes and applied all requested changes. If you are not yet happy with the wording, please let me know.

bubenheimer · 2024-05-17T14:30:01Z

maps-compose/src/main/java/com/google/maps/android/compose/RememberComposeBitmapDescriptor.kt

+
+    if (composeView.measuredWidth == 0 || composeView.measuredHeight == 0) {
+        throw IllegalStateException("The ComposeView was measured to have a width or height of " +
+                "zero. Make sure the parent and content have a non-zero size.")


According to your testing, "parent" size should no longer matter, right?

Yeah, right. I changed that.

ln-12 · 2024-05-17T16:23:09Z

I was using the wrong mail for my last commit (as I was on a different machine), so the CLA check failed. Fixed that as well. Sorry for the inconvenience!

ln-12 · 2024-06-11T09:19:54Z

Hey @dkhawk, is there any open TODO on my side? We would highly appreciate getting this fix rolled out 😅

## [5.0.4](v5.0.3...v5.0.4) (2024-06-20) ### Bug Fixes * fix release step ([#586](#586)) ([e5dc195](e5dc195)) * Return early in rememberComposeBitmapDescriptor for invalid view size ([#533](#533)) ([db97e65](db97e65))

googlemaps-bot · 2024-06-24T15:40:49Z

🎉 This PR is included in version 5.0.4 🎉

The release is available on:

v5.0.4
GitHub release

Your semantic-release bot 📦🚀

# [6.0.0](v5.0.3...v6.0.0) (2024-07-03) ### Bug Fixes * fix release step ([#586](#586)) ([e5dc195](e5dc195)) * leverage `@StateFactoryMarker` to flag State object creation without remember or similar mechanisms. ([#516](#516)) ([9ed3f7a](9ed3f7a)), closes [#506](#506) * Return early in rememberComposeBitmapDescriptor for invalid view size ([#533](#533)) ([db97e65](db97e65)) ### BREAKING CHANGES * leverage @StateFactoryMarker to flag State object creation without remember or similar mechanisms Co-authored-by: Uli Bubenheimer <bubenheimer@users.noreply.github.com>

googlemaps-bot · 2024-07-03T15:35:50Z

🎉 This issue has been resolved in version 6.0.0 🎉

The release is available on:

v6.0.0
GitHub release

Your semantic-release bot 📦🚀

blunderbuss-gcf bot assigned dkhawk Mar 15, 2024

ln-12 changed the title ~~Return early in rememberComposeBitmapDescriptor for invalid view size~~ fix: Return early in rememberComposeBitmapDescriptor for invalid view size Mar 15, 2024

bubenheimer reviewed Mar 15, 2024

View reviewed changes

bubenheimer reviewed May 16, 2024

View reviewed changes

maps-compose/src/main/java/com/google/maps/android/compose/RememberComposeBitmapDescriptor.kt Outdated Show resolved Hide resolved

fix: Don't throw IllegalArgumentException when parent of marker compo…

62c313d

…sable is zero sized

ln-12 force-pushed the 518_illegal_argument_exception branch from c679916 to 62c313d Compare May 17, 2024 09:26

refactor: Extracted measure specification

0a30e5d

bubenheimer reviewed May 17, 2024

View reviewed changes

bubenheimer approved these changes May 17, 2024

View reviewed changes

refactor: Changed exception wording

81423e7

ln-12 force-pushed the 518_illegal_argument_exception branch from 6a838ba to 81423e7 Compare May 17, 2024 16:22

dkhawk merged commit db97e65 into googlemaps:main Jun 18, 2024
5 checks passed

googlemaps-bot added the released label Jun 24, 2024

ln-12 mentioned this pull request Nov 22, 2024

Using Marker or MarkerComposable in Clustering #641

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Return early in rememberComposeBitmapDescriptor for invalid view size #533

fix: Return early in rememberComposeBitmapDescriptor for invalid view size #533

ln-12 commented Mar 15, 2024 •

edited

Loading

bubenheimer left a comment

bubenheimer Mar 15, 2024

ln-12 Mar 16, 2024 •

edited

Loading

bubenheimer Mar 16, 2024 •

edited

Loading

bubenheimer Mar 17, 2024

ln-12 Mar 18, 2024

bubenheimer May 16, 2024 •

edited

Loading

kikoso May 17, 2024

bubenheimer May 17, 2024

ln-12 Nov 18, 2024

bubenheimer Nov 18, 2024

el-qq commented May 16, 2024

ln-12 commented May 17, 2024

bubenheimer May 17, 2024

ln-12 May 17, 2024

ln-12 commented May 17, 2024

ln-12 commented Jun 11, 2024

googlemaps-bot commented Jun 24, 2024

googlemaps-bot commented Jul 3, 2024

	View.MeasureSpec.makeMeasureSpec(parent.width, View.MeasureSpec.AT_MOST),
	View.MeasureSpec.makeMeasureSpec(parent.height, View.MeasureSpec.AT_MOST),

fix: Return early in rememberComposeBitmapDescriptor for invalid view size #533

fix: Return early in rememberComposeBitmapDescriptor for invalid view size #533

Conversation

ln-12 commented Mar 15, 2024 • edited Loading

bubenheimer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ln-12 Mar 16, 2024 • edited Loading

Choose a reason for hiding this comment

bubenheimer Mar 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bubenheimer May 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

el-qq commented May 16, 2024

ln-12 commented May 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ln-12 commented May 17, 2024

ln-12 commented Jun 11, 2024

googlemaps-bot commented Jun 24, 2024

googlemaps-bot commented Jul 3, 2024

ln-12 commented Mar 15, 2024 •

edited

Loading

ln-12 Mar 16, 2024 •

edited

Loading

bubenheimer Mar 16, 2024 •

edited

Loading

bubenheimer May 16, 2024 •

edited

Loading