Conversation
DavisVaughan
left a comment
DavisVaughan
left a comment
There was a problem hiding this comment.
I'd like to get @lionel-'s opinion on a few questions about where things should live, but otherwise seems good
| # is not yet available (e.g. during development with mismatched builds). | ||
| if (!is.null(source_uri)) { | ||
| pushed <- tryCatch( | ||
| { .ps.Call("ps_graphics_push_source_context", source_uri); TRUE }, |
There was a problem hiding this comment.
Can you please reformat with Air? I should probably turn that on as a CI check...
There was a problem hiding this comment.
Yeah, it looks like a lot of the .R files are not air-compliant. I went ahead and formatted the whole workspace (LMK if you'd like to keep the changes scoped to just this new file)
| error = function(e) FALSE | ||
| ) | ||
| if (pushed) { | ||
| on.exit(tryCatch(.ps.Call("ps_graphics_pop_source_context"), error = function(e) NULL), add = TRUE) |
There was a problem hiding this comment.
We are trying to use defer() instead of on.exit(), which comes with the right defaults
|
|
||
| # Try to resolve the file URI early so we can attribute plots to this | ||
| # source file. This is best-effort; if it fails we proceed without attribution. | ||
| source_uri <- tryCatch(path_to_file_uri(file), error = function(e) NULL) |
There was a problem hiding this comment.
I see no reason to not just consistently call it uri everywhere rather than renaming it later on
| // Push execution context to graphics device for plot attribution | ||
| // Push execution context to graphics device for plot attribution. | ||
| // Extract code_location from the execute request for source file origin. | ||
| let code_location = exec_req.code_location().log_err().flatten().map(|mut loc| { |
There was a problem hiding this comment.
@lionel- should code_location() just normalize() the URI internally? It seems like the one other place we call code_location() also normalizes after the fact.
| /// Push a source file URI onto the source context stack. | ||
| /// Called from the `source()` hook when entering a sourced file. | ||
| #[harp::register] | ||
| unsafe extern "C-unwind" fn ps_graphics_push_source_context(uri: SEXP) -> anyhow::Result<SEXP> { | ||
| let uri_str: String = RObject::view(uri).try_into()?; | ||
| DEVICE_CONTEXT.with_borrow(|cell| cell.push_source_context(uri_str)); | ||
| Ok(harp::r_null()) | ||
| } | ||
|
|
||
| /// Pop a source file URI from the source context stack. | ||
| /// Called from the `source()` hook when leaving a sourced file. | ||
| #[harp::register] | ||
| unsafe extern "C-unwind" fn ps_graphics_pop_source_context() -> anyhow::Result<SEXP> { | ||
| DEVICE_CONTEXT.with_borrow(|cell| cell.pop_source_context()); | ||
| Ok(harp::r_null()) | ||
| } |
There was a problem hiding this comment.
IIUC the desire is to be able to query the name of the file that is currently being source()-ed at any time while inside the execution of that file.
That doesn't feel super specific to plots, so I would not be against that being a generic feature that lives in Console instead.
@lionel- what do you think?
| fn new_positron_page(&self) { | ||
| self.is_new_page.replace(true); | ||
| self.id.replace(Self::new_id()); | ||
| *self.pending_origin.borrow_mut() = None; |
There was a problem hiding this comment.
Use self.pending_origin.replace(None) like we've done for the other fields?
Here and elsewhere.
| // Clear any unconsumed pending origin so it doesn't leak into the | ||
| // next execute request. `process_changes()` above already consumed it | ||
| // if this execution produced a new plot; this handles the case where | ||
| // drawing occurred (e.g. `lines()` inside `source()`) but only as an | ||
| // update to an existing plot, leaving the pending origin unclaimed. | ||
| *cell.pending_origin.borrow_mut() = None; |
There was a problem hiding this comment.
Maybe we want set_pending_origin() and clear_pending_origin() to go along with set_execution_context() and clear_execution_context()? That probably cleans this up a little bit.
| args$catch.aborts <- NULL | ||
| } | ||
|
|
||
| # Try to resolve the file URI early so we can attribute plots to this |
There was a problem hiding this comment.
Since @lionel- is the author of this bit I think he should sign off on the changes in this file
Co-authored-by: Davis Vaughan <davis@rstudio.com>
2 participants
Companion to posit-dev/positron#12168; adds additional metadata to plots annotating where they originated, to make it easy to navigate to that location. See that PR for more info.
For linewise execution, this borrows from the source location infrastructure @lionel- set up already for debugging; for execution via
source(), some slightly more involved work is needed to store the source context so we can get at it later.