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.

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

Support annotation view lookup by id. #1512

Merged
merged 4 commits into from
Aug 9, 2022

Conversation

maios
Copy link
Contributor

@maios maios commented Aug 4, 2022

  • Allow adding annotation view with a custom id, that can be used later to look it up.

Pull request checklist:

  • Describe the changes in this PR, especially public API changes.
  • 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 a changelog entry to to bottom of the relevant section (typically the ## main heading near the top).

PRs must be submitted under the terms of our Contributor License Agreement CLA.

@maios maios requested a review from ank27 August 4, 2022 11:52
@maios maios requested a review from a team as a code owner August 4, 2022 11:52
Copy link
Contributor

@evil159 evil159 left a comment

Choose a reason for hiding this comment

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

I wonder if we should add a convenience method to update(remove to a lesser extent as well) a view annotation with id now when we allow passing custom id when adding a view annotation.
This change is requested in the context of making it easier to change a view annotation, so this addition, while being very cheap to us, might make this use case a bit more convenient.
E.g.:

// it is possible to get a view and update options for it currently
if let view = view(forId: id) {
    manager.update(view, options)
}

// with a convenience method this could be a single invocation
manager.update(forId: id, options)

// the convenience method
public func update(forId id: String, options: ViewAnnotationOptions) throws {
    guard let view = view(forId: id) else {
          throw ViewAnnotationManagerError.annotationNotFound
    }

    update(view, options: options)
}

@maios
Copy link
Contributor Author

maios commented Aug 8, 2022

I wonder if we should add a convenience method to update(remove to a lesser extent as well) a view annotation with id now when we allow passing custom id when adding a view annotation. This change is requested in the context of making it easier to change a view annotation, so this addition, while being very cheap to us, might make this use case a bit more convenient. E.g.:

// it is possible to get a view and update options for it currently
if let view = view(forId: id) {
    manager.update(view, options)
}

// with a convenience method this could be a single invocation
manager.update(forId: id, options)

// the convenience method
public func update(forId id: String, options: ViewAnnotationOptions) throws {
    guard let view = view(forId: id) else {
          throw ViewAnnotationManagerError.annotationNotFound
    }

    update(view, options: options)
}

I wanted to do the same, but I want to clarify some design intention behind the current ViewAnnotationsManager/update(_:options:) to accept new options with same associatedFeatureId (currently we don't accept it, so new associatedFeatureId has to be either nil or different value), after this we might want to change the current implementation so I want to cut it to another ticket.

@maios maios force-pushed the feature/MAPSIOS-245_ids-lookup-view-annotation-manager branch from a1ec8a8 to a0c39a6 Compare August 9, 2022 11:16
@maios maios merged commit d0b4ab6 into main Aug 9, 2022
@maios maios deleted the feature/MAPSIOS-245_ids-lookup-view-annotation-manager branch August 9, 2022 12:26
Copy link

@MICHELLGRONER88 MICHELLGRONER88 left a comment

Choose a reason for hiding this comment

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

Copy link

@MICHELLGRONER88 MICHELLGRONER88 left a comment

Choose a reason for hiding this comment

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

mapbox-github-ci-writer-public-1 bot pushed a commit that referenced this pull request May 3, 2023
* Update version numbering (#1499)

* Remove binary dependency on MME (#1510)

* Remove binary dependency on MME

* Add changelog

* Mai/prepare v10.13 (#1511)

* Bump CoreMaps to v10.13.0 and Common to v23.5.0

* Update CHANGELOG.md

* Remove carplay entry from changelog

---------

Co-authored-by: Patrick Leonard <pjleonard37@users.noreply.github.com>
Co-authored-by: Roman Gardukevich <roman.gardukevich@mapbox.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants