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

[Annotation plugin] Make getting annotation manager configurable. #47

Merged
merged 3 commits into from
Feb 1, 2021
Merged

[Annotation plugin] Make getting annotation manager configurable. #47

merged 3 commits into from
Feb 1, 2021

Conversation

Chaoba
Copy link
Contributor

@Chaoba Chaoba commented Jan 28, 2021 •
edited
Loading

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

Pull request checklist:

  • Briefly describe the changes in this PR.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality. If tests were not written, please explain why.
  • Add example if relevant.
  • Document any changes to public APIs.
  • Apply changelog label ('breaking change', 'bug 🪲', 'build', 'docs', 'feature 🍏', 'performance ⚡', 'testing 💯') or use the label 'skip changelog'
  • Add an entry inside this element for inclusion in the mapbox-maps-android changelog: <changelog>Make getting annotation manager configurable</changelog>.

Summary of changes

In this pr:

  • Add a configure class for annotation plugin, users can custom layer id and source id, etc. while getting the annotation manager instance.
  • Fix the issue that annotation will be erased while changing style
  • Update test demo to very changes
    QQ20210125-205122

User impact (optional)

@Chaoba Chaoba added bug 🪲 Something isn't working feature 🍏 labels Jan 28, 2021
@Chaoba Chaoba self-assigned this Jan 28, 2021
@CLAassistant
Copy link

CLAassistant commented Jan 28, 2021 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@Chaoba Chaoba requested a review from a team January 28, 2021 04:02
kiryldz
kiryldz reviewed Jan 28, 2021
View reviewed changes
plugin-annotation/src/main/java/com/mapbox/maps/plugin/annotation/AnnotationManagerImpl.kt
style.addLayer(layer)
} else {
style.addLayerBelow(layer, belowLayerId)
if (layer == null || source == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to confirm - we recreate source even it was not null but layer == null. Is it expected behavior?

Copy link
Contributor Author

@Chaoba Chaoba Jan 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

layer and source are created at the same time, here check them both just a double confirm.

kiryldz
kiryldz reviewed Jan 28, 2021
View reviewed changes
plugin-annotation/src/main/java/com/mapbox/maps/plugin/annotation/AnnotationManagerImpl.kt
literal(true)
)
) { features ->
features.value?.let { featureList ->
Copy link
Contributor

Choose a reason for hiding this comment

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

If feature.value == null - we will not call callback.onQueryAnnotation(null). Is it expected behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, everywhere get the result will check null. Have thought about it, it seems we don't need the callback interface to return a nullable result.

kiryldz
kiryldz reviewed Jan 28, 2021
View reviewed changes
sdk-base/src/main/java/com/mapbox/maps/plugin/annotation/AnnotationConfig.kt
*/
data class AnnotationConfig(
/** The id of the layer above the annotation layer*/
val belowLayerId: String? = null,
Copy link
Contributor

Choose a reason for hiding this comment

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

The doc states that id of the layer above the annotation layer while val is named belowLayerId 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We will add the annotation layer below the given layer, which is the same as the definition in Layer

@Chaoba Chaoba requested a review from kiryldz January 28, 2021 10:38
@Chaoba Chaoba requested a review from pengdev January 28, 2021 13:22
@Chaoba Chaoba force-pushed the kl-anntation-config branch from 0f9c824 to 63b3cca Compare January 28, 2021 14:37
kiryldz
kiryldz approved these changes Jan 29, 2021
View reviewed changes
Copy link
Contributor

@kiryldz kiryldz left a comment

Choose a reason for hiding this comment

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

@Chaoba - great job, LGTM!
Also please add changelog entry to PR description.

@Chaoba Chaoba merged commit e008d0a into main Feb 1, 2021
@Chaoba Chaoba deleted the kl-anntation-config branch February 1, 2021 02:55
@ManolescuSebastian ManolescuSebastian mentioned this pull request Mar 22, 2021
Closed
@scenemax3d scenemax3d mentioned this pull request Jan 28, 2023
Closed
@maratsct maratsct mentioned this pull request Jun 17, 2024
Closed
@xingzhongyi xingzhongyi mentioned this pull request Dec 6, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kiryldz kiryldz kiryldz approved these changes

@pengdev pengdev Awaiting requested review from pengdev

Assignees

@Chaoba Chaoba

Labels
bug 🪲 Something isn't working feature 🍏
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Chaoba @CLAassistant @pengdev @kiryldz