From 466344ae153947ca0fbcdb73848e527cc89da6a9 Mon Sep 17 00:00:00 2001 From: Teun van den Brand <49372158+teunbrand@users.noreply.github.com> Date: Mon, 7 Aug 2023 20:10:01 +0200 Subject: [PATCH] Miscellaneous improvements to guides (#5345) * Handle `labels = NULL` better * Convert `guides()` error to warning * Ignore no guides * Swap old train order * Fix `even.steps`/`show.limits` interaction * Change to soft deprecation * Fix old guide title * Fix `draw_axis()` with `NULL` labels * Default old guide title is `waiver()` * `guide_for_position` becomes method * GuideColoursteps is a named class * `guide_colourbar()` rejects discrete scales * Fix test TODO * Use `vec_slice()` to preserve attributes * Document extension points * Handle hashing in `train()` --- R/coord-cartesian-.R | 36 ++---------- R/guide-.R | 103 ++++++++++++++++++++++++++++++++--- R/guide-axis.R | 23 +++----- R/guide-bins.R | 11 ++-- R/guide-colorbar.R | 19 ++++++- R/guide-colorsteps.R | 8 +-- R/guide-legend.R | 10 +++- R/guide-old.R | 8 +-- R/guides-.R | 39 ++++++++++++- man/ggplot2-ggproto.Rd | 59 +++++++++++++++++++- tests/testthat/test-guides.R | 23 +++++--- 11 files changed, 256 insertions(+), 83 deletions(-) diff --git a/R/coord-cartesian-.R b/R/coord-cartesian-.R index 7c64ec9744..8451873b84 100644 --- a/R/coord-cartesian-.R +++ b/R/coord-cartesian-.R @@ -147,37 +147,9 @@ view_scales_from_scale <- function(scale, coord_limits = NULL, expand = TRUE) { } panel_guides_grob <- function(guides, position, theme) { - pair <- guide_for_position(guides, position) %||% - list(guide = guide_none(), params = NULL) - pair$guide$draw(theme, pair$params) -} - -guide_for_position <- function(guides, position) { - params <- guides$params - has_position <- vapply( - params, function(p) identical(p$position, position), logical(1) - ) - if (!any(has_position)) { - return(NULL) - } - - # Subset guides and parameters - guides <- guides$get_guide(has_position) - params <- params[has_position] - # Pair up guides with parameters - pairs <- Map(list, guide = guides, params = params) - - # Early exit, nothing to merge - if (length(pairs) == 1) { - return(pairs[[1]]) + if (!inherits(guides, "Guides")) { + return(zeroGrob()) } - - # TODO: There must be a smarter way to merge these - order <- order(vapply(params, function(p) as.numeric(p$order), numeric(1))) - Reduce( - function(old, new) { - old$guide$merge(old$params, new$guide, new$params) - }, - pairs[order] - ) + pair <- guides$get_position(position) + pair$guide$draw(theme, pair$params) } diff --git a/R/guide-.R b/R/guide-.R index b9e2685ff1..ae774d30c9 100644 --- a/R/guide-.R +++ b/R/guide-.R @@ -74,7 +74,79 @@ new_guide <- function(..., available_aes = "any", super) { #' To create a new type of Guide object, you typically will want to override #' one or more of the following: #' -#' TODO: Fill this in properly +#' Properties: +#' +#' - `available_aes` A `character` vector with aesthetics that this guide +#' supports. The value `"any"` indicates all non-position aesthetics. +#' +#' - `params` A named `list` of parameters that the guide needs to function. +#' It has the following roles: +#' +#' - `params` provides the defaults for a guide. +#' - `names(params)` determines what are valid arguments to `new_guide()`. +#' Some parameters are *required* to render the guide. These are: `title`, +#' `name`, `position`, `direction`, `order` and `hash`. +#' - During build stages, `params` holds information about the guide. +#' +#' - `elements` A named list of `character`s, giving the name of theme elements +#' that should be retrieved automatically, for example `"legend.text"`. +#' +#' - `hashables` An `expression` that can be evaluated in the context of +#' `params`. The hash of the evaluated expression determines the merge +#' compatibility of guides, and is stored in `params$hash`. +#' +#' Methods: +#' +#' - `extract_key()` Returns a `data.frame` with (mapped) breaks and labels +#' extracted from the scale, which will be stored in `params$key`. +#' +#' - `extract_decor()` Returns a `data.frame` containing other structured +#' information extracted from the scale, which will be stored in +#' `params$decor`. The `decor` has a guide-specific meaning: it is the bar in +#' `guide_colourbar()`, but specifies the `axis.line` in `guide_axis()`. +#' +#' - `extract_params()` Updates the `params` with other, unstructured +#' information from the scale. An example of this is inheriting the guide's +#' title from the `scale$name` field. +#' +#' - `transform()` Updates the `params$key` based on the coordinates. This +#' applies to position guides, as it rescales the aesthetic to the \[0, 1\] +#' range. +#' +#' - `merge()` Combines information from multiple guides with the same +#' `params$hash`. This ensures that e.g. `guide_legend()` can display both +#' `shape` and `colour` in the same guide. +#' +#' - `get_layer_key()` Extract information from layers. This can be used to +#' check that the guide's aesthetic is actually in use, or to gather +#' information about how legend keys should be displayed. +#' +#' - `setup_params()` Set up parameters at the beginning of drawing stages. +#' It can be used to overrule user-supplied parameters or perform checks on +#' the `params` property. +#' +#' - `override_elements()` Take populated theme elements derived from the +#' `elements` property and allows overriding these theme settings. +#' +#' - `build_title()` Render the guide's title. +#' +#' - `build_labels()` Render the guide's labels. +#' +#' - `build_decor()` Render the `params$decor`, which is different for every +#' guide. +#' +#' - `build_ticks()` Render tick marks. +#' +#' - `measure_grobs()` Measure dimensions of the graphical objects produced +#' by the `build_*()` methods to be used in the layout or assembly. +#' +#' - `arrange_layout()` Set up a layout for how graphical objects produced by +#' the `build_*()` methods should be arranged. +#' +#' - `assemble_drawing()` Take the graphical objects produced by the `build_*()` +#' methods, the measurements from `measure_grobs()` and layout from +#' `arrange_layout()` to finalise the guide. +#' #' @rdname ggplot2-ggproto #' @format NULL #' @usage NULL @@ -117,14 +189,15 @@ Guide <- ggproto( return(NULL) } params$decor <- inject(self$extract_decor(scale, !!!params)) - self$extract_params(scale, params, self$hashables, ...) + params <- self$extract_params(scale, params, ...) + # Make hash + # TODO: Maybe we only need the hash on demand during merging? + params$hash <- hash(lapply(unname(self$hashables), eval_tidy, data = params)) + params }, # Setup parameters that are only available after training - # TODO: Maybe we only need the hash on demand during merging? - extract_params = function(scale, params, hashables, ...) { - # Make hash - params$hash <- hash(lapply(unname(hashables), eval_tidy, data = params)) + extract_params = function(scale, params, ...) { params }, @@ -137,13 +210,18 @@ Guide <- ggproto( mapped <- scale$map(breaks) labels <- scale$get_labels(breaks) + # {vctrs} doesn't play nice with expressions, convert to list. + # see also https://github.com/r-lib/vctrs/issues/559 + if (is.expression(labels)) { + labels <- as.list(labels) + } key <- data_frame(mapped, .name_repair = ~ aesthetic) key$.value <- breaks key$.label <- labels if (is.numeric(breaks)) { - key[is.finite(breaks), , drop = FALSE] + vec_slice(key, is.finite(breaks)) } else { key } @@ -342,3 +420,14 @@ flip_names = c( # Shortcut for position argument matching .trbl <- c("top", "right", "bottom", "left") +# Ensure that labels aren't a list of expressions, but proper expressions +validate_labels <- function(labels) { + if (!is.list(labels)) { + return(labels) + } + if (any(vapply(labels, is.language, logical(1)))) { + do.call(expression, labels) + } else { + unlist(labels) + } +} diff --git a/R/guide-axis.R b/R/guide-axis.R index a6ce730476..eac32b2b98 100644 --- a/R/guide-axis.R +++ b/R/guide-axis.R @@ -103,9 +103,9 @@ GuideAxis <- ggproto( ticks_length = "axis.ticks.length" ), - extract_params = function(scale, params, hashables, ...) { + extract_params = function(scale, params, ...) { params$name <- paste0(params$name, "_", params$aesthetic) - Guide$extract_params(scale, params, hashables) + params }, extract_decor = function(scale, aesthetic, position, key, cap = "none", ...) { @@ -281,22 +281,14 @@ GuideAxis <- ggproto( }, build_labels = function(key, elements, params) { - labels <- key$.label + labels <- validate_labels(key$.label) n_labels <- length(labels) if (n_labels < 1) { return(list(zeroGrob())) } - pos <- key[[params$aes]] - - if (is.list(labels)) { - if (any(vapply(labels, is.language, logical(1)))) { - labels <- do.call(expression, labels) - } else { - labels <- unlist(labels) - } - } + pos <- key[[params$aes]] dodge_pos <- rep(seq_len(params$n.dodge %||% 1), length.out = n_labels) dodge_indices <- unname(split(seq_len(n_labels), dodge_pos)) @@ -432,9 +424,10 @@ draw_axis <- function(break_positions, break_labels, axis_position, theme, aes <- if (axis_position %in% c("top", "bottom")) "x" else "y" opp <- setdiff(c("x", "y"), aes) opp_value <- if (axis_position %in% c("top", "right")) 0 else 1 - key <- data_frame( - break_positions, break_positions, break_labels, - .name_repair = ~ c(aes, ".value", ".label") + key <- data_frame0( + !!aes := break_positions, + .value = break_positions, + .label = break_labels ) params$key <- key params$decor <- data_frame0( diff --git a/R/guide-bins.R b/R/guide-bins.R index bfdd9d0701..63c75bd0bd 100644 --- a/R/guide-bins.R +++ b/R/guide-bins.R @@ -266,7 +266,7 @@ GuideBins <- ggproto( return(key) }, - extract_params = function(scale, params, hashables, + extract_params = function(scale, params, title = waiver(), direction = NULL, ...) { show.limits <- params$show.limits %||% scale$show.limits %||% FALSE @@ -320,8 +320,7 @@ GuideBins <- ggproto( "not {.val {params$label.position}}." )) } - - Guide$extract_params(scale, params, hashables) + params }, setup_params = function(params) { @@ -340,7 +339,11 @@ GuideBins <- ggproto( }, build_labels = function(key, elements, params) { - key$.label[c(1, nrow(key))[!params$show.limits]] <- "" + n_labels <- length(key$.label) + if (n_labels < 1) { + return(list(labels = zeroGrob())) + } + key$.label[c(1, n_labels)[!params$show.limits]] <- "" just <- if (params$direction == "horizontal") { elements$text$vjust diff --git a/R/guide-colorbar.R b/R/guide-colorbar.R index b7ffdb9abf..092b5a35ba 100644 --- a/R/guide-colorbar.R +++ b/R/guide-colorbar.R @@ -317,6 +317,14 @@ GuideColourbar <- ggproto( theme.title = "legend.title" ), + extract_key = function(scale, aesthetic, ...) { + if (scale$is_discrete()) { + cli::cli_warn("{.fn guide_colourbar} needs continuous scales.") + return(NULL) + } + Guide$extract_key(scale, aesthetic, ...) + }, + extract_decor = function(scale, aesthetic, nbin = 300, reverse = FALSE, ...) { limits <- scale$get_limits() @@ -335,7 +343,7 @@ GuideColourbar <- ggproto( return(bar) }, - extract_params = function(scale, params, hashables, + extract_params = function(scale, params, title = waiver(), direction = "vertical", ...) { params$title <- scale$make_title( params$title %|W|% scale$name %|W|% title @@ -364,7 +372,7 @@ GuideColourbar <- ggproto( c(0.5, params$nbin - 0.5) / params$nbin, limits ) - Guide$extract_params(scale, params, hashables) + params }, merge = function(self, params, new_guide, new_params) { @@ -414,6 +422,11 @@ GuideColourbar <- ggproto( }, build_labels = function(key, elements, params) { + n_labels <- length(key$.label) + if (n_labels < 1) { + return(list(labels = zeroGrob())) + } + just <- if (params$direction == "horizontal") { elements$text$vjust } else { @@ -422,7 +435,7 @@ GuideColourbar <- ggproto( list(labels = flip_element_grob( elements$text, - label = key$.label, + label = validate_labels(key$.label), x = unit(key$.value, "npc"), y = rep(just, nrow(key)), margin_x = FALSE, diff --git a/R/guide-colorsteps.R b/R/guide-colorsteps.R index d964e1d058..e82715c543 100644 --- a/R/guide-colorsteps.R +++ b/R/guide-colorsteps.R @@ -74,7 +74,7 @@ guide_colorsteps <- guide_coloursteps #' @usage NULL #' @export GuideColoursteps <- ggproto( - NULL, GuideColourbar, + "GuideColoursteps", GuideColourbar, params = c( list(even.steps = TRUE, show.limits = NULL), @@ -135,7 +135,7 @@ GuideColoursteps <- ggproto( return(bar) }, - extract_params = function(scale, params, hashables, ...) { + extract_params = function(scale, params, ...) { if (params$even.steps) { params$nbin <- nbin <- sum(!is.na(params$key[[1]])) + 1 @@ -164,7 +164,7 @@ GuideColoursteps <- ggproto( from = c(0.5, nbin - 0.5) / nbin ) key <- params$key - limits <- attr(key, "limits", TRUE) + limits <- attr(key, "limits", TRUE) %||% scale$get_limits() key <- key[c(NA, seq_len(nrow(key)), NA), , drop = FALSE] key$.value[c(1, nrow(key))] <- edges key$.label[c(1, nrow(key))] <- scale$get_labels(limits) @@ -177,6 +177,6 @@ GuideColoursteps <- ggproto( params$key <- key } - GuideColourbar$extract_params(scale, params, hashables, ...) + GuideColourbar$extract_params(scale, params, ...) } ) diff --git a/R/guide-legend.R b/R/guide-legend.R index 2ac35f05b8..b80062d618 100644 --- a/R/guide-legend.R +++ b/R/guide-legend.R @@ -259,7 +259,7 @@ GuideLegend <- ggproto( theme.title = "legend.title" ), - extract_params = function(scale, params, hashables, + extract_params = function(scale, params, title = waiver(), direction = NULL, ...) { params$title <- scale$make_title( params$title %|W|% scale$name %|W|% title @@ -271,8 +271,7 @@ GuideLegend <- ggproto( if (isTRUE(params$reverse %||% FALSE)) { params$key <- params$key[nrow(params$key):1, , drop = FALSE] } - - Guide$extract_params(scale, params, hashables) + params }, merge = function(self, params, new_guide, new_params) { @@ -476,6 +475,11 @@ GuideLegend <- ggproto( }, build_labels = function(key, elements, params) { + n_labels <- length(key$.label) + if (n_labels < 1) { + out <- rep(list(zeroGrob()), nrow(key)) + return(out) + } lapply(key$.label, function(lab) { ggname( "guide.label", diff --git a/R/guide-old.R b/R/guide-old.R index 735dc96f21..2320b0bbf2 100644 --- a/R/guide-old.R +++ b/R/guide-old.R @@ -65,7 +65,7 @@ guide_gengrob.default <- guide_train.default #' @export #' @rdname old_guide old_guide <- function(guide) { - deprecate_warn0( + deprecate_soft0( when = "3.5.0", what = I("The S3 guide system"), details = c( @@ -88,10 +88,10 @@ GuideOld <- ggproto( "GuideOld", Guide, train = function(self, params, scale, aesthetic = NULL, - title = NULL, direction = NULL) { - params <- guide_train(params, scale, aesthetic) - params$title <- params$title %|W|% title + title = waiver(), direction = NULL) { + params$title <- scale$make_title(params$title %|W|% scale$name %|W|% title) params$direction <- params$direction %||% direction + params <- guide_train(params, scale, aesthetic) params }, diff --git a/R/guides-.R b/R/guides-.R index f8273d07e6..163559b99c 100644 --- a/R/guides-.R +++ b/R/guides-.R @@ -84,7 +84,12 @@ guides <- function(...) { return(guides_list(guides = args)) } - # Raise error about unnamed guides + # If there are no guides, do nothing + if (length(args) == 0) { + return(NULL) + } + + # Raise warning about unnamed guides nms <- names(args) if (is.null(nms)) { msg <- "All guides are unnamed." @@ -97,10 +102,11 @@ guides <- function(...) { msg <- "The {.and {unnamed}} guide{?s} {?is/are} unnamed." } } - cli::cli_abort(c( + cli::cli_warn(c( "Guides provided to {.fun guides} must be named.", i = msg )) + NULL } update_guides <- function(p, guides) { @@ -213,6 +219,35 @@ Guides <- ggproto( } }, + get_position = function(self, position) { + check_string("position") + + guide_positions <- lapply(self$params, `[[`, "position") + idx <- which(vapply(guide_positions, identical, logical(1), y = position)) + + if (length(idx) < 1) { + # No guide found for position, return missing (guide_none) guide + return(list(guide = self$missing, params = self$missing$params)) + } + if (length(idx) == 1) { + # Happy path when nothing needs to merge + return(list(guide = self$guides[[idx]], params = self$params[[idx]])) + } + + # Pair up guides and parameters + params <- self$params[idx] + pairs <- Map(list, guide = self$guides[idx], params = params) + + # Merge pairs sequentially + order <- order(vapply(params, function(p) as.numeric(p$order), numeric(1))) + Reduce( + function(old, new) { + old$guide$merge(old$params, new$guide, new$params) + }, + pairs[order] + ) + }, + ## Building ------------------------------------------------------------------ # The `Guides$build()` method is called in ggplotGrob (plot-build.R) and makes diff --git a/man/ggplot2-ggproto.Rd b/man/ggplot2-ggproto.Rd index a74d8877e8..37a042dd68 100644 --- a/man/ggplot2-ggproto.Rd +++ b/man/ggplot2-ggproto.Rd @@ -384,7 +384,64 @@ top-level \code{Guide}, and each implements their own methods for drawing. To create a new type of Guide object, you typically will want to override one or more of the following: -TODO: Fill this in properly +Properties: +\itemize{ +\item \code{available_aes} A \code{character} vector with aesthetics that this guide +supports. The value \code{"any"} indicates all non-position aesthetics. +\item \code{params} A named \code{list} of parameters that the guide needs to function. +It has the following roles: +\itemize{ +\item \code{params} provides the defaults for a guide. +\item \code{names(params)} determines what are valid arguments to \code{new_guide()}. +Some parameters are \emph{required} to render the guide. These are: \code{title}, +\code{name}, \code{position}, \code{direction}, \code{order} and \code{hash}. +\item During build stages, \code{params} holds information about the guide. +} +\item \code{elements} A named list of \code{character}s, giving the name of theme elements +that should be retrieved automatically, for example \code{"legend.text"}. +\item \code{hashables} An \code{expression} that can be evaluated in the context of +\code{params}. The hash of the evaluated expression determines the merge +compatibility of guides, and is stored in \code{params$hash}. +} + +Methods: +\itemize{ +\item \code{extract_key()} Returns a \code{data.frame} with (mapped) breaks and labels +extracted from the scale, which will be stored in \code{params$key}. +\item \code{extract_decor()} Returns a \code{data.frame} containing other structured +information extracted from the scale, which will be stored in +\code{params$decor}. The \code{decor} has a guide-specific meaning: it is the bar in +\code{guide_colourbar()}, but specifies the \code{axis.line} in \code{guide_axis()}. +\item \code{extract_params()} Updates the \code{params} with other, unstructured +information from the scale. An example of this is inheriting the guide's +title from the \code{scale$name} field. +\item \code{transform()} Updates the \code{params$key} based on the coordinates. This +applies to position guides, as it rescales the aesthetic to the [0, 1] +range. +\item \code{merge()} Combines information from multiple guides with the same +\code{params$hash}. This ensures that e.g. \code{guide_legend()} can display both +\code{shape} and \code{colour} in the same guide. +\item \code{get_layer_key()} Extract information from layers. This can be used to +check that the guide's aesthetic is actually in use, or to gather +information about how legend keys should be displayed. +\item \code{setup_params()} Set up parameters at the beginning of drawing stages. +It can be used to overrule user-supplied parameters or perform checks on +the \code{params} property. +\item \code{override_elements()} Take populated theme elements derived from the +\code{elements} property and allows overriding these theme settings. +\item \code{build_title()} Render the guide's title. +\item \code{build_labels()} Render the guide's labels. +\item \code{build_decor()} Render the \code{params$decor}, which is different for every +guide. +\item \code{build_ticks()} Render tick marks. +\item \code{measure_grobs()} Measure dimensions of the graphical objects produced +by the \verb{build_*()} methods to be used in the layout or assembly. +\item \code{arrange_layout()} Set up a layout for how graphical objects produced by +the \verb{build_*()} methods should be arranged. +\item \code{assemble_drawing()} Take the graphical objects produced by the \verb{build_*()} +methods, the measurements from \code{measure_grobs()} and layout from +\code{arrange_layout()} to finalise the guide. +} } \section{Positions}{ diff --git a/tests/testthat/test-guides.R b/tests/testthat/test-guides.R index 4ef7174f99..ce111a2fcc 100644 --- a/tests/testthat/test-guides.R +++ b/tests/testthat/test-guides.R @@ -128,11 +128,7 @@ test_that("a warning is generated when more than one position guide is drawn at ) built <- expect_silent(ggplot_build(plot)) - # TODO: These multiple warnings should be summarized nicely. Until this gets - # fixed, this test ignores all the following errors than the first one. - suppressWarnings( - expect_warning(ggplot_gtable(built), "Discarding guide") - ) + expect_warning(ggplot_gtable(built), "Discarding guide") }) test_that("a warning is not generated when properly changing the position of a guide_axis()", { @@ -311,6 +307,16 @@ test_that("guide_coloursteps and guide_bins return ordered breaks", { expect_true(all(diff(key$.value) < 0)) }) +test_that("guide_colourbar warns about discrete scales", { + + g <- guide_colourbar() + s <- scale_colour_discrete() + s$train(LETTERS[1:3]) + + expect_warning(g <- g$train(g$params, s, "colour"), "needs continuous scales") + expect_null(g) +}) + # Visual tests ------------------------------------------------------------ test_that("axis guides are drawn correctly", { @@ -760,15 +766,16 @@ test_that("a warning is generated when guides( = FALSE) is specified", { expect_snapshot_warning(ggplot_gtable(built)) }) -test_that("guides() errors if unnamed guides are provided", { - expect_error( +test_that("guides() warns if unnamed guides are provided", { + expect_warning( guides("axis"), "All guides are unnamed." ) - expect_error( + expect_warning( guides(x = "axis", "axis"), "The 2nd guide is unnamed" ) + expect_null(guides()) }) test_that("old S3 guides can be implemented", {