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

packages/google_maps_flutter/google_maps_flutter/CHANGELOG.md

packages/google_maps_flutter/google_maps_flutter_android/CHANGELOG.md

stuartmorgan · 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 · 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 · 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 · 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 · 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 · 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 · 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 · 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

Hari-07 added 5 commits November 4, 2024 01:49

Add zIndexInt param and use it conditionally in android and ios

533ed62

Update test to include zIndexInt

67db1c2

Changelog and pubspec changes

caab808

Make testing dependency override changes

a0ad241

Update android test related to encoding

56f8383

Hari-07 requested review from cbracken, reidbaker, ditman and stuartmorgan 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

Hari-07 added 2 commits November 4, 2024 11:17

Fix android test failures

52ff285

Redo the version changes

55925f1

reidbaker approved these changes Nov 4, 2024

View reviewed changes

Hari-07 added 4 commits November 4, 2024 20:08

Update deprecation explainer copy

2b39ba8

Test for prio

3b09731

Comment about prio

3dc50a1

Comment ios

c7696f8

ditman reviewed Nov 4, 2024

View reviewed changes

Hari-07 added 3 commits November 5, 2024 09:08

Use zIndexInt on web

49f3cb4

Comment improv

ffe8075

Test for web

87e304b

Hari-07 commented Nov 5, 2024

View reviewed changes

Hari-07 added 2 commits November 5, 2024 10:34

Fix android test

ab4f60e

Update changelog statement to match styleguide

8636abf

stuartmorgan requested changes Nov 22, 2024

View reviewed changes

Hari-07 added 3 commits January 11, 2025 17:47

Update base package changelog

3573ab5

Update changelog wording

b89dc4e

Deprecating wording

0721fc3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[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

Hari-07 commented Nov 3, 2024 •

edited by stuartmorgan

Loading

Hari-07 commented Nov 4, 2024 •

edited

Loading

reidbaker Nov 4, 2024

reidbaker Nov 4, 2024

reidbaker Nov 4, 2024

reidbaker Nov 4, 2024

ditman left a comment

ditman Nov 4, 2024

Hari-07 Nov 5, 2024

ditman Nov 12, 2024

Hari-07 Nov 5, 2024

Hari-07 Nov 5, 2024

ditman Nov 12, 2024 •

edited

Loading

Hari-07 Nov 12, 2024 •

edited

Loading

ditman Nov 12, 2024 •

edited

Loading

stuartmorgan Nov 22, 2024

stuartmorgan Nov 22, 2024

stuartmorgan Nov 22, 2024

stuartmorgan Nov 22, 2024

stuartmorgan Nov 22, 2024

Hari-07 Jan 11, 2025

stuartmorgan Jan 13, 2025

stuartmorgan Nov 22, 2024

stuartmorgan Nov 22, 2024

stuartmorgan commented Dec 10, 2024

Hari-07 commented Dec 10, 2024

stuartmorgan commented Dec 10, 2024

Hari-07 commented Jan 10, 2025

	'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

Are you sure you want to change the base?

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

Conversation

Hari-07 commented Nov 3, 2024 • edited by stuartmorgan Loading

Pre-launch Checklist

Hari-07 commented Nov 4, 2024 • edited Loading

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

ditman left a 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

ditman Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

Hari-07 Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

ditman Nov 12, 2024 • edited Loading

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

Hari-07 commented Dec 10, 2024

stuartmorgan commented Dec 10, 2024

Hari-07 commented Jan 10, 2025

Hari-07 commented Nov 3, 2024 •

edited by stuartmorgan

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