feat: Add marker drag event callback to Marker composable #11

chibatching · 2022-02-06T09:29:26Z

To observe dragged marker object, I added onMakerDrag callback lambda to Marker composable

Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

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 #10 🦕

jpoehnelt · 2022-02-06T09:30:24Z

@chibatching Please take a moment to fill out this short survey. Thank you!

This is an automated message, feel free to ignore.

arriolac

One minor suggestion. Would you mind also adding usage of this to the sample app to help folks how to use this?

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

chibatching · 2022-02-09T01:37:16Z

Ok, I'll add it as soon as possible!

chibatching · 2022-02-09T12:18:31Z

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

@@ -46,6 +46,9 @@ enum class DragState {
    START, DRAG, END
 }

+@Immutable
+class MarkerWrapper(val value: Marker? = null)


I made a wrapper class to add Immutable annotation to observe the marker object always.
But I think my class naming is not so good. So please let me know if you have any good ideas.

chibatching · 2022-02-09T12:27:07Z

I made changes to the sample code and fixed the observing marker state problem.

arriolac

I have a concern with how this is currently implemented, and may need to be redesigned due having two sources of truth for the marker's position. I proposed a new design, have a look.

arriolac · 2022-02-09T18:32:23Z

app/src/main/java/com/google/maps/android/compose/MapSampleActivity.kt

    // Observing and controlling the camera's state can be done with a CameraPositionState
    val cameraPositionState = rememberCameraPositionState {
        position = CameraPosition.fromLatLngZoom(singapore, 11f)
    }
+    val markerDragState = rememberMarkerDragState()
+    if (markerDragState.dragState == DragState.END) {
+        markerDragState.marker.value?.let { markerPosition = it.position }


Hm this is an interesting consequence of this feature. The initially set position state will no longer be accurate when dragged which is already true for the existing version of the library. Having to do the above is not ideal, the Marker composable should handle this for you.

I think a more intuitive design would be to emulate how CameraPositionState works. So something like the following:

class MarkerPositionState( position: LatLng = LatLng(0.0f, 0.0f) ) { var position: LatLng by mutableStateOf(position) var dragState: DragState by mutableStateOf(DragState.END) internal set }

This means updating the position property to accept MarkerPositionState and removal of the markerDragState as the DragState would now be accessed via MarkerPositionState.

Some pros/cons of this:

pro: position would always be consistent as the marker is dragged

con: would cause a breaking change

con: may have some odd behaviors when the same state is passed to multiple marker instances

Other considerations:

note that only the position is exposed and not the full Marker object. I'm not convinced that it should be exposed unless it is useful for the tag property to be accessed while the Marker is dragged.

chibatching · 2022-02-10T15:25:10Z

In my use cases, I want to access only positions.
So, it seems good to introduce MarkerPositionState. But

con: may have some odd behaviors when the same state is passed to multiple marker instances

If we want to avoid these odd behaviors, I think using a callback-based approach instead of the state-based is worth considering.
A callback-based approach could solve the problem without breaking changes.

arriolac · 2022-02-10T22:15:55Z

@chibatching after reading through Compose's API guidelines which states "Prefer stateless and controlled @composable functions", the callback-based approach makes more sense here over what I proposed in #11 (comment) However, I think there are a few behavior and API changes needed. These will be breaking changes and necessitate a major version update.

markerDragState should be removed from the Marker composable. Instead, it should accept a callback called onMarkerDrag of type (Marker, DragState) -> Unit.
it is expected that implementations of onMarkerDrag should modify a MutableState<LatLng> position (i.e. the position provided in the Marker.position composable). Failure to do so won't actually drag the marker. This way, the marker position state is fully controlled by the caller of Marker. Note that the existing implementation violates this assumption.

# Conflicts: # app/src/main/java/com/google/maps/android/compose/MapSampleActivity.kt # maps-compose/src/main/java/com/google/maps/android/compose/MapApplier.kt # maps-compose/src/main/java/com/google/maps/android/compose/Marker.kt

chibatching · 2022-02-15T01:56:40Z

I fixed conflicts

arriolac · 2022-02-15T17:54:29Z

app/src/main/java/com/google/maps/android/compose/MapSampleActivity.kt

            title = "Zoom in has been tapped $ticker times.",
            >

            
+            draggable = true,
+             marker, dragState ->
+                markerPositionState = marker.position


If this line was removed markerPositionState will be out-of-sync with the marker's actual position. I don't think there's really anyway around this given how the marker's position is managed by the map. Hoisting a state object as I mentioned in #11 (comment) may be the only appropriate solution here.

Ah, I'm sorry. I misunderstood and I agree with you. I'll fix it.

chibatching · 2022-02-16T01:18:46Z

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

 */
 @Composable
 fun Marker(
-    position: LatLng,
+    positionState: MarkerPositionState = rememberMarkerPositionState(),


I replaced raw position with position state completely. But should I remain position parameter or make overload?

An overload would be ideal, but I think it would be a maintenance burden to support polymorphic types for position and positionState. So, I'm in favor of replacing position and markerDragState here.

arriolac · 2022-03-03T03:38:06Z

@chibatching I've given this a bit more taught and while "prefer stateless and controlled @composable functions" is the recommendation, taking this approach will not behave well in Maps Compose since ultimately it is an interop library of the Maps SDK. And, the Maps SDK internally manages drag state of a marker. So, trying to combat that by designing a callback-based approach as I mentioned in #11 (comment) would cause marker dragging to be jerky. This would definitely be the preferred API though if Maps Compose were a Compose native implementation of Maps SDK.

Given this, the right approach here would be to use hoisted state types as I mentioned in #11 (comment). Apologies for the circuitous decisions around the correct API though I think it's important to weigh the pros/cons here.

Let me know if you think you can update this PR with the following changes. If not, I'm happy to take it from here. Really appreciate your contributions thus far.

chibatching · 2022-03-05T10:55:20Z

@arriolac Please don't worry about it. I support your deeply considered decision.
I am still new to building applications using Compose, so this was a great learning experience.

I fixed my PR. If you have any problems, please don't hesitate to tell me!

arriolac

This PR looks good to me! Since this is a breaking change, please change the base branch to maps-compose-2.0.x. I'll create a SNAPSHOT of 2.0 so folks can try out this change.
I'm considering adding another hoisted state object that contains MarkerPositionState (like MarkerState) which also contains controls for showing/hiding info window before promoting this change to 2.0.

arriolac · 2022-03-07T23:40:58Z

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

 */
 @Composable
 fun Marker(
-    position: LatLng,
+    positionState: MarkerPositionState = rememberMarkerPositionState(),


An overload would be ideal, but I think it would be a maintenance burden to support polymorphic types for position and positionState. So, I'm in favor of replacing position and markerDragState here.

chibatching · 2022-03-08T12:41:38Z

@arriolac Thank you for your review!!
And also I changed the base branch to maps-compose-2.0.x.

arriolac

Just one more modification and then should be good to merge.

arriolac · 2022-03-09T18:04:36Z

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

-}
+fun rememberMarkerPositionState(
+    position: LatLng = LatLng(0.0, 0.0)
+): MarkerPositionState = remember { MarkerPositionState(position) }


This should implement a rememberSaveable and expose a configurable key similar to rememberCameraPositionState. See the implementation of rememberCameraPositionState as a reference.

arriolac

LGTM. Thanks @chibatching

* Fix renamed property name on README * Replace MarkerDragState with onMarkerDrag callback * Add sample code to show how to use marker drag callback * Fix API doc * Introduce MarkerPositionState * Remove onMarkerDrag callback * Update sample * Implement `rememberSaveable` for MarkerPositionState

arriolac · 2022-03-16T17:18:02Z

FYI MarkerPositionState was renamed to MarkerState in #69 since that object now has functionality other than controlling the marker's position (i.e. showing/hiding the info window).

* chore: Fix recommendation on setting the camera's position. (googlemaps#51) Change-Id: I5c2fc72df6e1aaade50f62ab825b13b506160a67 * chore(Test): Add UI tests (googlemaps#37) * chore(Test): Add androidTest. Change-Id: I35a30c7b70bbb88804db8ce5e0e8146c4aa5cb53 * Add connectedCheck step. Change-Id: I0a1530cc9133f2d07f682917f7ac709aee3e8406 * Run connectedCheck on app only Change-Id: I58d97e288db98eac56ef6df9659b653ac2aa92b2 * Upload test reports. Change-Id: I41ec0a6b30d77bf9803e53e7d6f93efbf01e642b * Remove failing test. Change-Id: I14c1b96d13fe796c5145de79df3f1f25046f00c6 * Add timeout to await call. Change-Id: I7dfd70c1e15f7a9c357d506006aae3c3c3426636 * Inject maps API key. Change-Id: Ia3fdd45c839f64df9610b93cc0023c80ab72e465 * Injecting api key sooner. Change-Id: I106362be7128ebdeed133febfc1c8dc2ad062370 * Create local.properties file. Change-Id: Ifab425e1a664da74c407bb6d2bf7bf76d1cd3854 * Move api key inject a step before connectedCheck. Change-Id: Idf737d9d1e7eab3d9b94d60d11cd9f2b0d8a70b2 * Simplify test workflow. Change-Id: Ic55a817da8fb344ac0185c50c107e582b6f42de5 * Add back synced test.yml and rename to instrumentation-test.yml Change-Id: I3a9ffe7b48591e7e0ec0c79f79d86a34aec6110a * Add debug statement. Change-Id: Ia75de3fe64826dea3cbd807a1c82cbd06e06a291 * Inject API key directly in AndroidManifest.xml Change-Id: Ie922328946106712d66791a0589f592b3109b36a * Update layout on sample app. Change-Id: I26075247a5cf0ba5a889fb67fbb08b6312cd076a * Use a single API level. Change-Id: Ifd2aa8abfce30251207e057565b6694787164db6 * s/waitForIdle/waitUntil Change-Id: Icef7d759573d1459a37e3d9058a1214ee2e52301 * Adjust rounding error. Change-Id: Ieefa005ab8c83e3d287a53f00c8b79c229f5a372 * chore: Synced local '.github/' with remote 'sync-files/android/.github/' (googlemaps#56) * chore: Add license headers. (googlemaps#55) Change-Id: I0029c52b31fdc977e5c6e105ace5df7097b7c287 * docs(README): Clarify that MapUiSettings should be preferred. (googlemaps#59) Change-Id: I4af84fa16323ddd867c08f9d2e65ffc8da3fe6bb * feat: Add support to get projection. (googlemaps#53) * feat: Add support to get projection. Change-Id: I999c56e6a31e33dd4c8d20215d24f56509a3e967 * Add tests for projection. Change-Id: Ia1a9b3dd95080e5df62e4aadf5fd46b03d82175b * Update waitUntil duration. Change-Id: Ic71bdd2fa8efe9a7a949d96b25010ee744c54a68 * chore(release): 1.3.0 [skip ci] # [1.3.0](googlemaps/android-maps-compose@v1.2.0...v1.3.0) (2022-03-10) ### Features * Add support to get projection. ([googlemaps#53](googlemaps#53)) ([e085588](googlemaps@e085588)) * chore: Synced local '.github/' with remote 'sync-files/android/.github/' (googlemaps#63) * fix: Use firstOrNull vs first to prevent NoSuchElementException (googlemaps#43) * Fixed Collection contains no element matching the predicate. * first or null overlay node; no wildcard imports * chore(release): 1.3.1 [skip ci] ## [1.3.1](googlemaps/android-maps-compose@v1.3.0...v1.3.1) (2022-03-14) ### Bug Fixes * Use firstOrNull vs first to prevent NoSuchElementException ([googlemaps#43](googlemaps#43)) ([c68fc93](googlemaps@c68fc93)) * docs: Link releases for required version of Compose. (googlemaps#66) Change-Id: I00da3f9b6db4b406da5aca61ed25550feb311afe * feat: Add marker drag event callback to Marker composable (googlemaps#11) * Fix renamed property name on README * Replace MarkerDragState with onMarkerDrag callback * Add sample code to show how to use marker drag callback * Fix API doc * Introduce MarkerPositionState * Remove onMarkerDrag callback * Update sample * Implement `rememberSaveable` for MarkerPositionState * chore: Bump version to 2.0-SNAPSHOT (googlemaps#68) Change-Id: Id18d60619ad573ef79b0c81dd05cda7e84fc8a4a * feat: Support showing/hiding info window. (googlemaps#69) * feat: Support showing/hiding info window. Change-Id: I3de945bd8847edd533b89d7b8f1381114df56469 * Update waitUntil timeout. Change-Id: I5bc6b43171745e2d3df20a09e04fc116297c196d * Add test for reusing markerstate. Change-Id: Iccdbd0f53f424df3bf055f9b3083640716be6487 * docs: Update README to use MarkerState. (googlemaps#70) Change-Id: I35b57a23cf80d8bf48bba31191f5415ebec72a3d * chore: Synced local '.github/' with remote 'sync-files/android/.github/' (googlemaps#73) * chore(release): 2.0.0 Change-Id: Id72487859f494d724cc15c55e66eb63d61b52c2d * chore: Remove core-ktx from demo app. (googlemaps#77) Change-Id: Iffc775b16a8727589957551bd91d5029d5f7ae33 * chore: Synced local '.github/' with remote 'sync-files/android/.github/' (googlemaps#80) * chore: Update blunderbuss.yml. (googlemaps#81) Change-Id: Ic0b2a92f1d2b37b4f86a0ad2935961d8748b0673 * feat: Support specifying animation duration for camera changes. (googlemaps#83) * feat: Support specifying animation duration for camera changes. Change-Id: I18e60574610a5c248627af75369498c9e3e16889 * Use null to indicate default animation duration. Change-Id: I13fcb192b0e43d40380d07ed2933cd2c29b7d14a * Use Int.MAX_VALUE for default animation. Change-Id: Ib77e018ad175e591419191a736d38c1b01bf3c45 * Check for MAX_VALUE for default animation. Change-Id: I65c007e74dc4d5f2d9b3469d2d4edf4c542fad72 * chore(release): 2.1.0 [skip ci] # [2.1.0](googlemaps/android-maps-compose@v2.0.0...v2.1.0) (2022-03-31) ### Features * Support specifying animation duration for camera changes. ([googlemaps#83](googlemaps#83)) ([aa02773](googlemaps@aa02773)) * chore: Add maps_android_compose_dependency region tag. (googlemaps#96) * chore: Add maps_android_compose_dependency region tag. Change-Id: I1569e7586eaefa51c05a15d6bf300809dbdece4e * Fix spacing. Change-Id: Ia2eb9574e3137472d7ec8448a70adfc74992b52d * chore: Edit maps_compose_dependency region tag. (googlemaps#97) Change-Id: Id511cb48f8ce6bb8fde6ac467d3877ffc07dc7ed * fix: Ensure CameraPositionState map reference clears when GoogleMap l… (googlemaps#109) * fix: Ensure CameraPositionState map reference clears when GoogleMap leaves composition. Change-Id: Iccf615e1a468ea7fbddb652052cec51b5d98a763 * Adding test for GoogleMap coming in and out of the Composition. Change-Id: I4e4bcdd39561ca17515e4c4891742c50bd14e409 * PR feedback. Change-Id: I4e7ce6dc006cee72f49e29b7eb15303ae71e4654 * chore(release): 2.1.1 [skip ci] ## [2.1.1](googlemaps/android-maps-compose@v2.1.0...v2.1.1) (2022-05-06) ### Bug Fixes * Ensure CameraPositionState map reference clears when GoogleMap l… ([googlemaps#109](googlemaps#109)) ([2f09c6d](googlemaps@2f09c6d)) * docs: Add compose version requirement to installation (googlemaps#101) * docs: Add compose version requirement to installation Users are installing older version of Compose and hitting problems - see: * googlemaps#67 (comment) * https://discordapp.com/channels/676948200904589322/939194796138975263/966366331492524102 This change makes it clearer in the installation instructions what the current base Compose requirement is without needing to look through the release notes. * chore: Fix formatting * docs: Add missing Javadocs for GoogleMap contentPadding parameter (googlemaps#93) docs: Add missing Javadocs for GoogleMap contentPadding parameter * chore: Synced file(s) with googlemaps/.github (googlemaps#114) * chore: Created local '.github/sync-repo-settings.yaml' from remote '.github/sync-repo-settings.yaml' * chore: Created local '.github/workflows/dependabot.yml' from remote '.github/workflows/dependabot.yml' * build: standardize check name (googlemaps#115) * build: update required checks (googlemaps#117) * chore: fix typo in check name (googlemaps#118) * chore: Bump library dependency in app (googlemaps#111) * chore: Synced local '.github/workflows/dependabot.yml' with remote '.github/workflows/dependabot.yml' (googlemaps#119) * chore: Synced local '.github/workflows/dependabot.yml' with remote '.github/workflows/dependabot.yml' (googlemaps#120) * chore: simplify dependabot workflow * chore: update pull request approval comment * chore: fix approval by providing url to pr * chore: add --approve * chore: only approve * chore: Use explicitApi. (googlemaps#121) Change-Id: Ibd3d442cefe59a194bb648143b802262e0ce5649 * build: change dependabot interval to weekly (googlemaps#125) * feat: Update to Compose 1.2.0-beta02 (googlemaps#124) * feat: Update to Compose 1.2.0-beta02 Change-Id: I37b7a0e3388a0bbb10ae75149ebd81a07e2bde26 * Fix tests. Change-Id: I9c1a71e78f527a7c2041a5e4af327ecd1728cb47 * chore(release): 2.2.0 [skip ci] # [2.2.0](googlemaps/android-maps-compose@v2.1.1...v2.2.0) (2022-05-23) ### Features * Update to Compose 1.2.0-beta02 ([googlemaps#124](googlemaps#124)) ([09765d1](googlemaps@09765d1)) * chore: update owners (googlemaps#127) * chore: Add GoogleMapComposable annotation. (googlemaps#130) * chore: Add GoogleMapComposable annotation. * Address PR feedback. Change-Id: I2819da87f7b3c3f1505402ff718b2139b6b1d114 * Remove GoogleMapComposable annotation from sample. Change-Id: Ide6c50f04e62ed4f4fae1d50f4500d677d41bd76 * docs: Update demo app to include more examples (googlemaps#94) It now includes a new main activity with buttons for each example - the previous example is now "Basic Map Activity". It also adds an example for using the map in a column to serve as documentation for googlemaps#78 - "Map In Column Activity". * fix: Prevent MapView from being recreated when still in composition (googlemaps#137) Addresses a bug wherein the underlying MapView gets recreated due to lifecycle events, however, the GoogleMap composable never leaves the composition. In this state, map properties (markers, camera position, etc.) don't get restored. Change-Id: Iba141aa2f325fa505ef8cc2f015117bcaea856c4 * chore(release): 2.2.1 [skip ci] ## [2.2.1](googlemaps/android-maps-compose@v2.2.0...v2.2.1) (2022-06-07) ### Bug Fixes * Prevent MapView from being recreated when still in composition ([googlemaps#137](googlemaps#137)) ([83b1cd7](googlemaps@83b1cd7)) Co-authored-by: Chris Arriola <c.rriola@gmail.com> Co-authored-by: googlemaps-bot <googlemaps-bot@google.com> Co-authored-by: semantic-release-bot <semantic-release-bot@martynus.net> Co-authored-by: Takao Chiba <chibatching@users.noreply.github.com> Co-authored-by: Chris Arriola <chrisarriola@google.com> Co-authored-by: Sean Barbeau <sjbarbeau@gmail.com> Co-authored-by: Justin Poehnelt <jpoehnelt@google.com>

Fix renamed property name on README

a428082

blunderbuss-gcf bot assigned arriolac Feb 6, 2022

chibatching force-pushed the add_marker_drag_callback branch from bfb7c01 to 5b6257f Compare February 8, 2022 05:34

chibatching changed the title ~~feat: Add marker drag event callbacks to Marker composable~~ feat: Add marker object property to MarkerDragState Feb 8, 2022

chibatching mentioned this pull request Feb 8, 2022

Marker drag event callback #10

Closed

arriolac reviewed Feb 8, 2022

View reviewed changes

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

Merge branch 'googlemaps:main' into main

a760b63

chibatching commented Feb 9, 2022

View reviewed changes

arriolac reviewed Feb 9, 2022

View reviewed changes

chibatching force-pushed the add_marker_drag_callback branch 3 times, most recently from cca773a to 0b0d126 Compare February 11, 2022 20:56

chibatching added 2 commits February 12, 2022 05:57

Replace MarkerDragState with onMarkerDrag callback

d471f89

Add sample code to show how to use marker drag callback

10de5f4

chibatching force-pushed the add_marker_drag_callback branch from 0b0d126 to 10de5f4 Compare February 11, 2022 20:57

Fix API doc

fe7c358

chibatching requested a review from arriolac February 12, 2022 02:22

chibatching changed the title ~~feat: Add marker object property to MarkerDragState~~ feat: Add marker drag event to Marker composable Feb 12, 2022

chibatching changed the title ~~feat: Add marker drag event to Marker composable~~ feat: Add marker drag event callback to Marker composable Feb 12, 2022

chibatching and others added 2 commits February 15, 2022 10:40

Merge branch 'googlemaps:main' into main

2c15555

Merge branch 'main' into add_marker_drag_callback

5b86c41

# Conflicts: # app/src/main/java/com/google/maps/android/compose/MapSampleActivity.kt # maps-compose/src/main/java/com/google/maps/android/compose/MapApplier.kt # maps-compose/src/main/java/com/google/maps/android/compose/Marker.kt

arriolac reviewed Feb 15, 2022

View reviewed changes

Introduce MarkerPositionState

2e6904f

chibatching commented Feb 16, 2022

View reviewed changes

chibatching requested a review from arriolac February 20, 2022 19:18

Merge branch 'main' into add_marker_drag_callback

b86fc24

chibatching and others added 4 commits March 5, 2022 14:22

Merge branch 'googlemaps:main' into main

2eb193c

Remove onMarkerDrag callback

40ef5f8

Merge branch 'main' into add_marker_drag_callback

bfad031

Update sample

f4ed458

arriolac reviewed Mar 7, 2022

View reviewed changes

chibatching changed the base branch from main to maps-compose-2.0.x March 8, 2022 00:11

arriolac reviewed Mar 9, 2022

View reviewed changes

Implement rememberSaveable for MarkerPositionState

2d56288

chibatching requested a review from arriolac March 9, 2022 23:48

arriolac approved these changes Mar 9, 2022

View reviewed changes

Merge branch 'maps-compose-2.0.x' into add_marker_drag_callback

5920db1

arriolac mentioned this pull request Mar 10, 2022

Release 2.0-SNAPSHOT #60

Closed

arriolac merged commit 866ee1e into googlemaps:maps-compose-2.0.x Mar 10, 2022

chibatching deleted the add_marker_drag_callback branch March 11, 2022 00:39

arriolac mentioned this pull request Mar 17, 2022

chore: Maps Compose 2.0.0 #72

Merged

bubenheimer mentioned this pull request Jun 16, 2022

Marker dragging: fix loss of drag events and difficulty observing marker positions #149

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add marker drag event callback to Marker composable #11

feat: Add marker drag event callback to Marker composable #11

feat: Add marker drag event callback to Marker composable #11

feat: Add marker drag event callback to Marker composable #11

Conversation

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment