fix: Return early in rememberComposeBitmapDescriptor for invalid view size

[ln-12]

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]

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

[ln-12]

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.

[bubenheimer]

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.

[bubenheimer]

@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.

[ln-12]

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?

[bubenheimer]

If no one objects, I suggest going with the following (see above comment):

1. Set layoutParams (WRAP_CONTENT) on ComposeView, because docs say you have to.
2. Use View.MeasureSpec.UNSPECIFIED, because this makes the most sense to me. This is a breaking change, as it makes parent size 0. Before, parent size was typically the screen size, which seems an unlikely intentional input for determining marker size and can be obtained in other ways. This change needs to be documented.

[ln-12]

I changed the PR accordingly. After some testing, I could not get another crash with these changes, so it seems to work.

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

[bubenheimer]

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).

[kikoso]

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.

[bubenheimer]

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

[el-qq]

@ln-12 hi. Thank you for this contribution

Please add semantic prefixes to commits

[ln-12]

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

[bubenheimer]

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

[ln-12]

Yeah, right. I changed that.

[ln-12]

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!