[Feature Suggestion] Move additional functions to `leafem` from `mapview`

[r-spatial]

[Feature Suggestion] Move additional functions to `leafem` from `mapview`

[tim-salabim]

@lbusett I think we can close this, given that addExtent, addStaticLabels as well as addStarsImage and addRasterRGB are now in leafem.

Please re-open if you disagree

[tim-salabim]

Hi @lbusett
yes, I am aware of those but didn't deem them to be very important. I was planning on moving those for the next release.
I would appreciate a PR. I'm currently at useR!2019 and won't have too much time to work on those.
I agree, addStarsImage() should be taken care of in #1

[lbusett]

Ok. I'll start looking into this and let you know.

[tim-salabim]

Thanks!

[lbusett]

Hi @tim-salabim

currentlly working on this. Migrating the add functions is not difficult. However, I wanted to ask you how you would like to proceed with the helpers: for example, to migrate addStaticLabels, I would need to migrate or replicate also helper functions such as checkAdjustProjection, makelabels, getLayerNamesFromMap, getGeometryType.

What is your take on this?

[tim-salabim]

I remember that this was one of the reasons that made me skip migrating addStaticLabels. I guess the sane thing to do is to migrate these functions from mapview to leafem as we import it in mapview anyway. What do you think?

[lbusett]

Makes sense to me. Having them duplicated between the two could generate confusion in the long run, IMO.

I could put all the "required" functions within utils.R, check where they are called within mapview and substitute with a leafem:: construct.

However, I noticed that utils.R already contains several "helper" functions duplicated between the two packages (e.g., combineExtent, getLayerControlEntriesFromMap, getGeometryType and others wihtin utils.R). Is your idea to eliminate all duplicates? (that would require a more careful job of function-call hunting)

[tim-salabim]

Well spotted! Yes, my approach was to split out and duplicate all that it is needed first to make sure that everything works and then eventually (when things are stable) to address code completion. In the light of this, we could also just duplicate things for now and then I will use a dedicated development cycle to rid all packages of duplicates. Given that this involves some 6 packages, I think this may just be the easiest to do. So I would suggest not to worry about duplication, just make sure all helpers needed are in leafem.

Is that OK for you?

[lbusett]

Ok for me. I'll put everything needed in utils.R, then (I guess no worries about shadowing, because I think that none of the helpers is exported) .

One further question: I was also considering mofifyng addFeatures and addStsticLabels so that they can be used in a leaflet pipe without specifying the data argument, such as in:

indata <- sf::st_read(system.file("shape/nc.shp", package="sf"))
leaflet(indata)  %>% 
  addFeatures() %>%
  addStaticLabels()

would you be ok with that change?

[tim-salabim]

Yes, that would be great. In case you find any other cases that should be modified this way, feel free to change those too. And please also add yourself to the DESCRIPTION as an author!

[lbusett]

@tim-salabim

I tried a "coupled" PR on leafem and mapview to move functions and implement some additional changes. I tested as best as I could, and commented on changes I wasn't sure about.

HTH!

[lbusett]

Works for me. Thanks.