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

Allow recording/playing back inputs that do not have a binding #232

Merged
merged 6 commits into from
Jan 11, 2019

Conversation

wch
Copy link
Collaborator

@wch wch commented Dec 20, 2018

This PR adds the ability to record and play back inputs for which there is not an input binding, as is the case with htmlwidgets that have inputs, like DT and plotly.

When playing back, the input value will be set so that the R Shiny process knows about it, but the corresponding object in the browser will not -- so, for example, in the app below, if a row in the DT is selected during the recording phase, it will not be highlighted during playback. For some applications, this is OK, for others it is not.

Example test app with DT:

library(shiny)

ui <- fluidPage(
  numericInput("num", "number", 5),
  DT::dataTableOutput("table"),
  verbatimTextOutput("info")
)

server <- function(input, output, session) {
  output$table <- DT::renderDataTable({
    head(iris, input$num)
  })

  output$info <- renderText({
    paste0(
      "table_rows_selected:    ",  paste(input$table_rows_selected, collapse = ", "), "\n",
      "table_row_last_clicked: ",  paste(input$table_row_last_clicked, collapse = ", "), "\n",
      "table_cell_clicked:     ",  paste(input$table_cell_clicked, collapse = ", "), "\n"
    )
  })
}

shinyApp(ui = ui, server = server)

The resulting test script looks like this:

app <- ShinyDriver$new("../")
app$snapshotInit("mytest")

app$setInputs(table_rows_current = c(1, 2, 3, 4, 5), allowInputNoBinding_ = TRUE)
app$setInputs(table_rows_all = c(1, 2, 3, 4, 5), allowInputNoBinding_ = TRUE)
app$setInputs(table_rows_selected = 1, allowInputNoBinding_ = TRUE)
app$setInputs(table_row_last_clicked = 1, allowInputNoBinding_ = TRUE)
app$setInputs(table_cell_clicked = c("1", "5", "setosa"), allowInputNoBinding_ = TRUE)
app$setInputs(table_rows_selected = c(1, 3), allowInputNoBinding_ = TRUE)
app$setInputs(table_row_last_clicked = 3, allowInputNoBinding_ = TRUE)
app$setInputs(table_cell_clicked = c(3, 4, 0.2), allowInputNoBinding_ = TRUE)
app$snapshot()
app$setInputs(table_rows_selected = 1, allowInputNoBinding_ = TRUE)
app$setInputs(table_rows_selected = c(1, 5), allowInputNoBinding_ = TRUE)
app$setInputs(table_row_last_clicked = 5, allowInputNoBinding_ = TRUE)
app$setInputs(table_cell_clicked = c(5, 4, 0.2), allowInputNoBinding_ = TRUE)
app$snapshot()

Also note that some of the inputs should really be set simultaneously. I believe that table_rows_selected, table_row_last_clicked, and table_cell_clicked would all be set on the same "tick" of the JS event loop, so they could be coalesced into a single call to $setInputs(). It might be possible in the future to automatically detect these cases and coalesce these cases.

cc: @rpodcast, @cpsievert

)
),
value = FALSE
),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any real danger in having this default to TRUE? After running your example I was confused as to why the code generator wasn't working, then I realized I had to check this box...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I defaulted to FALSE because I didn't want people to start recording tests that use this feature, and then be surprised when the testing doesn't work perfectly -- screenshots that don't look right and/or behavior that isn't correct (due to an htmlwidget that needs actual interaction). By requiring users to opt-in, they're more likely to be aware of that.

I do get that the current situation is confusing. I can think of a few different alternatives:

  • In the right panel, input events without a binding could be highlighted in red. The drawback is that this isn't helpful for people who encounter it for the first time -- there needs to be some way to discover how to deal with it. Maybe a question mark and tooltip?
  • When the user clicks "Save and Quit", it could pop a confirmation dialog if there are any inputs without a binding and the box is unchecked. The dialog could say something roughly like, "There are input events without a binding. If you want them to be saved in the test script, press Cancel, then check the box, then click on Save and Quit again. If you don't want to save them, press OK."
  • By default, it could just generate a script with the code, but have it commented out. Then users could fix it by uncommenting the code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I finally got around to testing this out. I'd recommend the second alternative (confirmation dialog) if feasible.

paste0(
"app$setInputs(",
quoteName(event$name), " = ",
processInputValue(event$value, event$inputType),
args,
")"
)

} else {
if (allowInputNoBinding) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like you could get rid of a set of parentheses: else if (allowInputNoBinding) {

@rpodcast
Copy link
Contributor

rpodcast commented Jan 8, 2019

I am very excited to see this update! I did some testing and I only have some minor comments:

  • It might be worth calling out in the relevant documentation that in the case of a widget like DT, the recorder app will track all inputs produced by the widget during the interactions. By default there are 5 inputs produced by DT (see section 2.2 here). While I can delete the app$setInputs() calls to the non-relevant inputs after the fact, it would be really nice if the user could flag which of the inputs to track. That's certainly not a trivial issue to solve since each widget will do this differently, so that's more of a nice to have.
  • The documentation for app$setInputs could benefit from a statement explaining the allowInputNoBinding_ parameter.

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