-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Allow annotation images to be updated #3146
Conversation
Thanks for this @RomainQuidet. May take a bit, but we will try to review this soon. It looks pretty clean. /cc @kkaefer @jfirebaugh @1ec5 |
9d815ee
to
5dfab8a
Compare
@@ -13,12 +13,12 @@ NS_ASSUME_NONNULL_BEGIN | |||
* @param image The image to be displayed for the annotation. | |||
* @param reuseIdentifier The string that identifies that this annotation image is reusable. | |||
* @return The initialized annotation image object or `nil` if there was a problem initializing the object. */ | |||
+ (instancetype)annotationImageWithImage:(UIImage *)image reuseIdentifier:(NSString *)reuseIdentifier; | |||
+ (instancetype)annotationImageWithImage:(UIImage *)image reuseIdentifier:(nullable NSString *)reuseIdentifier; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MGLMapView currently assumes that the reuse identifier is non-null. With a nil identifier, this line would crash. Is there a need for it to be nullable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
That was a reminder to make it nullable as in the official MapKit. This identifier is used to recycle the instance for performance reason, it should not be used internally like today to reference the image view in a dictionary or even deeper with the sprites
@1ec5 : do you need me to update the pull request or will you merge it ? |
Since we use fast-forward merging, it'd be cleaner for you to rebase and resolve the conflicts. I'd be happy to do it but I'd have to open a new PR and close this one. |
5dfab8a
to
fbb5ece
Compare
@1ec5 : branch rebased and corrections done according to your code review |
…euseIdentifier of annotationImage
Merged in 9d0ab55...82a5575. The test failure above is a random KIF failure, and Bitrise won’t let me rebuild the fork branch. Thanks @RomainQuidet! |
Another pass at #3146, including a unit test.
When I update the
...I see the below block of code in mapbox-gl framework run, but the annotation image does not update:
Is this still an issue? |
@alexagat, can you open a new issue about this? It sounds like a regression if that’s the case. |
Add possibility to delete / update a sprite in order to update an AnnotationImage
#2210
Fixes #2517