Upgrade Tangram-ES to 0.7 #342
Conversation
- Refactor API and re-enable previous error checks where applicable - Deprecate functions not needed - Remove unnecessary files
Future commits will fix them.
This fixes unit test runtime exceptions by adding a test mapVC into the tests which allows the markers to sync up correctly with ES 0.7 changes. Also an oversight was found where we weren't clearing our own internal marker storage upon marker removal, and so that's now fixed.
| if marker.tgMarker == nil { | ||
| marker.tgMarker = tgViewController.markerAdd() | ||
| } | ||
| currentMarkers[marker.tgMarker!] = marker |
There was a problem hiding this comment.
Could this be useful or is this aiming for something different? https://github.com/tangrams/tangram-es/blob/master/platforms/ios/src/TangramMap/TGMapViewController.h#L319
There was a problem hiding this comment.
Looking more, it seems that a nice alternative would be to store the SDK marker as a user data of the Tangram marker, that the association can be easily retrieved without additional container.
https://github.com/tangrams/tangram-es/blob/master/platforms/ios/src/TangramMap/TGMarker.h#L103
I think it reminds me of some conversation we had on a previous PR relating to that.
There was a problem hiding this comment.
It's a slightly different use case. With this change we're effectively decoupling SDK markers from Tangram-es markers, meaning we no longer require a Tangram-es marker be created at initialization time of the SDK marker class. We're also allowing users to set their own underlying TGMarkers for their SDK markers if they want (it honestly makes it easier on us to implement as well).
The side effect of all that is we need to check we don't override previous markers when adding the SDK markers to the map.
There was a problem hiding this comment.
Ahhh slow github is slow - yeah the user data object is a good idea. Lemme think on how that might change the interaction model at the Mapzen layer.
There was a problem hiding this comment.
Ok so @sarahlensing and I discussed it further and I think we wanna go down the route of using the user data object on TGMarker. However, a void* there imported into Swift is difficult to use. Do you think it would be much effort to convert it to a nullable NSObject ?
There was a problem hiding this comment.
NSObject seems to narrow down the flexibility quite a lot, we should probably useid there?
There was a problem hiding this comment.
id could work. That at least converts to AnyObject in Swift which I can cast through easier.
There was a problem hiding this comment.
The other places I see in Foundation that have this type of construct (NSNotification for example) flat out use NSDictionary which is even more constraining.
There was a problem hiding this comment.
I actually like the NSDictionary idea, it would let you store different kind of data without having to create your own data structure holding it. Feel free to open an issue in ES with the different options we have.
MapzenSDK/Marker.swift
Outdated
| guard let marker = tgMarker else { return false } | ||
| marker.pointEased(coordinates, seconds: seconds, easeType: ease) | ||
| do { | ||
| try marker.getError() |
There was a problem hiding this comment.
The error is only checked on setPointEased, I assume that the inconvenience of Swift regarding properties that throw is a potential blocker, but the implementation might leave error incoming from previous usage of the marker, which would be misleading when using setPointEased. What seems correct is to check on every modification of the marker or check nowhere at all (while potentially wrapping getError to allow its usage externally).
There was a problem hiding this comment.
Hmmm yeah you're right. Probably should just pull this then and go back through a re-add it when we tackle #248
| @@ -234,12 +245,20 @@ public class PointMarker : GeometricMarker, GenericPointIconMarker { | |||
| - parameter ease: Easing to use for animation. | |||
| */ | |||
| public func setPointEased(_ coordinates: TGGeoPoint, seconds: Float, easeType ease: TGEaseType) -> Bool { | |||
| return tgMarker.setPointEased(coordinates, seconds: seconds, easeType: ease) | |||
| tgMarker?.pointEased(coordinates, seconds: seconds, easeType: ease) | |||
There was a problem hiding this comment.
Do we need to call I just saw @karimnaaji's comment, looks like you already plan to handle thisgetError before pointEased to avoid returning an error for a previous action taken on tgMarker? I'm wondering if we have an existing error on the tgMarker and then call pointEased which completes without an error, will we erroneously return false here?
| let _ = self.showCurrentLocation(true) | ||
| self.showFindMeButon(true) | ||
| try? loadStyleAsync(appDelegate.selectedMapStyle) { [weak self] (style) in | ||
| guard let unwrappedSelf = self else { print("Self is nil"); return } |
There was a problem hiding this comment.
nit: Might be useful to have a more descriptive message here, and maybe a description of how this can happen.
There was a problem hiding this comment.
Hah that is debug code - had a separate issue I was trying to figure out and forgot to remove that. Will do now.
Overview
This PR upgrades Tangram-es to the latest production stable version. It includes some major refactoring, some method deprecations, and some bug fixes and performance improvements.
Note that this includes a minor breaking change where we have to make the tgMarker property across the marker protocols optional now. This shouldn't affect most people but it should be called out in release notes.
Proposed Changes
Fixes #327