Add arguments to editFeatures allowing control over leafpm options

[JoshOBrien]

New arguments pmToolbarOpts, pmDrawOpts, pmEditOpts, and
pmCutOpts take lists of arguments that will be passed on to the
corresponding leafpm functions. (The elements of pmToolbarOpts,
for example, get passed on as supplied arguments to
leafpm::pmToolbarOptions().) This PR addresses (in part) Issue #92 .

(Note that, unless overridden by an element in user-supplied *Opts
argument, editFeatures retains its current leafpm defaults, such as
allowSelfIntersection = FALSE.)

This example demonstrates usage of one of the newly added arguments:

library(sf)
library(leaflet)
library(mapview)
library(mapedit)
library(leafpm)
## make a polygon for testing
x <- list(matrix(c(11,0,11,1,12,1,12,0,11,0), ncol=2, byrow=TRUE))
pp <- st_sf(geom = st_sfc(st_polygon(x)), crs=4326)

## Demonstrate `pmDrawOpts` argument usage
tst <-
editFeatures(pp, editor = "leafpm",
             pmDrawOpts = list(snappable = FALSE,
                               templineStyle = list(color="yellow")))

[tim-salabim]

Thanks for this! One thing I'd like to avoid (if possible) is too many arguments. So, if we can have one set of *Opts arguments for both editors that would be much preferable.

@timelyportfolio I vaguely remember you were trying to implement leafpm to be as similar as possible to leaflet.extras - leaflet.draw implementation. Do you think it's feasible to match option lists internally or would that be too much effort?

[timelyportfolio]

@JoshOBrien Thanks so much for this pull request.

@tim-salabim I have struggled with this decision since the inception of the project. Originally, I favored less arguments to the mapedit:*() set of functions with the belief that the more customized construction of the editors should be offloaded to the user. For instance in the edit module lines I was careful to look for an editor already added before overwriting or creating new.

After a lot of thought I think I still like this behavior. Otherwise, we will have a significant number of additional arguments to mapedit functions. I can rewrite these lines similar to the module lines mentioned above to default to user supplied editor if available. If we elect to go this direction, I also think I should better document this expectation and add more examples.

I'd love both of your thoughts on this approach.

[tim-salabim]

Thanks for your thoughts @timelyportfolio. How about we utilise ... for this? With proper documentation, I think this can be an elegant solution.

[JoshOBrien]

Thanks to both of you for picking this back up. I've got the same sense as @timelyportfolio that it's important to avoid a proliferation of (likely to be rarely used) formal arguments. At the same time, I think it's important to provide more advanced users with some way to reach those lower-level leafpm and leaflet.extras options.

It does seem possible to use ... for this, though without a few more details, I'm not sure I'm imagining the same approach as @tim-salabim.

My own inclination would be to add a single formal argument -- perhaps opts= -- that plays the same role in drawFeatures(), editFeatures(), etc. as the par.settings= argument does in the various lattice plotting functions (see, e.g., here). This has several advantages:

• It adds minimal clutter to the formal argument lists exposed to most users ...

• ... while still allowing more advanced users to modify any or all of the lower-level settings.

• It leaves ... free for other future uses.

• It imposes a definite and easily documented structure on the arguments passed in by users.

• It makes it possible for a user to construct a customized list of settings that they can then easily reuse and cleanly pass in to the function. In the case of par.settings=, this allows things like the following, which is very clean, even though myTheme is a length 35 list with 377 sub-elements:

  library(lattice)
  myTheme <- latticeExtra::custom.theme(pch = 16)
  xyplot(Sepal.Length ~ Petal.Length, group= Species, data = iris, par.settings = myTheme)

The main downside of this approach might be that constructing a list of options suitable for passing to opts= would require a fair amount of skill and care on the part of the user. If we went this way, it might be a good idea to provide a helper function, something along the lines of grid::gpar() or latticeExtra::custom.theme(), that performs some argument checking and list construction duties, and finally returns a list that can be passed in to the opts= argument.

Please let me know if any of that doesn't make sense, and I'll be happy to explain in more detail.

Looking forward to hearing your further thoughts!

[tim-salabim]

@JoshOBrien thanks for the lattice examples! I miss those olden days :-)

Regarding my suggestion to utilise ..., I envision something along the lines of leafem::granishMap() which is a rather generic function to map arguments to functions which are both supplied via ....
It enables things like:

library(leaflet)
library(leafem)

m = leaflet() %>% addTiles()

m1 = garnishMap(m, addScaleBar, addMouseCoordinates,
                position = "bottomleft")
m1

Though care needs to be taken to avoid unintended recycling of arguments in case they map to more than one of the provided functions.
If we had a similar helper function in mapedit we could easily map options for the respective editors. At least I think so.

That said, I am absolutely fine with having an options (instead of opts - to be more concise with upstream packages) argument. It would make it a little more obvious to the user where/how to supply the editor options. I totally agree with all the advantages you've listed above. And I don't think we get around the fact that controlling the editor options will require some skills that go beyond argument = value notation.

[JoshOBrien]

@tim-salabim Interesting. Thanks for sharing that garnishMap() pattern. In this case, I still like the relative simplicity of going the options= route.

Would you and @timelyportfolio like me to draw up a draft implementation of that? And if so, is there any reason not to do all of the processing of options= here in editMod(), moving e.g. this bit from editFeatures.sf() into editMod() to keep all of that logic together?

[tim-salabim]

@JoshOBrien a PR with a draft implementation would be great! One last thing, I think in order to be concise with the whole leaflet-verse, we should name the argument editorOptions, don't you think?

[tim-salabim]

Oh, and yes, moving those options to one place is preferable.

[timelyportfolio]

@JoshOBrien this sounds great. Yes, if possible please move the quick patch to its better home. @tim-salabim, since the output of the mapedit functions is not a leaflet map, using piped functions for construction is much more difficult.

I would like to express a preference for all other map customization to happen outside of the mapedit functions by the user. Otherwise, I think this gets out of hand. I do agree that since we are "editing" that it makes sense to include those options here. Leaflet customization can be nearly infinite, and I think that customization is best handled by leaflet and supporting packages.

@JoshOBrien, thanks again for jumping in to participate. If @tim-salabim agrees, I think you deserve a spot in DESCRIPTION.

[tim-salabim]

Absolutely! @JoshOBrien should be listed in DESCRIPTION

@timelyportfolio I totally agree that no map custimisation should happen in mapedit (other than rendering a standard mapview map if no map object is supplied). I was not implying anything regarding map customisation with the example of garnishMap(), it was merely meant as an example to hightlight how we might do argument matching to editor options.

[JoshOBrien]

OK, @tim-salabim , I just went ahead and submitted a PR (#100) with a draft implementation of the above. If it looks good to you all, I'd be happy to work a bit more on the documentation, to polish that for incorporation in the package.

Also, @timelyportfolio, I heartily agree with your suggestion that mapedit should not attempt to support additional map customization. In case it's of interest, my real reason for wanting finer control over the leafpm editor was so that I could control its snappable= setting. My hope is that this contribution ends up helping other users who need control over that or a few of the other "substantive", non-cosmetic, settings.

[tim-salabim]

Hi @JoshOBrien I think this looks great! I feek like this is worth a vignette. Would you be ok with writing one up showcasing these new options? We could also think about writing a blog post for r-spatial.org with the new options implementation.
Please note that I will be away next week (from tomorrow) with very limited access to my computer. So will not be able to review/merge/... any of your work next week. Not sure about @timelyportfolio 's availability.

[JoshOBrien]

@tim-salabim Super! I think a vignette would be an ideal way to document this, and will go ahead and draft one. I most likely won't be able to get to it in the next 10 days, but will pick it back up soon after that.

[tim-salabim]

No rush, as I said, I will be out of office too for the next week. And many thanks for implementing this!

[timelyportfolio]

@JoshOBrien if I understand correctly, this is is superseded by #100. If so, I will close and go with what I consider to be the preferred option of #100. Thanks!!!!!!

[JoshOBrien]

@timelyportfolio Super. That sounds good. Thanks!