Handle errors in display functions more gracefully and isolate full html pages

[jankatins]

Before, when one repr_xxx was raising an error, the complete display machinery
was impacted and no output was displayed. Now only the specific output is
omitted and a user friendly error message + stacktrace is shown on stderr and
on the console.

Also indicate that full html pages should be isolated. Full html pages should be
isolated in the frontend in an iframe. Check if the html is a complete document
(contains html) and set the right metadata.

Closes: IRkernel/IRdisplay#18
Closes: #284

[jankatins]

[The following discussion about formattable is obsolete...]

So, the main reason is that this now works:

#devtools::install_github("renkun-ken/formattable")
df <- data.frame(
  id = 1:10,
  name = c("Bob", "Ashley", "James", "David", "Jenny", 
    "Hans", "Leo", "John", "Emily", "Lee"), 
  age = c(28, 27, 30, 28, 29, 29, 27, 27, 31, 30),
  grade = c("C", "A", "A", "C", "B", "B", "B", "A", "C", "C"),
  test1_score = c(8.9, 9.5, 9.6, 8.9, 9.1, 9.3, 9.3, 9.9, 8.5, 8.6),
  test2_score = c(9.1, 9.1, 9.2, 9.1, 8.9, 8.5, 9.2, 9.3, 9.1, 8.8),
  final_score = c(9, 9.3, 9.4, 9, 9, 8.9, 9.25, 9.6, 8.8, 8.7),
  registered = c(TRUE, FALSE, TRUE, FALSE, TRUE, TRUE, TRUE, FALSE, FALSE, FALSE),
  stringsAsFactors = FALSE)
library(formattable)

f <- formattable(df, list(
  age = color_tile("white", "orange"),
  grade = formatter("span",
    style = x ~ ifelse(x == "A", style(color = "green", font.weight = "bold"), NA)),
  test1_score = normalize_bar("pink", 0.2),
  test2_score = normalize_bar("pink", 0.2),
  final_score = formatter("span",
    style = x ~ style(color = ifelse(rank(-x) <= 3, "green", "gray")),
    x ~ sprintf("%.2f (rank: %02d)", x, rank(-x))),
  registered = formatter("span", 
    style = x ~ style(color = ifelse(x, "green", "red")),
    x ~ icontext(ifelse(x, "ok", "remove"), ifelse(x, "Yes", "No")))
))
repr_html.htmlwidget <- function(x, ...){
    htmlfile <- tempfile(fileext = ".html")

    # save the widget to HTML 
    htmlwidgets::saveWidget(
     widget = x, 
     file = htmlfile,
     selfcontained = TRUE
    )

    html_string <- readChar(htmlfile, file.info(htmlfile)$size)
    return(html_string)
}

repr_html.formattable <- function(x){
    repr_html.htmlwidget(as.htmlwidget(x))
}
f

-> will show a colored table....

The problem here was that

x <- print(f)
str(x)

shows that x has something... :-(

[takluyver]

IIRC, we ran into something that hides output deliberately by making print.whatever() do nothing, because for whatever reason it couldn't use the normal visibility mechanism. We were unexpectedly displaying rich reprs in that case, whereas normally there would be no output. So we decided that if there's no plain text output, we should hide all rich reprs as well, to preserve behaviour.

[jankatins]

If that's so, we really need to change this here, so that if print(obj) returns something we do the repr dance for that returned object... But that means that we have to either remove the repr_text call and use print directly (if we can both capture the output and the return object) or add something to the repr_text.default to send back the returned object.

[jankatins]

Unfortunatelly, this does not tell what was the problem: 7d0dddc

BTW, this would again be solved if we only use repr_text per default and no repr_html... -> IRkernel/IRdisplay#18 (comment) :-)

[takluyver]

I think #127 is the relevant issue.

The object being displayed is a data table, for which it does make sense to have rich reprs, so it wouldn't be solved by not doing rich reprs for primitive types.

[flying-sheep]

CommManager also uses debug. i fixed it in a local branch so you don’t have to do anything

[flying-sheep]

hmm, the iframe has style="...;height:1px..."

why?

[flying-sheep]

so if we can’t fix that one, let’s redo the history of this branch with my fixes and then merge it in.

[jankatins]

Not sure about #127, but the 7d0dddc is the commit...

[jankatins]

I've just built this:

 handle_value <- function(obj) {
            data <- namedlist()
            metadata <- namedlist()

            # We have to check the return value of print if it is visible and use the last object
            # which is visible to compute the output from. There are a few packages which use print()
            # to convert itself to something else (and don't output anything to the console) and the
            # returned obj is the printed again.
            print_return <<- list(visible=TRUE, value=obj)
            send_even_if_no_print <- FALSE
            while(returned_obj$visible) {
                data[['text/plain']] <- paste(capture.output(print_return <<- withVisible(print(print_return$value))), collapse = '\n')
                if (returned_obj$visible) repr_even_if_no_print <- TRUE
            }
            obj <- print_return$value

            # data.table has some logic which can't use invisible() and therfore overwrites print to not 
            # return anything
            if (send_even_if_no_print || nchar(data[['text/plain']]) > 0) {
                if (getOption('jupyter.rich_display')) {
                    for (mime in getOption('jupyter.display_mimetypes')) {
                        # Use withCallingHandlers as that shows the inner stacktrace:
                        # https://stackoverflow.com/questions/15282471/get-stack-trace-on-trycatched-error-in-r
                        # the tryCatch is  still needed to prevent the error from showing
                        # up outside further up the stack :-/
                        tryCatch(withCallingHandlers({
                                r <- mime2repr[[mime]](obj)
                                if (!is.null(r)) {
                                    data[[mime]] <- r
                                    # Isolating full html pages (putting them in an iframe)
                                    if (identical(mime, 'text/html')) {
                                        if (grepl("<html.*>", r, ignore.case = TRUE)){
                                            jupyter_debug("Found full html page: %s", strtrim(r, 100))
                                            metadata[[mime]] <- list(isolated = TRUE)
                                        }
                                    }
                                }
                            }, error = handle_display_error),
                            error=function(x){})
                    }
                }
                jupyter_debug("Sending display_data: %s", paste(capture.output(str(data)), collapse = "\n"))
                send_response('display_data', request, 'iopub', list(
                    data = data,
                    metadata = metadata))
            }
        }

[flying-sheep]

dude. ````r`.

[jankatins]

@flying-sheep Just do it, you have edit right ... Also: sorry :-)

[flying-sheep]

haha np, it’s just that i did it so many times already 😉

you could cherry-pick 2bd6643 to get my changes into this PR.

[jankatins]

added my stuff and cherry picked your commit... I will do some tests here and then it would be nice if someone of you could do a last pass over the code and merge if everything looks ok.

[flying-sheep]

great! please post again once your side is ready

[jankatins]

Ok, the earlier commit was borked...

Anyway, I this now works:

• html widgets are now displayed (when adding a repr_html.htmlwidget)
• data.table are not displayed when adding a new column (unfortunately also not when it should print, eg a cell which just contains a reference to the data.table )

[flying-sheep]

code style: spaces around =, space between ) {

[jankatins]

will add a new commit for this... and s/"/'/g...

[takluyver]

data.table ... also not when it should print, eg a cell which just contains a reference to the data.table

Is there any way to avoid that? It seems likely to annoy people.

[flying-sheep]

what does “when adding a new column” mean here?

[jankatins]

library(data.table)
dat <- data.table(x1=1:10)
dat[, x2 := 1:10] # should not and does not print
dat # does not print anything but should
print(dat) # prints something

IMO the fix for the printing must go into data.table itself: it already has a workaround for knit_print

[flying-sheep]

uh, i remember something about this. someone explained that in some issue here

[jankatins]

Ok, this is still borked :-( I will add tests...

[flying-sheep]

OK, good idea!

[ericwatt]

data.table not printing after doing an assignment is referenced in this issue: Rdatatable/data.table#939

[jankatins]

Ach damn, now I see that print(formattable) doesn't even return a visible value.

At least now data.table behaves as I think it should... E.g. it prints a html table.

[jankatins]

So, using the debugger in RStudio (we really want browser() support :-/), I think I found out whats going on: print.formattable uses print internally again to print, and that triggers rstudios internal stuff: https://github.com/rstudio/rstudio/blob/374557bdaf4423c016f41b6a43ffb2e9854fc862/src/cpp/session/modules/NotebookHtmlWidgets.R#L18-L50

So IMO the solution is here to remove most of the stuff in this PR and implement a print.htmlwidgets method, which internally uses display / sends display_data directly instead of relying on the repr_xxx system in handle_value.

[flying-sheep]

hey @JanSchulz: i rebased and squashed your commits in here

then i fast-forwarded master to have the code style fixes and logging system.

so if you want to continue working on this, you shoud do:

git fetch origin
git checkout isolate_full_html
git reset --hard origin/isolate-full-html

and continue from there.

[jankatins]

Argh, the commit messages are all off :-/ (you have code style, I have commit messages :-)) Ok, will make a PR which has all the fixes for the tracebacks and then throw in a commit for a print.htmlwidgets in IRDisplay so that we handle that in a manner similar to RStudio.

[jankatins]

So, updated with only the relevant commits. This seems to work in my test notebook, but somehow travis does not kick in here?

[flying-sheep]

oh yeah, damn, should have used squash instead of fixup. sorry! but we can get the messages out of the git log of an untampered history.

[flying-sheep]

close→reopen→tests pass→merge

[jankatins]

Great!

[flying-sheep]

indeed! thank you!