Adds ability to update rank_list labels and exports update_rank_list

[yogat3ch]

Fixes #100

---

Hi sortable devs,

We needed the ability to update labels on a sortable rank list so I managed to get it working with shinyjs and figured I would adapt the solution to use the more conventional way by sending shiny custom messages.

This is my first go at building customMessageHandlers for shiny so I hope this is helpful!

This PR:

• Adds missing @examples tags to the update_rank_list docs
• Wraps updating text/labels in conditionals
• Adds a customMessageHandler to update the labels of a sortable::rank_list
• Updates the example shiny app to demo the new functionality

Any suggestions or is this good to go?

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 3 committers have signed the CLA.

✅ schloerke
❌ andrie
❌ yogat3ch
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[yogat3ch]

@schloerke This should be ready for another review

[schloerke]

LGTM! Thank you @yogat3ch

(Did not test)

[andrie]

Thank you. This looks very promising.

However, I found one problem: it doesn't seem to be possible to update an empty rank_list().

You can reproduce this using the modified example app at update/app.R with this code, that adds another button to update the rank list on the right. You'll notice that updating ONLY works if the list isn't empty.

## ---- update-bucket-list-app -----------------------------------------------
## Example shiny app with bucket list

library(shiny)
library(sortable)
library(magrittr)


ui <- fluidPage(
  tags$head(
    tags$style(HTML(".bucket-list-container {min-height: 350px;}"))
  ),
  fluidRow(
    column(
      width = 12,
      h2("Update the title"),
      actionButton("btnUpdateBucket", label = "Update bucket list title"),
      actionButton("btnUpdateLabelsFrom", label = "Add item to 'Drag from here'"),
      actionButton("btnUpdateLabelsTo", label = "Add item to 'To here'")
    )
  ),
  fluidRow(
    column(
      h2("Exercise"),
      width = 12,
      bucket_list(
        header = "Drag the items in any desired bucket",
        group_name = "bucket_list_group",
        orientation = "horizontal",
        add_rank_list(
          text = "Drag from here",
          labels = list(
            "1"
          ),
          input_id = "rank_list_from"
        ),
        add_rank_list(
          text = "to here",
          labels = NULL,
          input_id = "rank_list_to"
        )
      )
    )
  )
)

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

  # test updating the bucket list label
  counter_bucket <- reactiveVal(1)
  counter_label <- reactiveVal(1)
  observe({
    update_bucket_list(
      "bucket_list_group",
      header = paste("You pressed the update button", counter_bucket(), "times"),
      session = session
    )
    counter_bucket(counter_bucket() + 1)
  }) %>%
    bindEvent(input$btnUpdateBucket)

  observe({
    counter_label(counter_label() + 1)
    update_rank_list(
      "rank_list_from",
      labels = c(input$rank_list_from, counter_label())
    )
  }) %>%
    bindEvent(input$btnUpdateLabelsFrom)

  observe({
    counter_label(counter_label() + 1)
    update_rank_list(
      "rank_list_to",
      labels = c(input$rank_list_to, counter_label())
    )
  }) %>%
    bindEvent(input$btnUpdateLabelsTo)

  observe({
    len <- length(input$rank_list_to)
    count_word <- if(len == 0) "" else glue::glue("({len})")
    update_rank_list(
      "rank_list_to",
      text = paste0("To here ", count_word),
      session = session
    )
  }) %>%
    bindEvent(input$rank_list_to)
}

shinyApp(ui, server)

[yogat3ch]

Thanks @andrie, I'll look into this soon

[yogat3ch]

Hi @andrie,
update_rank_list now handles the case where a rank_list is instantiated with no labels.
As I was doing this I realized that using dropNulls in update_rank_list makes it such that update_rank_list is unable to update a rank_list to have no labels. Do we want to handle this case too?
The cons would be that it might reset someone's list with an unexpected firing of an observer with update_rank_list where labels aren't specified.

[yogat3ch]

It looks like the handling of the css_id internal to the update_rank_list label deviates pretty significantly from standard shiny handling of inputIds causing it to fail in modularized shiny apps.
We're going to have to change that handling to make it work

[yogat3ch]

Hey @andrie & @schloerke,
I've spent a handful of hours trying to figure out what's going on here and it looks like the issue goes beyond just the update module, it appears that the handling of the shiny input ids in sortable is incompatible with shiny modules.
I've added a contrived example in 5c9f4f7 that can be run with the console open. Clicking either of the buttons does not trigger the input methods.

I'm trying to hunt down the differences in how the input_id is handled in this package compared to others, but I'm wondering if anyone has a better understanding of what all needs to change to standardize the handling of the input_id argument internally such that sortable works in a modularized shiny app?

[schloerke]

@yogat3ch hoping I'll be able to look at it tomorrow

[yogat3ch]

@yogat3ch hoping I'll be able to look at it tomorrow

Awesome, thank you @schloerke

[schloerke]

Ok. So {sortable} needs to not set the id values directly. Ex:

• Mainly:

  sortable/R/bucket_list.R

  Line 154 in 8e24b8f

  id = paste0("bucket-list-", css_id),

• But also:

  sortable/R/bucket_list.R

  Line 17 in 8e24b8f

  input_id <- increment_rank_list_input_id()

When these values are set, they are unaware of the NS that is being used within a module.

---

Reasoning:

• update_bucket_list() calls session$sendInputMessage(id, msg). Link
• Within a module, sendInputMssage will prepend the module's ID in front of the supplied id. Link
• So the message in the example app is being sent to an id of "bucket_lists-bucket-list-bucket_list_group". ({namespace}-{sortableprefix}-{cssid}).

---

I'm too far removed from the code to remember why I used css_id and regular input_id`. Seems odd to me. 😞

If the css_id were to be altered by adding a suffix (rather than a prefix), it could be used within modules if exposed in the function api. But this seems like a bandaid on top of a bandaid. This change would most likely be a breaking change for some users.

---

The other workaround that I can think of is to use some form of https://github.com/rstudio/shiny/blob/e7b830755ac6da2773c9ff15fc6f9395c0c4bf87/R/modules.R#LL47C1-L61C2 to recursively look for the root Shiny session given a module's session object.

This method could be called within update_bucket_list() (and similar methods) to find the root Shiny session before sending the input message. Ex:

# Given a session_proxy, search `parent` recursively to find the real
# ShinySession object. If given a ShinySession, simply return it.
find_ancestor_session <- function(x, depth = 20) {
  if (depth < 0) {
    stop("ShinySession not found")
  }
  if (inherits(x, "ShinySession")) {
    return(x)
  }
  if (inherits(x, "session_proxy")) {
    return(find_ancestor_session(.subset2(x, "parent"), depth-1))
  }

  stop("ShinySession not found")
}

update_bucket_list <- function(css_id,
                               header = NULL,
                               labels = NULL,
                               session = shiny::getDefaultReactiveDomain()) {

  rootSession <- find_ancestor_session(session)

  inputId <- paste0("bucket-list-", css_id)
  message <- dropNulls(list(id = css_id, header = header, labels = labels))
  rootSession$sendInputMessage(inputId, message)
}

@yogat3ch I'm pretty sure this would work and users would not need to alter their current sortable code. While I'm usually opposed to using internal-like methods, I believe this is the cleanest solution going forward.

[schloerke]

I'm trying my suggestion above and I can not get it to work as I expected. I've run out of time for today.

Workaround is to not use modules for sortable. That stinks for an answer, but anything I'm coming up with is to undo what a module provides.

---

This being said, we should prolly fix {sortable} to be id friendly with modules. I'll make an issue

[schloerke]

@andrie I've added a news entry about the breaking ID values. The demo app shows off the module support nicely.

(No more fires. Stepping away from PR. Thank you for the reprex, @yogat3ch !)