[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

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

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

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-g · 2024-10-01T19:07:17Z

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

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-g · 2024-10-01T19:08:54Z

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

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-g

LG once the @visibleForTesting annotation is added.

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

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

 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-g

LGTM

…iptor` 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

…o typesafe subclasses (flutter#7699) `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 - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] page, which explains my responsibilities. - [x] I read and followed the [relevant style guides] and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use `dart format`.) - [ ] I signed the [CLA]. - [x] The title of the PR starts with the name of the package surrounded by square brackets, e.g. `[shared_preferences]` - [x] I [linked to at least one issue that this PR fixes] in the description above. - [x] I updated `pubspec.yaml` with an appropriate new version according to the [pub versioning philosophy], or this PR is [exempt from version changes]. - [x] I updated `CHANGELOG.md` to add a description of the change, [following repository CHANGELOG style], or this PR is [exempt from CHANGELOG changes]. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord].  [Contributor Guide]: https://github.com/flutter/packages/blob/main/CONTRIBUTING.md [Tree Hygiene]: https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md [relevant style guides]: https://github.com/flutter/packages/blob/main/CONTRIBUTING.md#style [CLA]: https://cla.developers.google.com/ [Discord]: https://github.com/flutter/flutter/blob/master/docs/contributing/Chat.md [linked to at least one issue that this PR fixes]: https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md#overview [pub versioning philosophy]: https://dart.dev/tools/pub/versioning [exempt from version changes]: https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#version [following repository CHANGELOG style]: https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#changelog-style [exempt from CHANGELOG changes]: https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#changelog [test-exempt]: https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md#tests

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-g 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-g requested changes Oct 1, 2024

View reviewed changes

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

stuartmorgan-g 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-g October 4, 2024 16:33

stuartmorgan-g reviewed Oct 4, 2024

View reviewed changes

Import correct annotation

4d52d25

stuartmorgan-g approved these changes Oct 4, 2024

View reviewed changes

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

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

e28f004 [google_maps_flutter_platform_interface] Convert `BitmapDescr…

acf8809

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

[google_maps_flutter_platform_interface] Convert BitmapDescriptor to typesafe subclasses #7699

[google_maps_flutter_platform_interface] Convert BitmapDescriptor to typesafe subclasses #7699

Uh oh!

Conversation

yaakovschectman commented Sep 25, 2024

Pre-launch Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

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

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