Move all plot watching logic into watchout #151
Conversation
| plot <- plot && identical(dev, dev.cur()) | ||
| out <- watcher(plot, incomplete_plots) | ||
| output <<- c(output, out) | ||
| out <- list( |
There was a problem hiding this comment.
I don't like this interface so it's likely to change in a future PR.
R/watcher.R
Outdated
| if (!identical(dev.cur(), dev)) { | ||
| return() | ||
| } | ||
| # cur_devs <- dev.list() |
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 dev <- dev.cur() was previously called evaluate_top_level_expression() so for each parsed expression
Lines 157 to 163 in e8bd6f5
Now we call it once when creating watcher with watchout() call, and the check in capture_plot() based on initial value.
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 evaluate("plot(1); lines(2)") and evaluate("plot(1)\nlines(2)")
| @@ -117,6 +95,7 @@ evaluate <- function(input, | |||
| output_handler = output_handler, | |||
| include_timing = include_timing | |||
| ) | |||
| watcher$check_devices() | |||
There was a problem hiding this comment.
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.
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.)
R/watcher.R
Outdated
| if (!identical(dev.cur(), dev)) { | ||
| return() | ||
| } | ||
| # cur_devs <- dev.list() |
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 dev <- dev.cur() was previously called evaluate_top_level_expression() so for each parsed expression
Lines 157 to 163 in e8bd6f5
Now we call it once when creating watcher with watchout() call, and the check in capture_plot() based on initial value.
I tried to see if this could cause problems, but it seems not.
This eliminates a bunch of fiddly state management, and centralises the concern of figuring out if a plot should be recorded into one place.