Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Trigger warning / error when non GoogleMapComposable is used inside of GoogleMap #344

Closed
DarrylBayliss opened this issue Jul 21, 2023 · 5 comments
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@DarrylBayliss
Copy link
Contributor

DarrylBayliss commented Jul 21, 2023

Is your feature request related to a problem? Please describe.

It's too easy to use Composables not intended for rendering to the map inside the GoogleMap content lambda. If I pass in a Column for example, the only hint I get is a non specific crash in Logcat:

Process: com.example.composeformaps, PID: 8771
java.lang.ClassCastException: androidx.compose.ui.node.LayoutNode cannot be cast to com.google.maps.android.compose.MapNode
at com.google.maps.android.compose.MapApplier.insertTopDown(MapApplier.kt:34)
at androidx.compose.runtime.ComposerImpl$createNode$2.invoke(Composer.kt:1613)
at androidx.compose.runtime.ComposerImpl$createNode$2.invoke(Composer.kt:1608)
at androidx.compose.runtime.ComposerImpl$recordInsert$2.invoke(Composer.kt:3546)
at androidx.compose.runtime.ComposerImpl$recordInsert$2.invoke(Composer.kt:3543)
at androidx.compose.runtime.CompositionImpl.applyChangesInLocked(Composition.kt:818)
at androidx.compose.runtime.CompositionImpl.applyChanges(Composition.kt:849)
at androidx.compose.runtime.Recomposer.composeInitial$runtime_release(Recomposer.kt:1041)
at androidx.compose.runtime.ComposerImpl$CompositionContextImpl.composeInitial$runtime_release(Composer.kt:4007)
at androidx.compose.runtime.CompositionImpl.setContent(Composition.kt:520)
at com.google.maps.android.compose.GoogleMapKt$GoogleMap$10.invokeSuspend(GoogleMap.kt:239)
at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:106)
at androidx.compose.ui.platform.AndroidUiDispatcher.performTrampolineDispatch(AndroidUiDispatcher.android.kt:81)
at androidx.compose.ui.platform.AndroidUiDispatcher.access$performTrampolineDispatch(AndroidUiDispatcher.android.kt:41)
at androidx.compose.ui.platform.AndroidUiDispatcher$dispatchCallback$1.run(AndroidUiDispatcher.android.kt:57)
at android.os.Handler.handleCallback(Handler.java:942)
at android.os.Handler.dispatchMessage(Handler.java:99)
at android.os.Looper.loopOnce(Looper.java:201)
at android.os.Looper.loop(Looper.java:288)
at android.app.ActivityThread.main(ActivityThread.java:7872)
at java.lang.reflect.Method.invoke(Native Method)
at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:548)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:936)
Suppressed: kotlinx.coroutines.DiagnosticCoroutineContextException: [androidx.compose.ui.platform.MotionDurationScaleImpl@293cb98, androidx.compose.runtime.BroadcastFrameClock@862ff1, StandaloneCoroutine{Cancelling}@12407d6, AndroidUiDispatcher@c3eba57]

Describe the solution you'd like
A warning or error triggered within the IDE, informing the developer of a possible misuse of a composable.

Describe alternatives you've considered

I'm conscious GoogleMapComposable is available to help with this. I am not sure it is working as extended. I see no warning when passing a non GoogleMapComposable into a GoogleMap.

Additional context
See #335 for an example of this situation.

@DarrylBayliss DarrylBayliss added triage me I really want to be triaged. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. labels Jul 21, 2023
@kikoso kikoso added priority: p2 Moderately-important priority. Fix may not be included in next release. and removed triage me I really want to be triaged. labels Jul 31, 2023
@kikoso
Copy link
Collaborator

kikoso commented Jul 31, 2023

Hi @DarrylBayliss !

Thanks for the issue. There are a few things in this issue to take into consideration.

  • The annotation GoogleMapComposable does not seem to work. Checking whether a Composable is run from within a Composable function is checked at the plugin level, and I am not sure if this was intended to be checked from a plugin or as an in-code annotation. But it is certainly not working as it is.
  • The documentation needs to be improved, but there is certainly another deeper issue. Any Composable that is passed into the content lambda needs to implement the MapNode interface, which is ultimately used by the MapApplier to achieve certain functions during Composition (onAttached, onRemoved, etc).
  • At the moment only a few items can be rendered from within the lambda content, i.e.,: Circle, Markers, Polylines, Polygons, etc). We could slowly expand the set of "renderable" components, but ultimately they need to manually implement MapNode. I think a few of them are probably clear (Button, for instance), but as it is will require a bit more work.

TL;DR: we need to improve the documentation, and think about how to pass components to content(). As a start, I would like to add a few components (there is this issue which is interesting since we need to model exactly this problem: #340)

@DarrylBayliss
Copy link
Contributor Author

Hi @kikoso,

You raise good points about the documentation needing improvement too to help developers understand expected usage.

I'd be interested in hearing more about your thoughts on the GoogleMapComposable annotation. I'm not sure who can help us understand the intention behind it, maybe there is someone @wangela can connect us to. 😃

@wangela
Copy link
Member

wangela commented Aug 11, 2023

Have you looked at #130 and related issue #30? That seems to be the original intent behind adding it.

@kikoso
Copy link
Collaborator

kikoso commented Aug 15, 2023

Hi @DarrylBayliss .

Upon further analysis, it seems that the annotation used within GoogleMapComposable was updated at some point, but it should be now triggering a warning. See the following message:

Screenshot 2023-08-15 at 06 28 46

This is the output of running a build or assemble operation. Of course, it would be great to have it as a Lint rule instead.

@nienienienie
Copy link

nienienienie commented Feb 5, 2024

please add warning, I was trying to animate marker fade out with

          // ouh!
          AnimatedVisibility(
            visible = true,
            exit = fadeOut(),
          ) {
            Marker(
              state = MarkerState(position = it),
              icon = icon,
            )
          }

it's even covered in https://www.composables.com/tutorials/animate-maps-compose

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

No branches or pull requests

4 participants