Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

Support Hybrid Composition through the GoogleMaps Widget #4082

Merged
merged 17 commits into from
Oct 28, 2021
Merged

Support Hybrid Composition through the GoogleMaps Widget #4082

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
b17d1dc
start
bparrishMines Jun 17, 2021
82c4614
change to hybrid composition in example
bparrishMines Jun 17, 2021
7b6f801
UPdate readme
bparrishMines Jun 17, 2021
736c073
Merge branch 'master' of github.com:flutter/plugins into android_maps
bparrishMines Jun 23, 2021
f6631b4
update README and remove unnecessary line
bparrishMines Jun 29, 2021
50b548e
Merge branch 'master' of github.com:flutter/plugins into android_maps
bparrishMines Aug 12, 2021
944d965
use buildViewWithTextDirection by default
bparrishMines Aug 12, 2021
e345f3a
remove hash
bparrishMines Aug 12, 2021
1809e66
add default value if no directionality ancestor
bparrishMines Aug 12, 2021
f504ffc
remove mactro
bparrishMines Aug 15, 2021
be87b94
change text direction to layout direction
bparrishMines Aug 17, 2021
cf87583
Merge branch 'master' of github.com:flutter/plugins into android_maps
bparrishMines Aug 20, 2021
d907845
create android settings class
bparrishMines Aug 23, 2021
58c3028
update readme
bparrishMines Aug 23, 2021
0c26d6c
Update google_map.dart
bparrishMines Sep 14, 2021
86f965c
Merge branch 'master' of github.com:flutter/plugins into android_maps
bparrishMines Oct 5, 2021
35a8186
Merge branch 'master' of github.com:flutter/plugins into android_maps
bparrishMines Oct 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
start
  • Loading branch information
@bparrishMines
bparrishMines committed Jun 17, 2021
commit b17d1dc99bd45f00ca920c3f8b85900bffc1c4e5
26 changes: 25 additions & 1 deletion packages/google_maps_flutter/google_maps_flutter/lib/src/google_map.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ class GoogleMap extends StatefulWidget {
this.tiltGesturesEnabled = true,
this.myLocationEnabled = false,
this.myLocationButtonEnabled = true,
this.textDirection,

/// If no padding is specified default padding will be 0.
this.padding = const EdgeInsets.all(0),
Expand Down Expand Up @@ -100,6 +101,13 @@ class GoogleMap extends StatefulWidget {
/// Type of map tiles to be rendered.
final MapType mapType;

/// {@template flutter.widgets.AndroidView.layoutDirection}
/// The text direction to use for the embedded view.
///
/// If this is null, the ambient [Directionality] is used instead.
/// {@endtemplate}
final TextDirection? textDirection;

/// Preferred bounds for the camera zoom level.
///
/// Actual bounds depend on map data and device.
Expand Down Expand Up @@ -250,7 +258,23 @@ class _GoogleMapState extends State<GoogleMap> {

@override
Widget build(BuildContext context) {
return GoogleMapsFlutterPlatform.instance.buildView(
final GoogleMapsFlutterPlatform platform =
GoogleMapsFlutterPlatform.instance;
if (platform is MethodChannelGoogleMapsFlutter) {
Copy link
Contributor

@stuartmorgan-g stuartmorgan-g Jun 24, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@bparrishMines bparrishMines Jun 29, 2021

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@stuartmorgan-g stuartmorgan-g Jun 29, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@bparrishMines bparrishMines Jun 30, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@stuartmorgan-g stuartmorgan-g Jun 30, 2021

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@bparrishMines bparrishMines Jul 20, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@stuartmorgan-g stuartmorgan-g Jul 20, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@bparrishMines bparrishMines Jul 26, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@stuartmorgan-g stuartmorgan-g Jul 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@bparrishMines bparrishMines Aug 2, 2021

Choose a reason for hiding this comment

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

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

return platform.buildViewWithTextDirection(
_mapId,
onPlatformViewCreated,
textDirection: widget.textDirection ?? Directionality.of(context),
initialCameraPosition: widget.initialCameraPosition,
markers: widget.markers,
polygons: widget.polygons,
polylines: widget.polylines,
circles: widget.circles,
gestureRecognizers: widget.gestureRecognizers,
mapOptions: _googleMapOptions.toMap(),
);
}
return platform.buildView(
_mapId,
onPlatformViewCreated,
initialCameraPosition: widget.initialCameraPosition,
Expand Down
5 changes: 4 additions & 1 deletion packages/google_maps_flutter/google_maps_flutter/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,10 @@ dependencies:
flutter:
sdk: flutter
flutter_plugin_android_lifecycle: ^2.0.1
google_maps_flutter_platform_interface: ^2.0.0
google_maps_flutter_platform_interface:
git:
url: [email protected]:flutter/plugins.git
path: packages/google_maps_flutter/google_maps_flutter_platform_interface

dev_dependencies:
flutter_test:
Expand Down
31 changes: 18 additions & 13 deletions packages/google_maps_flutter/google_maps_flutter/test/google_map_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import 'package:flutter/services.dart';
import 'package:flutter/widgets.dart';
import 'package:flutter_test/flutter_test.dart';
import 'package:google_maps_flutter/google_maps_flutter.dart';
import 'package:google_maps_flutter_platform_interface/google_maps_flutter_platform_interface.dart';

import 'fake_maps_controllers.dart';

Expand Down Expand Up @@ -604,17 +605,21 @@ void main() {
},
);

// TODO(bparrishMines): Uncomment once https://github.com/flutter/plugins/pull/4017 has landed.
// testWidgets('Use AndroidViewSurface on Android', (WidgetTester tester) async {
// await tester.pumpWidget(
// const Directionality(
// textDirection: TextDirection.ltr,
// child: GoogleMap(
// initialCameraPosition: CameraPosition(target: LatLng(10.0, 15.0)),
// ),
// ),
// );
//
// expect(find.byType(AndroidViewSurface), findsOneWidget);
// });
testWidgets('Use PlatformViewLink on Android', (WidgetTester tester) async {
final MethodChannelGoogleMapsFlutter platform =
GoogleMapsFlutterPlatform.instance as MethodChannelGoogleMapsFlutter;
platform.useAndroidViewSurface = true;

await tester.pumpWidget(
const Directionality(
textDirection: TextDirection.ltr,
child: GoogleMap(
initialCameraPosition: CameraPosition(target: LatLng(10.0, 15.0)),
),
),
);

expect(find.byType(PlatformViewLink), findsOneWidget);
platform.useAndroidViewSurface = false;
});
}