Allow addFeatures to work on leaflet_proxy object

[lbusett]

Currently, leafem::addFeatures does not work on leaflet_proxy objects, because it expects only a leaflet object as input. This PR simply modifies leafem::garnishMap so that it works also on leaflet_proxy maps.

It also slightly modifies leafem::addFeatures so that if the data layer provided in addFeature is not 4326, it is automatically reprojected (this for "coherence" with mapView, where I think layers are automatically reprojected).

Below, I report an example of a shiny app using addFeatures with a leafletProxy map, which works with the implemented changes.

HTH.

library(shiny)
library(leaflet)
library(RColorBrewer)

ui <- bootstrapPage(
  tags$style(type = "text/css", "html, body {width:100%;height:100%}"),
  leafletOutput("map", width = "100%", height = "100%"),
  absolutePanel(top = 10, right = 10,
                sliderInput("range", "Magnitudes", min(quakes$mag), max(quakes$mag),
                            value = range(quakes$mag), step = 0.1
                ),
                selectInput("colors", "Color Scheme",
                            rownames(subset(brewer.pal.info, category %in% c("seq", "div")))
                ),
                checkboxInput("legend", "Show legend", TRUE)
  )
)

quakes <- sf::st_transform(sf::st_as_sf(quakes, 
                                        coords = c("long", "lat"), crs = 4326, remove = FALSE), 3857)

server <- function(input, output, session) {
  
  # Reactive expression for the data subsetted to what the user selected
  filteredData <- reactive({
    quakes[quakes$mag >= input$range[1] & quakes$mag <= input$range[2],]
  })
  
  # This reactive expression represents the palette function,
  # which changes as the user makes selections in UI.
  colorpal <- reactive({
    colorNumeric(input$colors, quakes$mag)
  })
  
  # chk <- reactiveValues()
  # chk$mapok <- FALSE
  output$map <- renderLeaflet({
    # Use leaflet() here, and only include aspects of the map that
    # won't need to change dynamically (at least, not unless the
    # entire map is being torn down and recreated).
    leaflet(quakes) %>% addTiles() %>%
      fitBounds(~min(long), ~min(lat), ~max(long), ~max(lat))
  })
  
  # Incremental changes to the map (in this case, replacing the
  # circles when a new color is chosen) should be performed in
  # an observer. Each independent set of things that can change
  # should be managed in its own observer.
  observe({
    pal <- colorpal()
    
    leafletProxy("map") %>%
      clearMarkers()  %>% 
      leafem::addFeatures(data = filteredData(),
                          radius = ~4^mag/100,
                          weight = 1, color = "#777777",
                          fillColor = ~pal(mag), fillOpacity = 0.7, popup = ~paste(mag))
  })
  
  # Use a separate observer to recreate the legend as needed.
  observe({
    proxy <- leafletProxy("map", data = quakes)
    
    # Remove any existing legend, and only if the legend is
    # enabled, create a new one.
    proxy %>% clearControls()
    if (input$legend) {
      pal <- colorpal()
      proxy %>% addLegend(position = "bottomright",
                          pal = pal, values = ~mag
      )
    }
  })
}

shinyApp(ui, server)

[tim-salabim]

Thanks @lbusett !!
I've merged and modified a bit. I've deleted all examples using mapview to avoid the cyclic dependency (suggests). I've also deleted all calls to checkAdjustProjection as this should not happen here. This is something that should happen in in mapview.

[lbusett]

@tim-salabim

Thanks for considering merging! Concerning reprojection, I see your point, though I liked the possibility to have auto-reprojection in leaflet pipes, and I feel that it is a bit counterintuitive that with the current version these work:

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

while these do not :

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

However, it's no big deal and I'll just have to remember to reproject as needed in case I want to use leafem wrappers in leaflet pipes.

Consider however that removing the auto-reprojection in addStaticLabels is somewhat of a breaking change. Infact, it leads this "use case" in r-spatial/mapview#161:

leaflet() %>%
  addTiles() %>%
  addPolylines(data = st_transform(trails, 4326)) %>%
  addStaticLabels(trails)

to fail, while it was previously working.

HTH,

Lorenzo

[tim-salabim]

Thanks for these thoughts @lbusett, however I still think that in leafem we should not auto-reproject. leaflet makes it very clear that every data should be in epsg:4326 so all leafem::* functionality should behave similar in my opinion. We should, however, provide suitable warnings in case data is not in 4326

[lbusett]

Understood. Concerning Warnings, leaflet is already providing them:

> leaflet(indata) %>% addFeatures()

Warning message:
sf layer has inconsistent datum (+proj=longlat +datum=NAD27 +no_defs).
Need '+proj=longlat +datum=WGS84' 

I'd think it should throw an error instead, in these situations, but it is a leaflet problem, not a leafem one.