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.}

@schloerke This should be ready for another review

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)

Thanks @andrie, I'll look into this soon

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.

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

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?

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

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

Awesome, thank you @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.

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

@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 !)

FYI, I'm going to switch my attention to this topic.

I'm going to start by reviewing your approach, and have one idea in mind to deal with setting empty labels.

By default an argument of NULL means leave that element as is. But that leaves the question (as you point out) on how to set labels to be empty.

My proposal is to use an empty list to indicate this, e.g. labels = list().

I'm going to attempt to get this to work, starting from your PR.

@yogat3ch , the ideas in this PR are really valuable, and I have been cherry-picking some changes.

However, in the process I discovered that there were some problems with this implementation, particularly:

• Updating the labels destroyed some components of the rank_list, in particular the text
• Updating the labels did not change the rank_list value in a shiny app
• Some attributes of the labels were also destroyed in the process

In the end, I found it easier to re-implement the key ideas and will be tracking this in the PR #113.

That PR already has fully functional support for update_rank_list(). Once I have used the ideas to also implement update_bucket_list() I will close your PR in favour of #113.

Many thanks for your contribution.

Sorry for the long absence on this, I took a hiatus from data science. Thank you for extracting the working bits and ideas and creating a working implementation @andrie !