Vectorize arguments of webshot #33

FrancoisGuillem · 2016-11-28T14:00:36Z

Hello,

This pull request solves #32 . It vectorizes all arguments of "webshot" in order to improve performance when taking a lot of screenshots. For instance, on my laptop, taking 10 screenshots with default parameters takes 7 seconds instead of 27 seconds.

Parameters for each screenshot are passed to script webshot.js as a JSON array. Then casper.eachThen is used to loop over each configuration object.

timelyportfolio · 2016-11-28T14:43:17Z

Wow, this looks really nice! Thanks for your efforts adding this functionality.

wch

I like the general approach, although since the change is pretty substantial, I'm concerned about the risk of breaking existing behavior. If you can verify that you've tested the behavior for the following (vectorized and un-vectorized), that would help:

Broken URLs
Missing selectors
Different zoom levels
General error handling

Also, it would be good to have one or two examples in the README and in the vignette.

wch · 2016-11-28T17:13:26Z

R/webshot.R

-#' @param file Name of output file. Should end with \code{.png}, \code{.pdf}, or
-#'   \code{.jpeg}.
+#' @param url A vector of URLs to visit.
+#' @param file A Vector of names of output files. Should end with \code{.png},


Should be "vector" instead of "Vector".

Also, can you describe what happens to filenames if there are multiple URLs but only one filename?

wch · 2016-11-28T17:13:26Z

R/webshot.R

-#'   \code{.jpeg}.
+#' @param url A vector of URLs to visit.
+#' @param file A Vector of names of output files. Should end with \code{.png},
+#' \code{.pdf}, or \code{.jpeg}.


Could you indent consistently with the other documentation? If you're using RStudio, you can press Ctrl-Shift-/ (or Cmd-Shift-/ on Mac) to automatically reflow/indent correctly.

wch · 2016-11-28T17:13:26Z

R/webshot.R

@@ -112,6 +124,17 @@ webshot <- function(
    stop("Need url.")
  }

+  if (length(url) > 1) {
+    if (length(file) == 1) {
+      file <- sapply(1:length(url), function(i) {


Please use vapply or lapply instead of sapply -- this makes it easier for others to infer what is returned.

Also, use seq_along(url) instead of 1:length(url).

wch · 2016-11-28T17:13:26Z

R/webshot.R

+        gsub("\\.(.{3,4})$", replacement, file)
+      })
+    } else if (length(file) != length(url)) {
+      stop("parameters 'url' and 'file' should have same length")


Should say that file can also be length 1.

wch · 2016-11-28T17:13:26Z

R/webshot.R

@@ -112,6 +124,17 @@ webshot <- function(
    stop("Need url.")
  }

+  if (length(url) > 1) {
+    if (length(file) == 1) {


It would be helpful to have a short comment describing what this block does.

wch · 2016-11-28T17:13:26Z

inst/webshot.js

-if (opts.selector) {
-  opts.selector = opts.selector.split(",");
-}
+var data = JSON.parse(args[1]);


I think data is a bit vague. Something like allOpts would be better (though there's probably a better name that could be used).

wch · 2016-11-28T17:13:26Z

inst/webshot.js

+  var opts = response.data;
+
+  // Prepare options
+  opts = utils.merge(opt_defaults, opts);


I believe the utils.merge function has the side effect of modifying opts_defaults, which is undesirable, since that variable gets reused now.

Does replacing this by opts = utils.merge(opts, opt_defaults); resolve the problem ?

Ho no I see the problem, forget it.

That has different behavior: it will overwrite values in opts with those from opt_defaults, so that the defaults will always supersede the values in opts. I think it would make sense to add a (shallow) object cloning function called utils.clone.

wch · 2016-11-28T17:13:26Z

inst/webshot.js

-  this.capture(filename, cr);
+  // Go to url and perform the desired screenshot
+  this.thenOpen(url, function(r) {
+    this.zoom(opts.zoom)


I'm not 100% sure about this, but I believe that it's more efficient to do the zoom and viewport sizing before opening the URL.

I think it would be better to do the chaining like this:

casper .zoom(...) .viewport(...) .thenOpen(...) .wait(..., function() { ... });

wch · 2016-11-28T17:13:26Z

R/webshot.R

+    })
+  }
+
+  if (!is.null(selector)) data$selector <- argToVec(selector)


selector and expand should probably check for the correct number of items (either 1 or 4) in the R code instead of in the Javascript, because an error in the JS could now result in a partially-successful run. Better to have it not work at all than to partially work.

wch · 2016-11-28T17:13:26Z

inst/webshot.js

-  eval(opts.eval);
-}
+casper.eachThen(data, function(response) {
+  var url = response.data.url;


url and file don't need to be pulled out separately from the rest of the opts, do they?

… few comments

…optsList" in the R code for consistency

FrancoisGuillem · 2016-11-29T13:52:07Z

I think I have corrected every problem. I have tested unvectorized and vectorized versions of examples of the vignette and everything looks fine.

Invalid URLs are better handled. Before, webshot was creating en empty screenshot. Now it throws an explicit error.

I Have not tested functions resize and shrink because I have not the required tools on my profesional computer. Could you test it ?

wch

@cuche27 Looking good! I have more comments though... I don't have time right now to test out those functions but I may in the next couple of days.

@yihui with these changes, the object of class webshot can now be a vector of filenames instead of just one. What kind of changes will knit_print.webshot need to work with a vector of names?

@jimhester will the vector of names work with the code you've written to use webshot objects?

wch · 2016-11-29T16:13:42Z

R/image.R

@@ -21,6 +21,11 @@
 #' }
 #' @export
 resize <- function(filename, geometry) {
+  mapply(.resize, filename = filename, geometry = geometry)


2 issues with mapply:

Like sapply, the return type isn't known in advance (it could be a list, char vector, numeric vector, etc.) unless you use SIMPLIFY = FALSE.

If you pass an unnamed char vector to mapply, it returns a named vector or list, which is undesirable here. (For example, try mapply(function(x) 1, c("a","b")).)

To ensure that the result is an unnamed char vector, it should be something like:

as.character(mapply( ... , USE.NAMES = FALSE, SIMPLIFY = FALSE))

Also, I think the documentation needs to be updated to indicate that the args can be vectors. This would be similar to the documentation changes for webshot.

wch · 2016-11-29T16:13:42Z

R/image.R

+  structure(filename, class = "webshot")
+}
+
+.resize <- function(filename, geometry) {


I'd prefer this to be called resize_one.

wch · 2016-11-29T16:13:42Z

R/webshot.R

+  if(!is.null(expand) && !is.list(expand)) expand <- list(expand)
+
+  # Check length of arguments
+  lengthOfArgs <- vapply(environment(), length, numeric(1))


I don't like the vapply(environment()), because it's fragile (if code above is modified to add a new var, it could break) and because using vapply on an environment just seems strange.

wch · 2016-11-29T16:13:42Z

R/webshot.R

+      gsub("\\.(.{3,4})$", replacement, file)
+    })
+  }
+
  if (is_windows()) {
    url <- fix_windows_url(url)


I think fix_windows_url won't work with multiple URLs.

It looks fine actually. All functions used in fix_windows_url accept vectors.

Yes, but it calls if(grepl(...)), and if is not vectorized. For example:

> if (c(T, F, F)) 1 else 0 [1] 1 Warning message: In if (c(T, F, F)) 1 else 0 : the condition has length > 1 and only the first element will be used

Oh yes I missed that. It should be fixed now.

wch · 2016-11-29T16:13:42Z

inst/webshot.js

-    '  phantomjs webshot.js <url> <name>.png [options]');
+if (args.length < 2) {
+  console.log(
+    'usage: phantomjs webshot.js <optsList>\n' +


I'd prefer the original formatting, which puts "Usage:" on a separate line:

Usage: phantomjs webshot.js <optsList> ...

wch · 2016-11-29T16:13:42Z

inst/webshot.js

+  casper.zoom(opts.zoom)
+    .viewport(opts.zoom * opts.vwidth, opts.zoom * opts.vheight)
+    .thenOpen(opts.url, function(r) {
+      casper.wait(opts.delay * 1000, function() {


I think the usual convention is to use this instead of casper, right?

I fill unconfortable with this convention but indeed it is the case. I will change it back :)

wch · 2016-11-29T16:13:42Z

R/image.R

@@ -77,6 +81,11 @@ resize <- function(filename, geometry) {
 #' }
 #' @export
 shrink <- function(filename) {
+  mapply(.shrink, filename = filename)
+  structure(filename, class = "webshot")


Same issues here as with resize.

wch · 2016-11-29T16:13:42Z

R/webshot.R

+#'   of multiple URLs, this parameter can also be a list with same length as
+#'   \code{url} with each element of the list being "viewport" or a
+#'   four-elements numeric vector. This option is not compatible with
+#'   \code{selector}


Missing period at end of sentence.

wch · 2016-11-29T16:13:42Z

R/webshot.R

@@ -45,6 +56,12 @@
 #' # Might need a longer delay for all assets to display
 #' webshot("http://rstudio.github.io/leaflet", delay = 0.5)
 #'
+#' # One can also take screenshots of several URLs with only one command.
+#' # This is more efficient than calling multiple times 'webshot'.


"... calling 'webshot' multiple times."

wch · 2016-11-29T16:13:42Z

R/appshot.R

@@ -2,6 +2,8 @@
 #'
 #' @inheritParams webshot
 #' @param app A Shiny app object, or a string naming an app directory.
+#' @param file Name of output file. Should end with \code{.png}, \code{.pdf}, or


I don't think this is necessary, because appshot's documentation should inherit values from webshot.

yihui · 2016-11-29T16:44:34Z

I can make changes in knitr after this PR is merged.

…nd documentation

jimhester · 2016-11-29T19:28:11Z

No this does not currently work with printing within knitr documents. The knit_print.webshot() function assumes only one filename and would need to be modified to return more than one html_screenshot object. Not sure if there is some functionality in knitr to do this already or not.

jimhester · 2016-11-29T19:31:25Z

Actually this seems to be all that is needed to get it working.

diff --git a/R/webshot.R b/R/webshot.R
index 02f9c0d..1aadbdc 100644
--- a/R/webshot.R
+++ b/R/webshot.R
@@ -236,9 +236,11 @@ webshot <- function(
 }

 knit_print.webshot <- function(x, ...) {
-  res <- readBin(x, "raw", file.size(x))
-  ext <- gsub(".*[.]", "", basename(x))
-  structure(list(image = res, extension = ext), class = "html_screenshot")
+  lapply(x, function(filename) {
+    res <- readBin(filename, "raw", file.size(filename))
+    ext <- gsub(".*[.]", "", basename(filename))
+    structure(list(image = res, extension = ext), class = "html_screenshot")
+  })
 }

A simple test is

knit(text = c("```{r}", "webshot(c(\"https://www.r-project.org/\", \"https://cran.r-project.org/package=webshot\"))", "```"))
#> [1] "\n```r\nwebshot(c(\"https://www.r-project.org/\", \"https://cran.r-project.org/package=webshot\"))\n```\n\n![plot of chunk unnamed-chunk-1](figure/unnamed-chunk-1-1.png)![plot of chunk unnamed-chunk-1](figure/unnamed-chunk-1-2.png)"

FrancoisGuillem · 2016-11-29T22:52:00Z

I have tested functions shrink and resize. They seem to work correctly.

wch · 2016-12-01T16:07:43Z

Thanks!

wch · 2016-12-01T16:18:01Z

Oh, I just realized that the result of the mapply calls in resize and shrink aren't saved, so the SIMPLIFY and USE.NAMES args aren't needed. But there's also no harm in having those there.

FrancoisGuillem added 2 commits Nov 28, 2016

Vectorize arguments url and file of webshot

c09cef9

Vectorize all arguments and update documentation

Loading status checks…

42b4472

wch reviewed Nov 28, 2016

View changes

FrancoisGuillem added 15 commits Nov 28, 2016

Replace sapply by vapply, correct some indentation problems and add a…

7adec8a

… few comments

Test in the R code that argument "expand" is valid

a0c79dc

Update the R code that checks cliprect

7afce70

rename variable data by optsList which is a bit clearer

Loading status checks…

ed81948

BUGFIX: stop modifying values in opt_defaults

2254df8

BUGFIX: webshots crashes when 'cliprect' is specified

1af3c4b

Zoom and set viewport before opening URL + replaces "this" by "casper"

5f6a5e0

BUGFIX: casper was taking screenshot and then waiting delay

Loading status checks…

1ccedea

Stop execution if url cannot be loaded

Loading status checks…

558b5a6

Check length of arguments

Loading status checks…

120b303

Improve a bit documentation of argument "file" and rename "opts" in "…

de7b295

…optsList" in the R code for consistency

Add a few examples of vectorization on README

Loading status checks…

4e2fa10

Vectorize resize and shrink

Loading status checks…

7caddd5

Describe usage of webshot.js

Loading status checks…

15bf37f

remove extra-space

Loading status checks…

701192c

wch reviewed Nov 29, 2016

View changes

yihui mentioned this pull request Nov 29, 2016

Vectorized webshot::webshot yihui/knitr#1326

Open

FrancoisGuillem added 2 commits Nov 29, 2016

Vectorize fix_windows_url

6124aa4

Minor changes: rename some variables and functions, better comments a…

Loading status checks…

4962527

…nd documentation

Update documentation of shrink and resize

Loading status checks…

b3c9fd3

wch merged commit 1d6e6b1 into wch:master Dec 1, 2016
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

wch mentioned this pull request Dec 1, 2016

Vectorize webshot #32

Closed

wch / webshot

Join GitHub today

Vectorize arguments of webshot #33

Vectorize arguments of webshot #33

Conversation

FrancoisGuillem commented Nov 28, 2016 • edited

timelyportfolio commented Nov 28, 2016

wch left a comment

This comment has been minimized.

wch Nov 28, 2016 Owner

This comment has been minimized.

wch Nov 28, 2016 Owner

This comment has been minimized.

wch Nov 28, 2016 Owner

This comment has been minimized.

wch Nov 28, 2016 Owner

This comment has been minimized.

wch Nov 28, 2016 Owner

This comment has been minimized.

wch Nov 28, 2016 Owner

This comment has been minimized.

wch Nov 28, 2016 Owner

This comment has been minimized.

wch Nov 28, 2016 Owner

This comment has been minimized.

FrancoisGuillem Nov 28, 2016 Author Contributor

This comment has been minimized.

FrancoisGuillem Nov 28, 2016 Author Contributor

This comment has been minimized.

wch Nov 28, 2016 • edited Owner

This comment has been minimized.

wch Nov 28, 2016 Owner

This comment has been minimized.

wch Nov 28, 2016 Owner

This comment has been minimized.

wch Nov 28, 2016 Owner

FrancoisGuillem commented Nov 29, 2016

wch left a comment

This comment has been minimized.

wch Nov 29, 2016 Owner

This comment has been minimized.

wch Nov 29, 2016 Owner

This comment has been minimized.

wch Nov 29, 2016 Owner

This comment has been minimized.

wch Nov 29, 2016 Owner

This comment has been minimized.

FrancoisGuillem Nov 29, 2016 Author Contributor

This comment has been minimized.

wch Nov 29, 2016 Owner

This comment has been minimized.

FrancoisGuillem Nov 29, 2016 Author Contributor

This comment has been minimized.

wch Nov 29, 2016 Owner

This comment has been minimized.

wch Nov 29, 2016 Owner

This comment has been minimized.

FrancoisGuillem Nov 29, 2016 Author Contributor

This comment has been minimized.

wch Nov 29, 2016 Owner

This comment has been minimized.

wch Nov 29, 2016 Owner

This comment has been minimized.

wch Nov 29, 2016 Owner

This comment has been minimized.

wch Nov 29, 2016 Owner

yihui commented Nov 29, 2016

jimhester commented Nov 29, 2016

jimhester commented Nov 29, 2016 • edited

FrancoisGuillem commented Nov 29, 2016

wch commented Dec 1, 2016

wch commented Dec 1, 2016

Essential cookies

Always active

Analytics cookies

FrancoisGuillem commented Nov 28, 2016 •

edited

wch Nov 28, 2016
Owner

wch Nov 28, 2016
Owner

wch Nov 28, 2016
Owner

wch Nov 28, 2016
Owner

wch Nov 28, 2016
Owner

wch Nov 28, 2016
Owner

wch Nov 28, 2016
Owner

wch Nov 28, 2016
Owner

FrancoisGuillem Nov 28, 2016
Author Contributor

FrancoisGuillem Nov 28, 2016
Author Contributor

wch Nov 28, 2016 •

edited

Owner

wch Nov 28, 2016
Owner

wch Nov 28, 2016
Owner

wch Nov 28, 2016
Owner

wch Nov 29, 2016
Owner

wch Nov 29, 2016
Owner

wch Nov 29, 2016
Owner

wch Nov 29, 2016
Owner

FrancoisGuillem Nov 29, 2016
Author Contributor

wch Nov 29, 2016
Owner

FrancoisGuillem Nov 29, 2016
Author Contributor

wch Nov 29, 2016
Owner

wch Nov 29, 2016
Owner

FrancoisGuillem Nov 29, 2016
Author Contributor

wch Nov 29, 2016
Owner

wch Nov 29, 2016
Owner

wch Nov 29, 2016
Owner

wch Nov 29, 2016
Owner

jimhester commented Nov 29, 2016 •

edited