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

feat: customize Marker using Composable #355

Merged
merged 6 commits into from
Aug 17, 2023

Conversation

mosmb
Copy link
Contributor

@mosmb mosmb commented Jul 28, 2023

  • Create a Marker Composable that accepts a Composable content
  • Update BasicMapActivity to show the Composable Marker

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

Fixes #41 🦕

val compositionContext = rememberCompositionContext()
val currentContent by rememberUpdatedState(content)

return remember(parent, compositionContext, currentContent, *keys) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are having a funny error here since the vararg is not properly resolved, but does not seem to affect the compilation and we can probably live with it.

@kikoso
Copy link
Collaborator

kikoso commented Jul 31, 2023

@mosmb , thanks for the contribution, it looks great! I left a couple of comments, let me know what you think.

@mosmb
Copy link
Contributor Author

mosmb commented Jul 31, 2023

@kikoso
I updated based on what you suggest.
Feel free to suggest any other changes that you may think of :)

@mosmb mosmb requested a review from kikoso July 31, 2023 15:02
@kikoso
Copy link
Collaborator

kikoso commented Jul 31, 2023

LGTM! Thanks for the contribution, @mosmb !

@mosmb mosmb requested a review from kikoso August 1, 2023 07:38
@mosmb
Copy link
Contributor Author

mosmb commented Aug 1, 2023

@kikoso
Feel free to tell me if you see other things to edit

@mosmb
Copy link
Contributor Author

mosmb commented Aug 2, 2023

@kikoso
Is it possible to re-launch the instrumentation tests task please, as I think I don't have the permission to do that

@kikoso
Copy link
Collaborator

kikoso commented Aug 2, 2023

@kikoso Is it possible to re-launch the instrumentation tests task please, as I think I don't have the permission to do that

Hi @mosmb ! It is a topic more complex. The instrumentation tests are running on your branch, and you would need to set up the API Key for them to work.

There are a couple of approaches that can fix this, but they might pose a security risk (see https://securitylab.github.com/research/github-actions-preventing-pwn-requests/). We are currently exploring this.

In the meantime, adding the API Key as a secret in your repository should work out. What we generally do is to run the test before getting any PR merged, but this is something that we want to fix so any external PRs can properly report any failure on an instrumentation test.

@mosmb
Copy link
Contributor Author

mosmb commented Aug 3, 2023

@kikoso
Yep I added the API key as a secret in my repository, so I think it should work

@mosmb
Copy link
Contributor Author

mosmb commented Aug 3, 2023

@kikoso Tests seem to fail because of the API key
I added a secret ACTIONS_API_KEY to my rep but doesn't seem to work though

@mosmb
Copy link
Contributor Author

mosmb commented Aug 7, 2023

@kikoso
Any news for this PR to be able to be merged?

@kikoso
Copy link
Collaborator

kikoso commented Aug 10, 2023

Hi @mosmb ! We have a couple of PRs that we want to merge first and release a new version, but this should happen rather imminently.

@mosmb
Copy link
Contributor Author

mosmb commented Aug 10, 2023

@kikoso Ok! Thank you for the information!

Copy link
Member

@wangela wangela left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for contributing this highly requested feature, @mosmb! Although the github action can't access the secrets to run tests on GitHub, would you still please add tests for the new functionality to keep our code coverage even? We'll be able to merge this PR when that task is done.

@mosmb
Copy link
Contributor Author

mosmb commented Aug 15, 2023

Thank you @wangela for your feedback!
What kind of test do you expect?
As I can see, there's no test for now for Marker, except one testing that a state must not be reused.
Do you expect to do the same test but with MarkerComposable?

@kikoso
Copy link
Collaborator

kikoso commented Aug 17, 2023

Thank you @wangela for your feedback! What kind of test do you expect? As I can see, there's no test for now for Marker, except one testing that a state must not be reused. Do you expect to do the same test but with MarkerComposable?

I think this could be a good candidate, @mosmb !

@mosmb
Copy link
Contributor Author

mosmb commented Aug 17, 2023

@kikoso @wangela
I added the test.
Might be good later to add more tests around Markers but it's a good start!
Feel free to send any feedback!

@wangela wangela merged commit 51bb2bf into googlemaps:main Aug 17, 2023
8 of 9 checks passed
googlemaps-bot pushed a commit that referenced this pull request Aug 17, 2023
# [2.14.0](v2.13.1...v2.14.0) (2023-08-17)

### Features

* customize Marker using MarkerComposable ([#355](#355)) ([51bb2bf](51bb2bf))
@googlemaps-bot
Copy link
Contributor

🎉 This PR is included in version 2.14.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@mosmb mosmb deleted the composable-marker branch August 18, 2023 14:35
@WonderCsabo
Copy link

@mosmb checking out your code, i stumbled upon this line:

composeView.draw(fakeCanvas)

I was wondering, why is this needed?

@mosmb
Copy link
Contributor Author

mosmb commented Aug 31, 2023

@WonderCsabo In some cases, the ComposeView will not trigger the draw of its node. So in order to force that and not having a Composable that is not being drawn, we draw an empty canvas into the ComposeView to force it to enter into the drawing phase.
It's kinda of a hack but it works.
In Compose 1.5.0, they added some utility method to Picture to be able to draw Composable into bitmaps. But as this library is not based on this version of Compose yet, we stick with this hack

@kikoso
Copy link
Collaborator

kikoso commented Aug 31, 2023

@mosmb , which utility method do you mean?

We are working on some stuff for next month, but I would like to upgrade Compose as one of the next issues.

@mosmb
Copy link
Contributor Author

mosmb commented Aug 31, 2023

@kikoso
The Canvas API that will have a drawPicture method that will enable us to record Composable with the drawWithCache modifier

@WonderCsabo
Copy link

@mosmb thanks for the info. Good to know there will be an official method to do this. Now i see it in the docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Define custom marker images with Composables
5 participants