New methods for view annotations API #1136

evil159 · 2022-02-17T14:43:56Z

Summary

This PR introduces ViewAnnotationUpdateObserver protocol for notifying when annotion views get their frames or visibility changed. As well as corresponding methods to add/remove view annotation update observers to ViewAnnotationManager.

In addition removeAll() method is added to ViewAnnotationManager that removes all annotations added before.

Fixes: https://github.com/mapbox/mapbox-maps-internal/issues/1280

Pull request checklist:

Write tests for all new functionality. If tests were not written, please explain why.
Add documentation comments for any added or updated public APIs.
Add any new public, top-level symbols to the Jazzy config's custom_categories (scripts/doc-generation/.jazzy.yaml)
Describe the changes in this PR, especially public API changes.
Add a changelog entry to to bottom of the relevant section (typically the ## main heading near the top).
Review and agree to the Contributor License Agreement (CLA).

kiryldz

Did review protocol only 😉
API differs slightly from Android but in terms of usability and platform details we should be aligned.

OdNairy · 2022-02-22T13:05:28Z

Sources/MapboxMaps/Annotations/ViewAnnotationManager.swift

@@ -1,6 +1,7 @@
 import UIKit
 @_implementationOnly import MapboxCommon_Private
 @_implementationOnly import MapboxCoreMaps_Private
+import SwiftUI


Why do we need SwiftUI here?

We don't. Probably an accidental auto-import.

True this, gotta be more careful with Xcode auto-importing things.

macdrevx · 2022-02-22T14:37:56Z

Sources/MapboxMaps/Annotations/ViewAnnotationManager.swift

 /// Manager API to control View Annotations.
 ///
-/// View annotations are `UIView` instances that are drawn on top of the `MapView` and bound to some `Geometry` (only `Point` is supported for now).
+/// View annotations are `UIView` instances that are drawn on top of the ``MapView`` and bound to some ``Geometry`` (only ``Point`` is supported for now).


Will these links for Geometry and Point do the right thing? Turf docs aren't well integrated at the moment.

Not as such it seems. I did check that it generates links to those types, but there's nothing in the documentation pages for them. Doesn't look like a right thing, indeed. I'll revert these changes back.

Sources/MapboxMaps/Annotations/ViewAnnotationManager.swift

OdNairy · 2022-02-22T14:41:41Z

Sources/MapboxMaps/Annotations/ViewAnnotationManager.swift

@@ -123,19 +153,36 @@ public final class ViewAnnotationManager {
        }
    }

-    /// Update given `UIView` with `ViewAnnotationOptions`.
+    public func removeAll() {
+        let viewsByIdCopy = viewsById


Do we actually need a copy here?

I don't think so. Swift value types should avoid the troubles with mutation during iteration we had in Objective-C

Old habits...

Sources/MapboxMaps/Annotations/ViewAnnotationManager.swift

macdrevx · 2022-02-22T14:45:23Z

Sources/MapboxMaps/Annotations/ViewAnnotationManager.swift

            view.isHidden = false
            expectedHiddenByView[view] = false
            visibleAnnotationIds.insert(position.identifier)
        }

+        assert(!viewsWithUpdatedFrame.contains(where: {$0.isHidden == true }))
+        notifyViewAnnotationObserversFrameDidChange(for: Array(viewsWithUpdatedFrame))


Would it make sense to wait until the end of the method just before notifyViewAnnotationObserversVisibilityDidChange to make this call?

Just thinking it might avoid letting the developer see the views in an inconsistent state.

It also may make sense to put this block to the defer

I think the idea with defer will work well. It will make more sense to first notify about visibility changes and only after that is established to notify about frame changes.

Sources/MapboxMaps/Annotations/ViewAnnotationManager.swift

Tests/MapboxMapsTests/Annotations/ViewAnnotationManagerTests.swift

macdrevx

Did you preview the changes to the generated docs?

Co-authored-by: Andrew Hershberger <andrew.hershberger@mapbox.com>

…wift Co-authored-by: Andrew Hershberger <andrew.hershberger@mapbox.com>

evil159 · 2022-02-23T02:13:17Z

Did you preview the changes to the generated docs?

Yeah, looks good except for symbol references to Turf docs(fixed it).

OdNairy

LGTM, few nits.

Sources/MapboxMaps/Annotations/ViewAnnotationManager.swift

OdNairy · 2022-02-23T15:03:11Z

Sources/MapboxMaps/Annotations/ViewAnnotationManager.swift

            view.isHidden = true
            expectedHiddenByView[view] = true
        }
+
+        notifyViewAnnotationObserversVisibilityDidChange(for: Array(viewsWithUpdatedVisibility))


Can we make it defer as well for consistency?

Co-authored-by: Roman Gardukevich <roman.gardukevich@mapbox.com>

Add automation workflow to move new issues to backlog

evil159 force-pushed the new-viewpoint-annotations-api branch 2 times, most recently from d487975 to cfb4322 Compare February 18, 2022 13:05

evil159 added 3 commits February 22, 2022 10:17

Add a method to remove all view annotations

6401ddf

ViewAnnotationUpdateObserver implementation

4231b62

Unit tests for ViewAnnotationUpdateObserver

14a1e69

evil159 force-pushed the new-viewpoint-annotations-api branch from cfb4322 to 14a1e69 Compare February 22, 2022 09:18

evil159 added 2 commits February 22, 2022 11:46

lint

c50e33b

Add change log entry

9593d32

evil159 force-pushed the new-viewpoint-annotations-api branch from ca85176 to 9593d32 Compare February 22, 2022 09:46

Add ViewAnnotationManagerError to jazzy config

2ebbb4d

evil159 requested review from macdrevx, ZiZasaurus and OdNairy February 22, 2022 09:56

evil159 marked this pull request as ready for review February 22, 2022 09:57

kiryldz approved these changes Feb 22, 2022

View reviewed changes

OdNairy reviewed Feb 22, 2022

View reviewed changes

macdrevx reviewed Feb 22, 2022

View reviewed changes

Sources/MapboxMaps/Annotations/ViewAnnotationManager.swift Outdated Show resolved Hide resolved

OdNairy reviewed Feb 22, 2022

View reviewed changes

macdrevx reviewed Feb 22, 2022

View reviewed changes

Sources/MapboxMaps/Annotations/ViewAnnotationManager.swift Outdated Show resolved Hide resolved

macdrevx reviewed Feb 22, 2022

View reviewed changes

Sources/MapboxMaps/Annotations/ViewAnnotationManager.swift Outdated Show resolved Hide resolved

macdrevx reviewed Feb 22, 2022

View reviewed changes

Sources/MapboxMaps/Annotations/ViewAnnotationManager.swift Outdated Show resolved Hide resolved

macdrevx reviewed Feb 22, 2022

View reviewed changes

Sources/MapboxMaps/Annotations/ViewAnnotationManager.swift Outdated Show resolved Hide resolved

macdrevx reviewed Feb 22, 2022

View reviewed changes

Sources/MapboxMaps/Annotations/ViewAnnotationManager.swift Outdated Show resolved Hide resolved

macdrevx reviewed Feb 22, 2022

View reviewed changes

Tests/MapboxMapsTests/Annotations/ViewAnnotationManagerTests.swift Outdated Show resolved Hide resolved

macdrevx suggested changes Feb 22, 2022

View reviewed changes

evil159 and others added 3 commits February 23, 2022 03:26

Update Sources/MapboxMaps/Annotations/ViewAnnotationManager.swift

bdaecc0

Co-authored-by: Andrew Hershberger <andrew.hershberger@mapbox.com>

Update Sources/MapboxMaps/Annotations/ViewAnnotationManager.swift

ba197f0

Co-authored-by: Andrew Hershberger <andrew.hershberger@mapbox.com>

Update Sources/MapboxMaps/Annotations/ViewAnnotationManager.swift

407a8d1

Co-authored-by: Andrew Hershberger <andrew.hershberger@mapbox.com>

evil159 and others added 4 commits February 23, 2022 03:53

Update Sources/MapboxMaps/Annotations/ViewAnnotationManager.swift

3d04a0e

Co-authored-by: Andrew Hershberger <andrew.hershberger@mapbox.com>

Update Sources/MapboxMaps/Annotations/ViewAnnotationManager.swift

7f01957

Co-authored-by: Andrew Hershberger <andrew.hershberger@mapbox.com>

Update Sources/MapboxMaps/Annotations/ViewAnnotationManager.swift

dea274a

Co-authored-by: Andrew Hershberger <andrew.hershberger@mapbox.com>

Update Tests/MapboxMapsTests/Annotations/ViewAnnotationManagerTests.s…

cfa76f0

…wift Co-authored-by: Andrew Hershberger <andrew.hershberger@mapbox.com>

evil159 force-pushed the new-viewpoint-annotations-api branch from 305a6d6 to c15031b Compare February 23, 2022 02:06

Address PR feedback

35e76d7

evil159 force-pushed the new-viewpoint-annotations-api branch from c15031b to 35e76d7 Compare February 23, 2022 02:10

Merge branch 'main' into new-viewpoint-annotations-api

83234ad

evil159 requested review from macdrevx, kiryldz and OdNairy February 23, 2022 02:14

kiryldz approved these changes Feb 23, 2022

View reviewed changes

macdrevx approved these changes Feb 23, 2022

View reviewed changes

OdNairy approved these changes Feb 23, 2022

View reviewed changes

Update Sources/MapboxMaps/Annotations/ViewAnnotationManager.swift

561fa9a

Co-authored-by: Roman Gardukevich <roman.gardukevich@mapbox.com>

evil159 enabled auto-merge (squash) February 23, 2022 15:16

evil159 force-pushed the new-viewpoint-annotations-api branch from 0540c6f to 561fa9a Compare February 23, 2022 15:32

OdNairy disabled auto-merge February 23, 2022 15:46

evil159 merged commit ca2849c into main Feb 23, 2022

evil159 deleted the new-viewpoint-annotations-api branch February 23, 2022 15:49

OdNairy pushed a commit that referenced this pull request Aug 22, 2023

Merge pull request #1136 from mapbox/zjm-IssuesWorkflowTesting10

8545dac

Add automation workflow to move new issues to backlog

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New methods for view annotations API #1136

New methods for view annotations API #1136

evil159 commented Feb 17, 2022 •

edited

Loading

kiryldz left a comment

OdNairy Feb 22, 2022

macdrevx Feb 22, 2022

evil159 Feb 23, 2022

macdrevx Feb 22, 2022

evil159 Feb 23, 2022

OdNairy Feb 22, 2022

macdrevx Feb 22, 2022

evil159 Feb 23, 2022

macdrevx Feb 22, 2022

macdrevx Feb 22, 2022

OdNairy Feb 22, 2022

evil159 Feb 23, 2022

macdrevx left a comment

evil159 commented Feb 23, 2022

OdNairy left a comment

OdNairy Feb 23, 2022

evil159 Feb 23, 2022

New methods for view annotations API #1136

New methods for view annotations API #1136

Conversation

evil159 commented Feb 17, 2022 • edited Loading

Summary

Pull request checklist:

kiryldz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macdrevx left a comment

Choose a reason for hiding this comment

evil159 commented Feb 23, 2022

OdNairy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evil159 commented Feb 17, 2022 •

edited

Loading