Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Make features and annotations conform to NSSecureCoding #6559

Merged
merged 1 commit into from
Dec 22, 2016

Conversation

frederoni
Copy link
Contributor

@frederoni frederoni commented Oct 3, 2016

This PR fixes #6200

All feature and annotation classes now conform to NSSecureCoding which means they can be serialized and deserialized. This is useful for UIStateRestoring among other things.

  • MGLAnnotationImage
  • MGLAnnotationView
  • MGLUserLocation
  • MGLUserLocationAnnotationView
  • MGLShape
  • MGLPointAnnotation
  • MGLMultiPoint
  • MGLPolyline
  • MGLPolygon
  • MGLPointCollection
  • MGLMultiPolyline
  • MGLMultiPolygon
  • MGLShapeCollection
  • MGLPointFeature
  • MGLPolylineFeature
  • MGLPolygonFeature
  • MGLPointCollectionFeature
  • MGLMultiPolylineFeature
  • MGLMultiPolygonFeature
  • MGLShapeCollectionFeature

@incanus @1ec5 👀

@frederoni frederoni added iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS labels Oct 3, 2016
@mention-bot
Copy link

@frederoni, thanks for your PR! By analyzing the history of the files in this pull request, we identified @incanus, @1ec5 and @boundsj to be potential reviewers.

{
return NO;
}
if (other == self)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be the first check for better performance.

[coder encodeDouble:coordinate.longitude forKey:@"coordinateLongitude"];
}

- (BOOL)isEqual:(id)other
Copy link
Contributor

Choose a reason for hiding this comment

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

If you override -isEqual:, you also need to override -hash.

@1ec5
Copy link
Contributor

1ec5 commented Oct 3, 2016

Individual shape subclasses need custom implementations of these methods, because they can't be represented by only one coordinate. The MGLFeature classes also need overrides to check for attribute dictionary equality.

@frederoni frederoni changed the title [ios] WIP Conform to NSSecureCoding [ios] MGLPointAnnotation should conform to NSSecureCoding Oct 4, 2016
@1ec5 1ec5 added bug annotations Annotations on iOS and macOS or markers on Android labels Oct 4, 2016
@boundsj
Copy link
Contributor

boundsj commented Oct 13, 2016

Individual shape subclasses need custom implementations of these methods, because they can't be represented by only one coordinate. The MGLFeature classes also need overrides to check for attribute dictionary equality.

The geoJSONGeometryDictionary method added to MGLShape and geoJSONFeatureDictionary method added to MGLFeature coming in #6524 may help.

@1ec5 1ec5 added this to the ios-3.4.1 milestone Dec 5, 2016
@1ec5 1ec5 changed the title [ios] MGLPointAnnotation should conform to NSSecureCoding Make MGLPointAnnotation conform to NSSecureCoding Dec 5, 2016
@frederoni frederoni changed the base branch from master to release-ios-v3.4.0 December 8, 2016 12:37
@frederoni
Copy link
Contributor Author

frederoni commented Dec 8, 2016

MGLFeature is going to be quite bloated. Might split it up into one file per class.

@frederoni frederoni added the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Dec 8, 2016
@frederoni frederoni changed the title Make MGLPointAnnotation conform to NSSecureCoding Make features and annotations conform to NSSecureCoding Dec 8, 2016
if (self)
{
NSSet<Class> *identifierClasses = [NSSet setWithArray:@[NSString.class, NSNumber.class]];
identifier = [decoder decodeTopLevelObjectOfClasses:identifierClasses forKey:@"identifier" error:nil];
Copy link
Contributor Author

@frederoni frederoni Dec 8, 2016

Choose a reason for hiding this comment

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

Only available on iOS 9

@frederoni frederoni force-pushed the 6200-nssecurecoding branch 3 times, most recently from 2ffda25 to ec1ea25 Compare December 9, 2016 16:07
@@ -5,6 +5,37 @@
#include <mbgl/util/geo.hpp>
#include <mbgl/util/optional.hpp>

bool compareByCoordinate(CLLocationCoordinate2D lhs, CLLocationCoordinate2D rhs)
{
return lhs.latitude+lhs.longitude < rhs.latitude+rhs.longitude;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This means coordinate{0,10} is the same as {10,0} but it's only used for sorting and as long as lhs and rhs is sorted the same way it shouldn't matter.


#define MGLIsEqual(lhs,rhs) ((!lhs && !rhs) || [lhs isEqual:rhs])
#define MGLIsEqualToString(lhs,rhs) ((!lhs && !rhs) || [lhs isEqualToString:rhs])
#define MGLIsEqualToDictionary(lhs,rhs) ((!lhs && !rhs) || [lhs isEqualToDictionary:rhs])
Copy link
Contributor

Choose a reason for hiding this comment

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

This SDK should not get into the business of providing general-purpose Foundation equality macros to developers.

@@ -18,6 +18,12 @@
#define MGLColor NSColor
#endif

#define TARGET_OS_MACOS (TARGET_OS_MAC && !(TARGET_OS_IOS || TARGET_OS_TV || TARGET_OS_WATCH))
Copy link
Contributor

Choose a reason for hiding this comment

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

Let’s stick to a simple !TARGET_OS_IPHONE check at each call site for now. By the time we introduce support for tvOS and/or watchOS, we’ll’ve dropped support for macOS 10.10 and will be able to use the standard TARGET_OS_OSX macro anyways.

{
if (self == other) return YES;

MGLMultiPoint *otherMultipoint = other;
Copy link
Contributor

Choose a reason for hiding this comment

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

What if other is an instance of some other class that implements coordinates as an NSSet?

}

return ([super isEqual:otherMultipoint]
&& _coordinates == otherCoords);
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be written as _coordinates == otherCoords->_coordinates to avoid having to build a copy of the vector in the first place.

MGLMultiPoint *otherMultipoint = other;

CLLocationCoordinate2D *otherCoordinates = [otherMultipoint coordinates];
std::vector<CLLocationCoordinate2D> otherCoords;
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be written as otherCoords = { otherCoordinates, [otherMultiPoint pointCount] } to copy the array with the same efficiency as a memcpy().


NSMutableArray *coordinates = [NSMutableArray array];
for (auto coord : _coordinates) {
[coordinates addObject:@[@(coord.latitude), @(coord.longitude)]];
Copy link
Contributor

Choose a reason for hiding this comment

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

We should never store a coordinate pair as an array if we can help it. Instead, represent it as a dictionary so that the order is never ambiguous.

Copy link
Contributor

Choose a reason for hiding this comment

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

Better yet, stuff the CLLocationCoordinate2D in an NSValue, which already conforms to NSSecureCoding.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@encode CLLocationCoordinate2D would work as long as we don't use it in combination with NSKeyed(Un)Archiver. It only gets the layout of the struct but no information about the name of the fields which is causing the archiver to raise an exception when trying to archive structs. Will make it unambiguous by representing it as a dictionary instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

CLLocationCoordinate2D does work with keyed archivers and unarchivers, as long as you use NSValue to wrap it. Archiving a series of CLLocationCoordinate2Ds would be more efficient than archiving a series of dictionaries, and I don't think there's any ambiguity with that approach because we don't define an MGLSwappedCoordinate2D struct anywhere.

@frederoni frederoni force-pushed the 6200-nssecurecoding branch 4 times, most recently from 7ad498e to 8dfaa0b Compare December 12, 2016 15:31
{
if (lhs.size() != rhs.size()) return false;

std::sort(lhs.begin(), lhs.end(), compareByCoordinate);
Copy link
Contributor

Choose a reason for hiding this comment

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

Order matters. Two coincident lines, one of which goes in the opposite direction as the other, are not equal.

@frederoni frederoni force-pushed the 6200-nssecurecoding branch 5 times, most recently from 8ac1a08 to 9b37141 Compare December 14, 2016 14:08
bounds.extend(mbgl::LatLng(coordinate.latitude, coordinate.longitude));
}
_overlayBounds = MGLCoordinateBoundsFromLatLngBounds(bounds);
[self setupBounds];
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice if we could compute these bounds lazily the first time overlayBounds gets called, as in MGLMultiPoint.


MGLPolygonFeature *otherPointFeature = other;
return ([super isEqual:other]
&& ((!self.geoJSONDictionary && !otherPointFeature.geoJSONDictionary) || [self.geoJSONDictionary isEqualToDictionary:otherPointFeature.geoJSONDictionary]));
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you considered implementing feature and geometry equality as operators on mapbox::geometry classes in geometry.hpp or mbgl? That way other platforms could take advantage of your logic, and we could avoid roundtripping to Objective-C and incurring Objective-C's dynamic dispatch overhead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The equality operator on geometry is already implemented 🙈

return lhs.latitude != rhs.latitude || lhs.longitude != rhs.longitude;
}

bool operator==(std::vector<CLLocationCoordinate2D>& lhs, std::vector<CLLocationCoordinate2D>& rhs)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this operator even necessary? As long as the == operator is defined on CLLocationCoordinate2D, == should just work on a vector of them.

@@ -1,6 +1,11 @@
#import "MGLShape.h"

#import <mbgl/util/geometry.hpp>
#import <mbgl/util/geo.hpp>

bool operator!=(const CLLocationCoordinate2D lhs, const CLLocationCoordinate2D rhs);
Copy link
Contributor

Choose a reason for hiding this comment

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

It's customary to put operators like this entirely in a header and inline them.

- (void)encodeWithCoder:(NSCoder *)coder {
[coder encodeObject:_image forKey:@"image"];
[coder encodeObject:_reuseIdentifier forKey:@"reuseIdentifier"];
[coder encodeObject:_styleIconIdentifier forKey:@"styleIconIdentifier"];
Copy link
Contributor

Choose a reason for hiding this comment

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

This identifier is generated by MGLMapView and mbgl, so I'm not sure that it makes sense to encode it.

@@ -23,4 +23,29 @@ @implementation NSArray (MGLAdditions)
return vector;
}

+ (NSArray *)mgl_coordinatesFromCoordinates:(std::vector<CLLocationCoordinate2D>)coords {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's categorize NSCoder to have -encodeMGLLocationCoordinate2D:forKey: and -decodeMGLLocationCoordinate2DForKey:, as has been done for other structs like CGSize and UIEdgeInsets.

- (void)encodeWithCoder:(NSCoder *)coder {
[super encodeWithCoder:coder];
[coder encodeObject:identifier forKey:@"identifier"];
[coder encodeObject:attributes forKey:@"attributes"];
Copy link
Contributor

Choose a reason for hiding this comment

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

We can reduce the repetitiveness of implementing -initWithCoder:, -encodeWithCoder:, and -isEqual: across all the MGLFeature-conforming classes using a macro.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The macro will be obsolete after #7454 but let's use it in the meantime.

{
NSUInteger hash = [super hash];
for (auto coord : _coordinates) {
hash += coord.latitude+coord.longitude;
Copy link
Contributor

Choose a reason for hiding this comment

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

Normally we box doubles in NSNumber and take its hash. The assumption is that NSNumber’s hash is more collision-proof than the raw number itself. Unfortunately, that could be less performant than just adding the coordinate. I wonder if we could take the hash of an NSValue of the coordinate instead of the NSNumbers of the latitude and longitude separately, to halve the cost.


@interface NSCoder (MGLAdditions)

- (void)mgl_encodeLocationCoordinates2D:(std::vector<CLLocationCoordinate2D>)coordinates forKey:(NSString *)key;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be useful to publicly expose a method that encodes a single CLLocationCoordinate2D. It would be called -encodeMGLCoordinate:, for consistency with existing category methods on NSCoder. (See also -[NSValue valueWithMGLCoordinate:].)

@implementation NSCoder (MGLAdditions)

- (void)mgl_encodeLocationCoordinates2D:(std::vector<CLLocationCoordinate2D>)coordinates forKey:(NSString *)key {
[self encodeObject:[NSArray mgl_coordinatesFromCoordinates:coordinates] forKey:key];
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that we have dedicated methods for working with CLLocationCoordinate2Ds, let’s encode the result of -[NSValue(MGLAdditions) valueWithMGLCoordinate:] instead of a dictionary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I use NSKeyedArchiver for testing NSCoding which can't encode structs, not even structs wrapped in NSValue encoded with @encode(CLLocationCoordinate2D). ([NSKeyedArchiver encodeValueOfObjCType:at:]: this archiver cannot encode structs) Any other idea how to solve this? UIStateRestoring will probably still work if we switch to NSValue but a solution that works with archiving as well would be preferred.

Copy link
Contributor

@1ec5 1ec5 Dec 16, 2016

Choose a reason for hiding this comment

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

I stand corrected. Apparently the root cause is that CLLocationCoordinate2D is a typedef to an anonymous struct, so @encode can't do anything useful.

We could hard-code the type string {dd}, as many Swift developers have had to do, or we could define our own non-anonymous struct based on CLLocationCoordinate2D solely for the purpose of archiving, or we could eat the performance cost and use dictionaries here. I haven't profiled this code, so I don't know at what point performance becomes a problem or whether we're OK with performance degrading after a certain point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A non-anonymous struct sounded like the best option but still no luck trying to archive it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, looks like you’re right. Unfortunately we’ll have to stick to an array of dictionaries. If performance does turn out to be a major issue, we can flatten the array, at the cost of triggering another debate about latitude-longitude order versus longitude-latitude order (or is it longitude-latitude order versus latitude-longitude order?).

@@ -37,3 +37,31 @@ mbgl::Feature mbglFeature(mbgl::Feature feature, id identifier, NSDictionary *at
NS_DICTIONARY_OF(NSString *, id) *NSDictionaryFeatureForGeometry(NSDictionary *geometry, NSDictionary *attributes, id identifier);

NS_ASSUME_NONNULL_END

#define MGLFeatureInitCoding() \
Copy link
Contributor

Choose a reason for hiding this comment

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

Macros should be named in SCREAMING_SNAKE_CASE. 📣🐍 In this case, since this macro is defining a method, it should be named with a verb, like MGL_DEFINE_FEATURE_INIT_WITH_CODER().


#define MGLFeatureInitCoding() \
- (instancetype)initWithCoder:(NSCoder *)decoder { \
if (self = [super initWithCoder:decoder]) { \
Copy link
Contributor

Choose a reason for hiding this comment

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

Indent the macro to aid with readability. This line should start with two tabs.

- (BOOL)isEqual:(id)other { \
if (other == self) return YES; \
if (![other isKindOfClass:[self class]]) return NO; \
klass *otherFeature = other; \
Copy link
Contributor

Choose a reason for hiding this comment

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

typeof(self) also works in place of klass *, since we’ve already asserted that other is of the same kind as self.


@interface NSCoder (MGLAdditions)

- (void)encodeMGLCoordinate:(CLLocationCoordinate2D)coordinate forKey:(NSString *)key;
Copy link
Contributor

Choose a reason for hiding this comment

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

Publicly vending these methods would’ve made more sense if we could come up with a way to encode and decode the coordinate as a struct. But with the dictionary-based implementation is dependent on the keys we choose, so unfortunately it isn’t portable enough to expose publicly. MGLCode+MGLAdditions_Private.h can be folded into this header, which can be marked with Project visibility. Sorry for the detour.

Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

Looks good. Just one nit regarding indentation.

@@ -37,3 +37,31 @@ mbgl::Feature mbglFeature(mbgl::Feature feature, id identifier, NSDictionary *at
NS_DICTIONARY_OF(NSString *, id) *NSDictionaryFeatureForGeometry(NSDictionary *geometry, NSDictionary *attributes, id identifier);

NS_ASSUME_NONNULL_END

#define MGL_DEFINE_FEATURE_INIT_WITH_CODER() \
- (instancetype)initWithCoder:(NSCoder *)decoder { \
Copy link
Contributor

Choose a reason for hiding this comment

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

Also indent this line and the closing brace.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
annotations Annotations on iOS and macOS or markers on Android bug iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants