[google_maps_flutter] Implement google maps flutter heatmap #2454

SvenSlijkoord · 2020-01-08T20:21:41Z

Description

Implements heatmap support for the google_maps_flutter package.

Related Issues

flutter/flutter#33586

Checklist

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
No, this is not a breaking change.

urmilshroff · 2020-01-12T09:01:01Z

Thank you for this! Let's hope it gets merged soon 🙏

cyanglaz

Thank you for providing this PR. It looks great! Since it is a huge PR, I am not able to run through everything in one pass. I left some comments after a quick scan.

It also seems there are a lot of boiler plates in the Android implementation. Do you think you can refactor it to make the code a bit simpler? For example, I think we can remove some of the HeatmapOptionsSink implementations to make the code easier to read and maintain. I might be missing something though. WDYT?

packages/google_maps_flutter/android/src/main/java/io/flutter/plugins/googlemaps/Convert.java

cyanglaz · 2020-01-23T21:46:35Z

.../google_maps_flutter/android/src/main/java/io/flutter/plugins/googlemaps/HeatmapBuilder.java

+import com.google.maps.android.heatmaps.WeightedLatLng;
+import java.util.List;
+
+class HeatmapBuilder implements HeatmapOptionsSink {


I can see that it might be valuable to factor out everything related to the construction of the HeatmapOptions.
It doesn't seem to be necessary in this plugin. I would suggest to keep it simple to just create the HeatmapOptions object directly when it is required (which is only in a single place in this PR).

But I might be missing something?

cyanglaz · 2020-01-23T21:48:20Z

...ogle_maps_flutter/android/src/main/java/io/flutter/plugins/googlemaps/HeatmapController.java

+
+  private final TileOverlay mTileOverlay;
+
+  private final String mGoogleMapsHeatmapId;


seems not necessary to cache this in a private variable as it is only used in one place.

cyanglaz · 2020-01-23T21:57:52Z

.../google_maps_flutter/android/src/main/java/io/flutter/plugins/googlemaps/HeatmapOptions.java

+
+  private TileOverlay mTileOverlay;
+
+  public HeatmapOptions() {


I feel like all these methods should be package scoped instead of public.

packages/google_maps_flutter/example/ios/Flutter/Flutter.podspec

cyanglaz · 2020-01-23T22:07:44Z

packages/google_maps_flutter/ios/Classes/GoogleMapHeatmapController.m

+    NSNumber* latitude = data[i][0][0];
+    NSNumber* longitude = data[i][0][1];
+    NSNumber* intensity = data[i][1];


will any of these lines crash? For example, will data[i].length < 2 and thus crashes data[i][1]

That would most definitely crash, but that would also mean that the flutter part of this package is implemented improperly.
Should we throw an exception when the data is not correct, or how can this ideally be resolved?

cyanglaz · 2020-01-23T22:09:55Z

packages/google_maps_flutter/ios/Classes/GoogleMapHeatmapController.m

+static NSArray<UIColor*>* ToColors(NSArray* data) {
+  NSMutableArray* colors = [[NSMutableArray alloc] init];
+  for (unsigned i = 0; i < [data count]; i++) {
+    UIColor* color = data[i];
+    [colors addObject:color];
+  }
+
+  return colors;


The method seems unnecessary. It doesn't seem to be doing anything with the data in the array. The method just did a deep copy from data to the return value.

This method converts a JSON object (NSArray* data) to an array of UIColor*. I am no iOS developer but it didn't seem to work without this method.

cyanglaz · 2020-01-23T22:10:32Z

packages/google_maps_flutter/ios/Classes/GoogleMapHeatmapController.m

+static NSArray<NSNumber*>* ToStartsPoints(NSArray* data) {
+  NSMutableArray* startPoints = [[NSMutableArray alloc] init];
+  for (unsigned i = 0; i < [data count]; i++) {
+    NSNumber* startPoint = data[i];
+    [startPoints addObject:startPoint];
+  }
+
+  return startPoints;
+}


The method seems unnecessary. The method seems unnecessary. It doesn't seem to be doing anything with the data in the array. The method just did a deep copy from data to the return value.

This method converts a JSON object (NSArray* data) to a array of NSNumber*. I am no iOS developer but it didn't seem to work without this method.

cyanglaz · 2020-01-24T18:31:52Z

I think it might be a good idea to separate tileoverlay and heatmap into 2 different PRs. This will help the PRs to be reviewed faster. #2077 seems trying to add tile overlay for the plugin on Android. @SvenSlijkoord Do you think you can work with the author to put up a PR for tile overlay only? And then a separate PR for heat map?

SvenSlijkoord · 2020-01-24T18:52:30Z

@cyanglaz let me look into it.

Alystrasz · 2020-04-20T17:13:04Z

Are there any updates on this PR? Are CI-reported issues hard to fix?
Heatmaps feature is part of the flutter todo list and is long-awaited by devs (flutter/flutter#33586).

bmabir17 · 2020-04-20T18:26:25Z

Are there any updates on this PR? Are CI-reported issues hard to fix?
Heatmaps feature is part of the flutter todo list and is long-awaited by devs (flutter/flutter#33586).

@Alystrasz
I urgently needed this feature so i have forked this pull request
And published a forked package google_maps_flutter_heatmap (Tested only on android)

@SvenSlijkoord Thank you so much!! Hope you won't mind,that i published it on pub.

Alystrasz · 2020-04-20T18:41:06Z

@bmabir17
Actually, I'm using your fork right now in my project (thanks btw), but I need iOS support as well 😃

bmabir17 · 2020-04-20T19:09:09Z

@bmabir17
Actually, I'm using your fork right now in my project (thanks btw), but I need iOS support as well 😃

Can you tell me what's the error that you are facing for ios? Don't have an ios device to debug it 🙁

ditman

Hello there! Thank you very much for the PR!

We're about to merge a change to google_maps_flutter, so it follows the "federated implementation" style. This will allow new platforms (like web) to be supported.

Once that change lands, some of the code that you've created/touched should live in the google_maps_flutter_platform_interface package, and not here.

Check out this PR on how we migrated the whole plugin to the new architecture. It should give you clues on how to modify your code to conform to it.

Feel free to reach out to me if you need any assistance with this!

packages/google_maps_flutter/lib/src/controller.dart

packages/google_maps_flutter/lib/src/heatmap.dart

packages/google_maps_flutter/lib/src/heatmap_updates.dart

packages/google_maps_flutter/test/fake_maps_controllers.dart

YazeedAlKhalaf · 2020-04-22T21:43:08Z

Guys, When will this be merged?

ditman · 2020-04-24T19:36:40Z

The refactor has landed.

Alystrasz · 2020-04-26T22:19:03Z

@SvenSlijkoord do you plan to refactor your PR according to the recent plugins refactor ?

SvenSlijkoord · 2020-04-28T04:36:53Z

Yes, I will

…

On Mon, 27 Apr 2020, 12:19 am Rémy Raes, ***@***.***> wrote: @SvenSlijkoord <https://github.com/SvenSlijkoord> do you plan to refactor your PR according to the recent plugins refactor ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2454 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACW2NJNSA3K3BICKLGJL753ROSXOHANCNFSM4KEOBTBQ> .

…ement_google_maps_flutter_heatmap

…, from flutter#2454

stuartmorgan · 2021-02-11T19:35:12Z

Once the migration of maps to null-safety is complete, we can move forward with #3062, which takes on just the interface portion of this task, and then hopefully this can be updated to have a narrower scope, and we can get it reviewed.

stuartmorgan · 2021-12-14T16:36:32Z

We're starting the process of reviewing #3062. While that's happening, this could be updated in parallel to add tests (integration tests, unit tests for the native code) so that it will be closer to being review-ready once that PR is complete and landed.

…, from flutter#2454

stuartmorgan · 2022-02-08T16:10:31Z

Status update from PR triage: #3062 is still going through review.

However, this still needs tests before it could move forward even once that PR has landed.

ditman · 2022-02-09T01:57:42Z

Status update from PR triage: #3062 is still going through review.

@stuartmorgan The platform_interface on #3062 has diverged so much from the one here, that IMO we need to bring this PR up to date with the latest code there, and see how it works.

stuartmorgan · 2022-02-09T21:25:34Z

That makes sense. @SvenSlijkoord Are you still interested in updating this PR to the current state of the plugin?

stuartmorgan · 2022-03-22T17:04:25Z

Thank you for your contribution. I'm going to close this PR for now since there are outstanding comments, just to get this off our PR review queue. Please don't hesitate to submit a new PR if you have the time to address the review comments. Thanks!

SvenSlijkoord added 5 commits January 6, 2020 21:10

Implemented heatmap for google_maps_flutter.

0fdea83

Implemented iOS heatmaps.

8f8e8b6

Added tests for heatmaps

4800f2a

Removed unused methods

174a47d

Updated pubspec.yaml and CHANGELOG.md

2be40e4

SvenSlijkoord requested a review from iskakaushik as a code owner January 8, 2020 20:21

googlebot added the cla: yes label Jan 8, 2020

SvenSlijkoord changed the title ~~Implement google maps flutter heatmap~~ [google_maps_flutter] Implement google maps flutter heatmap Jan 11, 2020

SvenSlijkoord added 2 commits January 11, 2020 15:01

Ran flutter_plugin_tools format

b84ea79

Undo replace "YOUR KEY HERE" "YOUR_API_KEY"

45492f6

SvenSlijkoord requested a review from cyanglaz as a code owner January 11, 2020 14:15

SvenSlijkoord added 3 commits January 11, 2020 15:26

Formatted heatmap iOS code

99c6130

Added sven slijkoord to AUTHORS file

5750a9d

Ran flutter_plugin_tools format from mac OS X

3de3cfb

SvenSlijkoord mentioned this pull request Jan 11, 2020

[google_maps_flutter] Add support for heatmap layer flutter/flutter#33586

Open

cyanglaz reviewed Jan 23, 2020

View reviewed changes

cyanglaz mentioned this pull request Jan 23, 2020

[google_maps_flutter] Add TileOverlay support #2077

Closed

13 tasks

ditman suggested changes Apr 21, 2020

View reviewed changes

iskakaushik removed their request for review April 28, 2020 16:38

SvenSlijkoord added 2 commits April 29, 2020 08:37

Merge branch 'master' of https://github.com/flutter/plugins into impl…

d1890ba

…ement_google_maps_flutter_heatmap

Removed unused method and clarified variable names

9dd82f4

Alystrasz mentioned this pull request May 7, 2020

Adding support for heatmaps pandemia-app/pandemia#6

Closed

fergusdixon added a commit to fergusdixon/plugins that referenced this pull request Sep 21, 2020

add platform interface framwork for Google Maps heatmap functionality…

fca87d3

…, from flutter#2454

fergusdixon added a commit to fergusdixon/plugins that referenced this pull request Sep 21, 2020

add platform interface framwork for Google Maps heatmap functionality…

9cd065a

…, from flutter#2454

fergusdixon mentioned this pull request Sep 21, 2020

[google_maps_flutter] Heatmap platform interface #3062

Closed

13 tasks

fergusdixon added a commit to fergusdixon/plugins that referenced this pull request Sep 21, 2020

add platform interface framwork for Google Maps heatmap functionality…

c174166

…, from flutter#2454

fergusdixon added a commit to fergusdixon/plugins that referenced this pull request Sep 22, 2020

add platform interface framwork for Google Maps heatmap functionality…

a5fe695

…, from flutter#2454

tajchert mentioned this pull request Jan 7, 2021

[ios] Initializing google map with 'n' markers takes O(n) time to add and blocks the UI during this period flutter/flutter#60750

Open

stuartmorgan added the p: google_maps_flutter label Jan 29, 2021

stuartmorgan added the backlog label Feb 11, 2021

stuartmorgan removed the backlog label Dec 14, 2021

fergusdixon added a commit to fergusdixon/plugins that referenced this pull request Dec 15, 2021

add platform interface framwork for Google Maps heatmap functionality…

d094113

…, from flutter#2454

fergusdixon added a commit to fergusdixon/plugins that referenced this pull request Dec 20, 2021

add platform interface framwork for Google Maps heatmap functionality…

d13cf5e

…, from flutter#2454

godofredoc changed the base branch from master to main January 6, 2022 22:14

fergusdixon added a commit to fergusdixon/plugins that referenced this pull request Jan 8, 2022

add platform interface framwork for Google Maps heatmap functionality…

4448a1b

…, from flutter#2454

stuartmorgan closed this Mar 22, 2022

github-actions bot added platform-android platform-ios labels Mar 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[google_maps_flutter] Implement google maps flutter heatmap #2454

[google_maps_flutter] Implement google maps flutter heatmap #2454

SvenSlijkoord commented Jan 8, 2020 •

edited

urmilshroff commented Jan 12, 2020

cyanglaz left a comment

cyanglaz Jan 23, 2020

cyanglaz Jan 23, 2020

cyanglaz Jan 23, 2020

cyanglaz Jan 23, 2020

SvenSlijkoord Apr 29, 2020

cyanglaz Jan 23, 2020

SvenSlijkoord Apr 29, 2020

cyanglaz Jan 23, 2020

SvenSlijkoord Apr 29, 2020

cyanglaz commented Jan 24, 2020

SvenSlijkoord commented Jan 24, 2020

Alystrasz commented Apr 20, 2020

bmabir17 commented Apr 20, 2020 •

edited

Alystrasz commented Apr 20, 2020

bmabir17 commented Apr 20, 2020 •

edited

ditman left a comment

YazeedAlKhalaf commented Apr 22, 2020

ditman commented Apr 24, 2020

Alystrasz commented Apr 26, 2020

SvenSlijkoord commented Apr 28, 2020 via email

stuartmorgan commented Feb 11, 2021

stuartmorgan commented Dec 14, 2021

stuartmorgan commented Feb 8, 2022 •

edited

ditman commented Feb 9, 2022

stuartmorgan commented Feb 9, 2022

stuartmorgan commented Mar 22, 2022


		private final TileOverlay mTileOverlay;

		private final String mGoogleMapsHeatmapId;

[google_maps_flutter] Implement google maps flutter heatmap #2454

[google_maps_flutter] Implement google maps flutter heatmap #2454

Conversation

SvenSlijkoord commented Jan 8, 2020 • edited

Description

Related Issues

Checklist

Breaking Change

urmilshroff commented Jan 12, 2020

cyanglaz 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

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

cyanglaz commented Jan 24, 2020

SvenSlijkoord commented Jan 24, 2020

Alystrasz commented Apr 20, 2020

bmabir17 commented Apr 20, 2020 • edited

Alystrasz commented Apr 20, 2020

bmabir17 commented Apr 20, 2020 • edited

ditman left a comment

Choose a reason for hiding this comment

YazeedAlKhalaf commented Apr 22, 2020

ditman commented Apr 24, 2020

Alystrasz commented Apr 26, 2020

SvenSlijkoord commented Apr 28, 2020 via email

stuartmorgan commented Feb 11, 2021

stuartmorgan commented Dec 14, 2021

stuartmorgan commented Feb 8, 2022 • edited

ditman commented Feb 9, 2022

stuartmorgan commented Feb 9, 2022

stuartmorgan commented Mar 22, 2022

SvenSlijkoord commented Jan 8, 2020 •

edited

bmabir17 commented Apr 20, 2020 •

edited

bmabir17 commented Apr 20, 2020 •

edited

stuartmorgan commented Feb 8, 2022 •

edited