[google_maps_flutter] Add heatmap support

[Rexios80]

Adds heatmap support for web, iOS, and Android

List which issues are fixed by this PR. You must list at least one issue.

#33586 #86811

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

• [x] I read the Contributor Guide and followed the process outlined there for submitting PRs.
• [x] I read the Tree Hygiene wiki 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/plugins repo does use dart format.)
• [x] I signed the CLA.
• [x] The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
• [x] I listed 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.
• [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.

[Rexios80]

Some notes:

• All platforms seem to be in a pretty good spot. Android and iOS might need some extra work, but I think it's ready for a preliminary review to make sure the API is how we want it.
• The CI is complaining about web/index.html not containing the license text. Should this file have it? None of the other generated files seem to have it. Is there a whitelist somewhere?

[Rexios80]

I finally got pretty decent consistency across platforms

[stuartmorgan]

Thanks for the contribution! To set expectations here, this will likely take a little longer than usual for us to get to the review for given the scope of the PR.

As a high-level note from an initial skim: I see Dart unit tests, but no native unit tests or integration tests. That means that all of the native code here could be wrong, or even removed entirely, and no tests would fail, which means there's not sufficient test coverage to meet our testing requirements. You'll need to add at least one or the other to ensure coverage of the native code changes.

[Rexios80]

I based what I was doing off of the existing circle, marker, polygon, etc code. Are there native tests for those?

[Rexios80]

Oh yeah there are in java, but they seem to only test that values that are passed into the constructor are transformed properly which looks irrelevant to the heatmap code. I don't see any tests for those features in the iOS code. If you could be more specific about what kind of tests you want me to write I would be more than happy to.

[stuartmorgan]

I based what I was doing off of the existing circle, marker, polygon, etc code. Are there native tests for those?

See https://github.com/flutter/flutter/wiki/Plugin-Tests#faq-do-i-need-to-add-tests-even-if-the-part-of-the-code-im-changing-isnt-already-tested

If you could be more specific about what kind of tests you want me to write I would be more than happy to.

I can't give specifics until I've had time to review in more detail.

In general, my high level guidance would be: try completely breaking or reverting specific critical parts of your PR (e.g., the changes to the method handler in the iOS and Android code). Do all the tests still pass? If so, you need tests for that.

Looking at a plugin with more robust native test coverage (local_auth comes to mind) might be helpful to see how to structure native tests to provide substantial coverage of the native code.

[stuartmorgan]

(Alternately, if the map is sufficiently inspectable, then Dart integration tests that actually validate that the map has the expected items added to it would give you end-to-end coverage that includes the native code. I'm not familiar enough with the maps plugin to know if that's viable here.)

[Rexios80]

I do know that if the method handlers don't have a case for heatmaps#update the integration tests fail. Every update method gets called when the map is created.

[stuartmorgan]

I only see integration tests in google_maps_flutter_web. Maybe there are files missing from your PR?

[Rexios80]

CI failure from before I implemented the method on iOS: https://github.com/flutter/plugins/runs/6032704090

[stuartmorgan]

Oh, I see; I misread your comment. Yes, technically you couldn't break or remove every line of native code, but you could revert your entire ios/Classes/ diff and just add:

  } else if ([call.method isEqualToString:@"heatmaps#update"]) {
    result(nil);
  }

Presumably we can agree that this would not be a useful implementation of heatmaps.

The goal is not to have the minimal amount of testing such that it's theoretically possible to break tests in the absolute worst case. It's to have actual coverage of all of the code you are adding, ensuring that it actually does what it is supposed to do.

[leithalnajjar]

How can I use it at the moment?

[Rexios80]

I have a branch with this merged in with other changes:

dependencies:
  google_maps_flutter:
    git:
      url: https://github.com/Rexios80/plugins-1
      ref: 27485e299eb3aa0025b707e3a8a9d37da301c914
      path: packages/google_maps_flutter/google_maps_flutter
  google_maps_flutter_web:
    git:
      url: https://github.com/Rexios80/plugins-1
      ref: 27485e299eb3aa0025b707e3a8a9d37da301c914
      path: packages/google_maps_flutter/google_maps_flutter_web

dependency_overrides:
  google_maps_flutter_android:
    git:
      url: https://github.com/Rexios80/plugins-1
      ref: 27485e299eb3aa0025b707e3a8a9d37da301c914
      path: packages/google_maps_flutter/google_maps_flutter_android
  google_maps_flutter_ios:
    git:
      url: https://github.com/Rexios80/plugins-1
      ref: 27485e299eb3aa0025b707e3a8a9d37da301c914
      path: packages/google_maps_flutter/google_maps_flutter_ios
  google_maps_flutter_platform_interface:
    git:
      url: https://github.com/Rexios80/plugins-1
      ref: 27485e299eb3aa0025b707e3a8a9d37da301c914
      path: packages/google_maps_flutter/google_maps_flutter_platform_interface

[ditman]

PS, I've uploaded the demo (web) app here:

https://dit-maps-tests.web.app

(The only change I did was to move the Heatmaps section up in the main menu, so it is easier to find for people casually browsing) Looks great!

This is what the demo looks like in my screen (in Chrome on a Mac):

[Rexios80]

Is it just me or does the master branch have unresolved analysis issues?

[Rexios80]

I freshly cloned the actual repo (not my fork) to make sure I'm not crazy:

[Rexios80]

I noticed because I had to fix those same issues in my completely unrelated PR: https://github.com/flutter/plugins/pull/4916

[jmagman]

You were the first to notice this, you must be on top of tree in the flutter SDK https://github.com/flutter/flutter/issues/104453.

That shouldn't impact your tests in this PR, please ignore any prefer_const_literals_to_create_immutables warnings locally while we resolve the issue.

[Rexios80]

Are you saying this should only be an issue on flutter channel master? Because I'm on flutter channel stable.

[ditman]

@Rexios80 thanks for noticing!

this change here, made a couple of types @immutable, so a new lint was triggered in a different package that was made to use the flutter analysis rules earlier.

We didn't start seeing the breakage until the google_maps_flutter_platform_interface got published; then the lint started being applied in the now offending google_maps_flutter_web.

(The issue https://github.com/flutter/flutter/issues/104453 has been resolved, you should be able to rebase your branch with flutter/plugins main, and grab the fix.)

[Rexios80]

Alright this PR has had master merged into it so many times I think it's going to need another thorough review to make sure the changes are actually what they're supposed to be and that they match any new styling

[Rexios80]

At least the example code seems to still work on all platforms so that's good

[jmagman]

@GaryQian @camsim99 @cyanglaz would you mind giving this another look?

[stuartmorgan]

What's the status here (other than needing to be updated for federation, which moved some files); was this ready for another round of review, or were there still open questions?

[Rexios80]

I think the only unresolved conversation right now is why CI requires the swift version to be in the podspec

[jmagman]

I think the only unresolved conversation right now is why CI requires the swift version to be in the podspec

I was asking you that 🙂 why did you add it?

[Rexios80]

I think the only unresolved conversation right now is why CI requires the swift version to be in the podspec

I was asking you that 🙂 why did you add it?

Because this CI fails if it's not there: https://github.com/flutter/plugins/runs/7349570630

[Rexios80]

Okay running the format tool touched other packages apparently

[renanzdm]

How is the PR, is there anything that can help? This feature is very important for the package

[Rexios80]

I think the main blocker is tests. If there were existing tests for the other overlay features I could probably figure them out, but there aren't and I don't know enough about native testing to do so easily. I tried to see if we could read the heatmap layers from the native code and verify them in dart, but it doesn't seem possible to read the heatmap layers from the native maps UI elements.

Is it even possible to write meaningful tests if we can't inspect the heatmap layers?