[google_maps_flutter] Add a new zIndexInt param to marker and deprecate zIndex #8012

Hari-07 · 2024-11-03T22:28:08Z

Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.
zIndex on marker is currently a double. This introduces inconsistency between its behaviours on android and ios since ios truncates it to an int. This PR aims to smooth out that difference by accepting an int param instead through the marker API in dart

List which issues are fixed by this PR. You must list at least one issue.
Fixes flutter/flutter#155897

Pre-launch Checklist

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

Hari-07 · 2024-11-04T04:39:29Z

The checking for default value in native code feels a little weird, but since this is a parameter we have a default value for in the dart layer, not sure what other option is there.

The only other one I could think of was checking for 0 in dart itself and then passing down null, for zIndex if that was the case which would mean making it nullable . That might need a bunch of changes and wasn't sure if it's worth it for a deprecation

reidbaker · 2024-11-04T15:43:13Z

...google_maps_flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/Convert.java

+
+    int zIndexInt = marker.getZIndexInt().intValue();
+    float zIndex = marker.getZIndex().floatValue();
+    if (zIndexInt != 0) {


Can you add a comment here that the reason we check for zero is that zIndexInt is supposed to take priority over zindex? |

Please also add a test in https://github.com/flutter/packages/blob/2c8f22670812996a00e45e6304f79f1fd9b02fa1/packages/google_maps_flutter/google_maps_flutter_android/android/src/test/java/io/flutter/plugins/googlemaps/ConvertTest.java to verify the behavior.

reidbaker · 2024-11-04T15:57:46Z

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

@@ -150,7 +150,12 @@ class Marker implements MapsObject<Marker> {
    this.position = const LatLng(0.0, 0.0),
    this.rotation = 0.0,
    this.visible = true,
+    @Deprecated(
+      'Use zIndexInt instead. '
+      'Use of zIndex was deprecated as it gets truncated to int on iOS.',


Suggested change

'Use of zIndex was deprecated as it gets truncated to int on iOS.',

'On iOS zIndex is truncated to an int, which can lead to incorrect/unstable ordering.',

reidbaker · 2024-11-04T15:58:15Z

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

@@ -219,8 +224,19 @@ class Marker implements MapsObject<Marker> {
  ///
  /// Overlays are drawn in order of z-index, so that lower values means drawn
  /// earlier, and thus appearing to be closer to the surface of the Earth.
+  @Deprecated(
+    'Use zIndexInt instead. '
+    'Use of zIndex was deprecated as it gets truncated to int on iOS.',


Suggested change

'Use of zIndex was deprecated as it gets truncated to int on iOS.',

'On iOS zIndex is truncated to an int, which can lead to incorrect/unstable ordering.',

reidbaker · 2024-11-04T15:58:43Z

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

@@ -246,7 +262,12 @@ class Marker implements MapsObject<Marker> {
    LatLng? positionParam,
    double? rotationParam,
    bool? visibleParam,
+    @Deprecated(
+      'Use zIndexIntParam instead. '
+      'Use of zIndex was deprecated as it gets truncated to int on iOS.',


Suggested change

'Use of zIndex was deprecated as it gets truncated to int on iOS.',

'On iOS zIndex is truncated to an int, which can lead to incorrect/unstable ordering.',

ditman

This is probably minor, but the web version is not handling the new zIndexInt value. We should address that, or create a follow-up ticket so somebody else can implement the desired behavior there.

ditman · 2024-11-04T23:46:52Z

packages/google_maps_flutter/google_maps_flutter_web/CHANGELOG.md

-## NEXT
+## 0.5.10+1

+* Deprecate zIndex parameter in Marker and replace with zIndexInt.


I'd expect to see a change here for the web?

packages/packages/google_maps_flutter/google_maps_flutter_web/lib/src/convert.dart

Line 269 in 79f8b0b

..zIndex = marker.zIndex;

Thank you for the reminder, didn't think about web. I can just add that in right away

Please, remember the PR styleguide. This changelog entry should be "Deprecates zIndex parameter..."

Hari-07 · 2024-11-05T06:01:00Z

packages/google_maps_flutter/google_maps_flutter_web/example/integration_test/marker_test.dart

@@ -205,5 +206,21 @@ void main() {
        }, throwsAssertionError);
      });
    });
+
+    testWidgets('interpretCorrectZIndexInConversion',


I didn't find any conversion tests in the web package, so Im not 100% sure if this is in the right place. Maybe it should be in a dedicated convert_test file

Hari-07 · 2024-11-05T06:01:57Z

packages/google_maps_flutter/google_maps_flutter_web/lib/src/convert.dart

-Future<gmaps.MarkerOptions> _markerOptionsFromMarker(
+/// Computes the options for a new [gmaps.Marker] from an incoming set of options
+/// [marker], and the existing marker registered with the map: [currentMarker].
+@visibleForTesting


This feels a little weird, and I think maybe all the methods here could be static methods of a class

Yeah, this file started up being small, then grew to epic proportions. It should eventually disappear, but there's still some "stringly typed" API coming from the core plugin (everything that is backed by a JSON object, for example).

Thanks for adding a convert_test. We normally tested this as an integration test, by inderectly looking at the properties that we can read back from the google maps sdk if I'm not mistaken.

this file started up being small, then grew to epic proportions. It should eventually disappear

Haha, classic!

In order to not have to modify this method, I'd suggest you add the test to markers_test.dart, which is calling all the conversion methods when doing controller.addMarkers.

See this:

packages/packages/google_maps_flutter/google_maps_flutter_web/example/integration_test/markers_test.dart

Lines 57 to 95 in c77ab99

testWidgets('changeMarkers', (WidgetTester tester) async {

gmaps.Marker? marker;

gmaps.LatLng? position;

final Set<Marker> markers = <Marker>{

const Marker(markerId: MarkerId('1')),

};

await controller.addMarkers(markers);

marker = controller.markers[const MarkerId('1')]?.marker;

expect(marker, isNotNull);

expect(marker!.draggable, isFalse);

// By default, markers fall in LatLng(0, 0)

position = marker.position;

expect(position, isNotNull);

expect(position!.lat, equals(0));

expect(position.lng, equals(0));

// Update the marker with draggable and position

final Set<Marker> updatedMarkers = <Marker>{

const Marker(

markerId: MarkerId('1'),

draggable: true,

position: LatLng(42, 54),

),

};

await controller.changeMarkers(updatedMarkers);

expect(controller.markers.length, 1);

marker = controller.markers[const MarkerId('1')]?.marker;

expect(marker, isNotNull);

expect(marker!.draggable, isTrue);

position = marker.position;

expect(position, isNotNull);

expect(position!.lat, equals(42));

expect(position.lng, equals(54));

});

(You'd just need to add the marker to the controller, then retrieve it by its MarkerId, and then check the zIndex property, similarly to how you're doing it now)

It looks like this still needs to be done?

packages/google_maps_flutter/google_maps_flutter/CHANGELOG.md

packages/google_maps_flutter/google_maps_flutter_android/CHANGELOG.md

stuartmorgan-g · 2024-11-22T20:56:54Z

...google_maps_flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/Convert.java

+
+    // We compare with zero because zIndexInt if non zero should get higher priority than zIndex
+    int zIndexInt = marker.getZIndexInt().intValue();
+    float zIndex = marker.getZIndex().floatValue();


We should not be sending both values; see comments in platform_interface.

In fact, I believe once the other changes have been made, you can revert all of the Java changes in this PR.

stuartmorgan-g · 2024-11-22T20:57:35Z

packages/google_maps_flutter/google_maps_flutter_platform_interface/CHANGELOG.md

@@ -1,3 +1,7 @@
+## 2.9.6


Deprecations are defined to be minor version bumps in semver.

stuartmorgan-g · 2024-11-22T20:57:49Z

packages/google_maps_flutter/google_maps_flutter_platform_interface/CHANGELOG.md

@@ -1,3 +1,7 @@
+## 2.9.6
+
+* Deprecates zIndex parameter in Marker and replace with zIndexInt.


Same comments as in the other packages.

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

stuartmorgan-g · 2024-11-22T21:18:18Z

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

    this.zIndex = 0.0,
+    this.zIndexInt = 0,


zIndexInt should not be a new field, as this creates the possibility of inconsistent values in the same marker. Instead, it can be:

const Marker({ ... double zIndex = 0.0, int zIndexInt = 0, ... }): assert(zIndex == 0.0 || zIndexInt == 0, 'Only one of zIndex and zIndexInt can be provided'), zIndex = zIndexInt == 0 ? zIndex : zIndexInt.toDouble(); ... /// ... // TODO(stuartmorgan): Make this an int when removing the deprecated double zIndex parameter. @Deprecated(...) final double zIndex; /// ... int get zIndexInt => zIndex.round();

Then there is only a single value. iOS can read zIndexInt since it will be truncated anyway, and Android can read zIndex with a comment explaining that it's using a deprecated parameter for backward compatibility.

zIndex = zIndexInt == 0 ? zIndex : zIndexInt.toDouble()

This would need making Marker non -const

Ah, so it would.

We'll need to go the slightly clunkier route of using a num then:

const Marker({ ... double zIndex = 0.0, int zIndexInt = 0, ... }): assert(zIndex == 0.0 || zIndexInt == 0, 'Only one of zIndex and zIndexInt can be provided'), _zIndexNum = zIndexInt == 0 ? zIndex : zIndexInt; final num _zIndexNum; double get zIndex => _zIndexNum.toDouble(); int get zIndexInt => _zIndex.round();

stuartmorgan-g · 2024-11-22T21:19:35Z

packages/google_maps_flutter/google_maps_flutter_android/pigeons/messages.dart

@@ -198,6 +199,7 @@ class PlatformMarker {
  final double rotation;
  final bool visible;
  final double zIndex;
+  final int zIndexInt;


This should be removed; we don't need to send two values.

stuartmorgan-g · 2024-11-22T21:20:43Z

...gle_maps_flutter/google_maps_flutter_android/example/integration_test/google_maps_tests.dart

@@ -1503,6 +1503,7 @@ Marker _copyMarkerWithClusterManagerId(
    rotation: marker.rotation,
    visible: marker.visible,
    zIndex: marker.zIndex,
+    zIndexInt: marker.zIndexInt,


This won't be needed after the suggested change in the platform interface. The zIndex will just need a comment saying that the deprecated parameter is being used to avoid losing precision.

stuartmorgan-g · 2024-12-10T20:41:36Z

From triage: @Hari-07 Are you still planning on updating this based on the latest review?

Hari-07 · 2024-12-10T20:43:11Z

I have been on a small break, will get back to this next week. Is the packages repo also affected by the flutter code style change and require a rebase?

stuartmorgan-g · 2024-12-10T20:52:36Z

No, flutter/packages already uses dart format, so is unaffected.

Hari-07 · 2025-01-10T12:50:47Z

@stuartmorgan Sorry for further delays, got busy with the holiday season, will try and get to this at the earliest

stuartmorgan-g · 2025-02-04T20:51:30Z

From triage: @Hari-07 was this ready for re-review? If so, it looks like it'll need a sync with main and then we can take a look again.

Piinks · 2025-04-18T21:40:44Z

Greetings from stale PR triage, @Hari-07 do you have plans to return to this one?

Hari-07 · 2025-04-28T04:24:56Z

Hey @Piinks yeah sure, I had actually made the change he had suggested but forgot to push it up for some reason. Also looks like a sync with main is needed. Doing that now.

Missed the email about this reply

Hari-07 · 2025-06-09T08:24:58Z

I removed all overrides for the platform interface package from all other packages, the override that's remaining now is from the main package to android and ios. Does this mean android/ios needs to be merged in as a separate PR and then the main package needs to target the new published version?

Or is it fine to bump the main package's version requirement to a version that isn't published yet

stuartmorgan-g · 2025-06-09T11:48:49Z

Does this mean android/ios needs to be merged in as a separate PR and then the main package needs to target the new published version?

Yes.

Or is it fine to bump the main package's version requirement to a version that isn't published yet

That would fail essentially everything in CI, since the pub resolution would fail, and thus be unlandable.

Hari-07 · 2025-06-09T13:52:43Z

Ok cool, have made a PR with everything except the main package. Once that's in can update the main package here to target the newly released packages and then this should be good to merge finally

Hari-07 · 2025-06-09T14:15:53Z

@stuartmorgan-g it might be a good idea to have some kind of automated action that uses git checkout -- package/ to automatically create these partial PRs, that'd then allow you the reviewer to not have to check to make sure it has exactly the changes you expect, not more nor less, and easier for contributors too.

…param to marker and deprecate zIndex (#9408) *List which issues are fixed by this PR. You must list at least one issue.* This is just the `web`, `android` and `ios` package part of #8012 ## Pre-Review Checklist [^1]: Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling.

…os-truncation-fix

Hari-07 · 2025-06-16T17:37:29Z

Test failure

Running command: "flutter pub get" in /b/s/w/ir/x/w/packages/packages/google_maps_flutter/google_maps_flutter
Resolving dependencies...
Because google_maps_flutter depends on google_maps_flutter_ios ^2.15.4 which doesn't match any versions, version solving failed.


You can try the following suggestion to make the pubspec resolve:
* Consider downgrading your constraint on google_maps_flutter_ios: flutter pub add google_maps_flutter_ios:^2.15.3
Failed to update packages.

2.15.4 is the new version that was just merged in a few minutes back, so maybe it just takes time before that's published and then the tests need to be rerun?

stuartmorgan-g · 2025-06-16T17:41:56Z

Autopublish happens after all post-submit tests complete successfully, which isn't quite finished.

Hari-07 · 2025-06-17T09:30:14Z

@stuartmorgan-g Could you trigger a rerun of the tests here?

stuartmorgan-g

LGTM!

…nd deprecate zIndex (flutter/packages#8012)

flutter/packages@03a6abb...25d4fa4 2025-06-17 [email protected] [google_maps_flutter] Add a new zIndexInt param to marker and deprecate zIndex (flutter/packages#8012) 2025-06-17 [email protected] [pigeon] Use a const for custom type ids for gobject generated files (#156100) (flutter/packages#9306) 2025-06-16 [email protected] [google_maps_flutter_platform_(web/android/ios)] Add a new zIndexInt param to marker and deprecate zIndex (flutter/packages#9408) 2025-06-16 [email protected] [camera_avfoundation] fix race condition when starting image stream on iOS (flutter/packages#8733) 2025-06-16 [email protected] Roll Flutter from f79452e to 8303a96 (21 revisions) (flutter/packages#9433) 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

… marker and deprecate zIndex (flutter#9372) *List which issues are fixed by this PR. You must list at least one issue.* This is just the platform interface package part of flutter#8012 ## Pre-Review Checklist [^1]: Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling.

…param to marker and deprecate zIndex (flutter#9408) *List which issues are fixed by this PR. You must list at least one issue.* This is just the `web`, `android` and `ios` package part of flutter#8012 ## Pre-Review Checklist [^1]: Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling.

…te zIndex (flutter#8012) *Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.* `zIndex` on marker is currently a double. This introduces inconsistency between its behaviours on android and ios since ios truncates it to an int. This PR aims to smooth out that difference by accepting an int param instead through the marker API in dart *List which issues are fixed by this PR. You must list at least one issue.* Fixes flutter/flutter#155897

Hari-07 requested review from cbracken, reidbaker, ditman and stuartmorgan-g as code owners November 3, 2024 22:28

github-actions bot added p: google_maps_flutter platform-android platform-ios platform-web labels Nov 3, 2024

reidbaker approved these changes Nov 4, 2024

View reviewed changes

ditman reviewed Nov 4, 2024

View reviewed changes

Hari-07 commented Nov 5, 2024

View reviewed changes

stuartmorgan-g requested changes Nov 22, 2024

View reviewed changes

Hari-07 requested a review from vashworth as a code owner April 28, 2025 04:24

Hari-07 added 9 commits April 28, 2025 10:00

Add zIndexInt param and use it conditionally in android and ios

98a270a

Update test to include zIndexInt

c6fc547

Changelog and pubspec changes

6e1e926

Make testing dependency override changes

2fb938d

Update android test related to encoding

81a6243

Fix android test failures

fb6c050

Redo the version changes

42a3b7a

Update deprecation explainer copy

7e51cbd

Test for prio

b824ef8

Hari-07 added 2 commits June 9, 2025 12:01

Update the web test to avoid changing method signature

6a540f0

Remove interface dependency overrides

bd5db16

Hari-07 mentioned this pull request Jun 9, 2025

[google_maps_flutter_platform_(web/android/ios)] Add a new zIndexInt param to marker and deprecate zIndex #9408

Merged

11 tasks

Hari-07 added 3 commits June 16, 2025 21:06

Merge branch 'main' of github.com:flutter/packages into maps-zindex-i…

9fd95dc

…os-truncation-fix

Remove remaining dep overrides

184c2ef

Avoid reverting the changes

ea96882

github-actions bot removed platform-android platform-ios platform-web labels Jun 16, 2025

Hari-07 added 3 commits June 16, 2025 21:23

Use the correct android version

d313edc

Correct changelog error from upstream merge

a9aa804

Don't pass both zIndexes in copy with test

8ffeda1

stuartmorgan-g approved these changes Jun 17, 2025

View reviewed changes

stuartmorgan-g added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 17, 2025

auto-submit bot merged commit 25d4fa4 into flutter:main Jun 17, 2025
78 checks passed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 17, 2025

25d4fa425 [google_maps_flutter] Add a new zIndexInt param to marker a…

7c1c82e

…nd deprecate zIndex (flutter/packages#8012)

engine-flutter-autoroll mentioned this pull request Jun 17, 2025

Roll Packages from 03a6abbedabb to 25d4fa425508 (5 revisions) flutter/flutter#170743

Merged

Hari-07 deleted the maps-zindex-ios-truncation-fix branch June 17, 2025 15:07

	'Use of zIndex was deprecated as it gets truncated to int on iOS.',
	'On iOS zIndex is truncated to an int, which can lead to incorrect/unstable ordering.',

	testWidgets('changeMarkers', (WidgetTester tester) async {
	gmaps.Marker? marker;
	gmaps.LatLng? position;

	final Set<Marker> markers = <Marker>{
	const Marker(markerId: MarkerId('1')),
	};
	await controller.addMarkers(markers);

	marker = controller.markers[const MarkerId('1')]?.marker;
	expect(marker, isNotNull);
	expect(marker!.draggable, isFalse);

	// By default, markers fall in LatLng(0, 0)
	position = marker.position;
	expect(position, isNotNull);
	expect(position!.lat, equals(0));
	expect(position.lng, equals(0));

	// Update the marker with draggable and position
	final Set<Marker> updatedMarkers = <Marker>{
	const Marker(
	markerId: MarkerId('1'),
	draggable: true,
	position: LatLng(42, 54),
	),
	};
	await controller.changeMarkers(updatedMarkers);
	expect(controller.markers.length, 1);

	marker = controller.markers[const MarkerId('1')]?.marker;
	expect(marker, isNotNull);
	expect(marker!.draggable, isTrue);

	position = marker.position;
	expect(position, isNotNull);
	expect(position!.lat, equals(42));
	expect(position.lng, equals(54));
	});

		@@ -1,3 +1,7 @@
		## 2.9.6

		* Deprecates zIndex parameter in Marker and replace with zIndexInt.

[google_maps_flutter] Add a new zIndexInt param to marker and deprecate zIndex #8012

[google_maps_flutter] Add a new zIndexInt param to marker and deprecate zIndex #8012

Uh oh!

Conversation

Hari-07 commented Nov 3, 2024 • edited by stuartmorgan-g Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Hari-07 commented Nov 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

ditman 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!

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!

ditman Nov 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hari-07 Nov 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ditman Nov 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

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 commented Dec 10, 2024

Uh oh!

Hari-07 commented Dec 10, 2024

Uh oh!

stuartmorgan-g commented Dec 10, 2024

Uh oh!

Hari-07 commented Jan 10, 2025

Uh oh!

stuartmorgan-g commented Feb 4, 2025

Uh oh!

Piinks commented Apr 18, 2025

Uh oh!

Hari-07 commented Nov 3, 2024 •

edited by stuartmorgan-g

Loading

Hari-07 commented Nov 4, 2024 •

edited

Loading

ditman Nov 12, 2024 •

edited

Loading

Hari-07 Nov 12, 2024 •

edited

Loading

ditman Nov 12, 2024 •

edited

Loading