New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement generic view() #373

Closed
krlmlr opened this Issue Jan 20, 2018 · 13 comments

Comments

Projects
None yet
3 participants
@krlmlr
Copy link
Member

krlmlr commented Jan 20, 2018

view.default <- function(x) {
  if (interactive()) {
    View(x)
  }
  invisible(x)
}

I wonder if this function should do anything useful in non-interactive mode.

@krlmlr krlmlr referenced this issue Jan 20, 2018

Closed

Implement view #3280

krlmlr added a commit that referenced this issue Jan 20, 2018

@krlmlr

This comment has been minimized.

Copy link
Member

krlmlr commented Jan 21, 2018

If I call utils::View() from tibble, I get the "old" data viewer, even in RStudio. @hadley: Please advise.

@hadley

This comment has been minimized.

Copy link
Member

hadley commented Jan 22, 2018

As opposed to the RStudio one, you mean?

@krlmlr

This comment has been minimized.

Copy link
Member

krlmlr commented Jan 22, 2018

Yes.

@hadley

This comment has been minimized.

Copy link
Member

hadley commented Jan 22, 2018

From @kevinushey: we can get the RStudio version with get("View", envir = as.environment("package:utils")). In the long-run, there will be an exported rstudioapi function

@krlmlr

This comment has been minimized.

Copy link
Member

krlmlr commented Jan 22, 2018

Maybe rstudioapi should just implement view()? I still wonder if that generic should live even in a lower-level package, maybe rlang?

@krlmlr

This comment has been minimized.

Copy link
Member

krlmlr commented Jul 5, 2018

I'm convinced this should live in a separate package, I have drafted https://github.com/krlmlr/tidyview.

I understood the following requirements:

  • A function that acts like utils::View()
    • The title for the viewer is autodetected
  • It is easy to override the default behavior

I also added my own:

  • It is easy to turn off an override
  • It is easy to add custom behavior: for instance, an sf object could be viewed with a widget capable of displaying spatial data
    • view() isn't tied to a particular type of object, e.g. non-rectangular data should also be viewable

The "simple" solution in #428 has a few deficiencies which led to implementing tidyview:

  • Getting the label for an expression is difficult for S3 methods: r-lib/rlang#553

  • I don't like overriding by implementing a generic

    • can't easily turn off the override
    • override needs to reimplement autodetection of title

As for the other requirements, we should talk about how important they are.

@rpruim

This comment has been minimized.

Copy link

rpruim commented Jul 5, 2018

I think this is heading in a good direction, and I installed tidyview to take a quick look at the current implementation. A few questions to think about:

  • When you talk about turning off an override, do you mean doing that for individual viewers (say just for sf objects) or just returning to the default state and starting fresh?
  • Should it be easy to turn on a set of custom viewers? To save the current set? (like with options())
  • If you add several custom viewers (perhaps for some special objects, or perhaps to get non-standard behavior), should it be easy to toggle back and forth between the default and this set of customizations or will will returning to the default essentially throw away the current set of customizations?

I can envision two ways viewer customization might get used:

  • Micro: Viewers get created for new types of objects (as in your sf example) or to modify how one particular type of object is viewed.
  • Macro: A user might have several sets of viewers (each set of viewers handling the same (large?) collection of object types) that reflect different styles/verbosity/etc. and choose among them depending on the situation.

(Note: I'm not quite sure whether what I am thinking of when I say "viewer" is the same as what you mean by an "override". I'm using viewer much like the factories in your current tidyview, but not bound to a particular implementation.)

@krlmlr

This comment has been minimized.

Copy link
Member

krlmlr commented Jul 5, 2018

Thanks for your feedback.

Good point about starting afresh, that's relatively easy to add.

I'd say recording/recreating state is out of scope (and difficult too), users should rerun code that sets up their preferred viewers.

Eventually, the default view() could even be a no-op, and IDEs or users need to actively set up their viewers.

Would it be too bad to rename view_handler to viewer in the code, so that view() calls a viewer?

@rpruim

This comment has been minimized.

Copy link

rpruim commented Jul 6, 2018

I like the shortened names. In fact, in my quick test, I renamed things. (The details of my viewers are not really important -- I just needed something to give it a test drive.)

vector_viewer <- function(x) {
  if (! is.vector(x)) return (NULL)
  
  function(x, title) {
    if (interactive()) {
      get("View", envir = as.environment("package:utils"))(x, title)
    } else {
      message(title, " (", abbreviate(class(x)[1], 3), " [1:", length(x), "]): ", 
              paste(head(x, 6), collapse = ", "), if (length(x) > 6) ", ..." else "", sep = "")
    }
  }
}

data_frame_viewer <- function(x) {
  if (! inherits(x, "data.frame")) return (NULL)
  
  function(x, title) {
    if (interactive()) {
      get("View", envir = as.environment("package:utils"))(x, title)
    } else {
      message(title)
      message(class(x)[1], " ", nrow(x), " x ", ncol(x))
      message(paste(capture.output(tibble::as_tibble(x)), collapse = "\n"))
    }
  }
}

register_viewer <- function(...) {
  dots <- list(...)
  lapply(unlist(dots), register_view_handler_factory)
}

# register the viewers all at once
register_viewer(list(vector_viewer, data_frame_viewer))
# or this way
register_viewer(vector_viewer, data_frame_viewer)

I thought it would be nice if register_viewer() could take either multiple viewers or a list of viewers as input. That way a suite of viewers could be registered all at once, much like a theme in ggplot2.

I also played around a bit with cat() vs message() to produce output. Is one of these clearly the right/wrong way to go?

@krlmlr

This comment has been minimized.

Copy link
Member

krlmlr commented Jul 6, 2018

I think, the function returned by a factory should only do one thing, and not change behavior based on conditions (like interactive()).

message() prints to stderr (and in red in the RStudio IDE); if you capture/sink() only stdout, the messages will still be printed to the console.

@rpruim

This comment has been minimized.

Copy link

rpruim commented Jul 13, 2018

@krlmlr Sorry for the delay in responding -- was busy with other things.

Regarding interactive() -- will the system take care of dispatching differently based on whether one is working interactively or in an RMarkdown document, or will the user have to reregister all the viewers?

I understand the difference between cat() and message(). Just wondering where view() ought to display its output. Thinking too about message = FALSE in RMarkdown chunks as a way to suppress the output in the final document but allow it for debugging along the way.

Should this sort of conversation be moved to an issue in the tidyview repo now?

@krlmlr

This comment has been minimized.

Copy link
Member

krlmlr commented Jul 14, 2018

The factory functions will be called in turn for each call to view(). Can you change interactivity of a running R session?

Maybe we need three mute handlers -- one "really mute", one "stdout", one "stderr".

I need some more time to think about tidyview vs. a method in an existing package, let's keep the discussion here for now.

@krlmlr

This comment has been minimized.

Copy link
Member

krlmlr commented Aug 15, 2018

Let's do a non-generic view() first (which can be overridden in an .Rmd file by defining a local view() function) and think about a better solution later.

@krlmlr krlmlr closed this in #428 Aug 20, 2018

krlmlr added a commit that referenced this issue Aug 20, 2018

Merge pull request #428 from tidyverse/f-373-view
- New `view()` function that returns its input invisibly and calls `utils::View()` in interactive mode (#373).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment