[google_maps_flutter] Implement polyline patterns in google maps ios #5757

Hari-07 · 2023-12-26T09:25:35Z

This PR Implements patterns in google maps ios polylines. Currently the patterns param is simply ignored on ios, despite the official google maps SDK for ios having instructions on how to achieve repeated patterns: https://developers.google.com/maps/documentation/ios-sdk/shapes#add-a-repeating-color-pattern-to-a-polyline

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

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

Pre-launch Checklist

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

flutter-dashboard · 2023-12-26T09:25:39Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Hari-07 · 2023-12-26T09:29:28Z

I don't currently have any tests, however that is because looking at the existing tests, since I only made changes to the ios package, it didn't seem right to author tests in the google_maps_flutter package.

But looking at tests in google_maps_flutter_ios it seemed like they do not test things like what this PR covers

Open to authoring tests, if someone could let me know what the tests should test

stuartmorgan · 2024-01-02T14:31:58Z

Marking as a draft for now since this needs native unit tests before it will be ready for review (per discussion in Discord).

Hari-07 · 2024-01-04T07:11:39Z

I implemented tests for 2 out of 3 functions I wrote. For the 3rd function, the test should look something like

- (void)testStrokeStylesFromPatterns {
    NSArray* patterns = @ [@[@"gap", @10], @[@"dash", @10]];
    UIColor* strokeColor = [UIColor redColor];
    
    NSArray<GMSStrokeStyle*> *patternStrokeStyle = [FLTGoogleMapJSONConversions strokeStylesFromPatterns:patterns strokeColor:strokeColor];
    
    XCTAssertEqual([patternStrokeStyle count], 2);
    
    GMSStrokeStyle* firstStrokeStyle = patternStrokeStyle[0];
    GMSStrokeStyle* secondStrokeStyle = patternStrokeStyle[1];
    
    // XCTAssertEqualObjects(firstStrokeStyle, [GMSStrokeStyle solidColor:[UIColor clearColor]]);
    // XCTAssertEqualObjects(secondStrokeStyle, [GMSStrokeStyle solidColor:strokeColor]);
}

However, the color property is not readable so I couldn't get this to work.

...r/google_maps_flutter_ios/example/ios11/ios/RunnerTests/GoogleMapsPolylinesControllerTests.m

vashworth · 2024-04-03T21:02:10Z

@Hari-07 Is this ready for re-review?

Hari-07 · 2024-04-28T06:27:17Z

@Hari-07 Is this ready for re-review?

Yeah it should be good to go, I had a couple of questions that I've commented above. I believe I've addressed all the comments @stuartmorgan had made, except for the type generics. I have update that too but had a question which I've commented above on that thread

yaakovschectman

LGTM for Android changes

stuartmorgan

We've collapsed the iOS 12 example into the iOS 14 example due to dropping iOS 12 and iOS 13 support, so you'll need to migrate the test changes to that example. Sorry for the churn!

stuartmorgan · 2024-05-07T16:36:49Z

packages/google_maps_flutter/google_maps_flutter_ios/CHANGELOG.md

@@ -1,3 +1,7 @@
+## 2.3.6
+
+* Adds support for patterns in polylines on iOS.


This still says "on iOS". Please make sure you are not marking things as resolved when the requested change has not been made, as it makes review much harder.

stuartmorgan · 2024-05-07T16:37:26Z

...ter/google_maps_flutter_ios/example/ios12/ios/RunnerTests/GoogleMapPolylineController_Test.h

+
+/**
+ *  Internal APIs exposed for unit testing
+ */


We've recently standardized on /// for declaration comments in Obj-C code, so these should all be updated to use that.

(Applies throughout the PR.)

stuartmorgan · 2024-05-07T16:37:50Z

...ter/google_maps_flutter_ios/example/ios12/ios/RunnerTests/GoogleMapPolylineController_Test.h

+ */
+@interface FLTPolylinesController (Test)
+/**
+ * Returns the path for polyline based on the points(locations) the polyline has.


Missing space before (.

stuartmorgan · 2024-05-07T16:38:51Z

...ter/google_maps_flutter_ios/example/ios12/ios/RunnerTests/GoogleMapPolylineController_Test.h

_Test headers go next to the actual headers, not in the test directory.

stuartmorgan · 2024-05-07T16:39:44Z

...r/google_maps_flutter_ios/example/ios12/ios/RunnerTests/GoogleMapsPolylinesControllerTests.m

+
+@implementation GoogleMapsPolylinesControllerTests
+
+- (FLTGoogleMapPolylineController *)setUpPolyLineControllerWithMockedMap {


This needs a declaration comment, per style guide. Test helper code is still code that humans have to understand and maintain, which is what comments are to help with.

stuartmorgan · 2024-05-07T16:40:26Z

...r/google_maps_flutter_ios/example/ios12/ios/RunnerTests/GoogleMapsPolylinesControllerTests.m

+
+@implementation GoogleMapsPolylinesControllerTests
+
+- (FLTGoogleMapPolylineController *)setUpPolyLineControllerWithMockedMap {


This should just be called polylineControllerWithMockedMap since it returns one; setUp... doesn't sound like a method that returns anything.

stuartmorgan · 2024-05-07T16:44:17Z

packages/google_maps_flutter/google_maps_flutter_ios/ios/Classes/FLTGoogleMapJSONConversions.m

+
+ (NSArray<NSNumber *> *)spanLengthsFromPatterns:(NSArray<NSArray *> *)patterns {
+  NSMutableArray *lengths = [[NSMutableArray alloc] initWithCapacity:[patterns count]];
+  for (unsigned int i = 0; i < [patterns count]; i++) {


This should use fast enumeration rather than indexing, and addObject: to lengths.

stuartmorgan · 2024-05-07T16:44:40Z

packages/google_maps_flutter/google_maps_flutter_ios/ios/Classes/FLTGoogleMapJSONConversions.m

+ (NSArray<GMSStrokeStyle *> *)strokeStylesFromPatterns:(NSArray<NSArray *> *)patterns
+                                            strokeColor:(UIColor *)strokeColor {
+  NSMutableArray *strokeStyles = [[NSMutableArray alloc] initWithCapacity:[patterns count]];
+  for (unsigned int i = 0; i < [patterns count]; i++) {


This should use fast enumeration rather than indexing, and addObject: to strokeStyles.

stuartmorgan · 2024-05-07T16:45:37Z

packages/google_maps_flutter/google_maps_flutter_ios/ios/Classes/GoogleMapPolylineController.m

@@ -171,7 +191,7 @@ - (bool)hasPolylineWithIdentifier:(NSString *)identifier {
  }
  return self.polylineIdentifierToController[identifier] != nil;
 }
-+ (GMSMutablePath *)getPath:(NSDictionary *)polyline {
+- (GMSMutablePath *)pathForPolyline:(NSDictionary *)polyline {
  NSArray *pointArray = polyline[@"points"];
  NSArray<CLLocation *> *points = [FLTGoogleMapJSONConversions pointsFromLatLongs:pointArray];


I don't see a response or change related to this comment.

Hari-07 · 2024-03-25T02:27:43Z

packages/google_maps_flutter/google_maps_flutter_ios/ios/Classes/GoogleMapPolylineController.m

+id GetValueOrNilFromDict(NSDictionary * dict, NSString *key) {
+    id value = dict[key];
+    return value == [NSNull null] ? nil : value;
+}


What'a s good place to put this function? Feels kinda out of place here, maybe a new utils file?

It's fine to leave this in the file that needs it, although it should be declared static.

(It will go way soon, when we convert to Pigeon, so making a new file will just add extra work to undo later.)

Hari-07 · 2024-03-25T02:37:19Z

packages/google_maps_flutter/google_maps_flutter_ios/ios/Classes/GoogleMapPolylineController.m

@@ -171,7 +191,7 @@ - (bool)hasPolylineWithIdentifier:(NSString *)identifier {
  }
  return self.polylineIdentifierToController[identifier] != nil;
 }
-+ (GMSMutablePath *)getPath:(NSDictionary *)polyline {
+- (GMSMutablePath *)pathForPolyline:(NSDictionary *)polyline {


Don't remember off the top of my head. Reverting back to static

Correct me if im wrong, since it's not listed in the .h file, I understand that this method isn't available publically to import. If that's the case, why make it static if not needed?

Hari-07 · 2024-03-25T02:41:01Z

packages/google_maps_flutter/google_maps_flutter_ios/ios/Classes/GoogleMapPolylineController.m

@@ -171,7 +191,7 @@ - (bool)hasPolylineWithIdentifier:(NSString *)identifier {
  }
  return self.polylineIdentifierToController[identifier] != nil;
 }
-+ (GMSMutablePath *)getPath:(NSDictionary *)polyline {
+- (GMSMutablePath *)pathForPolyline:(NSDictionary *)polyline {
  NSArray *pointArray = polyline[@"points"];
  NSArray<CLLocation *> *points = [FLTGoogleMapJSONConversions pointsFromLatLongs:pointArray];


This is only being used internally so I think it makes sense to have it in here. I'd assume the FLTJSONConversion file ought to be dumb and just convert and needn't know about the structure of the polyline object (polyline[@"points"])

Hari-07 · 2024-03-25T02:43:31Z

packages/google_maps_flutter/google_maps_flutter_ios/ios/Classes/GoogleMapPolylineController.m

@@ -171,7 +191,7 @@ - (bool)hasPolylineWithIdentifier:(NSString *)identifier {
  }
  return self.polylineIdentifierToController[identifier] != nil;
 }
-+ (GMSMutablePath *)getPath:(NSDictionary *)polyline {
+- (GMSMutablePath *)pathForPolyline:(NSDictionary *)polyline {
  NSArray *pointArray = polyline[@"points"];
  NSArray<CLLocation *> *points = [FLTGoogleMapJSONConversions pointsFromLatLongs:pointArray];


Or we could move this to the polylineController instead of polylinesController. Then it'd have access to the polyline instance instead of passing that in

Hari-07 · 2024-04-28T06:42:29Z

packages/google_maps_flutter/google_maps_flutter_ios/ios/Classes/FLTGoogleMapJSONConversions.m

@@ -141,4 +141,34 @@ + (nullable GMSCameraUpdate *)cameraUpdateFromChannelValue:(NSArray *)channelVal
  }
  return nil;
 }
+
+ (NSArray<GMSStrokeStyle *> *)strokeStylesFromPatterns:(NSArray *)patterns


I specified that its an array of arrays like so

(NSArray<GMSStrokeStyle *> *)strokeStylesFromPatterns:(NSArray<NSArray *> *)patterns strokeColor:(UIColor *)strokeColor

As for the inner array, it has 2 members the first one being a string, and the second a number so I wasn't sure how that ought to be defined. Would be better as a map, but this is how the data is passed in to the channel

Hari-07 · 2024-04-28T06:44:34Z

packages/google_maps_flutter/google_maps_flutter_ios/ios/Classes/FLTGoogleMapJSONConversions.m

@@ -141,4 +141,34 @@ + (nullable GMSCameraUpdate *)cameraUpdateFromChannelValue:(NSArray *)channelVal
  }
  return nil;
 }
+
+ (NSArray<GMSStrokeStyle *> *)strokeStylesFromPatterns:(NSArray *)patterns


I could also specify the internal array as an NSObject and then cast it explicitly. Not sure if that's any better though

+ (NSArray<GMSStrokeStyle *> *)strokeStylesFromPatterns:(NSArray<NSArray<NSObject *> *> *)patterns strokeColor:(UIColor *)strokeColor { NSMutableArray *strokeStyles = [[NSMutableArray alloc] initWithCapacity:[patterns count]]; for (unsigned int i = 0; i < [patterns count]; i++) { NSString *patternType = (NSString *)patterns[i][0];

Hari-07 · 2024-05-07T16:51:51Z

packages/google_maps_flutter/google_maps_flutter_ios/ios/Classes/GoogleMapPolylineController.m

@@ -171,7 +191,7 @@ - (bool)hasPolylineWithIdentifier:(NSString *)identifier {
  }
  return self.polylineIdentifierToController[identifier] != nil;
 }
-+ (GMSMutablePath *)getPath:(NSDictionary *)polyline {
+- (GMSMutablePath *)pathForPolyline:(NSDictionary *)polyline {
  NSArray *pointArray = polyline[@"points"];
  NSArray<CLLocation *> *points = [FLTGoogleMapJSONConversions pointsFromLatLongs:pointArray];


It was meant to be a question. I think it makes sense to leave it here or move it to polylines contoller neither of which are as per your suggestion of moving it to JsonConversions. Was asking what'd be better

Hari-07 · 2024-05-08T05:18:47Z

packages/google_maps_flutter/google_maps_flutter_ios/CHANGELOG.md

@@ -1,3 +1,7 @@
+## 2.3.6
+
+* Adds support for patterns in polylines on iOS.


Agreed 100%. Definitely resolved it accidentally, have made that change now

stuartmorgan · 2024-05-08T15:07:47Z

I could also specify the internal array as an NSObject and then cast it explicitly. Not sure if that's any better though

For some reason GitHub won't let me respond inline to this, but yes, that would be better. It makes it explicit in the code that the array is heterogenous, which is unusual and thus better to have be as explicit as possible.

…ern-ios

Hari-07 requested review from hellohuanlin, reidbaker and stuartmorgan as code owners December 26, 2023 09:25

github-actions bot added p: google_maps_flutter platform-android platform-ios labels Dec 26, 2023

Hari-07 force-pushed the polyline-pattern-ios branch from 8334dd1 to 348b17f Compare December 26, 2023 09:40

stuartmorgan marked this pull request as draft January 2, 2024 14:32

Hari-07 commented Jan 4, 2024

View reviewed changes

...r/google_maps_flutter_ios/example/ios11/ios/RunnerTests/GoogleMapsPolylinesControllerTests.m Outdated Show resolved Hide resolved

Hari-07 marked this pull request as ready for review January 4, 2024 07:14

Hari-07 requested review from cbracken, jmagman, LouiseHsu, vashworth, camsim99, gmackall, ditman, bparrishMines, yjbanov, tarrinneal, Piinks, Hixie, keyonghan, chunhtai, domesticmouse and gspencergoog as code owners January 4, 2024 07:49

Hari-07 added 9 commits March 24, 2024 09:13

Set version in pubspec

91439c5

Make the polyline controller test stateless

ef1d1a6

Remove empty line

5c6a694

Dont make polyline controller public

cb7b9ea

add comments and avoid making get path public

762741e

Fix test

90537da

Add stroke style json conversion test

76b3d59

Fix formatting issue in new test

c10082c

Format comment

28a7840

Hari-07 force-pushed the polyline-pattern-ios branch from 0e0b9e7 to 28a7840 Compare March 24, 2024 03:44

Hari-07 added 5 commits March 24, 2024 09:15

Update changelog

41e4a2c

Remove android changelog comment

ef19218

Fix some of the comments

25e003e

Use helper function to extract nullable field from dict

6d7f7c1

Revert path for polyline into static function

2a58ab9

Specify the type more

8428c96

yaakovschectman approved these changes May 2, 2024

View reviewed changes

stuartmorgan requested changes May 7, 2024

View reviewed changes

Hari-07 commented May 8, 2024

View reviewed changes

Remove on ios in changelog

4eca5a6

Hari-07 added 7 commits May 11, 2024 16:28

Move the header file

3975be2

Remove test temporarily

0daeda6

Merge branch 'main' of github.com:flutter/packages into polyline-patt…

a1daf7a

…ern-ios

Add back tests

3506885

Add back changelog change

64006eb

Bump pubspec version

9649254

Add polyline test file to xcode

4b3ddad

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[google_maps_flutter] Implement polyline patterns in google maps ios #5757

[google_maps_flutter] Implement polyline patterns in google maps ios #5757

Hari-07 commented Dec 26, 2023 •

edited

flutter-dashboard bot commented Dec 26, 2023

Hari-07 commented Dec 26, 2023

stuartmorgan commented Jan 2, 2024

Hari-07 commented Jan 4, 2024

vashworth commented Apr 3, 2024

Hari-07 commented Apr 28, 2024 •

edited

yaakovschectman left a comment

stuartmorgan left a comment

stuartmorgan May 7, 2024

stuartmorgan May 7, 2024

stuartmorgan May 7, 2024

stuartmorgan May 7, 2024

stuartmorgan May 7, 2024

stuartmorgan May 7, 2024

stuartmorgan May 7, 2024

stuartmorgan May 7, 2024

stuartmorgan May 7, 2024

Hari-07 Mar 25, 2024

stuartmorgan May 8, 2024

Hari-07 Mar 25, 2024

Hari-07 Mar 25, 2024

Hari-07 Mar 25, 2024

Hari-07 Apr 28, 2024

Hari-07 Apr 28, 2024

Hari-07 May 7, 2024

Hari-07 May 8, 2024

stuartmorgan commented May 8, 2024

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

		* Adds support for patterns in polylines on iOS.


		@implementation GoogleMapsPolylinesControllerTests

		- (FLTGoogleMapPolylineController *)setUpPolyLineControllerWithMockedMap {

[google_maps_flutter] Implement polyline patterns in google maps ios #5757

Are you sure you want to change the base?

[google_maps_flutter] Implement polyline patterns in google maps ios #5757

Conversation

Hari-07 commented Dec 26, 2023 • edited

Pre-launch Checklist

flutter-dashboard bot commented Dec 26, 2023

Hari-07 commented Dec 26, 2023

stuartmorgan commented Jan 2, 2024

Hari-07 commented Jan 4, 2024

vashworth commented Apr 3, 2024

Hari-07 commented Apr 28, 2024 • edited

yaakovschectman left a comment

Choose a reason for hiding this comment

stuartmorgan 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

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 May 8, 2024

Hari-07 commented Dec 26, 2023 •

edited

Hari-07 commented Apr 28, 2024 •

edited