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

Isolate side effects in graphics functions #228

Closed
wants to merge 6 commits into from
Closed

Conversation

nanxstats
Copy link
Collaborator

Fixes #227

This PR introduces safe_par() and safe_strwidth() as alternatives to their original graphics functions. These wrappers will be evaluated in a temporary BMP device, to avoid generating Rplots.pdf when running r2rtf code under certain context - such as clicking the test buttons in RStudio environments, even for third-party packages.

@yihui I'm not sure if this is the optimal solution so I'm open to suggestions.

Using these wrappers can be be 10x slower than calling the graphics functions directly:

microbenchmark::microbenchmark(
  r2rtf:::safe_strwidth("Hello world!", units = "inches", cex = 1, font = 1, family = "sans"),
  graphics::strwidth("Hello world!", units = "inches", cex = 1, font = 1, family = "sans"),
  times = 5000
)

#>    min     lq      mean median     uq      max neval cld
#> 44.526 46.945 53.097288 47.806 49.692 4399.300  5000   a
#>  2.747  2.952  3.199353  3.198  3.362   16.482  5000   b

@nanxstats nanxstats requested review from yihui and elong0527 June 3, 2024 01:26
#' @noRd
with_bmp <- function(expr) {
expr <- substitute(expr)
grDevices::bmp(file_null())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just for my understanding. Why would bmp() device used instead of pdf()

eval(expr, envir = parent.frame())
}

file_null <- function() if (.Platform$OS.type == "windows") "nul:" else "/dev/null"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we use "tempfile" to avoid a OS dependent logic?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess writing to the null device is often faster than a real file.

Copy link
Collaborator

@yihui yihui left a comment

Choose a reason for hiding this comment

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

I think a more elegant and faster solution is to use the null pdf device, i.e.,

opts <- options(device = function(...) pdf(NULL, ...))
on.exit(options(opts), add = TRUE)

Comment on lines +152 to +165
expr <- substitute(expr)
grDevices::bmp(file_null())
on.exit(
{
current <- grDevices::dev.cur()
if (current > 1) {
prev <- grDevices::dev.prev(current)
grDevices::dev.off(current)
if (prev != current) grDevices::dev.set(prev)
}
},
add = TRUE
)
eval(expr, envir = parent.frame())
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think you need substitute() + eval(), but can just take advantage of R's lazy evaluation of arguments, i.e., simply put expr at the end, and R will evaluate it at that time.

Suggested change
expr <- substitute(expr)
grDevices::bmp(file_null())
on.exit(
{
current <- grDevices::dev.cur()
if (current > 1) {
prev <- grDevices::dev.prev(current)
grDevices::dev.off(current)
if (prev != current) grDevices::dev.set(prev)
}
},
add = TRUE
)
eval(expr, envir = parent.frame())
grDevices::bmp(file_null())
on.exit(
{
current <- grDevices::dev.cur()
if (current > 1) {
prev <- grDevices::dev.prev(current)
grDevices::dev.off(current)
if (prev != current) grDevices::dev.set(prev)
}
},
add = TRUE
)
expr

@elong0527 elong0527 closed this Oct 6, 2024
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.

Blank Rplots.pdf generated when running snapshot tests interactively from RStudio
3 participants