Support Hybrid Composition through the GoogleMaps Widget #4082

bparrishMines · 2021-06-23T06:23:20Z

Since the changes in #4017 avoids a breaking change, a new buildView method needed to be added. This switches the GoogleMaps widget to use this new platform specific method.

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

stuartmorgan · 2021-06-24T19:51:40Z

packages/google_maps_flutter/google_maps_flutter/README.md

@@ -46,6 +46,19 @@ This means that app will only be available for users that run Android SDK 20 or
               android:value="YOUR KEY HERE"/>
 ```

+#### Hybrid Composition
+
+To use Hybrid Composition to render the `GoogleMap` widget on Android. Set the


Nit: s/. Set the/, set/

stuartmorgan · 2021-06-24T19:52:19Z

packages/google_maps_flutter/google_maps_flutter/README.md

@@ -46,6 +46,19 @@ This means that app will only be available for users that run Android SDK 20 or
               android:value="YOUR KEY HERE"/>
 ```

+#### Hybrid Composition
+
+To use Hybrid Composition to render the `GoogleMap` widget on Android. Set the


Nit: maybe link "Hybrid Composition" to an explanation of what that is for people who don't know?

stuartmorgan · 2021-06-24T19:55:27Z

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

@@ -66,5 +68,11 @@ class MapsDemo extends StatelessWidget {
 }

 void main() {
+  WidgetsFlutterBinding.ensureInitialized();


Why is this necessary? Setting that flag doesn't send a message, does it?

You're right. Force of habit. removed

stuartmorgan · 2021-06-24T19:57:40Z

packages/google_maps_flutter/google_maps_flutter/lib/src/google_map.dart

-    return GoogleMapsFlutterPlatform.instance.buildView(
+    final GoogleMapsFlutterPlatform platform =
+        GoogleMapsFlutterPlatform.instance;
+    if (platform is MethodChannelGoogleMapsFlutter) {


Why can't the new method be called unconditionally? The delegation to the old method was supposed to be handled by a default implementation in the platform interface.

The default implementation (MethodChannelGoogleMapsFlutter) does call the old method if the platform is not android or hybrid composition is turned off. However, web switches to another platform instance: https://github.com/flutter/plugins/blob/master/packages/google_maps_flutter/google_maps_flutter_web/lib/src/google_maps_flutter_web.dart#L13.

So in this case, we would need to call the other method, right?

Sorry, that was poorly phrased; by "default implementation" I mean the code that's part of the base class that all implementations extend from. Having a fallback implementation there is what makes this overall change harmless even for federated implementations we don't even know about.

Ah ok. So buildViewWithTextDirection was only added to MethodChannelGoogleMapsFlutter and not to GoogleMapsFlutterPlatform. This is same for the flag useAndroidViewSurface. This was done because this was an Android only requirement.

Were you thinking that buildViewWithTextDirection should have been added to GoogleMapsFlutterPlatform as well? I think that makes more sense and could be done without a breaking change.

That was what I had intended in the discussion, yes. However, IIRC there was discussion of this being temporary, in which case I'm okay with this approach if you don't want to go back and make another change to the platform interface given that it's already been done. (If this were permanent, it would be more important that we not embed implementation-specific code here as much as possible.)

For this method to only be temporary, the platform_interface would have to be broken and bumped to version 3.0. This is because any implementation would need to add the additional parameter to their method. However, I believe the policy is to avoid breaking changes to the platform_interface.

An alternative is to have two methods, and to call buildViewWithTextDirection by default in google_maps_flutter. But this would still break any implementation that doesn't implement buildViewWithTextDirection as the method would throw an UnimplementedError().

This PR essentially makes buildViewWithTextDirection unique to MethodChannelPlatform. The GoogleMap widget then calls it iff it is MethodChannelPlatform. I believe this avoids any breaking changes and any of the implementations would still work with the plugin.

An alternative is to have two methods, and to call buildViewWithTextDirection by default in google_maps_flutter. But this would still break any implementation that doesn't implement buildViewWithTextDirection as the method would throw an UnimplementedError().

That's why I had recommended having the method not throw UnimplementedError, but instead calling the old method.

But my question wasn't about implemented, but the ideal goal: do we want this new parameter forever, or is the need for this parameter temporary? I had thought it was the latter, but again, I may have misunderstood.

Ah, you wanted that done at the interface level and not the implementation level. That makes since.

This parameter is required for Android hybrid composition unless we don't want the user to be able to change the text direction. I haven't seen a way around it yet that doesn't involve framework changes. My intention is that it's a permanent parameter.

Okay, sorry for the confusion on my part about the intended lifespan. In that case yes, I think we should update the platform interface to have a default implementation that calls the old method with a default value. Then implementations (those that want a different behavior than that default) can override both, but nobody will be broken and the details of who implements what won't bleed into this package.

That works for me! I updated #4121 to reflect this.

bparrishMines · 2021-08-15T18:10:44Z

packages/google_maps_flutter/google_maps_flutter/lib/src/google_map.dart

      _mapId,
      onPlatformViewCreated,
+      textDirection: widget.textDirection ??
+          Directionality.maybeOf(context) ??


This may return null if there is no Directionality ancestor. This was the case with one of the tests, so I also added a default TextDirection.ltr.

bparrishMines · 2021-08-17T06:00:51Z

@stuartmorgan I believe this is ready for another review.

stuartmorgan

Sorry for the delay, just a couple of small things.

packages/google_maps_flutter/google_maps_flutter/README.md

stuartmorgan · 2021-09-30T20:25:54Z

Was this ready for me to review again?

bparrishMines · 2021-10-05T18:47:02Z

Was this ready for me to review again?

@stuartmorgan I forgot that this hadn't landed yet. But, yes it's ready for another review.

stuartmorgan

LGTM other than a nit about assertions.

stuartmorgan · 2021-10-21T15:12:11Z

packages/google_maps_flutter/google_maps_flutter/lib/src/google_map.dart

@@ -38,6 +38,48 @@ class UnknownMapObjectIdError extends Error {
  }
 }

+/// Android specific settings for [GoogleMap].
+class AndroidGoogleMapsFlutter {


I would expect this to be a MethodChannelGoogleMapsFlutter subclass. I guess we can add that later without it being a breaking change though, and doing it now would have registration complexities since we don't have dartPluginClass support for mobile on stable yet.

stuartmorgan · 2021-10-21T15:13:56Z

packages/google_maps_flutter/google_maps_flutter/lib/src/google_map.dart

+      GoogleMapsFlutterPlatform.instance is MethodChannelGoogleMapsFlutter,
+      'This feature is only supported when `GoogleMapsFlutterPlatform.instance` '
+      'is a `MethodChannelGoogleMapsFlutter`; The default implementation for Android.',
+    );


This will interfere with people setting mock platform implementations for tests, which is the recommended way of mocking federated plugins. We should no-op it (returning false) rather than asserting.

stuartmorgan · 2021-10-21T15:14:14Z

packages/google_maps_flutter/google_maps_flutter/lib/src/google_map.dart

+      GoogleMapsFlutterPlatform.instance is MethodChannelGoogleMapsFlutter,
+      'This feature is only supported when `GoogleMapsFlutterPlatform.instance` '
+      'is a `MethodChannelGoogleMapsFlutter`; The default implementation for Android.',
+    );


…tter/plugins#4082)

* master: [webview_flutter] Deprecate evaluateJavascript in favour of runJavaScript and runJavaScriptForResult. (flutter#4403) [webview_flutter] Add interface methods to load local files and HTML strings. (flutter#4446) [ci] add pedantic dep to new in_app_purchase pkgs (flutter#4471) [ci] Remove unused dep from file_selector. (flutter#4468) [ci] update build_runner dep in android_intent and file_selector example (flutter#4467) [webview_flutter] Add platform interface method `loadRequest`. (flutter#4450) Remove -Werror from deprecated plugin Android builds (flutter#4466) [webview_flutter] Update webview packages for Android and iOS to implement `runJavascript` and `runJavascriptReturningResult`. (flutter#4402) [camera] Fix CamcorderProfile Usages (flutter#4423) [google_sign_in] Use a transparent image as a placeholder for the `GoogleUserCircleAvatar` (flutter#4391) [in_app_purchase]IAP/platform interface add cancel status (flutter#4093) [image_picker] doc:readme image picker misprints (flutter#4421) Support Hybrid Composition through the GoogleMaps Widget (flutter#4082) [path_provider] started supporting background platform channels (flutter#4443) [device_info] started using Background Platform Channels (flutter#4456) # Conflicts: # packages/webview_flutter/webview_flutter/CHANGELOG.md # packages/webview_flutter/webview_flutter/pubspec.yaml

bparrishMines added 4 commits June 16, 2021 17:52

start

b17d1dc

change to hybrid composition in example

82c4614

UPdate readme

7b6f801

Merge branch 'master' of github.com:flutter/plugins into android_maps

736c073

bparrishMines requested a review from cyanglaz as a code owner June 23, 2021 06:23

google-cla bot added the cla: yes label Jun 23, 2021

github-actions bot added the p: google_maps_flutter label Jun 23, 2021

bparrishMines changed the title ~~Android maps~~ Support Hybrid Composition through the GoogleMaps Widget Jun 23, 2021

stuartmorgan suggested changes Jun 24, 2021

View reviewed changes

update README and remove unnecessary line

f6631b4

bparrishMines mentioned this pull request Jun 30, 2021

Add buildViewWithTextDirection to platform interface #4121

Merged

11 tasks

bparrishMines added 4 commits August 12, 2021 11:10

Merge branch 'master' of github.com:flutter/plugins into android_maps

50b548e

use buildViewWithTextDirection by default

944d965

remove hash

e345f3a

add default value if no directionality ancestor

1809e66

bparrishMines commented Aug 15, 2021

View reviewed changes

bparrishMines added 2 commits August 15, 2021 13:39

remove mactro

f504ffc

change text direction to layout direction

be87b94

stuartmorgan suggested changes Aug 18, 2021

View reviewed changes

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

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

bparrishMines and others added 4 commits August 19, 2021 21:12

Merge branch 'master' of github.com:flutter/plugins into android_maps

cf87583

create android settings class

d907845

update readme

58c3028

Update google_map.dart

0c26d6c

stuartmorgan mentioned this pull request Sep 30, 2021

[Android 12][google_maps_flutter] BufferQueue has no connected producer flutter/flutter#90556

Closed

Merge branch 'master' of github.com:flutter/plugins into android_maps

86f965c

stuartmorgan approved these changes Oct 21, 2021

View reviewed changes

Merge branch 'master' of github.com:flutter/plugins into android_maps

35a8186

maheshmnj mentioned this pull request Oct 28, 2021

Displaying an AdWidget with another Platform view like GoogleMap or WebView freeze those views. googleads/googleads-mobile-flutter#32

Closed

bparrishMines merged commit 34ea0c3 into flutter:master Oct 28, 2021

bparrishMines deleted the android_maps branch October 28, 2021 17:54

engine-flutter-autoroll mentioned this pull request Oct 28, 2021

Roll Plugins from 93f04832c622 to 34ea0c3c1a09 (1 revision) flutter/flutter#92677

Merged

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

34ea0c3 Support Hybrid Composition through the GoogleMaps Widget (flu…

d469122

…tter/plugins#4082)

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

Support Hybrid Composition through the GoogleMaps Widget (flutter#4082)

93a870a

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

Support Hybrid Composition through the GoogleMaps Widget (flutter#4082)

b01926d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Hybrid Composition through the GoogleMaps Widget #4082

Support Hybrid Composition through the GoogleMaps Widget #4082

bparrishMines commented Jun 23, 2021 •

edited

stuartmorgan Jun 24, 2021

stuartmorgan Jun 24, 2021

stuartmorgan Jun 24, 2021

bparrishMines Jun 29, 2021

stuartmorgan Jun 24, 2021

bparrishMines Jun 29, 2021

stuartmorgan Jun 29, 2021

bparrishMines Jun 30, 2021

stuartmorgan Jun 30, 2021

bparrishMines Jul 20, 2021

stuartmorgan Jul 20, 2021

bparrishMines Jul 26, 2021

stuartmorgan Jul 26, 2021 •

edited

bparrishMines Aug 2, 2021

bparrishMines Aug 15, 2021 •

edited

bparrishMines commented Aug 17, 2021

stuartmorgan left a comment

stuartmorgan commented Sep 30, 2021

bparrishMines commented Oct 5, 2021

stuartmorgan left a comment

stuartmorgan Oct 21, 2021

stuartmorgan Oct 21, 2021

stuartmorgan Oct 21, 2021

Support Hybrid Composition through the GoogleMaps Widget #4082

Support Hybrid Composition through the GoogleMaps Widget #4082

Conversation

bparrishMines commented Jun 23, 2021 • edited

Pre-launch Checklist

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

stuartmorgan Jul 26, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparrishMines Aug 15, 2021 • edited

Choose a reason for hiding this comment

bparrishMines commented Aug 17, 2021

stuartmorgan left a comment

Choose a reason for hiding this comment

stuartmorgan commented Sep 30, 2021

bparrishMines commented Oct 5, 2021

stuartmorgan left a 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

bparrishMines commented Jun 23, 2021 •

edited

stuartmorgan Jul 26, 2021 •

edited

bparrishMines Aug 15, 2021 •

edited