vincentarelbundock / tinytable

Simple and Customizable Tables in `R`
https://vincentarelbundock.github.io/tinytable
GNU General Public License v3.0
196 stars 16 forks source link

Add positron viewer support #299

Closed kylebutts closed 2 months ago

kylebutts commented 2 months ago

getOption("viewer", utils::browseURL) works in VSCode, positron, and RStudio. Falls back to utils::browseURL if none are available

https://github.com/vincentarelbundock/tinytable/assets/19961439/fab421fb-af46-4711-80c4-3fd966ca3fa4

kylebutts commented 2 months ago

After a bit of digging, rstudioapi::viewer just calls getOption("viewer") under the hood. It has extra logic if run in a job which this doesn't work with. But that has interactive() == FALSE, so that logic is not needed.

> rstudioapi::viewer
function (url, height = NULL) 
{
    callFun("viewer", url, height = height)
}
<bytecode: 0x10edabd78>
<environment: namespace:rstudioapi>

> rstudioapi::callFun
function (fname, ...) 
{
    if (isJob()) 
        return(callRemote(sys.call(), parent.frame()))
    verifyAvailable()
    f <- tryCatch(findFun(fname, mode = "function"), error = identity)
    if (inherits(f, "error")) 
        stop("Function ", fname, " not found in RStudio", call. = FALSE)
    args <- list(...)
    if (!"..." %in% names(formals(f))) 
        if (length(args) > length(formals(f))) 
            length(args) <- length(formals(f))
    do.call(f, args)
}
<bytecode: 0x1350122b0>
<environment: namespace:rstudioapi>

> rstudioapi::findFun
function (name, version_needed = NULL, ...) 
{
    verifyAvailable(version_needed)
    if (usingTools()) 
        get(toolsName(name), toolsEnv(), ...)
    else get(name, envir = asNamespace("rstudio"), ...)
}
<bytecode: 0x13502a908>
<environment: namespace:rstudioapi>

> get(rstudioapi:::toolsName("viewer"), rstudioapi:::toolsEnv())
function (url, height = NULL) 
{
    if (!is.character(url) || (length(url) != 1)) 
        stop("url must be a single element character vector.")
    if (identical(height, "maximize")) 
        height <- -1
    if (!is.null(height) && (!is.numeric(height) || (length(height) != 
        1))) 
        stop("height must be a single element numeric vector or 'maximize'.")
    invisible(.Call("rs_viewer", url, height, PACKAGE = "(embedding)"))
}
<environment: 0x135bc0078>

> getOption("viewer")
function (url, height = NULL) 
{
    if (!is.character(url) || (length(url) != 1)) 
        stop("url must be a single element character vector.", 
            call. = FALSE)
    if (identical(height, "maximize")) 
        height <- -1
    if (!is.null(height) && (!is.numeric(height) || (length(height) != 
        1))) 
        stop("height must be a single element numeric vector or 'maximize'.", 
            call. = FALSE)
    invisible(.Call("rs_viewer", url, height, PACKAGE = "(embedding)"))
}
<environment: 0x155a09450>
vincentarelbundock commented 2 months ago

Oh wow, thanks for the investigation and the code!

So knowing this, is this PR still your recommended fix? In your view, that's what I should review and merge?

kylebutts commented 2 months ago

Yes, I think so for two reasons. 1. It seems to have become an unofficial "standard" to attach a "viewer" to options (at least in VSCode, Positron, and RStudio). 2. The only thing that rstudioapi::viewer() does in addition is stuff with background jobs (e.g. printing a table from a background job). I don't think this is desirable behavior and anyways interactive() == FALSE in background jobs, so it wouldn't plot.

vincentarelbundock commented 2 months ago

Thanks a ton for the code, I really appreciate your help with this issue. I was really dreading having to figure this out, and your solution works as expected on my machine in the terminal, vscode, and rstudio. Merged!