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

MapApplier and MapNode is not public #28

Closed
kozaxinan opened this issue Feb 11, 2022 · 12 comments
Closed

MapApplier and MapNode is not public #28

kozaxinan opened this issue Feb 11, 2022 · 12 comments
Assignees
Labels
priority: p3 Desirable enhancement or fix. May not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@kozaxinan
Copy link

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

In order to be able to add more functionality with GoogleMap instance, I wanted to create new Composable like existing ones; Polygon, Polyline... But MapApplier and MapNode are internal and we cannot extend the composable google map functionality.

We already have classes to configure GoogleMap instance. If we can write our own Composable function to update GoogleMap instance, we can migrate to Compose Map much easier.

Describe the solution you'd like
A clear and concise description of what you want to happen.

Make MapApplier and MapNode public

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.

Create and API to provide GoogleMap instance so we can configure.

@kozaxinan kozaxinan added triage me I really want to be triaged. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. labels Feb 11, 2022
@jpoehnelt
Copy link
Contributor

@kozaxinan Thank you for opening this issue. 🙏
Please check out these other resources that might be applicable:

This is an automated message, feel free to ignore.

@arriolac
Copy link
Contributor

Can you provide more info/examples on what kind of customizations you would like to make?

@kozaxinan
Copy link
Author

Hi, main idea is using already existing GoogleMap features while migrating Compose. We have a class that gets GoogleMaps as parameter and configure it. If the MapApplier is public, we can write our own composable and configure GoogleMap as we configure now. And one by one migrate to new APIs.

Besides at the moment, we are unable to use full features. for example, #29

@arriolac
Copy link
Contributor

arriolac commented Feb 15, 2022

Thanks for clarifying @kozaxinan. If you wouldn't mind sharing, what are other extensions you want to write in the form of composable elements? Anything other than GeoJson?

I'd like to eventually add the functionality in the utility library here. Probably as a separate maven artifact. Since so much of the GoogleMap SDK object is managed by the library, my concern is that it might encourage incorrect usage of it and create more headaches than not. For example, if the GoogleMap object were exposed and you attempted to use the utility library's geo json feature, some object will be managed by the compose runtime, and others will not.

@kozaxinan
Copy link
Author

As long as we will get geojson function, we don't mind the artifacts. I can also create a pr into this repo to add function. I also agree increasing Api surface might lead more problems. Or if you guide me, I can try to add in utils repo.

One other usecase we need is projection.visibleBounds. I will open a new issue for new feature request about getting visibleBounds of map since I cannot use MapApplier.

@arriolac
Copy link
Contributor

arriolac commented Feb 16, 2022

@kozaxinan I'd like for the utility library to be a separate module/artifact but it also would entail modifying the visibility of MapApplier, MapNode, etc. since that new package would need access to the GoogleMap object. Another approach is to add utility functionality to the maps-compose module to main scoping, however, that's also not ideal since it bloats the library. I'm leaning towards the former solution but would like to give this a bit more thought... Near-term goal would be to put the scaffolding in place + migrate a few features so you or anyone in the community can contribute utility library functionality into it. Will circle back to this.

@kozaxinan
Copy link
Author

Thank you for the brainstorm. We have features that directly depend on geojson and visibleBounds. We are in process of rewriting main result screen including map. If map compose won't have geojson feature, we have to continue using map fragment we have.

I am thinking of what might go wrong if MapApplier is public. People have to continue using compose APIs after using MapApplier. That should be okay. (More experienced compose library developer can correct me)

Is it possible to have a ComposeMapNode method that has factory and update parameters like original ComposeNode method but passes map instance to factory. In that case only MapNode needs to be public. (This is best I can come up with my compose knowledge)

@kozaxinan
Copy link
Author

Actually, I remember internal keyword is just for Kotlin. I wrote a Java static method to get map from MapApplier. I created our node in Java by extending MapNode. I can already play with the api and google map in same way library does. internal modifier doesnt prevent usages from Java.

Let me try to add our missing features and I can experience what would happen if MapAppier becomes public.

@kozaxinan
Copy link
Author

I managed to make geojson work with two simple java helper classes. I rendered geojson. With that approach a solution can be created in utils library. I am not proud but using workaround but calling kotlin from java is already possible.

@arriolac arriolac added priority: p3 Desirable enhancement or fix. May not be included in next release. and removed triage me I really want to be triaged. labels Mar 1, 2022
@ashdavies
Copy link

I think it could also be achieved with a bit of reflection, also NotAdvised™️

I totally agree with the concern that it could lead to abuse of the API, but for some projects that need a staged migration to Compose, from an XML layout based infrastructure, it's really hard to adapt to an all-or-nothing approach.

Creating a potential point of exploit is undesirable, but allowing some kind of interop would really help facilitate the adoption of this library IMO. Rather than provide a facility to retrieve the GoogleMap instance managed by this library, would it be possible to inject an instance with partial functionality?

@ashdavies
Copy link

ashdavies commented May 6, 2022

Ultimately, if people need the behaviour, people are going to find a way to do it, what I could imagine would be to create a composition local for GoogleMap with some heavily annotated warnings and opt-ins expressing the potential vulnerabilities, Kotlin is pretty good at providing opt-in behaviours.

The library itself already casts the Applier which is not so great for code coupling.

image

@barbeau
Copy link
Contributor

barbeau commented Jun 30, 2022

An experimental MapEffect is now available that exposes access to the underlying GoogleMap object - for more details see https://github.com/googlemaps/android-maps-compose#obtaining-access-to-the-raw-googlemap-experimental.

I'm going to close this issue as I believe it should cover the use cases discussed here, but if not please feel free to re-open.

@barbeau barbeau closed this as completed Jun 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p3 Desirable enhancement or 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

5 participants