[google_maps_flutter] Marker APIs are now widget based (iOS Changes)#1240
[google_maps_flutter] Marker APIs are now widget based (iOS Changes)#1240iskakaushik merged 6 commits intoflutter:masterfrom iskakaushik:feature/maps-ios-impl
Conversation
bparrishMines
changed the title
|
|
||
| #import "JsonConversions.h" | ||
|
|
||
| bool toBool(id json) { |
There was a problem hiding this comment.
Don't add functions to the global namespace.
Make these class methods of an ObjC class.
| } | ||
| @end | ||
|
|
||
| static void interpretMarkerOptions(id json, id<FLTGoogleMapMarkerOptionsSink> sink, |
There was a problem hiding this comment.
This should start with a capital letter, see: http://google.github.io/styleguide/objcguide.html#function-names
| NSArray* iconData = icon; | ||
| UIImage* image; | ||
| if ([iconData[0] isEqualToString:@"defaultMarker"]) { | ||
| CGFloat hue = (iconData.count == 1) ? 0.0f : toDouble(iconData[1]); |
There was a problem hiding this comment.
(not for this PR) it's a little weird that we pass just the hue parameter, would it make sense to pass the exact color?
| } | ||
| id icon = data[@"icon"]; | ||
| if (icon) { | ||
| NSArray* iconData = icon; |
There was a problem hiding this comment.
nit: I'd consider extract the iconData parsing to a separate function, this one is becoming a little long.
There was a problem hiding this comment.
probably same for infoWindow
| } | ||
| - (void)addMarkers:(id)markersToAdd { | ||
| NSArray<id>* markers = markersToAdd; | ||
| if (!markers) { |
There was a problem hiding this comment.
A safer check would be:
if (![markersToAdd isKindOfClass:[NSArray class]]), this also covers the cases that it's NSNull (if it was explicitly set to null), or that it is a wrong type.
cyanglaz
left a comment
cyanglaz
left a comment
There was a problem hiding this comment.
I have added more detailed comments on the other PR you originally created.
One thing is that there are a lot usage of id which I don't think they are necessary. When getting values from NSDictionary like map[@"key"] you can totally assume you would get a NSString, e.g. NSString *value = map[@"key"] and then check the type to be sure by if([value isKindOfClass:[NSString class]]); This way you avoid a lot of id usage and you can get useful compiler warnings if a wrong type is used.
I left an example comment in this PR but you can find more detailed review on the other PR you originally created.
| } | ||
| return self; | ||
| } | ||
| - (void)addMarkers:(id)markersToAdd { |
There was a problem hiding this comment.
The type of the parameter can be NSArray if you only want to work with NSArray, use id only when the type of the object is possible to be more than 1.
| - (void)addMarkers:(id)markersToAdd { | |
| - (void)addMarkers:(NSArray *)markersToAdd { |
Same for other similar places in this PR that use id as type.
| BOOL _consumeTapEvents; | ||
| } | ||
| - (instancetype)initWithPosition:(CLLocationCoordinate2D)position mapView:(GMSMapView*)mapView { | ||
| - (instancetype)initWithPositionAndId:(CLLocationCoordinate2D)position |
There was a problem hiding this comment.
Objc usually only have the first parameter name in the method body
| - (instancetype)initWithPositionAndId:(CLLocationCoordinate2D)position | |
| - (instancetype)initWithPosition:(CLLocationCoordinate2D)position andMarkerId:.... |
| if (icon) { | ||
| NSArray* iconData = icon; | ||
| UIImage* image; | ||
| if ([iconData[0] isEqualToString:@"defaultMarker"]) { |
There was a problem hiding this comment.
it would crash if iconData is size 0. If you only want to escape this if block, then maybe use .firstObject of NSArray which returns nil when the array is nil or empty; and makes the if statement return NO(false).
| if ([iconData[0] isEqualToString:@"defaultMarker"]) { | |
| if ([iconData.firstObject isEqualToString:@"defaultMarker"]) { |
| NSArray* iconData = icon; | ||
| UIImage* image; | ||
| if ([iconData[0] isEqualToString:@"defaultMarker"]) { | ||
| CGFloat hue = (iconData.count == 1) ? 0.0f : toDouble(iconData[1]); |
There was a problem hiding this comment.
Are we sure iconData.count > 1?
| } else if ([iconData[0] isEqualToString:@"fromAsset"]) { | ||
| if (iconData.count == 2) { | ||
| image = [UIImage imageNamed:[registrar lookupKeyForAsset:iconData[1]]]; | ||
| } else { |
There was a problem hiding this comment.
Does this else covers only iconData.count > 2?
| return; | ||
| } | ||
| for (id markerId in markerIds) { | ||
| if (!markerId) { |
There was a problem hiding this comment.
Can markerId be nil? The check seems not necessary.
|
|
|
LGTM |
…lutter#1240) * Maps Widget Marker IOS impl * Make JSON Conversions part of a class * fix formatting issues * Remove all usages of id * Fix NSDictionary usage * Fix compile error
|
If the markers are now widget based, does this mean that they can be accessed by the animation features of flutter? (i.e. AnimatedContainer, AnimatedOpacity) |
|
7 participants
No description provided.