Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[google_maps_flutter] Add heatmap support #3257

Open
wants to merge 79 commits into
base: main
Choose a base branch
from

Conversation

Rexios80
Copy link
Contributor

@Rexios80 Rexios80 commented Feb 22, 2023

Transferred from flutter/plugins#5274

Adds heatmap support for web, iOS, and Android

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

Fixes #33586
Fixes #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.

IF YOU WANT TO USE THIS NOW

pubspec.yaml:

dependencies:
  google_maps_flutter: ^2.6.0

dependency_overrides:
  google_maps_flutter:
    git:
      url: https://github.com/Rexios80/packages_flutter
      ref: 01ab1db2d2ac3a13d70d25576c8aab0c5ba38a7c
      path: packages/google_maps_flutter/google_maps_flutter
  google_maps_flutter_web:
    git:
      url: https://github.com/Rexios80/packages_flutter
      ref: 01ab1db2d2ac3a13d70d25576c8aab0c5ba38a7c
      path: packages/google_maps_flutter/google_maps_flutter_web
  google_maps_flutter_android:
    git:
      url: https://github.com/Rexios80/packages_flutter
      ref: 01ab1db2d2ac3a13d70d25576c8aab0c5ba38a7c
      path: packages/google_maps_flutter/google_maps_flutter_android
  google_maps_flutter_ios:
    git:
      url: https://github.com/Rexios80/packages_flutter
      ref: 01ab1db2d2ac3a13d70d25576c8aab0c5ba38a7c
      path: packages/google_maps_flutter/google_maps_flutter_ios
  google_maps_flutter_platform_interface:
    git:
      url: https://github.com/Rexios80/packages_flutter
      ref: 01ab1db2d2ac3a13d70d25576c8aab0c5ba38a7c
      path: packages/google_maps_flutter/google_maps_flutter_platform_interface

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • 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.)
  • I signed the CLA.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

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

@reidbaker
Copy link
Contributor

Thank you for your contribution. It looks like the tests are failing. Before we review the PR, please see what you can do to resolve the test failures. If you are unsure how to proceed, please reach out for help on the #hackers-new channel.

@Rexios80
Copy link
Contributor Author

Rexios80 commented Mar 3, 2023

@reidbaker The actions seem to only be failing due to the required dependency overrides since this PR requires changes to the platform interface. Until we are ready to split out the platform interface changes into their own PR and get that merged, the actions will continue to fail on this PR.

@Rexios80
Copy link
Contributor Author

Rexios80 commented Mar 8, 2023

@stuartmorgan The example ios app Podfile needs to specify platform :ios, '13.0' in order to use the latest Google Maps iOS Cocoapod. Should I make a PR just for that or include it here?

@stuartmorgan
Copy link
Contributor

The example ios app Podfile needs to specify platform :ios, '13.0' in order to use the latest Google Maps iOS Cocoapod.

Why do you need the latest version? It looks like building and testing are passing as-is; doesn't that mean the APIs you are using are available back to the oldest version that supports 11.0?

@Rexios80
Copy link
Contributor Author

Rexios80 commented Mar 8, 2023

I don't necessarily need it, but since we were seeing behavior changes between versions on Android I thought it might be good to make sure the example app is actually using the latest.

@Rexios80 Rexios80 requested a review from stuartmorgan May 8, 2024 22:32
@reidbaker reidbaker self-requested a review May 9, 2024 18:45
@trevor-nomadik
Copy link

trevor-nomadik commented May 16, 2024

The heatmaps are not buildable without the radius, which is defined (even if it does nothing), however every time I try to use it with the heatmap it throws an undefined specifically for the HeatmapRadius. The Heatmap and even the HeatmapId work fine. Attempting to define it myself suddenly lets the compiler know that it is actually defined already, but as soon I remove my definition it goes back to not being defined.

The method 'HeatmapRadius' isn't defined for the type 'ReportsModel'.
Try correcting the name to the name of an existing method, or defining a method named 'HeatmapRadius'.

return Heatmap(heatmapId: const HeatmapId("debris"), data: data, radius: HeatmapRadius(radius: 10));

/// A wrapper around platform specific behavior for the radius of a [Heatmap].
///
/// Currently this class does nothing. See https://github.com/flutter/flutter/issues/145411
@immutable
class HeatmapRadius {
  const HeatmapRadius._(this.radius);

  /// Create a [HeatmapRadius] with a radius in pixels.
  ///
  /// This value will be used verbatim on all platforms. It is up to the
  /// developer to ensure that the value is appropriate for the platform.
  const HeatmapRadius.fromPlatformSpecificValue(int radius) : this._(radius);

  /// Create a [HeatmapRadius] from a platform value.
  ///
  /// In the future, this will do the opposite conversion that [pixels] does.
  const HeatmapRadius.fromJson(dynamic json) : this._(json as int);

  /// The platform-independent value of the radius.
  final int radius;

  /// The radius in pixels derived from [radius].
  ///
  /// In the future, this will convert [radius] to the current platform's
  /// expected value.
  int get pixels => radius;

  @override
  bool operator ==(Object other) {
    if (identical(this, other)) {
      return true;
    }
    if (other.runtimeType != runtimeType) {
      return false;
    }
    return other is HeatmapRadius && other.radius == radius;
  }

  @override
  int get hashCode => radius.hashCode;
}

I can get it working by just setting a default value for radius in the Heatmap class and removing the required keyword for what its worth.

@Rexios80
Copy link
Contributor Author

@trevor-nomadik There is no un-named constructor for the HeatmapRadius class. Thus HeatmapRadius(radius: 10) is invalid code.

Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately we're still waiting on a usable version of the Utils pod.

@@ -204,6 +205,262 @@ void runTests() {
},
);
}, skip: isWeb /* Tiles not supported on the web */);

/// Check that two lists of [WeightedLatLng] are more or less equal
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is missing a period.

}
}

/// Check that two [HeatmapGradient]s are more or less equal
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same.

void expectHeatmapEquals(Heatmap heatmap1, Heatmap heatmap2) {
expectHeatmapDataMoreOrLessEquals(heatmap1.data, heatmap2.data);
expectHeatmapGradientMoreOrLessEquals(heatmap1.gradient, heatmap2.gradient);
if (Platform.isAndroid) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please comment here and below as to why equality would depend on the platform; that seems very counter-intuitive.


expectHeatmapEquals(heatmap1New, heatmapInfo1);
},
skip: isAndroid,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please repeat the comment on each skip so it's not relying on someone knowing to go look at another test (and on that test still being there, and being the only reason any test in the file might skip Android).

import com.google.maps.android.heatmaps.WeightedLatLng;
import java.util.List;

public class HeatmapController implements HeatmapOptionsSink {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please comment classes and public methods (applies throughout the PR).

/// The radius in pixels derived from [radius].
///
/// In the future, this will convert [radius] to the current platform's
/// expected value.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment implies that the behavior of this method will change in the future; that's not the case. The purpose of the factory constructors is that it's the factories that will have different behavior, not this. Changing this method would be breaking, and avoiding that is the whole point of making HeatmapRadius in the first place.

this.dissipating = true,
this.gradient,
this.maxIntensity,
this.opacity = 0.7,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where did this magic number come from?

this.opacity = 0.7,
required this.radius,
this.minimumZoomIntensity = 0,
this.maximumZoomIntensity = 21,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question here.

/// This list must not be empty.
final List<WeightedLatLng> data;

/// Specifies whether heatmaps dissipate on zoom.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"the heatmap" rather than "heatmaps". An instance of this class is only one heatmap.


/// The data points to display.
///
/// This list must not be empty.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please assert this in the constructor if it's a requirement.

@stuartmorgan stuartmorgan added the triage-android Should be looked at in Android triage label Jun 3, 2024
@vishaldhaduk9986

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet