file extension parameter for ggsave() to allow device specification for files without extension

[sebkopf]

Thank you for the fantastic ggplots package!

This is a suggested implementation for specifying a file extension in ggsave to allow device matching (pdf, png, jpg, etc.) for filenames without extension. The suggestion is motivated by the problem of using ggsave in a shinyApp for letting the user download a plot (for example addressed on StackOverflow here: http://stackoverflow.com/questions/14810409/save-plots-made-in-a-shiny-app). Because the shiny server provides a temporary file without extension, ggsave can't match to any of the internally defined device functions. I'm not aware of a currently available solution to this problem and although there are several work-arounds, I like the simplicity of ggsave and wanted to suggest this solution (not sure if it's the best way to do it, but it's the most elegant I could think of). This would allow use of ggsave for a shinyApp downloadHandler like so:

# In ui.R:
downloadLink('downloadPlot', 'Download')

# In server.R:
output$downloadPlot <- downloadHandler(
  filename = "myplot.pdf",
  content = function(file) {
    ggsave(file, plot = qplot(speed, dist, data = cars), ext = "pdf")
  }
)

I'm relatively new to GitHub and this is my second contribution to someone else's project, please forgive if I'm missing some specific conventions or etiquette about forking/pull requests.

Sebastian

[BrianDiggs]

What advantage does this approach have over passing this information as the device argument?

ggsave(file, plot = qplot(speed, dist, data=cars), device = pdf)

[sebkopf]

Hi Brian,
I agree, that would be great if it worked that way but it doesn't have the desired effect. ggsave will call the passed in pdf function directly rather than the wrapper defined within ggsave. This works out okay for pdf because the only parameter ggsave sets with the wrapper is version = "1.4" (my bad for poor choice of pdf in the sample code, sorry). When you try the same with png or jpeg you run into trouble because now the ggsave wrapper does more:

ggsave(file, plot = qplot(speed, dist, data=cars), device = png)

will save a png with tiny width and height because the default units are "px" and ggsave passes the parameters in inches. Instead, a workaround requires specifying these parameters (res and units) in a separate wrapper (verbatim from ggsave):

png <- function(..., width, height) {
     grDevices::png(...,  width=width, height=height, res = 300, units = "in")
}
ggsave(file, plot = qplot(speed, dist, data=cars), device = png)

This works just fine and I'm using this approach myself but it's not very intuitive. I'm happy to keep using this workaround but wanted to suggest the above solution in case you guys would be interested in a more intuitive implementation.

Another way of doing it without introducing any extra parameters would be by allowing device = 'character' ('pdf', 'png', 'jpg', etc.) in addition to device = function(...) {} and simply adding this code block in ggsave:

 if (is.character(device)) {
      device <- match.fun(device)
 }

which would make this work directly:

ggsave(file, plot = qplot(speed, dist, data=cars), device = 'png')

What do you think?

[BrianDiggs]

Thanks for the explanation; you are right that passing the device function misses the ggsave defaults that get set when the device is set via the defaults based on filename extension. I like the idea of allowing device to be a character argument which is then matched to the (internal, first) device functions better than the approach of adding a new parameter. But you can also wait to hear the opinion of an actual maintainer before making that change.

[hadley]

• Motivate the change in one paragraph, and include it in NEWS.
  In parentheses, reference your github user name and this issue:
  (@hadley, #1234)
• Check pull request only includes relevant changes.
• Use the official style.
• Update documentation and re-run roxygen2
• Add minimal example, if new graphical feature

[hadley]

Looks good. Can you please do the last two things listed above?

[sebkopf]

Thanks Hadley, can you clarify real quick if you prefer the implementation discussed with Brian above allowing device as a character argument (that can be matched against the internally defined device functions) rather than the new file extension parameter?

I'm happy to implement the latter and file as a separate pull request (+NEWS) or stick with the current and include it in NEWS. Let me know, thanks.

[hadley]

Ok, yeah, I'd prefer match.fun(device)

[sebkopf]

Sounds good, I implemented it with match.fun(device) instead, added an example in @examples and a line in the NEWS. Please let me know if there's anything else missing.

Update: the Travis CI build is failing but it appears to be because of a syntax error elsewhere, not in this pull request (see #949 for reference)

[JHonaker]

I'm not really sure how to resubmit this pull request for Travis CI, but it should build fine now. #949 fixed the error.

[sebkopf]

Yeah, I didn't know how to do that (resubmit the request for Travis), it builds just fine for me with #949 fixed. I'm happy to resubmit but the only way I can think of doing that is closing this pull request and creating a new one - is that the recommended approach?

[BrianDiggs]

According to this: http://stackoverflow.com/q/17606874/892313 it should be possible to trigger a new attempt at the build. But maybe Hadley (or one of the repo owners) has to do it because I don't see the triggering icon there.

[sebkopf]

Just a quick follow-up, I think this feature could still be very useful to users judging from the traffic that the original post on stack overflow still receives (save-plots-made-in-a-shiny-app).

As far as I can tell, all that's missing is a retriggering of the Travis CI build here by a repo owner. @hadley could you help out with this?

Alternatively, I'm happy to close this pull request and create a new one for this feature if that's easier, let me know.

[hadley]

Can you please add call. = FALSE here?

[hadley]

Just update this PR by merging/rebasing against master, and re-pushing the branch to github.

[hadley]

Are you still interested in this PR?

[sebkopf]

Okay, should be all ready to go now (including the requested .call = FALSE addition and some commit cleanup). Thanks for the reminder msg, missed last week's while traveling. And thanks a lot for the rebasing hint to retrigger Travis CI build, that's exactly what I was missing - learned something new again.

[hadley]

Thanks!

[lock]

This old issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with reprex) and link to this issue. https://reprex.tidyverse.org/