[google_maps_flutter_platform_interface] Convert `BitmapDescriptor` to typesafe subclasses #7699

yaakovschectman · 2024-09-25T17:11:08Z

BitmapDescriptor is currently just a wrapper around JSON, with the exception of two of its subclasses. This converts all cases to typesafe structures without breaking the current API.

Part of flutter/flutter#155122

Pre-launch Checklist

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

yaakovschectman · 2024-09-30T18:46:24Z

packages/google_maps_flutter/google_maps_flutter_platform_interface/test/types/bitmap_test.dart

-      expect(identical(descriptorFromJson.toJson(), json), isTrue); // Same JSON
+      expect(descriptorFromJson.toJson(), json);


Since the bitmaps are no longer just wrapping JSON, toJson will not return the same reference as was passed to fromJson, though they will still be equal by comparison.

reidbaker · 2024-10-01T14:43:31Z

packages/google_maps_flutter/google_maps_flutter_platform_interface/lib/src/types/bitmap.dart

@@ -44,14 +51,14 @@ const double _naturalPixelRatio = 1.0;
 /// a default marker icon.
 /// Use the [BitmapDescriptor.defaultMarkerWithHue] to create a
 /// [BitmapDescriptor] for a default marker icon with a hue value.
-class BitmapDescriptor {
-  const BitmapDescriptor._(this._json);
+abstract class BitmapDescriptor {


I thought this was a breaking change but a spot check of the public usages in github show I am incorrect.

The only ways to construct a BitmapDescriptor without constructing one of its subclasses was either through this private _ factory, which is removed here, or the below fromJson public factory, which this PR converts to a static method, so unless we are expecting public usages of this package to add new subclasses of BitmapDescriptor, I believe that neither making it abstract nor removing the _ factory should impact any public uses.

reidbaker · 2024-10-01T14:47:08Z

packages/google_maps_flutter/google_maps_flutter_platform_interface/lib/src/types/bitmap.dart


  /// The inverse of .toJson.
  // TODO(stuartmorgan): Remove this in the next breaking change.
  @Deprecated('No longer supported')
-  BitmapDescriptor.fromJson(Object json) : _json = json {
-    assert(_json is List<dynamic>);
+  static BitmapDescriptor fromJson(Object json) {


Why not use the : _json syntax?

This PR removes the _json field from BitmapDescriptor, and since it also makes BitmapDescriptor an abstract class with a concrete subtype for each possible set of member data, fromJson needs to return an instance of one of those subtypes.

stuartmorgan · 2024-10-01T19:07:17Z

packages/google_maps_flutter/google_maps_flutter_platform_interface/lib/src/types/bitmap.dart

@@ -29,6 +29,13 @@ enum MapBitmapScaling {
  none,
 }

+/// Convert a string from provided JSON to a MapBitmapScaling enum.
+MapBitmapScaling mapBitmapScalingFromString(String mode) => switch (mode) {


This should be private so we're not increasing the API surface of the package.

Would you prefer we mark it @visibleForTesting, or remove the test https://github.com/flutter/packages/pull/7699/files#diff-a8066076d538f1e1e21b27bac466302cb9317b643500ce81b991102a94611070R685?

My preference would be visible for testing and keep the test.

That would be my thought as well, @stuartmorgan do you agree with this decision?

Yes, that's fine; from a semver standpoint we consider those equivalent.

stuartmorgan · 2024-10-01T19:08:54Z

packages/google_maps_flutter/google_maps_flutter_platform_interface/lib/src/types/bitmap.dart

+/// A BitmapDescriptor using the default marker.
+class DefaultMarker extends BitmapDescriptor {
+  /// Provide an optional [hue] for the default marker.
+  const DefaultMarker([this.hue]) : super._();


Please use named parameters so that we can change the parameter set in the future without breaking changes (or being stuck with a growing positional list, which doesn't scale).

stuartmorgan

LG once the @visibleForTesting annotation is added.

stuartmorgan · 2024-10-04T16:35:34Z

packages/google_maps_flutter/google_maps_flutter_platform_interface/lib/src/types/bitmap.dart

@@ -5,6 +5,7 @@
 import 'dart:async' show Future;
 import 'dart:typed_data' show Uint8List;

+import 'package:flutter/cupertino.dart';


Looks like your IDE chose a weird auto-import here; this should be a new show below instead.

(Specifically, from foundation.dart)

stuartmorgan

LGTM

…criptor` to typesafe subclasses (flutter/packages#7699)

flutter/packages@05bf1d4...bb00d34 2024-10-07 [email protected] [google_sign_in] Update Pigeon for non-nullable generics (flutter/packages#7785) 2024-10-07 [email protected] [path_provider] Update Android Pigeon for non-nullable generics (flutter/packages#7783) 2024-10-07 [email protected] [rfw] Increase tolerance for material widget tests (flutter/packages#7148) 2024-10-05 [email protected] [various] Update Java compatibility version to 11 (flutter/packages#7795) 2024-10-04 [email protected] [video_player] Update Pigeon for non-nullable generics (flutter/packages#7790) 2024-10-04 [email protected] [go_router] Added missing implementation for the routerNeglect parameter in GoRouter (flutter/packages#7752) 2024-10-04 [email protected] [google_maps_flutter_platform_interface] Convert `BitmapDescriptor` to typesafe subclasses (flutter/packages#7699) 2024-10-04 [email protected] [google_maps_flutter_platform_interface] Convert `PatternItem` and `Cap` to typesafe structures. (flutter/packages#7703) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

yaakovschectman requested a review from reidbaker September 25, 2024 17:11

yaakovschectman self-assigned this Sep 25, 2024

github-actions bot added the p: google_maps_flutter label Sep 25, 2024

yaakovschectman marked this pull request as ready for review September 25, 2024 17:35

yaakovschectman requested a review from stuartmorgan as a code owner September 25, 2024 17:35

yaakovschectman commented Sep 30, 2024

View reviewed changes

reidbaker approved these changes Oct 1, 2024

View reviewed changes

stuartmorgan requested changes Oct 1, 2024

View reviewed changes

yaakovschectman requested a review from stuartmorgan October 1, 2024 21:11

stuartmorgan reviewed Oct 4, 2024

View reviewed changes

yaakovschectman added 13 commits October 4, 2024 12:31

Convert DefaultMarker

f267e01

Convert BytesBitmap

1705306

Convert AssetBitmap

8c3a086

Convert AssetImageBitmap

510dbac

Convet AssetMapBitmap

a7ccb6b

Convert BytesMapBitmap

3066ecc

Convert BitmapDescriptor to abstract

3c8a00a

Update tests

0fb4809

Doc comment

ae91edb

Format

a10ef96

Cleanup

480772c

Mark hue as named

daa5918

Mark for testing

cdcae66

yaakovschectman force-pushed the maps_bmd branch from 1ff2c26 to cdcae66 Compare October 4, 2024 16:33

Whitespace

2d2acfa

yaakovschectman requested a review from stuartmorgan October 4, 2024 16:33

stuartmorgan reviewed Oct 4, 2024

View reviewed changes

Import correct annotation

4d52d25

stuartmorgan approved these changes Oct 4, 2024

View reviewed changes

yaakovschectman merged commit e28f004 into flutter:main Oct 4, 2024
76 checks passed

engine-flutter-autoroll mentioned this pull request Oct 7, 2024

Roll Packages from 05bf1d482dd9 to bb00d34a4280 (8 revisions) flutter/flutter#156322

Merged

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

e28f004e4 [google_maps_flutter_platform_interface] Convert `BitmapDes…

acf8809

…criptor` to typesafe subclasses (flutter/packages#7699)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[google_maps_flutter_platform_interface] Convert `BitmapDescriptor` to typesafe subclasses #7699

[google_maps_flutter_platform_interface] Convert `BitmapDescriptor` to typesafe subclasses #7699

yaakovschectman commented Sep 25, 2024

yaakovschectman Sep 30, 2024

reidbaker Oct 1, 2024

yaakovschectman Oct 1, 2024

reidbaker Oct 1, 2024

yaakovschectman Oct 1, 2024

stuartmorgan Oct 1, 2024

yaakovschectman Oct 1, 2024

reidbaker Oct 1, 2024

yaakovschectman Oct 1, 2024

stuartmorgan Oct 4, 2024

stuartmorgan Oct 1, 2024

stuartmorgan left a comment

stuartmorgan Oct 4, 2024

stuartmorgan Oct 4, 2024

stuartmorgan left a comment

		expect(identical(descriptorFromJson.toJson(), json), isTrue); // Same JSON
		expect(descriptorFromJson.toJson(), json);

[google_maps_flutter_platform_interface] Convert BitmapDescriptor to typesafe subclasses #7699

[google_maps_flutter_platform_interface] Convert BitmapDescriptor to typesafe subclasses #7699

Conversation

yaakovschectman commented Sep 25, 2024

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

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

stuartmorgan left a comment

Choose a reason for hiding this comment

[google_maps_flutter_platform_interface] Convert `BitmapDescriptor` to typesafe subclasses #7699

[google_maps_flutter_platform_interface] Convert `BitmapDescriptor` to typesafe subclasses #7699