[google_maps_flutter] Objective-C code clean up #5780

cyanglaz · 2022-05-17T22:24:58Z

Objective-C code cleanup. See: flutter/flutter#102597

It has a lot of re-naming, let me know if it is hard to review. I can try separate it to smaller patches.

Pre-launch Checklist

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

flutter-dashboard · 2022-05-17T22:25:00Z

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 Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on 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.

Hixie · 2022-05-18T00:06:00Z

test-exempt: code reactor

yay cleanup!

stuartmorgan-g

Mostly the comments are just nits or suggested follow-ups for later PRs. Lots of great cleanup here!

In a few places where things were substantially moved around I couldn't really tell what changed, but I'm assuming it was either nothing, or pretty mechanical changes.

stuartmorgan-g · 2022-05-18T00:30:44Z

...ages/google_maps_flutter/google_maps_flutter/ios/Classes/FLTGoogleMapTileOverlayController.h

 @interface FLTTileProviderController : GMSTileLayer
-@property(copy, nonatomic, readonly) NSString *tileOverlayId;
- (instancetype)init:(FlutterMethodChannel *)methodChannel tileOverlayId:(NSString *)tileOverlayId;
+@property(copy, nonatomic, readonly) NSString *tileOverlayIdentifier;


Potential follow up: all of these (class, property, method) are missing comments. That's true for a lot of code in the plugin.

Updated the issue adding a new item about comment/docs: flutter/flutter#102597

stuartmorgan-g · 2022-05-18T00:31:37Z

...ages/google_maps_flutter/google_maps_flutter/ios/Classes/FLTGoogleMapTileOverlayController.h

 - (instancetype)init:(FlutterMethodChannel *)methodChannel
             mapView:(GMSMapView *)mapView
           registrar:(NSObject<FlutterPluginRegistrar> *)registrar;
 - (void)addTileOverlays:(NSArray *)tileOverlaysToAdd;


Potential follow up: Do a sweep over the plugin to add generics to all the collections.

Updated the issue adding a new item about adding generics to the collection types: flutter/flutter#102597

stuartmorgan-g · 2022-05-18T00:31:44Z

...ages/google_maps_flutter/google_maps_flutter/ios/Classes/FLTGoogleMapTileOverlayController.h

+    tileOverlayIdentifier:(NSString *)identifier;
 @end

 @interface FLTTileOverlaysController : NSObject


Potential follow up: comments.

Updated the issue adding a new item about comment/docs: flutter/flutter#102597

stuartmorgan-g · 2022-05-18T00:34:31Z

...ages/google_maps_flutter/google_maps_flutter/ios/Classes/FLTGoogleMapTileOverlayController.h

- (instancetype)init:(FlutterMethodChannel *)methodChannel tileOverlayId:(NSString *)tileOverlayId;
+@property(copy, nonatomic, readonly) NSString *tileOverlayIdentifier;
+- (instancetype)init:(FlutterMethodChannel *)methodChannel
+    tileOverlayIdentifier:(NSString *)identifier;


withTileOverlayIdentifier:

stuartmorgan-g · 2022-05-18T00:35:04Z

...ages/google_maps_flutter/google_maps_flutter/ios/Classes/FLTGoogleMapTileOverlayController.m

           registrar:(NSObject<FlutterPluginRegistrar> *)registrar {
  self = [super init];
  if (self) {
    self.methodChannel = methodChannel;


These selfs should all become ivar assignments.

stuartmorgan-g · 2022-05-18T01:23:05Z

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

+
 - (instancetype)initPolylineWithPath:(GMSMutablePath *)path
-                          polylineId:(NSString *)polylineId
+                  polylineIdentifier:(NSString *)polylineIdentifier


Can also just be identifier

stuartmorgan-g · 2022-05-18T01:24:38Z

packages/google_maps_flutter/google_maps_flutter/ios/Classes/JsonConversions.h

+ (NSDictionary<NSString *, NSNumber *> *)getJsonFromPoint:(CGPoint)point;
+ (nullable NSDictionary *)getJsonFromCoordinateBounds:(nullable GMSCoordinateBounds *)bounds;
+ (nullable GMSCameraPosition *)getOptionalCameraPostionFromData:(nullable NSDictionary *)data;
+ (CGPoint)getPointFromDictioanry:(NSDictionary *)dictionary;


Typo: Dictionary

stuartmorgan-g · 2022-05-18T01:25:13Z

packages/google_maps_flutter/google_maps_flutter/ios/Classes/JsonConversions.h

+ (float)getFloatFromData:(NSNumber *)data;
+ (CLLocationCoordinate2D)getLocationFromData:(NSArray *)data;
+ (CGPoint)getPointFromArray:(NSArray *)data;
+ (NSArray *)getJsonFromLocation:(CLLocationCoordinate2D)location;


All the getJsons should be JSON

stuartmorgan-g · 2022-05-18T01:25:48Z

packages/google_maps_flutter/google_maps_flutter/ios/Classes/JsonConversions.h

+    (nullable GMSCameraPosition *)position;
+ (NSDictionary<NSString *, NSNumber *> *)getJsonFromPoint:(CGPoint)point;
+ (nullable NSDictionary *)getJsonFromCoordinateBounds:(nullable GMSCoordinateBounds *)bounds;
+ (nullable GMSCameraPosition *)getOptionalCameraPostionFromData:(nullable NSDictionary *)data;


I've never seen nullability encoded in a name like this. Is this version used enough to need a wrapper, vs callers doing the short-circuit on null (which is more common for a nullable language than having two versions of the API)?

Removed the nullability naming. Also removed the private method.

stuartmorgan-g · 2022-05-18T01:26:03Z

packages/google_maps_flutter/google_maps_flutter/ios/Classes/JsonConversions.h

+ (nullable NSDictionary *)getJsonFromCoordinateBounds:(nullable GMSCoordinateBounds *)bounds;
+ (nullable GMSCameraPosition *)getOptionalCameraPostionFromData:(nullable NSDictionary *)data;
+ (CGPoint)getPointFromDictioanry:(NSDictionary *)dictionary;
+ (nullable GMSCoordinateBounds *)getOptionalCoordinateBoundsFromData:(NSArray *)data;


Same nullability variant comment here.

stuartmorgan-g · 2022-05-18T01:32:42Z

packages/google_maps_flutter/google_maps_flutter/ios/Classes/JsonConversions.m

  return holes;
 }

+ (nullable NSDictionary<NSString *, id> *)getJsonFromPosition:(GMSCameraPosition *)position {


Oops, one thing I missed: these newly-added-to-this-class methods should get new unit tests since they should now be easily unit-testable.

Good point! This PR will have some tests after all :)

cc @Hixie

jmagman · 2022-05-23T20:55:41Z

Looks like the app is crashing during tests? Also there are format issues.

remove unnecessary static funcctions clean up method name and remove unused code update version remove all static methods fix remove unnecessary imports review part 1 review 2 drat 2 format, crash, more tests

jmagman

I commented in a few areas, but it looks like we can simplify things by removing the FLTGoogleMapTileOverlayOptionsSink, FLTGoogleMapOptionsSink FLTGoogleMapCircleOptionsSink FLTGoogleMapMarkerOptionsSink and FLTGoogleMapPolygonOptionsSink protocols.

...ter/google_maps_flutter/example/ios/RunnerTests/FLTGoogleMapJSONConversionsConversionTests.m

...ages/google_maps_flutter/google_maps_flutter/ios/Classes/FLTGoogleMapTileOverlayController.h

...ages/google_maps_flutter/google_maps_flutter/ios/Classes/FLTGoogleMapTileOverlayController.m

jmagman · 2022-05-25T00:40:17Z

...ages/google_maps_flutter/google_maps_flutter/ios/Classes/FLTGoogleMapTileOverlayController.m

+ (void)interpretTileOverlayOptions:(NSDictionary *)data
+                               sink:(id<FLTGoogleMapTileOverlayOptionsSink>)sink {
+  NSNumber *visible = data[@"visible"];
+  if (visible != nil && visible != (id)[NSNull null]) {
+    [sink setVisible:visible.boolValue];
+  }


This class method is kind of odd, looks like it should be an instance method on FLTGoogleMapTileOverlayController like:

Suggested change

+ (void)interpretTileOverlayOptions:(NSDictionary *)data

sink:(id<FLTGoogleMapTileOverlayOptionsSink>)sink {

NSNumber *visible = data[@"visible"];

if (visible != nil && visible != (id)[NSNull null]) {

[sink setVisible:visible.boolValue];

}

- (void)setTileOverlayOptionsFromData:(NSDictionary *)data {

NSNumber *visible = data[@"visible"];

if (visible != nil && visible != (id)[NSNull null]) {

self.visible = visible.boolValue;

}

And at that point the data dictionary could be passed into -initWithTileLayer and populate the backing ivars and FLTGoogleMapTileOverlayOptionsSink can go away (same comment for all the Sink protocols).

Done.
I didn't update the init method if the interpret method includes a registrar parameter. I feel like as the registrar is not owned by the controller, it feels odd to have it in an init method.

packages/google_maps_flutter/google_maps_flutter/ios/Classes/JsonConversions.h

packages/google_maps_flutter/google_maps_flutter/ios/Classes/GoogleMapCircleController.h

packages/google_maps_flutter/google_maps_flutter/ios/Classes/GoogleMapController.m

packages/google_maps_flutter/google_maps_flutter/ios/Classes/GoogleMapPolygonController.m

cyanglaz · 2022-05-26T22:24:37Z

@stuartmorgan @jmagman I updated the code per your suggestions! PTAL

stuartmorgan-g

LGTM once @jmagman signs off. I didn't do a full re-review, just spot-checked around some of my previous comments. I'm sure I could continue to find things to suggest if I keep staring at this code, but it's already a big change and clearly an incremental improvement as-is 🙂

stuartmorgan-g · 2022-05-27T18:37:01Z

packages/google_maps_flutter/google_maps_flutter/ios/Classes/JsonConversions.h

+
+@interface FLTGoogleMapJSONConversions : NSObject
+
+ (CLLocationCoordinate2D)locationFromLatlong:(NSArray *)latlong;


Optional nit: here and in all the instances below, I think LatLong would be better, as it's two (abbbreviated) words. I think treating it as one word once abbreviated hurts readability.

stuartmorgan-g · 2022-05-27T18:46:12Z

packages/google_maps_flutter/google_maps_flutter/ios/Classes/JsonConversions.h

-+ (NSArray<NSArray<CLLocation *> *> *)toHoles:(NSArray *)data;
+NS_ASSUME_NONNULL_BEGIN
+
+@interface FLTGoogleMapJSONConversions : NSObject


Nit: The file should be named FLTGoogleMapJSONConversions.h.

(Or even better, the name should stay the same and all of these should become C functions—since "classes" that are just class methods are an abomination IMO—at which point the name should be the same. But I know many ObjC developers like this style so I'm not going to die on that hill today.)

jmagman · 2022-05-27T19:30:47Z

...ages/google_maps_flutter/google_maps_flutter/ios/Classes/FLTGoogleMapTileOverlayController.m


 @interface FLTTileProviderController ()

-@property(weak, nonatomic) FlutterMethodChannel *methodChannel;


Do you remember why you made this weak in the first place?
https://github.com/flutter/plugins/pull/3434/files#diff-7013871fe3ee02101b28aad71693b5ef3d5f48414c744ddef944e57fe886c5f4R100

The same reason I made all method channels to be weak in this PR. The owner of the methodchannel is FLTGoogleMapController. And FLTGoogleMapController owns and out lives all the sub controllers, so I thought it is not necessary for the sub controllers to strongly own methodChannel.

Then did you mean to make it strong in this PR?

Yep, to match other controllers.

So should they all be weak as you had it before? I'm not trying to argue one way or the other.

I think Strong is better. There won't be a retain cycle as method channel is not going to have a strong reference to the controllers.
Having them as weak makes the methodChannel's life cycle confusing. One needs to figure out who strongly owns the methodChannel (GoogleMapsController) to understand its life cycle.

…o google_map_clean_up

jmagman

LGTM

jmagman · 2022-05-31T21:58:29Z

Once format is happy.

cyanglaz · 2022-06-03T00:41:20Z

Release bot won't be happy due to some out of band failure. Manually published the change.

flutter-dashboard bot added the needs tests label May 17, 2022

github-actions bot added p: google_maps_flutter platform-ios labels May 17, 2022

cyanglaz requested a review from stuartmorgan-g May 17, 2022 23:36

cyanglaz force-pushed the google_map_clean_up branch from 81ca7fc to a0e4f33 Compare May 17, 2022 23:43

stuartmorgan-g suggested changes May 18, 2022

View reviewed changes

stuartmorgan-g reviewed May 18, 2022

View reviewed changes

draft

b376693

remove unnecessary static funcctions clean up method name and remove unused code update version remove all static methods fix remove unnecessary imports review part 1 review 2 drat 2 format, crash, more tests

cyanglaz force-pushed the google_map_clean_up branch from 5753e7a to b376693 Compare May 24, 2022 22:58

Chris Yang added 2 commits May 24, 2022 16:01

update version

fcd3c69

fix crashes

367466b

cyanglaz removed the needs tests label May 24, 2022

jmagman reviewed May 25, 2022

View reviewed changes

review

fb856e6

method channels to strong

459555d

stuartmorgan-g approved these changes May 27, 2022

View reviewed changes

jmagman reviewed May 27, 2022

View reviewed changes

Chris Yang added 2 commits May 31, 2022 10:28

renaming

f23fcbe

Merge branch 'google_map_clean_up' of github.com:cyanglaz/plugins int…

fccc746

…o google_map_clean_up

jmagman approved these changes May 31, 2022

View reviewed changes

format

da044f1

cyanglaz added the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label Jun 1, 2022

fluttergithubbot merged commit 801c1f6 into flutter:main Jun 1, 2022

cyanglaz deleted the google_map_clean_up branch June 3, 2022 00:40

engine-flutter-autoroll mentioned this pull request Jun 3, 2022

Roll Plugins from ae6757435c97 to d5fd7e437e1a (14 revisions) flutter/flutter#105312

Closed

engine-flutter-autoroll mentioned this pull request Jun 6, 2022

Roll Plugins from ae6757435c97 to f781b036c830 (21 revisions) flutter/flutter#105445

Closed

engine-flutter-autoroll mentioned this pull request Jun 6, 2022

Roll Plugins from ae6757435c97 to 8ded25d1b674 (22 revisions) flutter/flutter#105451

Merged


		@interface FLTGoogleMapJSONConversions : NSObject

		+ (CLLocationCoordinate2D)locationFromLatlong:(NSArray *)latlong;


		@interface FLTTileProviderController ()

		@property(weak, nonatomic) FlutterMethodChannel *methodChannel;

[google_maps_flutter] Objective-C code clean up #5780

[google_maps_flutter] Objective-C code clean up #5780

Uh oh!

Conversation

cyanglaz commented May 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented May 17, 2022

Uh oh!

Hixie commented May 18, 2022

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmagman commented May 23, 2022

Uh oh!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cyanglaz commented May 26, 2022

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyanglaz commented May 17, 2022 •

edited

Loading