Move all plot watching logic into watchout #151
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,45 +1,3 @@ | ||
| looks_different <- function(old_dl, new_dl) { | ||
| if (identical(old_dl, new_dl)) { | ||
| return(FALSE) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,15 +1,23 @@ | ||
| watchout <- function(handler = new_output_handler(), | ||
| new_device = TRUE, | ||
| debug = FALSE, | ||
| frame = parent.frame()) { | ||
| last_plot <- NULL | ||
|
|
||
| if (new_device) { | ||
| # Ensure we have a graphics device available for recording, but choose | ||
| # one that's available on all platforms and doesn't write to disk | ||
| pdf(file = NULL) | ||
| dev.control(displaylist = "enable") | ||
| dev <- dev.cur() | ||
| defer(dev.off(dev), frame) | ||
| } | ||
|
|
||
| # record current devices | ||
| devs <- dev.list() | ||
| devn <- length(devs) | ||
| dev <- dev.cur() | ||
|
|
||
| con <- file("", "w+b") | ||
| defer(frame = frame, { | ||
| if (!test_con(con, isOpen)) { | ||
|
|
@@ -24,20 +32,63 @@ watchout <- function(handler = new_output_handler(), | |
| old <- options(try.outFile = con) | ||
| defer(options(old), frame = frame) | ||
|
|
||
| capture_plot <- function(incomplete = FALSE) { | ||
| # only record plots for our graphics device | ||
| if (!identical(dev.cur(), dev)) { | ||
| return() | ||
| } | ||
| # cur_devs <- dev.list() | ||
| # # No graphics devices | ||
|
There was a problem hiding this comment. @cderv I left this code in but commented out to deliberately draw your attention to it. I'm pretty sure the logic is redundant (because if the device list has changed then that implies the current device has changed) and it doesn't cause any tests to fail. I think it was included because the logic was previously split over two functions and so it was harder to see that it was redundant. But it is a change and I wanted to double check that I didn't miss anything. There was a problem hiding this comment. I agree this seems redundant. It seems all this have been introduced over time to fix different bugs. Some have tests that are passing, and other may not have test. I am seeing that Lines 157 to 163 in e8bd6f5 Now we call it once when creating I tried to see if this could cause problems, but it seems not. There was a problem hiding this comment. Yeah, I'm reasonably certain that I got that logic right, and I added a few more tests to capture various situations. There was a problem hiding this comment. BTW in general if you're trying to find problems it's useful to consider the distinction between |
||
| # if (is.null(cur_devs)) { | ||
| # return() | ||
| # } | ||
| # # Current graphics device changed since evaluate started | ||
| # if (!identical(cur_devs, devs)) { | ||
| # return() | ||
| # } | ||
|
|
||
| # current page is incomplete | ||
| if (!par("page") && !incomplete) { | ||
| return() | ||
| } | ||
|
|
||
| plot <- recordPlot() | ||
| if (!makes_visual_change(plot[[1]])) { | ||
| return() | ||
| } | ||
|
|
||
| if (!looks_different(last_plot[[1]], plot[[1]])) { | ||
| return() | ||
| } | ||
|
|
||
| last_plot <<- plot | ||
| handler$graphics(plot) | ||
| plot | ||
| } | ||
|
|
||
| capture_output <- function() { | ||
| out <- read_con(con) | ||
| if (!is.null(out)) { | ||
| handler$text(out) | ||
| } | ||
| out | ||
| } | ||
|
|
||
| check_devices <- function() { | ||
| # if dev.off() was called, make sure to restore device to the one opened | ||
| # when watchout() was called | ||
| if (length(dev.list()) < devn) { | ||
| dev.set(dev) | ||
| } | ||
| devn <<- length(dev.list()) | ||
| invisible() | ||
| } | ||
|
|
||
| list( | ||
| capture_plot = capture_plot, | ||
| capture_output = capture_output, | ||
| check_devices = check_devices | ||
| ) | ||
| } | ||
|
|
||
| read_con <- function(con, buffer = 32 * 1024) { | ||
|
|
||
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 curious: what does motivate the move for this refactored code after
evaluate_top_level_expression?All tests are passing so seems good - but wanted to understand the thinking as I would have place it in the same place by precaution
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.
Over time, my plan is to move to more and more logic in to
evaluate()so the logic is more clear. In this case specifically, I think it makes sense to check the devices after you've run the user code, rather than before. (Although the net effect is generally going to be the same.)