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

Add basic appshot.shiny.appobj functionality #55

Merged
merged 13 commits into from
Feb 28, 2018

Conversation

schloerke
Copy link
Collaborator

Adds an appshot method for shiny app objects.

  • Saves all elements in the same environment as the server function of the app.
  • Saves all currently attached packages.
  • In a separate process,
    • Loads the environment, app, and packages.
    • Run the app
  • Take a screenshot

@schloerke
Copy link
Collaborator Author

Could not use observe() from Shiny because the system2() command to call phantom within webshot became blocked.

library(shiny)

ui <- fluidPage(
  plotOutput("plot")
)

server <- function(input, output, session) {
  output$plot <- renderPlot({
    plot(cars)
  })
}

obj <- shinyApp(ui, server)

run_and_snapshot <- function(shinyAppObj) {
  initialized <- FALSE
  observe({
    if (!initialized) {
      invalidateLater(5000)
      initialized <<- TRUE
      return()
    }
    
    # Do whatever
    message("Take snapshot")
    shiny::stopApp()
  })
  
  print(obj)
  message("Got here")
}

@jcheng5
Copy link

jcheng5 commented Feb 22, 2018

@schloerke Can you elaborate on that? Calling system2 didn't ever return if you called it from where message is in the code snippet?

@schloerke
Copy link
Collaborator Author

Updated to use shiny::observe and keep checking until the callr r session closes, rather than looking for the file.

R/appshot.R Outdated
if (r_session$is_alive()) {
# try again later
shiny::invalidateLater(200)
} else {
Copy link
Owner

Choose a reason for hiding this comment

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

It would also be good to have a timeout check, just in case the R process with webshot hangs for some reason.

R/appshot.R Outdated
},
args
)

Copy link
Owner

@wch wch Feb 23, 2018

Choose a reason for hiding this comment

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

Maybe add on.exit(r_session$kill()) to make sure that it goes away? Although now that I think of it, that won't ensure that phantomjs process will exit -- in fact, it's probably the reverse, since if the child R process receives the hard kill signal, it won't be able to send any signals to the phantomjs process. Anyway, something should happen to ensure the child R process and grandchild phantomjs process exit.

Now that I think of it again, maybe killing the R process will cause the phantomjs process to exit. But the behavior might also be different on Windows.

R/appshot.R Outdated
r_session <- callr::r_bg(
function(...) {
# Wait for app to start
Sys.sleep(0.5)
Copy link
Owner

Choose a reason for hiding this comment

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

This delay should be a parameter. Same for appshot.character.

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 think that is fair for us to keep it internal. It is a delay for webshot to start processing. The delay in taking the picture could be increased by the 'delay' parameter. By keeping it static at 0.5, it is a minimum of 0.5 seconds in processing before the webshot is taken.

R/appshot.R Outdated
@@ -40,14 +42,73 @@ appshot.character <- function(app, file = "webshot.png", ...,
p <- processx::process$new("R", args = c("--slave", "-e", cmd))
Copy link
Owner

Choose a reason for hiding this comment

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

Switch to callr::r_bg. We might not even need the processx package as a (direct) dependency.

R/appshot.R Outdated
webshot_timeout <- webshot_timeout + 10
}
webshot_timeout_ms <- webshot_timeout * 1000
count <- 0
Copy link
Owner

Choose a reason for hiding this comment

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

Use times instead of counts.

R/appshot.R Outdated
shiny::runApp(app, port = port, display.mode = "normal")

# return webshot::webshot file value
r_session$get_result() # safe to call as the r_session must have ended
Copy link
Owner

Choose a reason for hiding this comment

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

Does this need to be invisible()?

@schloerke
Copy link
Collaborator Author

@wch Added

  • shiny rmdshot example
  • appshot shiny timeout adds delay value
  • removed render quotes for do.call
  • make wrapper to callr::r_bg to upgrade envvars from NULL to callr default env value

@wch wch merged commit c05f8e0 into wch:master Feb 28, 2018
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