Brush select fix, new elements API

[DivadNojnarg]

• This allows brush_select to correctly update the list of selected nodes, edges and combos...
• New API for elements like nodes, edges and combos. More robust checks ...

[nbenn]

Reprex:

library(shiny)
library(bslib)
library(g6R)

ui <- page_fillable(
  actionButton("add", "Add node"),
  g6_output("dag")
)

server <- function(input, output, session) {

  output$dag <- render_g6(g6())

  observeEvent(
    input$add,
    {
      message("adding node")
      g6_add_nodes(
        g6_proxy("dag"),
        list(
          list(
            id = "a",
            label = "A",
            combo = list()
          )
        )
      )
    }
  )
}

shinyApp(ui, server)

Passing combo = list() seems to be the culprit. If I change to combo = NULL, all is fine. Maybe not related to this change after all?

[DivadNojnarg]

The issue is a missing list() wrapper when length(nodes) == 1 (also applies for edges, combos). That's something missing in g6R. I do it in other places:

if (length(el) == 1) {
  el <- list(el)
}

g6 would expect g6_add_nodes(proxy, list(list(id = 1, ...))). With multiple nodes, it feels quite natural to write list(list(), list()), for 1 node you pass g6_add_nodes(proxy, list(id = 1, ...)) which does not work.

[nbenn]

@DivadNojnarg just so that we're on the same page. What I meant above is passing nodes as

list(
  list(
    id = "a",
    label = "A",
    combo = list()
  )
)

does not work, while

list(
  list(
    id = "a",
    label = "A",
    combo = NULL
  )
)

does. Which is fine. The issue probably is more the error msg, which is a bit confusing:

Error: Node not found for id: . Graph may not work anymore.

What you say above about 1 vs multiple nodes could be a convenience feature, but what you have right now, where you expect a list of nodes (which for 1 node means a list(list(.)) construct), is fine I think. It's predictable that way. But don't worry about all of this. It's a minor thing. When I wrote my first comment, I thought it was a more fundamental problem.

[nbenn]

@DivadNojnarg ah, now I understand why I was a bit confused about the right way to pass an empty node combo spec. It is different for nodes in g6(). Doing

g6(
  nodes = list(
    list(
      id = "a",
      label = "A",
      combo = NULL
    )
  )
)

causes a JS error

TypeError: null is not an object (evaluating 'node.combo.toString')

So in one case, you have to pass combo = NULL and in the other as combo = list(). I guess some consistency would be nice. But again, once I figured things out, all was fine. It was just a bit cumbersome to get there.

[nbenn]

A full example would be

library(shiny)
library(bslib)
library(g6R)

ui <- page_fillable(
  actionButton("add", "Add node"),
  g6_output("dag")
)

server <- function(input, output, session) {

  output$dag <- render_g6(
    g6(
      nodes = list(
        list(
          id = "a",
          label = "A",
          combo = list()
        )
      )
    ) |>
    g6_behaviors(
      drag_canvas(),
      zoom_canvas()
    )
  )

  observeEvent(
    input$add,
    {
      message("adding node")
      g6_add_nodes(
        g6_proxy("dag"),
        list(
          list(
            id = "b",
            label = "B",
            combo = NULL
          )
        )
      )
    }
  )
}

shinyApp(ui, server)

but here the node addition somehow does not work. Do you see what I'm doing wrong here?

[nbenn]

@DivadNojnarg one additional thing to consider: while you can catch much of the "internal ID renaming" in JS <-> R code, whenever 3rd party JS code is involved, e.g. in context menu items, this has to be handled separately. Not a problem, just something to document and be aware of.

Code like

(value, target, current) => {
  Shiny.setInputValue('%s', current.id, {priority: 'event'});
}

has to handle its own ID renaming.

[DivadNojnarg]

@nbenn I'll continue on this PR (renaming it also) as I am gonna change the implementation. Instead of prefixing, I'll maintain a mapping registry of elements (type/id) when they are created and destroyed:

graph.on(GraphEvent.AFTER_ELEMENT_CREATE, (e) => {
  // add to registry
 })

graph.on(GraphEvent.AFTER_ELEMENT_DESTROY, (e) => {
   // remove from registry
 })

Then looking up the registry for an element id, you can find it's type.

[nbenn]

@DivadNojnarg yeah, I think this might be the better approach.

[DivadNojnarg]

@nbenn With the latest commit, you should be ok with BristolMyersSquibb/blockr.dag#33.

Regarding the other point above, ie passing combo = NULL (valid) or combo = list() invalid, a quality of live improvement would be to expose higher level helpers for elements like g6_node, g6_edge, g6_combo ... instead of dummy lists. This would allow to perform data consistency checks at the element level based on the g6 API specifications. I would have expected g6 JS to raise an error when something invalid is passed from R but this isn't the case for all properties.

Of course, I can't stop supporting basic lists for backward compatibility reasons.

[DivadNojnarg]

TBD

• Use the g6 data helpers in proxy functions
• Check the g6_set_* functions
• Fix pkgdown issue: some demo datasets have invalid data structure for the new API

[DivadNojnarg]

TBD (not mandatory):

• Add vignette on the existing graph inputs.
• More examples for the proxy functions.