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.

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

Shiny interactivity #60

Merged
merged 15 commits into from
Apr 28, 2015
Merged

Shiny interactivity #60

merged 15 commits into from
Apr 28, 2015

Conversation

jcheng5
Copy link
Member

@jcheng5 jcheng5 commented Apr 24, 2015

The long-awaited resurrection of Shiny interactivity for the new bindings.

(@wch @yihui No need to review this during your conferences, we can go over it together in person on Monday.)

Background

The new Leaflet bindings make it quite convenient to create and customize Leaflet map definitions and then render them. Some limited interactivity is available via 1) the popup argument, 2) you can use map click events in Shiny to drive other (non-map) behavior, and 3) using renderLeaflet you can reactively replace an entire map with an entire new map.

What has been missing is the ability to add/remove features from, and customize bounds on, a running map. (This was possible in the previous-generation binding, using map$addPolygons or whatever; in fact, the only way that binding worked was using those kinds of imperative runtime commands.) For example, adding new polygons as new data comes in; removing outliers based on user clicks; driving the map view from one location to another.

API Design

The new interactivity style aims to restore the power of the old binding, while preserving the ease-of-use that we have aimed for with the new bindings.

For the purposes of this explanation, we'll refer to the usual style of Leaflet map creation to be local, as in, we're building up a local representation of a map, to be rendered to HTML or sent to Shiny when we're done; and the new capability introduced by this PR as being remote, that is, we're issuing commands from R to a map instance in the browser that already exists.

This PR introduces the ability for all of the Leaflet layer (e.g. addPolygons) and view (e.g. 'fitBounds') functions to operate in both local and remote modes. The local mode has not changed at all. The remote mode we're introducing is invoked when you construct a map not with leaflet() but with the new getMapProxy() function.

A small example (which you can run with example(getMapProxy)):

library(shiny)

ui <- fluidPage(
  leafletOutput("map1")
)

server <- function(input, output, session) {
  output$map1 <- renderLeaflet({
    # local
    leaflet() %>% addCircleMarkers(
      lng = runif(10),
      lat = runif(10),
      layerId = paste0("marker", 1:10))
  })
  observeEvent(input$map1_marker_click, {
    # remote
    getMapProxy("map1", session) %>%
      removeMarker(input$map1_marker_click$id)
  })
}

shinyApp(ui, server)

In this example we're calling removeMarker but we could have called any mapping function there. Currently, anything that's valid for local maps is also valid for remote maps (though the removeXXX/clearXXX functions are pretty sketchy when used for local, see the Note under ?removeTiles).

I find this symmetry extremely elegant. This approach had occurred to me in the past but I was worried that the implementation would be unwieldy (e.g. two separate code paths for every map function) but the implementation ended up being quite clean and easy as well.

My only design-related question is whether the local/remote distinction can be easily grokked by users--feedback welcome!

Implementation

getMapProxy(mapId, session, data): Creates a proxy object for a remote map, the one that has the given mapId in the given session. data plays the same role it does in leaflet().

appendMapData(map, data, component, ...): This non-exported function previously existed, and was the common code path through which all of the layer-adding code passed through; it basically took a method name and arguments, and added them to a map object. (Really, it should be renamed invokeMapMethod or something.) This function now works for both local and remote maps; local objects continue to append the method/args to the map, while proxy objects send them directly to the remote map for immediate invocation, using invokeRemote (see next).

invokeRemote(map, method, args = list()): Non-exported. Sends a method and arg list to the remote map to be invoked. These are fire-and-forget calls so don't expect a return value from your JS method, you won't get any.

dispatch(map, local, remote): Non-exported. This is basically a convenient way to say:

if (map.remote)
  local
else
  remote

Most high-level R map functions should just call appendMapData and can be ignorant of the difference between local and remote maps. Those functions that want to customize behavior based on one or the other should call dispatch instead; the body of the remote branch will probably involve invokeRemote.

Open issues

  • I anticipate some discussion about the getMapProxy function name. I'm happy to entertain suggestions but let's not let this PR drag on solely because of the function name, as we are sometimes prone to do. :)
  • Automated tests
  • Check that no errors occur if getMapProxy is called for a map that doesn't exist
  • Allow getMapProxy calls to be deferred until after session flushes outputs
  • Better error message when dispatch isn't provided with the appropriate action type
  • Change dispatch branches from local/remote to class names

@chris-holcomb
Copy link

This is great Joe. With respect to getMapProxy I suspect getMap might be more clear (or getLeafletMap if that's too general). To make this more complete, I would suggest the inclusion of a removeGeoJSON and clearGeoJSON methods with code similar to removeShape and clearShapes. I've tested this and see no issues. I do see an issue that I also saw with the old leaflet package, where the lat and lng were not included in the input$mapX_geojson_click object. This can be rectified by altering the mouseHandler function to set "lat: e.latlng.lat, lng: e.latlng.lng" but I'm not yet certain if there are conflicts with this.

@jcheng5
Copy link
Member Author

jcheng5 commented Apr 24, 2015

For removeGeoJSON, the reason I haven't included that yet is because currently layerId applies to the entire GeoJSON blob, not individual features. I'm assuming you'd want to be able to remove an individual feature, no?

@jcheng5
Copy link
Member Author

jcheng5 commented Apr 24, 2015

I was concerned about getMap not making clear enough that you're doing something that's quite different in nature from leaflet(), even if it's not obvious to the naive reader what "proxy" is referring to. (Maybe getLeafletProxy would be more consistent...)

@jcheng5
Copy link
Member Author

jcheng5 commented Apr 24, 2015

@cholcomb The lat/lng bug should be fixed in #61. Note that #61 branches from master, not this branch, so you'll need to merge them locally if you want both (or just wait until we merge them both to master).

@chris-holcomb
Copy link

That's an interesting thought with respect to removing individual GeoJSON features. Ideally, I'd suppose you'd want removeGeoJSON to take two parameters, layerId and optionally featureId, but then you may have to require a certain GeoJSON format or assign a featureId on the fly which adds to the size of GeoJSON (perhaps optionally in addGeoJSON with featureID = order() by default).

That all sounds a bit messy and I can't currently think of any urgent reason to be able to remove specific GeoJSON, but removing specific GeoJSON layers to show other layers or restyle for example, is critical.

@jcheng5
Copy link
Member Author

jcheng5 commented Apr 24, 2015

Fair enough--I'll do that shortly.

@jcheng5 jcheng5 force-pushed the joe/feature/shiny-interactions branch from 68633c4 to 7e2de8b Compare April 27, 2015 22:32
@jcheng5 jcheng5 force-pushed the joe/feature/shiny-interactions branch from a344d85 to 361485e Compare April 27, 2015 23:40
@jcheng5
Copy link
Member Author

jcheng5 commented Apr 28, 2015

All that's left now is finalizing the getMapProxy function name. How about leafletRemote or leafletProxy, for consistency with leaflet()?

@yihui
Copy link
Member

yihui commented Apr 28, 2015

leafletProxy() sounds better to me

@jcheng5
Copy link
Member Author

jcheng5 commented Apr 28, 2015

OK, changed. If no other feedback, I'll merge after the Travis tests pass.

jcheng5 added a commit that referenced this pull request Apr 28, 2015
@jcheng5 jcheng5 merged commit 3a3830b into master Apr 28, 2015
@jcheng5 jcheng5 deleted the joe/feature/shiny-interactions branch April 28, 2015 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants