Skip to content

fix getLayerAs return optional Layer type.#673

Merged
ank27 merged 2 commits intomainfrom
ak-get-layer-as-fix
Oct 5, 2021
Merged

fix getLayerAs return optional Layer type.#673
ank27 merged 2 commits intomainfrom
ak-get-layer-as-fix

Conversation

@ank27
Copy link
Copy Markdown
Contributor

@ank27 ank27 commented Sep 24, 2021

PRs must be submitted under the terms of our Contributor License Agreement CLA.
Fixes: < Link to related issues that will be fixed by this pull request, if they exist >

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>Update getLayerAs function to return nullable Layer type.</changelog>.

Summary of changes

Currently StyleMangerInterface.getLayerAs has return type which is non-nullable.
It should be a nullable type.

User impact (optional)

Breaking change to get layer from style using getLayerAs function.

fun <T : Layer> StyleManagerInterface.getLayerAs(layerId: String): T {
  @Suppress("UNCHECKED_CAST")
  return getLayer(layerId) as T
}

Changed to

inline fun <reified T : Layer> StyleManagerInterface.getLayerAs(layerId: String): T? {
  val layer = getLayer(layerId)
  if (layer !is T) {
    Logger.e("StyleLayerPlugin", "Given layerId = $layerId is not requested type in Layer")
    return null
  }
  return layer
}

@ank27 ank27 self-assigned this Sep 24, 2021
Comment thread extension-style/src/main/java/com/mapbox/maps/extension/style/layers/LayerExt.kt Outdated
@ank27 ank27 requested a review from a team September 24, 2021 11:50
@ank27 ank27 force-pushed the ak-get-layer-as-fix branch from 68760e9 to d8a306e Compare September 24, 2021 11:59
inline fun <reified T : Layer> StyleManagerInterface.getLayerAs(layerId: String): T? {
val layer = getLayer(layerId)
if (layer !is T) {
Logger.e("StyleLayerPlugin", "Given layerId = $layerId is not requested type in Layer")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: introduce TAG in companion object and use that as first parameter of Logger.e (I do think we had a convention of prefixing with mbgl_ but better to check the codebase).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Given layerId = $layerId is not requested type in Layer the error statement is a bit confusing.
The layer itself could also just be null

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Any suggestion for error message ? :D

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

nit: introduce TAG in companion object and use that as first parameter of Logger.e (I do think we had a convention of prefixing with mbgl_ but better to check the codebase).

actually, this is not a class, it's an extension function file.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could we do something as:

private const val TAG = "mgbl-LayerUtils"

at bottom or top of file?

LayerUtils naming comes from the java name of the LayerExt file.

@ank27 ank27 force-pushed the ak-get-layer-as-fix branch 2 times, most recently from 8cdceda to 4c10161 Compare September 27, 2021 13:10
@ank27 ank27 requested a review from tobrun September 27, 2021 13:11
} No newline at end of file
}

const val TAG = "Mbgl-LayerUtils" No newline at end of file
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

could be made private? We might be leaking this tag if not?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

getLayerAs is a public inline function, which cannot use Private / internal declarations.
Although I can use @PublishedApi annotation to make TAG internal and let the function use it. but basically, that would be public TAG in a way.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed by annotating TAG with @PublishedApi in order to use it in public inline function getLayerAs.

@ank27 ank27 force-pushed the ak-get-layer-as-fix branch from 4c10161 to e947591 Compare September 27, 2021 14:49
@ank27 ank27 requested review from kiryldz and tobrun September 28, 2021 07:30
@kiryldz
Copy link
Copy Markdown
Contributor

kiryldz commented Sep 28, 2021

@ank27 how does it all look from Java side?

@ank27
Copy link
Copy Markdown
Contributor Author

ank27 commented Sep 28, 2021

@ank27 how does it all look from Java side?
we can't use inline functions from java code directly. For that we have getLayer function already. Its the same with SourceExt as well.

Copy link
Copy Markdown
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.

LGTM

@ank27 ank27 requested a review from pengdev September 29, 2021 13:16
@ank27 ank27 merged commit d7a9d57 into main Oct 5, 2021
@ank27 ank27 deleted the ak-get-layer-as-fix branch October 5, 2021 13:15
@onXAdamBrown
Copy link
Copy Markdown

Awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants