Google maps marker drag events impl #2838

JamesMcIntosh · 2020-06-19T02:04:08Z

Description

Add events to track start of drag and position during drag

Related Issues

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
No, this is not a breaking change.

ened

Looks really good.

...ges/google_maps_flutter/google_maps_flutter_platform_interface/lib/src/events/map_event.dart

packages/google_maps_flutter/google_maps_flutter_platform_interface/CHANGELOG.md

..._maps_flutter/android/src/test/java/io/flutter/plugins/googlemaps/MarkersControllerTest.java

ditman · 2021-07-26T23:12:11Z

@stuartmorgan this is part 2 of #2653, which is blocked in "needs tests" status. I'll try to add some tests to 2653 so this one can be reviewed+merged too!

JamesMcIntosh · 2021-09-16T11:43:57Z

@stuartmorgan this is part 2 of #2653, which is blocked in "needs tests" status. I'll try to add some tests to 2653 so this one can be reviewed+merged too!

Hi @ditman, did you have any suggestions for what tests would be suitable for the platform interface to get this finally merged?

stuartmorgan-g · 2021-09-16T14:14:35Z

Hi @ditman, did you have any suggestions for what tests would be suitable for the platform interface to get this finally merged?

You should add tests that cover all of the new code you're adding in that PR:

That the new method channel code calls through as expected: https://github.com/flutter/plugins/blob/master/packages/google_maps_flutter/google_maps_flutter_platform_interface/test/method_channel/method_channel_google_maps_flutter_test.dart
That the new Marker code works as expected (e.g., is preserved in copies, which we've had bugs with in the past). Looks like Marker is unfortunately not tested at the moment, but you can look at other object tests as a starting point: https://github.com/flutter/plugins/tree/master/packages/google_maps_flutter/google_maps_flutter_platform_interface/test/types

packages/google_maps_flutter/google_maps_flutter/example/pubspec.yaml

..._flutter_platform_interface/test/method_channel/method_channel_google_maps_flutter_test.dart

packages/google_maps_flutter/google_maps_flutter/CHANGELOG.md

packages/google_maps_flutter/google_maps_flutter_platform_interface/CHANGELOG.md

ditman · 2021-09-24T01:37:04Z

(Apologies, I didn't mean to do a full review of this, but I got carried away with a bunch of "single comments" :/)

JamesMcIntosh · 2021-09-24T04:21:40Z

@ditman I've made those changes, it looks like the build is currently failing the submit-queue with test failures in webview_flutter/webview_flutter:
example/integration_test/webview_flutter_test.dart failed tests

ditman · 2021-09-24T20:23:17Z

@JamesMcIntosh yes, we suspect those tests are flaky, and most likely don't have anything to do with the changes in this branch. I can restart them to get them to pass!

ditman · 2021-09-24T23:32:09Z

I'm more concerned about the web-platform_tests, I suspect they're breaking because of forcing the platform interface to be ^2.1 before implementing the marker events in web. I'm trying to reproduce the issue, but it happens only on this branch, not in the current master (good :P)

JamesMcIntosh · 2021-09-24T23:50:43Z

@ditman You are correct the test is failing because these are not implemented on web. From CI logs:

The following UnimplementedError was thrown running a test:
onMarkerDragEnd() has not been implemented.

I don't think this should stop this PR from being merged as implementing these methods for web need to be done in a separate PR.

JamesMcIntosh · 2021-09-25T11:56:28Z

@ditman I've fixed up the web version and submitted a PR #4385, could you please review it also?

JamesMcIntosh · 2021-09-29T10:34:33Z

@ditman Where are we at now with getting this merged?
Were you able to get a better understanding of the issue which is causing the web-platform to fail?
Can this branch be published with the failing checks?
Will the new PR which I have submitted for the web-platform be enough to fix these failing checks, maybe if it is published first and this branch then rebased on top of it?

stuartmorgan-g · 2021-09-29T15:53:52Z

Can this branch be published with the failing checks?

No. If something is deliberately being left un-implemented, that un-implemented behavior must not break our automated tests.

Will the new PR which I have submitted for the web-platform be enough to fix these failing checks, maybe if it is published first and this branch then rebased on top of it?

If there's a web implementation already ready to review and land, then landing that first such that this works on all platforms when enabled is much preferable.

ditman · 2021-09-29T17:40:44Z

I'm sorry @JamesMcIntosh, I took a couple of days off, but forgot to update my github status before leaving. I'll work on getting this feature merged this week. Thanks for your patience!

ditman · 2021-09-29T18:35:12Z

I wanted to understand better this failure, because adding methods to a platform interface should be supported without adding it to all platforms at the same time.

The issue is that google_maps_flutter_web uses google_maps_flutter in one of its integration tests. Since this change inconditionally calls the new methods here when initializing a Maps Controller, the web implementation breaks.

New methods need to be either called conditionally, wrapped in try/catch or avoided (in cross-platform code paths) until the web implementation (actually: all other platforms) land.

(I'll take a look at the web implementation PR mentioned above!)

ditman · 2021-09-30T01:23:14Z

Will the new PR which I have submitted for the web-platform be enough to fix these failing checks, maybe if it is published first and this branch then rebased on top of it?

Yes, the web PR seems to be enough to fix the tests that are failing in this PR (verified by cloning this branch, and running the web tests, then cherry-picking the web PR and running the tests again).

The only change required to this branch for tests to pass after the web branch is merged is one final rebase with master, since the dependency on the test app is by path, not a download from pub.dev.

ditman · 2021-09-30T01:49:10Z

(I've landed the web PR: 90b2844)

ditman · 2021-09-30T17:04:33Z

(Pushing the merge with master to see if it makes the tests here green)

ditman

This looks good to me, a little bit light on the testing front, but at least we're checking that things are being initialized as needed. Let's go!

ditman · 2021-09-30T17:54:22Z

..._maps_flutter/android/src/test/java/io/flutter/plugins/googlemaps/MarkersControllerTest.java

+    final GoogleMap googleMap = mock(GoogleMap.class);
+    controller.setGoogleMap(googleMap);
+
+    final zzt z = mock(zzt.class);


What is class zzt? It's being imported from internal, will it keep its name in the future?

Why not mock Marker instead of zzt?

This may actually be very fragile if it's actually obfuscated code (with a name like that it could very well be).
I think that zzt might be something to do with the map projection but don't quote me, it's been a year and a half since I wrote that so I'm not totally sure without digging back into the source code as creation of Markers is hidden behind a factory pattern.
I just tested mocking only Marker and the test work fine, I'm not not sure why I didn't do that in the first place.
I'll create a new PR with this change.

Thanks for coming back to this one, I'll take a look at the new PR!!

ditman · 2021-09-30T18:48:51Z

packages/google_maps_flutter/google_maps_flutter/example/lib/main.dart

@@ -34,6 +35,7 @@ final List<GoogleMapExampleAppPage> _allPages = <GoogleMapExampleAppPage>[
  PlacePolylinePage(),
  PlacePolygonPage(),
  PlaceCirclePage(),
+  DragMarkerPage(),


There's another demo that deals with dragging markers, maybe this one can be merged into the PlaceMarkerPage (or the dragging demo of the PlaceMarkerPage can be moved to this one)

I've merged them and add it to the PR #4400

…#2838)

* master: [google_maps_flutter] Add Marker drag events (flutter#2838) [flutter_plugin_tools] Validate pubspec description (flutter#4396) Add file_selector to the repo list (flutter#4395) [in_app_purchase] Fix in_app_purchase_android/README.md (flutter#4363) [google_maps_flutter_web] Add Marker drag events (flutter#4385) [webview_flutter] Fixed todos in FlutterWebView.java (flutter#4394) Handle `PurchaseStatus.restored` correctly in example. (flutter#4393) Handle restored purchases in iOS example app (flutter#4392) [file_selector] Remove custom analysis options (flutter#4382) [flutter_plugin_tools] Check licenses in Kotlin (flutter#4373) Fixed _CastError when running example App (flutter#4390) [in_app_purchase] Ensure the introductoryPriceMicros field is transported as a String. (flutter#4370) Load navigation controls immediately. (flutter#4377) [camera] Fix IllegalStateException being thrown in Android implementation when switching activities. (flutter#4319) # Conflicts: # packages/webview_flutter/webview_flutter/CHANGELOG.md # packages/webview_flutter/webview_flutter_android/CHANGELOG.md

* master: (1126 commits) [webview_flutter] Adjust test URLs again (flutter#4407) [google_sign_in] Add serverAuthCode attribute to google_sign_in_platform_interface user data (flutter#4179) [camera] Add filter for unsupported cameras on Android (flutter#4418) [webview_flutter] Update webview platform interface with new methods for running JavaScript. (flutter#4401) [webview_flutter] Add zoomEnabled to webview flutter platform interface (flutter#4404) [ci] Remove obsolete Dockerfile (flutter#4405) Fix order-dependant platform interface tests (flutter#4406) [google_maps_flutter]: LatLng longitude loses precision in constructor #90574 (flutter#4374) [google_maps_flutter] Add Marker drag events (flutter#2838) [flutter_plugin_tools] Validate pubspec description (flutter#4396) Add file_selector to the repo list (flutter#4395) [in_app_purchase] Fix in_app_purchase_android/README.md (flutter#4363) [google_maps_flutter_web] Add Marker drag events (flutter#4385) [webview_flutter] Fixed todos in FlutterWebView.java (flutter#4394) Handle `PurchaseStatus.restored` correctly in example. (flutter#4393) Handle restored purchases in iOS example app (flutter#4392) [file_selector] Remove custom analysis options (flutter#4382) [flutter_plugin_tools] Check licenses in Kotlin (flutter#4373) Fixed _CastError when running example App (flutter#4390) [in_app_purchase] Ensure the introductoryPriceMicros field is transported as a String. (flutter#4370) ... # Conflicts: # packages/quick_actions/ios/Classes/FLTQuickActionsPlugin.m

googlebot added the cla: yes label Jun 19, 2020

JamesMcIntosh mentioned this pull request Jun 19, 2020

[google_maps_flutter] Marker dragging events #2653

Merged

13 tasks

JamesMcIntosh force-pushed the google-maps-marker-drag-events-impl branch from d85ddc9 to 56fdb95 Compare September 10, 2020 03:09

JamesMcIntosh force-pushed the google-maps-marker-drag-events-impl branch from 4a3e901 to cf31ef1 Compare October 14, 2020 20:55

JamesMcIntosh requested a review from cyanglaz as a code owner October 14, 2020 20:55

stuartmorgan-g added the p: google_maps_flutter label Jan 29, 2021

JamesMcIntosh force-pushed the google-maps-marker-drag-events-impl branch from cf31ef1 to 10c71a9 Compare April 26, 2021 13:35

github-actions bot added platform-android platform-ios labels Apr 26, 2021

ened reviewed Apr 29, 2021

View reviewed changes

JamesMcIntosh force-pushed the google-maps-marker-drag-events-impl branch 4 times, most recently from 51e1faf to f80d45d Compare July 26, 2021 05:09

stuartmorgan-g requested a review from ditman July 26, 2021 17:46

JamesMcIntosh force-pushed the google-maps-marker-drag-events-impl branch 2 times, most recently from 65741b8 to dceda4d Compare September 22, 2021 11:33

ditman reviewed Sep 24, 2021

View reviewed changes

packages/google_maps_flutter/google_maps_flutter/example/pubspec.yaml Outdated Show resolved Hide resolved

ditman reviewed Sep 24, 2021

View reviewed changes

..._flutter_platform_interface/test/method_channel/method_channel_google_maps_flutter_test.dart Outdated Show resolved Hide resolved

ditman reviewed Sep 24, 2021

View reviewed changes

packages/google_maps_flutter/google_maps_flutter/CHANGELOG.md Outdated Show resolved Hide resolved

ditman reviewed Sep 24, 2021

View reviewed changes

packages/google_maps_flutter/google_maps_flutter_platform_interface/CHANGELOG.md Outdated Show resolved Hide resolved

JamesMcIntosh force-pushed the google-maps-marker-drag-events-impl branch 2 times, most recently from 8f0623c to 693adf9 Compare September 24, 2021 04:18

JamesMcIntosh mentioned this pull request Sep 25, 2021

[google_maps_flutter_web] Add Marker drag events #4385

Merged

ditman self-assigned this Sep 29, 2021

Add drag events

5f527e3

JamesMcIntosh force-pushed the google-maps-marker-drag-events-impl branch from 693adf9 to 5f527e3 Compare September 29, 2021 20:16

Merge branch 'master' into google-maps-marker-drag-events-impl

21cc7ab

This comment has been minimized.

Sign in to view

google-cla bot added cla: no and removed cla: yes labels Sep 30, 2021

This comment has been minimized.

Sign in to view

google-cla bot added cla: yes and removed cla: no labels Sep 30, 2021

Merge branch 'master' into google-maps-marker-drag-events-impl

af08afe

ditman approved these changes Sep 30, 2021

View reviewed changes

ditman merged commit f6d93a7 into flutter:master Sep 30, 2021

engine-flutter-autoroll mentioned this pull request Sep 30, 2021

Roll Plugins from 63eb67532a7a to f6d93a765063 (2 revisions) flutter/flutter#91058

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 30, 2021

f6d93a7 [google_maps_flutter] Add Marker drag events (flutter/plugins…

e5b15ad

…#2838)

JamesMcIntosh mentioned this pull request Oct 1, 2021

[google_maps_flutter] Clean Java test, consolidate Marker example. #4400

Merged

amantoux pushed a commit to amantoux/plugins that referenced this pull request Dec 11, 2021

[google_maps_flutter] Add Marker drag events (flutter#2838)

588e89b

KyleFin pushed a commit to KyleFin/plugins that referenced this pull request Dec 21, 2021

[google_maps_flutter] Add Marker drag events (flutter#2838)

36c886d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Google maps marker drag events impl #2838

Google maps marker drag events impl #2838

JamesMcIntosh commented Jun 19, 2020

ened left a comment

ditman commented Jul 26, 2021

JamesMcIntosh commented Sep 16, 2021

stuartmorgan-g commented Sep 16, 2021

ditman commented Sep 24, 2021

JamesMcIntosh commented Sep 24, 2021

ditman commented Sep 24, 2021

ditman commented Sep 24, 2021

JamesMcIntosh commented Sep 24, 2021 •

edited

Loading

JamesMcIntosh commented Sep 25, 2021

JamesMcIntosh commented Sep 29, 2021 •

edited

Loading

stuartmorgan-g commented Sep 29, 2021

ditman commented Sep 29, 2021

ditman commented Sep 29, 2021

ditman commented Sep 30, 2021

ditman commented Sep 30, 2021

ditman commented Sep 30, 2021

This comment has been minimized.

This comment has been minimized.

ditman left a comment

ditman Sep 30, 2021

JamesMcIntosh Oct 1, 2021

JamesMcIntosh Oct 1, 2021

ditman Oct 1, 2021

ditman Sep 30, 2021

JamesMcIntosh Oct 1, 2021 •

edited

Loading

Google maps marker drag events impl #2838

Google maps marker drag events impl #2838

Conversation

JamesMcIntosh commented Jun 19, 2020

Description

Related Issues

Checklist

Breaking Change

ened left a comment

Choose a reason for hiding this comment

ditman commented Jul 26, 2021

JamesMcIntosh commented Sep 16, 2021

stuartmorgan-g commented Sep 16, 2021

ditman commented Sep 24, 2021

JamesMcIntosh commented Sep 24, 2021

ditman commented Sep 24, 2021

ditman commented Sep 24, 2021

JamesMcIntosh commented Sep 24, 2021 • edited Loading

JamesMcIntosh commented Sep 25, 2021

JamesMcIntosh commented Sep 29, 2021 • edited Loading

stuartmorgan-g commented Sep 29, 2021

ditman commented Sep 29, 2021

ditman commented Sep 29, 2021

ditman commented Sep 30, 2021

ditman commented Sep 30, 2021

ditman commented Sep 30, 2021

This comment has been minimized.

This comment has been minimized.

ditman left a comment

Choose a reason for hiding this comment

ditman Sep 30, 2021

Choose a reason for hiding this comment

JamesMcIntosh Oct 1, 2021

Choose a reason for hiding this comment

JamesMcIntosh Oct 1, 2021

Choose a reason for hiding this comment

ditman Oct 1, 2021

Choose a reason for hiding this comment

ditman Sep 30, 2021

Choose a reason for hiding this comment

JamesMcIntosh Oct 1, 2021 • edited Loading

Choose a reason for hiding this comment

JamesMcIntosh commented Sep 24, 2021 •

edited

Loading

JamesMcIntosh commented Sep 29, 2021 •

edited

Loading

JamesMcIntosh Oct 1, 2021 •

edited

Loading