Use 'magick' package for resize() #68
Conversation
There was a problem hiding this comment.
I like the general idea, but I'm worried that it'll make webshot more difficult to install in Linux, or anywhere that compilation is required. Specifically, users will need to install magick, which requires the ImageMagick++ dev library, as well as the additional R packages Rcpp, knitr, and curl (and their dependencies).
Currently, the resize() function works only if ImageMagick/GraphicsMagick are installed, but even if they aren't installed, one can still install and use webshot. It would be good if the magick package was similarly optional, by moving it into Suggests, and having resize do a check for the package first.
A related concern I have is that people with existing installations of webshot (and IM/GM) will experience breakage when they upgrade. They'll try to upgrade their packages and, with the current PR, it will fail due to to compilation problems. On the other hand, with the change I proposed (moving magick to Suggests), the upgrade could succeed and then the actual use of resize() could fail.
How about this: when resize() is called, first check if magick is installed, and if not, then fall back to using the command-line tools. If neither is available, throw an error that suggests installing the magick package or IM/GM, and mention that installing imagick is preferable.
It's unfortunate that there's not a single best solution for this. On Windows and Mac, where binary packages are available, installing magick and its dependencies is trivially easy, but on Linux, where compilation is necesssary, installing magick can be difficult.
R/appshot.R
Outdated
| @@ -86,6 +86,7 @@ appshot.character <- function( | |||
|
|
|||
|
|
|||
| #' @rdname appshot | |||
| #' @importFrom utils getFromNamespace | |||
There was a problem hiding this comment.
R CMD check gave a NAMESPACE warning (unrelated to this PR).
There was a problem hiding this comment.
Oh, I see it now: https://github.com/wch/webshot/blob/bb2a4486/R/appshot.R#L110
The funny thing is that importing the function into webshot is irrelevant for that line: that code isn't even run in this R process -- it's run in a newly spawned R process. Better to just use utils::getFromNamespace().
There was a problem hiding this comment.
Yea, I think it's a false positive. I suspect the same warning would occur with a fully qualified reference.
There was a problem hiding this comment.
It won't raise a warning with utils::getFromNamespace.
There was a problem hiding this comment.
Didn't realize that. Just fixed.
|
I could definitely reconfigure in that way but then it might not be worth it as it creates the slightly awkward double dependency of gm/im command line tools and the package. It's not urgent from my perspective, so I'm happy to let this simmer a bit and see if there's community feedback at all. |
This PR removes the GraphicsMagic/ImageMagick dependency used in
resize()by instead using magick, which simplifies things (especially on Windows).