-
Notifications
You must be signed in to change notification settings - Fork 20
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
Conversation
#' @noRd | ||
with_bmp <- function(expr) { | ||
expr <- substitute(expr) | ||
grDevices::bmp(file_null()) |
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this 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)
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()) |
There was a problem hiding this comment.
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.
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 |
Fixes #227
This PR introduces
safe_par()
andsafe_strwidth()
as alternatives to their original graphics functions. These wrappers will be evaluated in a temporary BMP device, to avoid generatingRplots.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: