[google_maps_flutter_platform_interface] Convert PatternItem and Cap to typesafe structures.
#7703
Conversation
| /// [refWidth] is the reference stroke width (in pixels) - the stroke width for which | ||
| /// the cap bitmap at its native dimension is designed. Must be positive. Default value | ||
| /// is 10 pixels. | ||
| const CustomCap(this.bitmapDescriptor, [this.refWidth = 10]) |
There was a problem hiding this comment.
Given that this is a public API we will ~never be able to change, let's make it a named parameter rather than an optional positional parameter, as named parameters can much more easily be added to later.
| /// Cap that is squared off exactly at the start or end vertex of a [Polyline] | ||
| /// with solid stroke pattern, equivalent to having no additional cap beyond | ||
| /// the start or end vertex. | ||
| butt('buttCap'), |
There was a problem hiding this comment.
Since the only use case for the strings is toJson, and we want to discourage people relying on strings in the interface of this package, I would much prefer that these be completely internal to the implementation of toJson rather than public API, so we aren't effectively expanding our legacy API surface.
| /// Enumeration of types of pattern items. | ||
| enum PatternItemType { | ||
| /// A dot used in the stroke pattern for a [Polyline]. | ||
| dot('dot'), |
| } | ||
|
|
||
| final Object _json; | ||
| final PatternItemType _type; | ||
| final double? _length; |
There was a problem hiding this comment.
It seems like we'll need subclasses that expose length publicly, using a similar structure to your cap changes, otherwise the platform implementation code would still need to call and interpret toJson to get the required info.
packages/google_maps_flutter/google_maps_flutter_platform_interface/test/types/cap_test.dart
Show resolved
Hide resolved
| CapType.round: 'roundCap', | ||
| CapType.square: 'squareCap', | ||
| CapType.custom: 'customCap', | ||
| }[capType]!; |
There was a problem hiding this comment.
Please use a switch instead; a switch would fail analyze if a new enum value were added, while this would not, making this version harder to maintain.
There was a problem hiding this comment.
I had not considered that, I will make the change and try to keep that in mind going forward. Thanks.
| PatternItemType.dot: 'dot', | ||
| PatternItemType.dash: 'dash', | ||
| PatternItemType.gap: 'gap', | ||
| }[itemType]!; |
...s/google_maps_flutter/google_maps_flutter_platform_interface/lib/src/types/pattern_item.dart
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
A couple more minor notes, but mostly looks good.
If we were writing this from scratch I would say we should make an abstract sealed base class and then subclasses for each type, rather than using enums to distinguish, but technically adding sealed to the existing classes would be a breaking change (even though I really doubt it would be breaking), and I don't think the different for clients (being able to directly pattern-switch instead of doing an enum switch + casting, which is not as type-safe) is worth a breaking change to a platform interface package.
...s/google_maps_flutter/google_maps_flutter_platform_interface/lib/src/types/pattern_item.dart
Outdated
Show resolved
Hide resolved
packages/google_maps_flutter/google_maps_flutter_platform_interface/lib/src/types/cap.dart
Outdated
Show resolved
Hide resolved
...s/google_maps_flutter/google_maps_flutter_platform_interface/lib/src/types/pattern_item.dart
Outdated
Show resolved
Hide resolved
| custom, | ||
| } | ||
|
|
||
| String _capTypeToJson(CapType capType) => switch (capType) { |
There was a problem hiding this comment.
If there is documentation for where these json values come from please link it here.
There was a problem hiding this comment.
These are the old string values used before this PR's diff which are then used the platform packages' JSON-based approach. They do not correspond to any values outside this repo, e.g. the Maps SDK.
There was a problem hiding this comment.
Perfect that means if we change them then nothing bad happens.
There was a problem hiding this comment.
Perfect that means if we change them then nothing bad happens.
No, that would be extremely bad. It would break all older versions of google_maps_flutter.
We're still cleaning up from tech debt mistakes in federating this plugin, where during federation magic string values that used to be internal to the monolithic plugin package were separated, with the Dart code producing them moved to google_maps_flutter_platform_interface, while the native code consuming them was left in the main package (and then eventually moved to platform implementation packages).
What should have happened was that we stopped using toJson from platform_interface as the serialization mechanism for the platform channels, in favor of package-internal versions. But since we didn't, these string values are effectively part of the public API surface of this package, consumed by our own implementations, with which we have to maintain compatibility.
There was a problem hiding this comment.
Ah ok so these strings while not needing to mirror something external like the google maps sdk they do need to mirror something external to the package google_maps_flutter_platform_interface for cross version compatibility.
If we changed the values then set a new minimum for each of the platform implementations would that prevent a breakage? I am not asking because I want to do that but because I want to understand the relationships.
The reason why I was asking what they needed to align to was to add documentation to warn future maintainers what those values needed to align with.
There was a problem hiding this comment.
If we changed the values then set a new minimum for each of the platform implementations would that prevent a breakage?
No, because the versions of the platform implementations that call these toJson functions (and rely on the result matching what's in the native code) already exist, and people are using them. And those packages use standard ^2.x.y constraints on google_maps_flutter_platform_interface and rely on the fact that doing so is safe because a change that breaks them wouldn't be made in a 2.* version.
Releasing a new google_maps_flutter_android would mean that people using the new google_maps_flutter_android would only get a compatible version of google_maps_flutter_platform_interface, but it would not stop people using an old version of google_maps_flutter_android from getting a new and incompatible version of google_maps_flutter_platform_interface.
I am not asking because I want to do that but because I want to understand the relationships.
I think the easiest mental model is: because of the federation mistake, the exact structure and string values returned by these toJson functions are now, in practice, part of the official API surface of the method, and so changing any existing details of what it returns would be just like any incompatible change to the documented behavior of any public method: a breaking change requiring a major version bump.
From a docs standpoint, explicitly documenting what the return value structure is in the dart doc would accurately express the unfortunate state we are in now.
| /// [refWidth] is the reference stroke width (in pixels) - the stroke width for which | ||
| /// the cap bitmap at its native dimension is designed. Must be positive. Default value | ||
| /// is 10 pixels. | ||
| const CustomCap(this.bitmapDescriptor, {this.refWidth = 10}) |
There was a problem hiding this comment.
Is it documented if these are logical pixels or physical pixels? If so please specify.
There was a problem hiding this comment.
A few redirections through the Maps SDK documentation describes these as "screen pixels." I will update the comment to include a link to the page.
...s/google_maps_flutter/google_maps_flutter_platform_interface/lib/src/types/pattern_item.dart
Show resolved
Hide resolved
| custom, | ||
| } | ||
|
|
||
| String _capTypeToJson(CapType capType) => switch (capType) { |
There was a problem hiding this comment.
Perfect that means if we change them then nothing bad happens.
Convert
PatternItemandCapfrom JSON wrappers to typesafe classes.flutter/flutter#155121
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style, or this PR is exempt from CHANGELOG changes.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.