Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix view annotation losing its feature association after update #1446

Merged
merged 3 commits into from
Jul 13, 2022
Merged

Fix view annotation losing its feature association after update #1446

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
82d2fee
Fix view annotation losing its feature association after update
evil159 Jul 12, 2022
854b68c
Add unit test
evil159 Jul 13, 2022
a29ca8b
Update changelog
evil159 Jul 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ Mapbox welcomes participation and contributions from everyone.
* Introduce `GestureOptions.simultaneousRotateAndPinchZoomEnabled` and deprecate `GestureOptions.pinchRotateEnabled` in favor of `GestureOptions.rotateEnabled`. ([1429](https://github.com/mapbox/mapbox-maps-ios/pull/1429))
* Add experimental `Puck3DConfiguration.modelCastShadows` option to control shadow casting for the 3D puck. ([#1435](https://github.com/mapbox/mapbox-maps-ios/pull/1435))
* Expose public initializer for `TilesetDescriptorOptionsForTilesets`. ([1431](https://github.com/mapbox/mapbox-maps-ios/pull/1431))
* Fix view annotation losing its feature association after update. ([1446](https://github.com/mapbox/mapbox-maps-ios/pull/1446))

## 10.7.0-beta.1 - June 29, 2022

Expand Down
2 changes: 1 addition & 1 deletion Sources/MapboxMaps/Annotations/ViewAnnotationManager.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ public final class ViewAnnotationManager {
let isHidden = !(options.visible ?? true)
expectedHiddenByView[view] = isHidden
viewsById[id]?.isHidden = isHidden
if let id = currentFeatureId, id != options.associatedFeatureId {
if let id = currentFeatureId, let updatedId = options.associatedFeatureId, id != updatedId {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit:

Suggested change
if let id = currentFeatureId, let updatedId = options.associatedFeatureId, id != updatedId {
if !(currentFeatureId == options.associatedFeatureId) {

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 also probably make sense to add an explicit comment above

viewsByFeatureIds[id] = nil
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not quite understand this change. It looks like the same lines to me. Can you explain to me, please?

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, now I see – options.associatedFeatureId has to be non-nil, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By unwrapping the options.associatedFeatureId this makes sure that a view loses its feature id association only when the feature id got updated, previously it would loose the association on every update.

Some sample code to illustrate this:

let view = UIView()
let id = UUID().uuidString
let options = ViewAnnotationOptions(associatedFeatureId: id)

try manager.add(view, options: options)
try manager.update(view, options: ViewAnnotationOptions(width: 100, height: 100))

// previosly
manager.view(forFeatureId: id) // nil 💥

// fixed
manager.view(forFeatureId: id) // returns the view ✅

}
if let featureId = options.associatedFeatureId {
Expand Down
19 changes: 19 additions & 0 deletions Tests/MapboxMapsTests/Annotations/ViewAnnotationManagerTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,25 @@ final class ViewAnnotationManagerTests: XCTestCase {
XCTAssertEqual(optionsB, manager.options(forFeatureId: testIdB))
}

func testAssociatedFeatureIdUpdateDoesNotDissociate() throws {
let testIdA = "testIdA"
let testView = UIView()
let optionsA = ViewAnnotationOptions(geometry: Point(.random()),
width: 0,
height: 0,
associatedFeatureId: testIdA)
let updateOptions = ViewAnnotationOptions(geometry: optionsA.geometry,
width: 100,
height: 100,
associatedFeatureId: nil)
try manager.add(testView, options: optionsA)
mapboxMap.optionsForViewAnnotationWithIdStub.defaultReturnValue = optionsA

try manager.update(testView, options: updateOptions)

XCTAssertEqual(testView, manager.view(forFeatureId: testIdA))
}

func testUpdate() {
let annotationView = addTestAnnotationView()
XCTAssertEqual(mapboxMap.updateViewAnnotationStub.invocations.count, 0)
Expand Down