Set as interactive on startup, set options(device =) to a string (#905)

* Set as interactive on startup, set `options(device =)` to a string

* Add note about `.Rprofile` tweaking and fix typos

Author: DavisVaughan

crates/ark/src/modules/positron/graphics.R

@@ -5,6 +5,14 @@
 #
 #
 
+# The Ark graphics device name
+ARK_GRAPHICS_DEVICE_NAME <- ".ark.graphics.device"
+
+# Declare the function name that `dev.new()` and `GECurrentDevice()`
+# go looking for to create a new graphics device when the current one
+# is `"null device"` and a new plot is requested
+options(device = ARK_GRAPHICS_DEVICE_NAME)
+
 # Set up "before plot new" hooks. This is our cue for
 # saving up the state of a plot before it gets wiped out.
 setHook("before.plot.new", action = "replace", function(...) {
@@ -14,42 +22,30 @@ setHook("before.grid.newpage", action = "replace", function(...) {
     .ps.Call("ps_graphics_before_plot_new", "before.grid.newpage")
 })
 
-# A persistent list mapping plot `id`s to their display list recording.
-# Used for replaying recordings under a new device or new width/height/resolution.
-RECORDINGS <- list()
-
-# Retrieves a recording by its `id`
-#
-# Returns `NULL` if no recording exists
-get_recording <- function(id) {
-    RECORDINGS[[id]]
-}
-
-add_recording <- function(id, recording) {
-    RECORDINGS[[id]] <<- recording
-}
-
-# Called when a plot comm is closed by the frontend
-remove_recording <- function(id) {
-    RECORDINGS[[id]] <<- NULL
-}
-
-render_directory <- function() {
-    directory <- file.path(tempdir(), "positron-plot-renderings")
-    ensure_directory(directory)
-    directory
-}
-
-render_path <- function(id, format) {
-    directory <- render_directory()
-    file <- paste0("render-", id, ".", format)
-    file.path(directory, file)
+#' Ark's graphics device creation entry point
+#'
+#' It is critical that this function:
+#' - Be findable by `get0(".ark.graphics.device", globalenv(), inherits = TRUE)`
+#' - Match the name of `ARK_GRAPHICS_DEVICE_NAME`
+#' - Match the name we set in `options(device = ARK_GRAPHICS_DEVICE_NAME)`
+#'
+#' That teaches `dev.new()` and `GECurrentDevice()` (used by grid, among other
+#' things) how to create a new Ark graphics device when the first plot opens.
+#'
+#' `options(device =)` also takes a function, but in a fresh session if
+#' `grDevices::dev.interactive(orNone = TRUE)` is called when the device is
+#' `"null device"` and `getOption("device")` returns a function, then it can't
+#' determine if the device that function would create is interactive or not, and
+#' we incorrectly look non-interactive, so we don't use that feature, and
+#' neither does RStudio (posit-dev/positron#7681).
+#'
+#' @export
+.ark.graphics.device <- function() {
+    .ps.Call("ps_graphics_device")
 }
 
 #' @export
 .ps.graphics.create_device <- function() {
-    name <- "Ark Graphics Device"
-
     # Create the graphics device that we are going to shadow.
     # Creating a graphics device mutates global state, we don't need to capture
     # the return value.
@@ -58,7 +54,7 @@ render_path <- function(id, format) {
     # Update the device name + description in the base environment.
     index <- grDevices::dev.cur()
     old_device <- .Devices[[index]]
-    new_device <- name
+    new_device <- ARK_GRAPHICS_DEVICE_NAME
 
     # Copy device attributes. Usually, this is just the file path.
     attributes(new_device) <- attributes(old_device)
@@ -70,11 +66,18 @@ render_path <- function(id, format) {
     env_bind_force(baseenv(), ".Devices", .Devices)
     env_bind_force(baseenv(), ".Device", new_device)
 
-    # Also set ourselves as a known interactive device.
+    # Now return back to Rust to modify the wrapped callbacks directly
+}
+
+#' @export
+.ps.graphics.register_as_interactive <- function() {
+    # Set ourselves as a known interactive device.
     # Used by `dev.interactive()`, which is used in `stats:::plot.lm()`
     # to determine if `devAskNewPage(TRUE)` should be set to prompt before
-    # each new plot is drawn.
-    grDevices::deviceIsInteractive(name)
+    # each new plot is drawn. We set this on startup rather than on creation
+    # of the first graphics device because that is sometimes too late
+    # (posit-dev/positron#7681).
+    grDevices::deviceIsInteractive(ARK_GRAPHICS_DEVICE_NAME)
 }
 
 # Create a recording of the current plot.
@@ -478,3 +481,35 @@ default_device_type_linux <- function() {
 stop_no_plotting_capabilities <- function() {
     stop("This version of R wasn't built with plotting capabilities")
 }
+
+# A persistent list mapping plot `id`s to their display list recording.
+# Used for replaying recordings under a new device or new width/height/resolution.
+RECORDINGS <- list()
+
+# Retrieves a recording by its `id`
+#
+# Returns `NULL` if no recording exists
+get_recording <- function(id) {
+    RECORDINGS[[id]]
+}
+
+add_recording <- function(id, recording) {
+    RECORDINGS[[id]] <<- recording
+}
+
+# Called when a plot comm is closed by the frontend
+remove_recording <- function(id) {
+    RECORDINGS[[id]] <<- NULL
+}
+
+render_directory <- function() {
+    directory <- file.path(tempdir(), "positron-plot-renderings")
+    ensure_directory(directory)
+    directory
+}
+
+render_path <- function(id, format) {
+    directory <- render_directory()
+    file <- paste0("render-", id, ".", format)
+    file.path(directory, file)
+}

crates/ark/src/modules/positron/options.R

@@ -21,11 +21,6 @@ options(browser = function(url) {
     .ps.Call("ps_browse_url", as.character(url))
 })
 
-# Set up graphics device
-options(device = function() {
-    .ps.Call("ps_graphics_device")
-})
-
 # Register our password handler as the generic `askpass` option.
 # Same as RStudio, see `?rstudioapi::askForPassword` for rationale.
 options(askpass = function(prompt) {

crates/ark/src/plots/graphics_device.rs

@@ -65,14 +65,20 @@ thread_local! {
 
 const POSITRON_PLOT_CHANNEL_ID: &str = "positron.plot";
 
-// Expose thread initialization via function so we can keep the structs private
+// Expose thread initialization via function so we can keep the structs private.
+// Must be called from the main R thread.
 pub(crate) fn init_graphics_device(
     comm_manager_tx: Sender<CommManagerEvent>,
     iopub_tx: Sender<IOPubMessage>,
     graphics_device_rx: AsyncUnboundedReceiver<GraphicsDeviceNotification>,
 ) {
     DEVICE_CONTEXT.set(DeviceContext::new(comm_manager_tx, iopub_tx));
 
+    // Declare our graphics device as interactive
+    if let Err(err) = RFunction::from(".ps.graphics.register_as_interactive").call() {
+        log::error!("Failed to register Ark graphics device as interactive: {err:?}");
+    };
+
     // Launch an R thread task to process messages from the frontend
     r_task::spawn_interrupt(|| async move { process_notifications(graphics_device_rx).await });
 }

crates/ark/tests/plots.rs

@@ -101,3 +101,59 @@ par(mfrow = c(1, 1))
     frontend.recv_iopub_idle();
     assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count);
 }
+
+#[test]
+fn test_graphics_device_initialization() {
+    let frontend = DummyArkFrontend::lock();
+
+    // On startup we are in the interactive list, but not current device
+    let code = "'.ark.graphics.device' %in% grDevices::deviceIsInteractive()";
+    frontend.send_execute_request(code, ExecuteRequestOptions::default());
+    frontend.recv_iopub_busy();
+    let input = frontend.recv_iopub_execute_input();
+    assert_eq!(input.code, code);
+    assert_eq!(frontend.recv_iopub_execute_result(), "[1] TRUE");
+    frontend.recv_iopub_idle();
+    assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count);
+
+    // The current device is `"null device"`
+    let code = ".Device";
+    frontend.send_execute_request(code, ExecuteRequestOptions::default());
+    frontend.recv_iopub_busy();
+    let input = frontend.recv_iopub_execute_input();
+    assert_eq!(input.code, code);
+    assert_eq!(frontend.recv_iopub_execute_result(), "[1] \"null device\"");
+    frontend.recv_iopub_idle();
+    assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count);
+
+    // The current `"null device"` is not interactive
+    let code = "grDevices::dev.interactive()";
+    frontend.send_execute_request(code, ExecuteRequestOptions::default());
+    frontend.recv_iopub_busy();
+    let input = frontend.recv_iopub_execute_input();
+    assert_eq!(input.code, code);
+    assert_eq!(frontend.recv_iopub_execute_result(), "[1] FALSE");
+    frontend.recv_iopub_idle();
+    assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count);
+
+    // But `orNone = TRUE` looks at `options(device =)` in this case, which
+    // we set to us, so this works (and is used by `demo(graphics)`)
+    let code = "grDevices::dev.interactive(orNone = TRUE)";
+    frontend.send_execute_request(code, ExecuteRequestOptions::default());
+    frontend.recv_iopub_busy();
+    let input = frontend.recv_iopub_execute_input();
+    assert_eq!(input.code, code);
+    assert_eq!(frontend.recv_iopub_execute_result(), "[1] TRUE");
+    frontend.recv_iopub_idle();
+    assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count);
+
+    // Now simulate the user creating a plot, which makes us the current graphics device
+    let code = "x <- .ark.graphics.device(); grDevices::dev.interactive()";
+    frontend.send_execute_request(code, ExecuteRequestOptions::default());
+    frontend.recv_iopub_busy();
+    let input = frontend.recv_iopub_execute_input();
+    assert_eq!(input.code, code);
+    assert_eq!(frontend.recv_iopub_execute_result(), "[1] TRUE");
+    frontend.recv_iopub_idle();
+    assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count);
+}

doc/graphics-devices.md

@@ -50,19 +50,35 @@ For pure Jupyter settings outside of Positron, we don't have "dynamic" plots. In
 
 # Ark graphics device
 
-Ark creates a "shadow" graphics device that manages other graphics devices. We technically wrap a `grDevices::png()` and inherit all of the default hooks and behaviors that come with that, and then we inject our own hooks on top of specific png hooks. For example, when the `newPage` hook is called we call the png device's `newPage` hook manually and then also layer in our own `newPage` behavior. This is how we learn when changes have occurred, when to record a plot, when we are deactivated, etc.
+Ark creates a "shadow" graphics device that manages other graphics devices. We technically wrap `grDevices::png()` or `ragg::agg_record()` and inherit all of the default hooks and behaviors that come with that, and then we inject our own hooks on top of specific png hooks. For example, when the `newPage` hook is called we call the device's `newPage` hook manually and then also layer in our own `newPage` behavior. This is how we learn when changes have occurred, when to record a plot, when we are deactivated, etc.
+
+We prefer using the ragg device if it is available, as it is cheaper and has better behavior with some features that surprisingly do still use the underlying device, like rendering of Chinese text in a plot.
 
 # Device interactivity
 
-Certain plotting functions like `stats:::plot.lm()` will draw multiple plots in a row, pausing for the user to hit enter before advancing to the next page. This requires a few things to work properly:
+Certain plotting functions like `stats:::plot.lm()` or `demo(graphics)` will draw multiple plots in a row, pausing for the user to hit enter before advancing to the next page. This requires a few things to work properly:
+
+-   Internally, `ARK_GRAPHICS_DEVICE_NAME` is set to `".ark.graphics.device"`, which matches the name of a function we expose named `.ark.graphics.device()`, which is in charge of creating a new Ark graphics device and is findable by `get0(".ark.graphics.device", globalenv(), inherits = TRUE)`.
+
+-   `grDevices::deviceIsInteractive(ARK_GRAPHICS_DEVICE_NAME)` must be called during startup to "register" ourselves as a known interactive graphics device.
+
+-   `options(device = ARK_GRAPHICS_DEVICE_NAME)` must be called during startup so that:
+
+    -   `dev.new()` and `GECurrentDevice()` know the function name to look up to create a new device when the current is `"null device"`.
+
+    -   `grDevices::dev.interactive(orNone = TRUE)` has a name to look up in the list returned from `deviceIsInteractive()` in a fresh session where the device is otherwise still `"null device"`. This is used by `demo(graphics)`.
+
+-   When we send Positron a notification about a new plot, we *also* send along an initial version of that plot to show the user. Even if our dimensions are wrong, this allows Positron to show the user a plot after each `Enter` keypress, where ark is otherwise paused waiting for the user to do something and cannot handle a render request.
+
+With all of that in place:
 
--   `grDevices::deviceIsInteractive(name)` must be called during Ark graphics device initialization with `name` set to our Ark graphics device name to "register" ourselves as a known interactive graphics device.
+-   `grDevices::dev.interactive(orNone = TRUE)` should always return `TRUE`, even in a fresh session when the current device is technically `"null device"`.
 
--   `grDevices::dev.interactive()` should then return `TRUE`, and this is used in the default value of the `ask` argument of `stats:::plot.lm()`.
+-   `grDevices::dev.interactive()` should return `TRUE` after you've created your first plot (before then, the device is `"null device"` and this returns `FALSE`, this matches RStudio).
 
--   We must be able to render plots (i.e. replay plot recordings) at any time. In particular, we must be able to handle a render request from Positron at interrupt time. We currently wait until the next idle time, which happens after the user has hit enter and advanced through all the plots - at which point we actually render them all at once, which is a suboptimal experience. We've been actively avoiding doing much at interrupt time because it is rather unsafe to do so, and here we'd have to change graphics devices at interrupt time, which feels very unsafe.
+    -   This is used in the default value of the `ask` argument of `stats:::plot.lm()` and by the time `ask` is evaluated our device has been created so this returns `TRUE` there as intended.
 
-    -   One way we could possibly improve on this is to render an initial version of each plot in the opening notification we send to Positron using some default dimensions and device type. Positron could also supply us these defaults to use during the startup procedure.
+Note that with this approach, a motivated user can still set `options(device = "quartz")` in their `.Rprofile` if they'd like to use their own default graphics device rather than the one that comes with ark / Positron.
 
 # Structures and terminology