From 25d5532ba36001b82ea341c48e477a2dcd90f10a Mon Sep 17 00:00:00 2001 From: Teun van den Brand <49372158+teunbrand@users.noreply.github.com> Date: Tue, 7 Nov 2023 17:27:12 +0100 Subject: [PATCH 01/19] remove element mechanism from guide constructor --- R/guide-.R | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/R/guide-.R b/R/guide-.R index bdf360db8e..c722bbaf1d 100644 --- a/R/guide-.R +++ b/R/guide-.R @@ -25,13 +25,8 @@ new_guide <- function(..., available_aes = "any", super) { params <- intersect(names(args), param_names) params <- defaults(args[params], super$params) - # Set elements - elems_names <- names(super$elements) - elems <- intersect(names(args), elems_names) - elems <- defaults(args[elems], super$elements) - # Warn about extra arguments - extra_args <- setdiff(names(args), union(param_names, elems_names)) + extra_args <- setdiff(names(args), param_names) if (length(extra_args) > 0) { cli::cli_warn(paste0( "Ignoring unknown {cli::qty(extra_args)} argument{?s} to ", @@ -56,8 +51,7 @@ new_guide <- function(..., available_aes = "any", super) { ggproto( NULL, super, - params = params, - elements = elems, + params = params, available_aes = available_aes ) } From 8be1ab5e8007885f5a8bbba9f8095d69d04fb53e Mon Sep 17 00:00:00 2001 From: Teun van den Brand <49372158+teunbrand@users.noreply.github.com> Date: Tue, 7 Nov 2023 17:29:18 +0100 Subject: [PATCH 02/19] cleanup legend code --- R/guide-legend.R | 107 +++++++++++++++++++---------------------------- 1 file changed, 44 insertions(+), 63 deletions(-) diff --git a/R/guide-legend.R b/R/guide-legend.R index 341bee47c8..dd0315336d 100644 --- a/R/guide-legend.R +++ b/R/guide-legend.R @@ -168,25 +168,32 @@ guide_legend <- function( if (!is.null(label.position)) { label.position <- arg_match0(label.position, .trbl) } + label.theme <- if (!isFALSE(label)) label.theme else element_blank() + + internal_theme <- theme( + legend.text = combine_elements( + label.theme, + element_text(hjust = label.hjust, vjust = label.vjust, inherit.blank = TRUE) + ), + legend.title = combine_elements( + title.theme, + element_text(hjust = title.hjust, vjust = title.vjust, inherit.blank = TRUE) + ), + legend.key.width = keywidth, + legend.key.height = keyheight, + legend.direction = direction + ) new_guide( # Title title = title, title.position = title.position, - title.theme = title.theme, - title.hjust = title.hjust, - title.vjust = title.vjust, # Label - label = label, label.position = label.position, - label.theme = label.theme, - label.hjust = label.hjust, - label.vjust = label.vjust, - # Key size - keywidth = keywidth, - keyheight = keyheight, + # Theme + internal_theme = internal_theme, # General direction = direction, @@ -214,21 +221,12 @@ GuideLegend <- ggproto( params = list( title = waiver(), title.position = NULL, - title.theme = NULL, - title.hjust = NULL, - title.vjust = NULL, - label = TRUE, label.position = NULL, - label.theme = NULL, - label.hjust = NULL, - label.vjust = NULL, - keywidth = NULL, - keyheight = NULL, + internal_theme = NULL, # General - direction = NULL, override.aes = list(), nrow = NULL, ncol = NULL, @@ -256,7 +254,7 @@ GuideLegend <- ggproto( key.height = "legend.key.height", key.width = "legend.key.width", text = "legend.text", - theme.title = "legend.title" + title = "legend.title" ), extract_params = function(scale, params, @@ -390,50 +388,25 @@ GuideLegend <- ggproto( params }, - override_elements = function(params, elements, theme) { + setup_elements = function(params, elements, theme) { + default_just <- legend_label_just(params$label.position) - # Title - title <- combine_elements(params$title.theme, elements$theme.title) - title$hjust <- params$title.hjust %||% title$hjust %||% 0 - title$vjust <- params$title.vjust %||% title$vjust %||% 0.5 - elements$title <- title - - # Labels - if (!is.null(elements$text)) { - label <- combine_elements(params$label.theme, elements$text) - if (!params$label || is.null(params$key$.label)) { - label <- element_blank() - } else { - hjust <- unname(label_hjust_defaults[params$label.position]) - vjust <- unname(label_vjust_defaults[params$label.position]) - # Expressions default to right-justified - if (hjust == 0 && any(is.expression(params$key$.label))) { - hjust <- 1 - } - # Breaking justification inheritance for intuition purposes. - if (is.null(params$label.theme$hjust) && - is.null(theme$legend.text$hjust)) { - label$hjust <- NULL - } - if (is.null(params$label.theme$vjust) && - is.null(theme$legend.text$vjust)) { - label$vjust <- NULL - } - label$hjust <- params$label.hjust %||% label$hjust %||% hjust - label$vjust <- params$label.vjust %||% label$vjust %||% vjust - } - elements$text <- label - } + # We break inheritance of text justification for intuition purposes + theme$legend.text$hjust <- theme$legend.text$hjust %||% default_just[1] + theme$legend.text$vjust <- theme$legend.text$vjust %||% default_just[2] - # Keys - if (any(c("key.width", "key.height") %in% names(elements))) { - elements$key.width <- width_cm( params$keywidth %||% elements$key.width) - elements$key.height <- height_cm(params$keyheight %||% elements$key.height) - } + theme <- theme + compact(params$internal_theme) + + Guide$setup_elements(params, elements, theme) + }, + + override_elements = function(params, elements, theme) { + + elements$key.width <- width_cm(elements$key.width) + elements$key.height <- height_cm(elements$key.height) # Spacing - gap <- title$size %||% elements$theme.title$size %||% - elements$text$size %||% 11 + gap <- elements$title$size %||% elements$text$size %||% 11 gap <- unit(gap * 0.5, "pt") # Should maybe be elements$spacing.{x/y} instead of the theme's spacing? elements$hgap <- width_cm( theme$legend.spacing.x %||% gap) @@ -725,8 +698,16 @@ GuideLegend <- ggproto( } ) -label_hjust_defaults <- c(top = 0.5, bottom = 0.5, left = 1, right = 0) -label_vjust_defaults <- c(top = 0, bottom = 1, left = 0.5, right = 0.5) +legend_label_just <- function(position) { + default_just <- switch( + position, + top = c(0.5, 0), + bottom = c(0.5, 0), + left = c(1, 0.5), + right = c(0, 0.5), + arg_match0(position, .trbl, arg = "label.position") + ) +} measure_legend_keys <- function(decor, n, dim, byrow = FALSE, default_width = 1, default_height = 1) { From aa15ae3e09c07824a511dbfbbecb20830eff1f03 Mon Sep 17 00:00:00 2001 From: Teun van den Brand <49372158+teunbrand@users.noreply.github.com> Date: Tue, 7 Nov 2023 17:36:14 +0100 Subject: [PATCH 03/19] cleanup colourbar code --- R/guide-colorbar.R | 71 +++++++++++++++++++++++----------------------- 1 file changed, 35 insertions(+), 36 deletions(-) diff --git a/R/guide-colorbar.R b/R/guide-colorbar.R index 7e71eaba0c..690fe4ed01 100644 --- a/R/guide-colorbar.R +++ b/R/guide-colorbar.R @@ -205,6 +205,25 @@ guide_colourbar <- function( ticks$linewidth <- ticks.linewidth %||% ticks$linewidth %||% (0.5 / .pt) } + label.theme <- if (!isFALSE(label)) label.theme else element_blank() + + internal_theme <- theme( + legend.text = combine_elements( + label.theme, + element_text(hjust = label.hjust, vjust = label.vjust) + ), + legend.title = combine_elements( + title.theme, + element_text(hjust = title.hjust, vjust = title.vjust) + ), + legend.key.width = barwidth, + legend.key.height = barheight, + legend.direction = direction, + frame = frame, + ticks = ticks, + ticks.length = ticks.length + ) + # Trick to re-use this constructor in `guide_coloursteps()`. args <- list2(...) super <- args$super %||% GuideColourbar @@ -214,29 +233,14 @@ guide_colourbar <- function( # title title = title, title.position = title.position, - title.theme = title.theme, - title.hjust = title.hjust, - title.vjust = title.vjust, - - # label - label = label, label.position = label.position, - label.theme = label.theme, - label.hjust = label.hjust, - label.vjust = label.vjust, + + # theme + internal_theme = internal_theme, # bar - keywidth = barwidth, - keyheight = barheight, nbin = nbin, raster = raster, - - # frame - frame = frame, - - # ticks - ticks = ticks, - ticks_length = ticks.length, draw_lim = c(isTRUE(draw.llim), isTRUE(draw.ulim)), # general @@ -267,20 +271,10 @@ GuideColourbar <- ggproto( # title title = waiver(), title.position = NULL, - title.theme = NULL, - title.hjust = NULL, - title.vjust = NULL, - - # label - label = TRUE, label.position = NULL, - label.theme = NULL, - label.hjust = NULL, - label.vjust = NULL, # bar - keywidth = NULL, - keyheight = NULL, + internal_theme = NULL, nbin = 300, raster = TRUE, @@ -314,7 +308,7 @@ GuideColourbar <- ggproto( key.height = "legend.key.height", key.width = "legend.key.width", text = "legend.text", - theme.title = "legend.title" + title = "legend.title" ), extract_key = function(scale, aesthetic, ...) { @@ -395,15 +389,20 @@ GuideColourbar <- ggproto( params }, - override_elements = function(params, elements, theme) { - # These key sizes are the defaults, the GuideLegend method may overrule this + setup_elements = function(params, elements, theme) { + # Key sizes are already calculated before `Guides$draw()` if (params$direction == "horizontal") { - elements$key.width <- elements$key.width * 5 + theme$legend.key.width <- theme$legend.key.width * 5 } else { - elements$key.height <- elements$key.height * 5 + theme$legend.key.height <- theme$legend.key.height * 5 } - elements$ticks <- combine_elements(elements$ticks, theme$line) - elements$frame <- combine_elements(elements$frame, theme$rect) + GuideLegend$setup_elements(params, elements, theme) + }, + + override_elements = function(params, elements, theme) { + itheme <- params$internal_theme + elements$ticks <- combine_elements(itheme$ticks, elements$ticks) + elements$frame <- combine_elements(itheme$frame, elements$frame) GuideLegend$override_elements(params, elements, theme) }, From d363f10387b02c7525b8a4089016159beb2731d7 Mon Sep 17 00:00:00 2001 From: Teun van den Brand <49372158+teunbrand@users.noreply.github.com> Date: Tue, 7 Nov 2023 17:40:43 +0100 Subject: [PATCH 04/19] cleanup guide_bins code --- R/guide-bins.R | 68 +++++++++++++++++++++----------------------------- 1 file changed, 28 insertions(+), 40 deletions(-) diff --git a/R/guide-bins.R b/R/guide-bins.R index f20adee759..fb7cea567d 100644 --- a/R/guide-bins.R +++ b/R/guide-bins.R @@ -144,29 +144,33 @@ guide_bins <- function( ticks$arrow <- NULL } + label.theme <- if (!isFALSE(label)) label.theme else element_blank() + + internal_theme <- theme( + legend.text = combine_elements( + label.theme, + element_text(hjust = label.hjust, vjust = label.vjust) + ), + legend.title = combine_elements( + title.theme, + element_text(hjust = title.hjust, vjust = title.vjust) + ), + legend.key.width = keywidth, + legend.key.height = keyheight, + legend.direction = direction, + axis_line = axis, + ticks = ticks, + ticks_length = ticks.length + ) + new_guide( # title title = title, title.position = title.position, - title.theme = title.theme, - title.hjust = title.hjust, - title.vjust = title.vjust, - - # label - label = label, label.position = label.position, - label.theme = label.theme, - label.hjust = label.hjust, - label.vjust = label.vjust, - - # key - keywidth = keywidth, - keyheight = keyheight, - # ticks - line = axis, - ticks = ticks, - ticks_length = ticks.length, + # theme + internal_theme = internal_theme, # general direction = direction, @@ -193,20 +197,11 @@ GuideBins <- ggproto( params = list( title = waiver(), title.position = NULL, - title.theme = NULL, - title.hjust = NULL, - title.vjust = NULL, - - label = TRUE, label.position = NULL, - label.theme = NULL, - label.hjust = NULL, - label.vjust = NULL, - keywidth = NULL, - keyheight = NULL, + internal_theme = NULL, - direction = NULL, + # direction = NULL, override.aes = list(), reverse = FALSE, order = 0, @@ -221,8 +216,8 @@ GuideBins <- ggproto( elements = c( GuideLegend$elements, list( - line = "line", - ticks = "line", + axis_line = "line", + ticks = "line", ticks_length = unit(0.2, "npc") ) ), @@ -329,8 +324,9 @@ GuideBins <- ggproto( }, override_elements = function(params, elements, theme) { - elements$ticks <- combine_elements(elements$ticks, theme$line) - elements$line <- combine_elements(elements$line, theme$line) + itheme <- params$internal_theme + elements$ticks <- combine_elements(itheme$ticks, elements$ticks) + elements$line <- combine_elements(itheme$axis_line, elements$axis_line) GuideLegend$override_elements(params, elements, theme) }, @@ -341,13 +337,6 @@ GuideBins <- ggproto( } key$.label[c(1, n_labels)[!params$show.limits]] <- "" - just <- switch( - params$direction, - horizontal = elements$text$vjust, - vertical = elements$text$hjust, - 0.5 - ) - if (params$direction == "vertical") { key$.value <- 1 - key$.value } @@ -356,7 +345,6 @@ GuideBins <- ggproto( elements$text, label = key$.label, x = unit(key$.value, "npc"), - y = rep(just, nrow(key)), margin_x = FALSE, margin_y = TRUE, flip = params$direction == "vertical" From d2a99772c885fc85b3468a40cc8625ce0fd89485 Mon Sep 17 00:00:00 2001 From: Teun van den Brand <49372158+teunbrand@users.noreply.github.com> Date: Wed, 8 Nov 2023 10:02:35 +0100 Subject: [PATCH 05/19] utility function for common pattern --- R/utilities-grid.R | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/R/utilities-grid.R b/R/utilities-grid.R index 389dad3eea..72ccfddf33 100644 --- a/R/utilities-grid.R +++ b/R/utilities-grid.R @@ -35,3 +35,10 @@ height_cm <- function(x) { cli::cli_abort("Don't know how to get height of {.cls {class(x)}} object") } } + +set_default_unit <- function(x, default.unit) { + if (is.null(x) || is.unit(x)) { + return(x) + } + unit(x, default.unit) +} From c46fb12115c6cc1096a7e2b650e16c9cae81fee6 Mon Sep 17 00:00:00 2001 From: Teun van den Brand <49372158+teunbrand@users.noreply.github.com> Date: Wed, 8 Nov 2023 10:32:46 +0100 Subject: [PATCH 06/19] Add logic for merging themes --- R/guide-.R | 1 + R/guide-legend.R | 4 +--- R/theme.R | 22 ++++++++++++++++++++++ 3 files changed, 24 insertions(+), 3 deletions(-) diff --git a/R/guide-.R b/R/guide-.R index c722bbaf1d..76b975e17b 100644 --- a/R/guide-.R +++ b/R/guide-.R @@ -268,6 +268,7 @@ Guide <- ggproto( # Converts the `elements` field to proper elements to be accepted by # `element_grob()`. String-interpolates aesthetic/position dependent elements. setup_elements = function(params, elements, theme) { + theme <- add_theme_preserve_blank(theme, params$internal_theme) is_char <- vapply(elements, is.character, logical(1)) elements[is_char] <- lapply(elements[is_char], calc_element, theme = theme) elements diff --git a/R/guide-legend.R b/R/guide-legend.R index dd0315336d..79dd80743d 100644 --- a/R/guide-legend.R +++ b/R/guide-legend.R @@ -389,14 +389,12 @@ GuideLegend <- ggproto( }, setup_elements = function(params, elements, theme) { - default_just <- legend_label_just(params$label.position) # We break inheritance of text justification for intuition purposes + default_just <- legend_label_just(params$label.position) theme$legend.text$hjust <- theme$legend.text$hjust %||% default_just[1] theme$legend.text$vjust <- theme$legend.text$vjust %||% default_just[2] - theme <- theme + compact(params$internal_theme) - Guide$setup_elements(params, elements, theme) }, diff --git a/R/theme.R b/R/theme.R index e07fd29214..32ecc1edb2 100644 --- a/R/theme.R +++ b/R/theme.R @@ -560,6 +560,28 @@ add_theme <- function(t1, t2, t2name, call = caller_env()) { t1 } +# Calls `add_theme()`, but drops the elements of the new theme that inherit +# blank and that are blank in the old theme. +add_theme_preserve_blank <- function(old, new, new_name = caller_arg(new)) { + if (is.null(new)) { + return(old) + } + # Get non empty names of new theme + nms <- names(new)[!vapply(new, is.null, logical(1))] + + # Does any of the new theme elements carry over blank elements? + inherit_blank <- vapply( + new[nms], FUN.VALUE = logical(1), + function(x) is.list(x) && isTRUE(x$inherit.blank) + ) + # Are their equivalents in the old theme blank? + is_blank <- vapply(old[nms], inherits, logical(1), what = "element_blank") + + # Only merge in elements that shouldn't become blank + new[] <- new[!(inherit_blank & is_blank)] + add_theme(old, new, t2name = new_name) +} + #' Calculate the element properties, by inheriting properties from its parents #' From 53da3f625cf69b15e874595e1270a942d3abe6a9 Mon Sep 17 00:00:00 2001 From: Teun van den Brand <49372158+teunbrand@users.noreply.github.com> Date: Wed, 8 Nov 2023 10:37:07 +0100 Subject: [PATCH 07/19] More cleanup --- R/guide-bins.R | 47 ++++++++++++++++++---------------------------- R/guide-colorbar.R | 39 +++++++++++++++----------------------- R/guide-legend.R | 31 ++++++++++++------------------ 3 files changed, 45 insertions(+), 72 deletions(-) diff --git a/R/guide-bins.R b/R/guide-bins.R index fb7cea567d..71a938cf8e 100644 --- a/R/guide-bins.R +++ b/R/guide-bins.R @@ -108,16 +108,6 @@ guide_bins <- function( show.limits = NULL, ... ) { - - if (!(is.null(keywidth) || is.unit(keywidth))) { - keywidth <- unit(keywidth, default.unit) - } - if (!(is.null(keyheight) || is.unit(keyheight))) { - keyheight <- unit(keyheight, default.unit) - } - if (!is.unit(ticks.length)) { - ticks.length <- unit(ticks.length, default.unit) - } if (!is.null(title.position)) { title.position <- arg_match0(title.position, .trbl) } @@ -127,13 +117,12 @@ guide_bins <- function( if (!is.null(label.position)) { label.position <- arg_match0(label.position, .trbl) } - if (is.logical(axis)) { - axis <- if (axis) element_line() else element_rect() + axis <- if (!isFALSE(axis)) element_line() else element_blank() } if (inherits(axis, "element_line")) { - axis$colour <- axis.colour %||% axis$colour %||% "black" - axis$linewidth <- axis.linewidth %||% axis$linewidth %||% (0.5 / .pt) + axis$colour <- axis.colour %||% axis$colour %||% "black" + axis$linewidth <- axis.linewidth %||% axis$linewidth %||% (0.5 / .pt) axis$arrow <- axis.arrow %||% axis$arrow } else { axis <- element_blank() @@ -145,22 +134,23 @@ guide_bins <- function( } label.theme <- if (!isFALSE(label)) label.theme else element_blank() + label.theme <- combine_elements( + label.theme, + element_text(hjust = label.hjust, vjust = label.vjust, inherit.blank = TRUE) + ) + title.theme <- combine_elements( + title.theme, + element_text(hjust = title.hjust, vjust = title.vjust, inherit.blank = TRUE) + ) internal_theme <- theme( - legend.text = combine_elements( - label.theme, - element_text(hjust = label.hjust, vjust = label.vjust) - ), - legend.title = combine_elements( - title.theme, - element_text(hjust = title.hjust, vjust = title.vjust) - ), - legend.key.width = keywidth, - legend.key.height = keyheight, - legend.direction = direction, - axis_line = axis, - ticks = ticks, - ticks_length = ticks.length + legend.text = label.theme, + legend.title = title.theme, + legend.key.width = set_default_unit(keywidth, default.unit), + legend.key.height = set_default_unit(keyheight, default.unit), + axis_line = axis, + ticks = ticks, + ticks_length = set_default_unit(ticks.length, default.unit) ) new_guide( @@ -198,7 +188,6 @@ GuideBins <- ggproto( title = waiver(), title.position = NULL, label.position = NULL, - internal_theme = NULL, # direction = NULL, diff --git a/R/guide-colorbar.R b/R/guide-colorbar.R index 690fe4ed01..8c634703e1 100644 --- a/R/guide-colorbar.R +++ b/R/guide-colorbar.R @@ -158,16 +158,6 @@ guide_colourbar <- function( available_aes = c("colour", "color", "fill"), ... ) { - if (!(is.null(barwidth) || is.unit(barwidth))) { - barwidth <- unit(barwidth, default.unit) - } - if (!(is.null(barheight) || is.unit(barheight))) { - barheight <- unit(barheight, default.unit) - } - if (!is.unit(ticks.length)) { - ticks.length <- unit(ticks.length, default.unit) - } - if (!is.null(title.position)) { title.position <- arg_match0(title.position, .trbl) } @@ -206,22 +196,23 @@ guide_colourbar <- function( } label.theme <- if (!isFALSE(label)) label.theme else element_blank() + label.theme <- combine_elements( + label.theme, + element_text(hjust = label.hjust, vjust = label.vjust) + ) + title.theme <- combine_elements( + title.theme, + element_text(hjust = title.hjust, vjust = title.vjust) + ) internal_theme <- theme( - legend.text = combine_elements( - label.theme, - element_text(hjust = label.hjust, vjust = label.vjust) - ), - legend.title = combine_elements( - title.theme, - element_text(hjust = title.hjust, vjust = title.vjust) - ), - legend.key.width = barwidth, - legend.key.height = barheight, - legend.direction = direction, - frame = frame, - ticks = ticks, - ticks.length = ticks.length + legend.text = label.theme, + legend.title = title.theme, + legend.key.width = set_default_unit(barwidth, default.unit), + legend.key.height = set_default_unit(barheight, default.unit), + frame = frame, + ticks = ticks, + ticks.length = set_default_unit(ticks.length, default.unit) ) # Trick to re-use this constructor in `guide_coloursteps()`. diff --git a/R/guide-legend.R b/R/guide-legend.R index 79dd80743d..7ea9c7efee 100644 --- a/R/guide-legend.R +++ b/R/guide-legend.R @@ -156,12 +156,6 @@ guide_legend <- function( ... ) { # Resolve key sizes - if (!inherits(keywidth, c("NULL", "unit"))) { - keywidth <- unit(keywidth, default.unit) - } - if (!inherits(keyheight, c("NULL", "unit"))) { - keyheight <- unit(keyheight, default.unit) - } if (!is.null(title.position)) { title.position <- arg_match0(title.position, .trbl) } @@ -169,19 +163,20 @@ guide_legend <- function( label.position <- arg_match0(label.position, .trbl) } label.theme <- if (!isFALSE(label)) label.theme else element_blank() + label.theme <- combine_elements( + label.theme, + element_text(hjust = label.hjust, vjust = label.vjust, inherit.blank = TRUE) + ) + title.theme <- combine_elements( + title.theme, + element_text(hjust = title.hjust, vjust = label.vjust, inherit.blank = TRUE) + ) internal_theme <- theme( - legend.text = combine_elements( - label.theme, - element_text(hjust = label.hjust, vjust = label.vjust, inherit.blank = TRUE) - ), - legend.title = combine_elements( - title.theme, - element_text(hjust = title.hjust, vjust = title.vjust, inherit.blank = TRUE) - ), - legend.key.width = keywidth, - legend.key.height = keyheight, - legend.direction = direction + legend.text = label.theme, + legend.title = title.theme, + legend.key.width = set_default_unit(keywidth, default.unit), + legend.key.height = set_default_unit(keyheight, default.unit), ) new_guide( @@ -221,9 +216,7 @@ GuideLegend <- ggproto( params = list( title = waiver(), title.position = NULL, - label.position = NULL, - internal_theme = NULL, # General From c11b594bca789d48f8f609f0f6e9872c188e748c Mon Sep 17 00:00:00 2001 From: Teun van den Brand <49372158+teunbrand@users.noreply.github.com> Date: Wed, 8 Nov 2023 10:44:26 +0100 Subject: [PATCH 08/19] Fix bug --- R/theme.R | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/R/theme.R b/R/theme.R index 32ecc1edb2..54d7f69a29 100644 --- a/R/theme.R +++ b/R/theme.R @@ -578,8 +578,8 @@ add_theme_preserve_blank <- function(old, new, new_name = caller_arg(new)) { is_blank <- vapply(old[nms], inherits, logical(1), what = "element_blank") # Only merge in elements that shouldn't become blank - new[] <- new[!(inherit_blank & is_blank)] - add_theme(old, new, t2name = new_name) + keep <- nms[!(inherit_blank & is_blank)] + add_theme(old, new[keep], t2name = new_name) } From 9028521d3da61a825bf373f958aaa14c073df5c0 Mon Sep 17 00:00:00 2001 From: Teun van den Brand <49372158+teunbrand@users.noreply.github.com> Date: Wed, 8 Nov 2023 10:45:01 +0100 Subject: [PATCH 09/19] Document a bit --- R/guide-.R | 5 +++++ R/theme.R | 19 +++++++++++++++++-- man/ggplot2-ggproto.Rd | 12 +++++++++--- 3 files changed, 31 insertions(+), 5 deletions(-) diff --git a/R/guide-.R b/R/guide-.R index 76b975e17b..cabd2abdb2 100644 --- a/R/guide-.R +++ b/R/guide-.R @@ -122,6 +122,11 @@ new_guide <- function(..., available_aes = "any", super) { #' It can be used to overrule user-supplied parameters or perform checks on #' the `params` property. #' +#' - `setup_elements()` Used to extract elements from the theme. The base +#' `Guide` uses this method to merge a potential `internal_theme` with the +#' global theme and to calculate the elements declared in `Guide$elements`. +#' For other guides, this is a good place to intervene in theme inheritance. +#' #' - `override_elements()` Take populated theme elements derived from the #' `elements` property and allows overriding these theme settings. #' diff --git a/R/theme.R b/R/theme.R index 54d7f69a29..7d516e2012 100644 --- a/R/theme.R +++ b/R/theme.R @@ -560,8 +560,23 @@ add_theme <- function(t1, t2, t2name, call = caller_env()) { t1 } -# Calls `add_theme()`, but drops the elements of the new theme that inherit -# blank and that are blank in the old theme. +#' Combine themes while preserving blank elements +#' +#' This calls `add_theme()` but with the following modifications: +#' +#' * Elements in `new` that have `inherit.blank = TRUE` and are blank in `old` +#' will remain blank. +#' * `NULL` elements in `new` are ignored. +#' +#' This logic is used in the guide system to merge theme settings provided +#' at the guide level with theme settings provided at the plot level. +#' +#' @param old A theme object, typically the plot level theme. +#' @param new A theme object +#' @param new_name A name to display in error messages +#' +#' @keywords internal +#' @noRd add_theme_preserve_blank <- function(old, new, new_name = caller_arg(new)) { if (is.null(new)) { return(old) diff --git a/man/ggplot2-ggproto.Rd b/man/ggplot2-ggproto.Rd index 37a042dd68..e6f9f25210 100644 --- a/man/ggplot2-ggproto.Rd +++ b/man/ggplot2-ggproto.Rd @@ -421,12 +421,18 @@ 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{process_layers()} Extract information from layers. This acts mostly +as a filter for which layers to include and these are then (typically) +forwarded to \code{get_layer_key()}. +\item \code{get_layer_key()} This can be used 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{setup_elements()} Used to extract elements from the theme. The base +\code{Guide} uses this method to merge a potential \code{internal_theme} with the +global theme and to calculate the elements declared in \code{Guide$elements}. +For other guides, this is a good place to intervene in theme inheritance. \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. From e93bde9b9a9be7939da674f000962b85c0ee95a5 Mon Sep 17 00:00:00 2001 From: Teun van den Brand <49372158+teunbrand@users.noreply.github.com> Date: Wed, 8 Nov 2023 11:15:33 +0100 Subject: [PATCH 10/19] Drop the 'expressions are right-aligned' bit from the docs --- R/guide-legend.R | 3 +-- man/guide_bins.Rd | 3 +-- man/guide_colourbar.Rd | 3 +-- man/guide_coloursteps.Rd | 3 +-- man/guide_legend.Rd | 3 +-- 5 files changed, 5 insertions(+), 10 deletions(-) diff --git a/R/guide-legend.R b/R/guide-legend.R index 7ea9c7efee..f651512489 100644 --- a/R/guide-legend.R +++ b/R/guide-legend.R @@ -32,8 +32,7 @@ #' object of [element_text()] is expected. By default, the theme is #' specified by `legend.text` in [theme()]. #' @param label.hjust A numeric specifying horizontal justification of the -#' label text. The default for standard text is 0 (left-aligned) and 1 -#' (right-aligned) for expressions. +#' label text. #' @param label.vjust A numeric specifying vertical justification of the label #' text. #' @param keywidth A numeric or a [grid::unit()] object specifying diff --git a/man/guide_bins.Rd b/man/guide_bins.Rd index 6eeada9598..dcca3c5a33 100644 --- a/man/guide_bins.Rd +++ b/man/guide_bins.Rd @@ -64,8 +64,7 @@ object of \code{\link[=element_text]{element_text()}} is expected. By default, t specified by \code{legend.text} in \code{\link[=theme]{theme()}}.} \item{label.hjust}{A numeric specifying horizontal justification of the -label text. The default for standard text is 0 (left-aligned) and 1 -(right-aligned) for expressions.} +label text.} \item{label.vjust}{A numeric specifying vertical justification of the label text.} diff --git a/man/guide_colourbar.Rd b/man/guide_colourbar.Rd index 7813c12b1c..3ae922e0dc 100644 --- a/man/guide_colourbar.Rd +++ b/man/guide_colourbar.Rd @@ -103,8 +103,7 @@ object of \code{\link[=element_text]{element_text()}} is expected. By default, t specified by \code{legend.text} in \code{\link[=theme]{theme()}}.} \item{label.hjust}{A numeric specifying horizontal justification of the -label text. The default for standard text is 0 (left-aligned) and 1 -(right-aligned) for expressions.} +label text.} \item{label.vjust}{A numeric specifying vertical justification of the label text.} diff --git a/man/guide_coloursteps.Rd b/man/guide_coloursteps.Rd index e97230b6f4..76f611ca79 100644 --- a/man/guide_coloursteps.Rd +++ b/man/guide_coloursteps.Rd @@ -92,8 +92,7 @@ label. One of "top", "bottom" (default for horizontal guide), "left", or object of \code{\link[=element_text]{element_text()}} is expected. By default, the theme is specified by \code{legend.text} in \code{\link[=theme]{theme()}}.} \item{\code{label.hjust}}{A numeric specifying horizontal justification of the -label text. The default for standard text is 0 (left-aligned) and 1 -(right-aligned) for expressions.} +label text.} \item{\code{label.vjust}}{A numeric specifying vertical justification of the label text.} \item{\code{order}}{positive integer less than 99 that specifies the order of diff --git a/man/guide_legend.Rd b/man/guide_legend.Rd index 21dcbe7833..d3831b0c4d 100644 --- a/man/guide_legend.Rd +++ b/man/guide_legend.Rd @@ -60,8 +60,7 @@ object of \code{\link[=element_text]{element_text()}} is expected. By default, t specified by \code{legend.text} in \code{\link[=theme]{theme()}}.} \item{label.hjust}{A numeric specifying horizontal justification of the -label text. The default for standard text is 0 (left-aligned) and 1 -(right-aligned) for expressions.} +label text.} \item{label.vjust}{A numeric specifying vertical justification of the label text.} From 83b4142700257824d0dc7967a220371583acdb54 Mon Sep 17 00:00:00 2001 From: Teun van den Brand <49372158+teunbrand@users.noreply.github.com> Date: Mon, 20 Nov 2023 21:10:43 +0100 Subject: [PATCH 11/19] Simplify margin adjustment --- R/guide-legend.R | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/R/guide-legend.R b/R/guide-legend.R index a9bc02a8ad..e4e0e537cc 100644 --- a/R/guide-legend.R +++ b/R/guide-legend.R @@ -400,6 +400,13 @@ GuideLegend <- ggproto( theme$legend.text$hjust <- theme$legend.text$hjust %||% default_just[1] theme$legend.text$vjust <- theme$legend.text$vjust %||% default_just[2] + # We break inheritance of the text margins to add small spacings between + # labels and keys. + # We set the global text's margin to NULL here, so that a NULL in the + # computed elements indicates that no explicit margin has been set in + # neither the guide nor the theme's legend.text/legend.title. + theme$text$margin <- NULL + Guide$setup_elements(params, elements, theme) }, @@ -426,22 +433,16 @@ GuideLegend <- ggproto( "cm", valueOnly = TRUE ) - # When no explicit margin has been set, either in this guide or in the - # theme, we add a default text margin to leave a small gap in between - # the label and the key. - margin_absent <- is.null( - params$internal_theme$legend.text$margin %||% theme$legend.text$margin - ) - if (margin_absent && !inherits(elements$text, "element_blank")) { - i <- match(params$label.position, .trbl[c(3, 4, 1, 2)]) - elements$text$margin[i] <- elements$text$margin[i] + gap + # When no explicit margin has been set, we set a default text margin + # to leave a small gap in between the label and the key. + margin <- theme$text$margin %||% margin() + if (is.null(elements$text$margin)) { + i <- match(params$label.position, .trbl[c(3, 4, 1, 2)]) # match opposite + elements$text$margin <- replace(margin, i, margin[i] + gap) } - margin_absent <- is.null( - params$internal_theme$legend.title$margin %||% theme$legend.title$margin - ) - if (margin_absent && !inherits(elements$title, "element_blank")) { + if (is.null(elements$title$margin)) { i <- match(params$title.position, .trbl[c(3, 4, 1, 2)]) - elements$title$margin[i] <- elements$title$margin[i] + gap + elements$title$margin <- replace(margin, i, margin[i] + gap) } # Evaluate backgrounds early From 06bf5c968e490186b5080c200a59d3415ad857cb Mon Sep 17 00:00:00 2001 From: Teun van den Brand <49372158+teunbrand@users.noreply.github.com> Date: Wed, 22 Nov 2023 12:14:24 +0100 Subject: [PATCH 12/19] fix text just bug --- R/guide-legend.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/guide-legend.R b/R/guide-legend.R index e4e0e537cc..ebcdf8d0d3 100644 --- a/R/guide-legend.R +++ b/R/guide-legend.R @@ -737,7 +737,7 @@ legend_label_just <- function(position) { default_just <- switch( position, top = c(0.5, 0), - bottom = c(0.5, 0), + bottom = c(0.5, 1), left = c(1, 0.5), right = c(0, 0.5), arg_match0(position, .trbl, arg = "label.position") From 59d1b9398846f1c741b20c3b0dc4127d5b590bdf Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Mon, 27 Nov 2023 12:01:49 +0100 Subject: [PATCH 13/19] match opposite more explicitly --- R/guide-legend.R | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/R/guide-legend.R b/R/guide-legend.R index ebcdf8d0d3..65f0e380f8 100644 --- a/R/guide-legend.R +++ b/R/guide-legend.R @@ -437,11 +437,11 @@ GuideLegend <- ggproto( # to leave a small gap in between the label and the key. margin <- theme$text$margin %||% margin() if (is.null(elements$text$margin)) { - i <- match(params$label.position, .trbl[c(3, 4, 1, 2)]) # match opposite + i <- match(opposite_position(params$label.position), .trbl) # match opposite elements$text$margin <- replace(margin, i, margin[i] + gap) } if (is.null(elements$title$margin)) { - i <- match(params$title.position, .trbl[c(3, 4, 1, 2)]) + i <- match(opposite_position(params$title.position), .trbl) elements$title$margin <- replace(margin, i, margin[i] + gap) } From 148006f3039183240c67251c8072263405c84552 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Mon, 27 Nov 2023 12:49:37 +0100 Subject: [PATCH 14/19] use merge_element instead of combine_elements --- R/guide-legend.R | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/R/guide-legend.R b/R/guide-legend.R index 65f0e380f8..f42552398b 100644 --- a/R/guide-legend.R +++ b/R/guide-legend.R @@ -169,13 +169,13 @@ guide_legend <- function( label.position <- arg_match0(label.position, .trbl) } label.theme <- if (!isFALSE(label)) label.theme else element_blank() - label.theme <- combine_elements( - label.theme, - element_text(hjust = label.hjust, vjust = label.vjust, inherit.blank = TRUE) + label.theme <- merge_element( + element_text(hjust = label.hjust, vjust = label.vjust, inherit.blank = TRUE), + label.theme ) - title.theme <- combine_elements( - title.theme, - element_text(hjust = title.hjust, vjust = label.vjust, inherit.blank = TRUE) + title.theme <- merge_element( + element_text(hjust = title.hjust, vjust = label.vjust, inherit.blank = TRUE), + title.theme ) internal_theme <- theme( @@ -184,6 +184,7 @@ guide_legend <- function( legend.key.width = set_default_unit(keywidth, default.unit), legend.key.height = set_default_unit(keyheight, default.unit), ) + internal_theme <- compact(internal_theme) # Resolve spacing key.spacing.x <- set_default_unit(key.spacing.x %||% key.spacing, default.unit) From 36993dc1d094e9674533deee5990f61efaca7d4f Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Mon, 27 Nov 2023 14:19:33 +0100 Subject: [PATCH 15/19] rewire theme merging logic --- R/guide-.R | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++- R/theme.R | 38 ------------------------------------- 2 files changed, 54 insertions(+), 39 deletions(-) diff --git a/R/guide-.R b/R/guide-.R index 5c86c06b03..b6a01796cf 100644 --- a/R/guide-.R +++ b/R/guide-.R @@ -273,7 +273,7 @@ Guide <- ggproto( # Converts the `elements` field to proper elements to be accepted by # `element_grob()`. String-interpolates aesthetic/position dependent elements. setup_elements = function(params, elements, theme) { - theme <- add_theme_preserve_blank(theme, params$internal_theme) + theme <- merge_internal_theme(theme, params$internal_theme) is_char <- vapply(elements, is.character, logical(1)) elements[is_char] <- lapply(elements[is_char], calc_element, theme = theme) elements @@ -457,3 +457,56 @@ validate_labels <- function(labels) { unlist(labels) } } + +# This logic is similar to `add_theme()` with the following exceptions: +# +# 1. Elements in `new` that have `inherit.blank = TRUE` and are blank in `old` +# will remain blank. +# 2. `NULL` elements in `new` are ignored. +# 3. When an `old` element is a subclass of the `new` element, that subclass +# is preserved. +merge_internal_theme <- function(old, new, new_name = caller_arg(new)) { + if (is.null(new)) { + return(old) + } + # Get non empty names of new theme + nms <- names(new)[!vapply(new, is.null, logical(1))] + + # Does any of the new theme elements carry over blank elements? + inherit_blank <- vapply( + new[nms], FUN.VALUE = logical(1), + function(x) is.list(x) && isTRUE(x$inherit.blank) + ) + # Are their equivalents in the old theme blank? + is_blank <- vapply(old[nms], inherits, logical(1), what = "element_blank") + + # Only merge in elements that shouldn't become blank + keep <- nms[!(inherit_blank & is_blank)] + + try_fetch( + for (item in keep) { + x <- merge_subclass(new[[item]], old[[item]]) + old[item] <- list(x) + }, + error = function(cnd) { + cli::cli_abort( + "Problem merging the {.var {item}} theme element.", + parent = cnd + ) + } + ) + old +} + +merge_subclass <- function(new, old) { + if (!is.subclass(old, new)) { + return(merge_element(new, old)) + } + if (is.null(old)) { + return(new) + } + idx <- !vapply(new, is.null, logical(1)) + idx <- names(new)[idx] + old[idx] <- new[idx] + old +} diff --git a/R/theme.R b/R/theme.R index 7d516e2012..f1315e0881 100644 --- a/R/theme.R +++ b/R/theme.R @@ -560,44 +560,6 @@ add_theme <- function(t1, t2, t2name, call = caller_env()) { t1 } -#' Combine themes while preserving blank elements -#' -#' This calls `add_theme()` but with the following modifications: -#' -#' * Elements in `new` that have `inherit.blank = TRUE` and are blank in `old` -#' will remain blank. -#' * `NULL` elements in `new` are ignored. -#' -#' This logic is used in the guide system to merge theme settings provided -#' at the guide level with theme settings provided at the plot level. -#' -#' @param old A theme object, typically the plot level theme. -#' @param new A theme object -#' @param new_name A name to display in error messages -#' -#' @keywords internal -#' @noRd -add_theme_preserve_blank <- function(old, new, new_name = caller_arg(new)) { - if (is.null(new)) { - return(old) - } - # Get non empty names of new theme - nms <- names(new)[!vapply(new, is.null, logical(1))] - - # Does any of the new theme elements carry over blank elements? - inherit_blank <- vapply( - new[nms], FUN.VALUE = logical(1), - function(x) is.list(x) && isTRUE(x$inherit.blank) - ) - # Are their equivalents in the old theme blank? - is_blank <- vapply(old[nms], inherits, logical(1), what = "element_blank") - - # Only merge in elements that shouldn't become blank - keep <- nms[!(inherit_blank & is_blank)] - add_theme(old, new[keep], t2name = new_name) -} - - #' Calculate the element properties, by inheriting properties from its parents #' #' @param element The name of the theme element to calculate From eaab63eec2913e885a1a1d2301f74acfa79920ab Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Mon, 27 Nov 2023 14:20:48 +0100 Subject: [PATCH 16/19] prevent 0-length size/linewidth --- R/theme.R | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/R/theme.R b/R/theme.R index f1315e0881..1b36401dc7 100644 --- a/R/theme.R +++ b/R/theme.R @@ -774,12 +774,12 @@ combine_elements <- function(e1, e2) { # Calculate relative sizes if (is.rel(e1$size)) { - e1$size <- e2$size * unclass(e1$size) + e1$size <- (e2$size %||% rel(1)) * unclass(e1$size) } # Calculate relative linewidth if (is.rel(e1$linewidth)) { - e1$linewidth <- e2$linewidth * unclass(e1$linewidth) + e1$linewidth <- (e2$linewidth %||% rel(1)) * unclass(e1$linewidth) } # If e2 is 'richer' than e1, fill e2 with e1 parameters From a513fdf6968f93fc617b7eff0baae874c0e60cfb Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Mon, 27 Nov 2023 14:21:28 +0100 Subject: [PATCH 17/19] Revert "use merge_element instead of combine_elements" This reverts commit 148006f3039183240c67251c8072263405c84552. --- R/guide-legend.R | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/R/guide-legend.R b/R/guide-legend.R index f42552398b..65f0e380f8 100644 --- a/R/guide-legend.R +++ b/R/guide-legend.R @@ -169,13 +169,13 @@ guide_legend <- function( label.position <- arg_match0(label.position, .trbl) } label.theme <- if (!isFALSE(label)) label.theme else element_blank() - label.theme <- merge_element( - element_text(hjust = label.hjust, vjust = label.vjust, inherit.blank = TRUE), - label.theme + label.theme <- combine_elements( + label.theme, + element_text(hjust = label.hjust, vjust = label.vjust, inherit.blank = TRUE) ) - title.theme <- merge_element( - element_text(hjust = title.hjust, vjust = label.vjust, inherit.blank = TRUE), - title.theme + title.theme <- combine_elements( + title.theme, + element_text(hjust = title.hjust, vjust = label.vjust, inherit.blank = TRUE) ) internal_theme <- theme( @@ -184,7 +184,6 @@ guide_legend <- function( legend.key.width = set_default_unit(keywidth, default.unit), legend.key.height = set_default_unit(keyheight, default.unit), ) - internal_theme <- compact(internal_theme) # Resolve spacing key.spacing.x <- set_default_unit(key.spacing.x %||% key.spacing, default.unit) From aaaf17e5c0a6b74467d4fec2ca8f1497ff4064db Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Mon, 27 Nov 2023 14:23:39 +0100 Subject: [PATCH 18/19] should not receive NULL --- R/guide-.R | 3 --- 1 file changed, 3 deletions(-) diff --git a/R/guide-.R b/R/guide-.R index b6a01796cf..543d23b03e 100644 --- a/R/guide-.R +++ b/R/guide-.R @@ -502,9 +502,6 @@ merge_subclass <- function(new, old) { if (!is.subclass(old, new)) { return(merge_element(new, old)) } - if (is.null(old)) { - return(new) - } idx <- !vapply(new, is.null, logical(1)) idx <- names(new)[idx] old[idx] <- new[idx] From 56401d1550b64c86d8871ecb6257dfad4ad191d7 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Mon, 27 Nov 2023 14:33:50 +0100 Subject: [PATCH 19/19] early exit empty themes as well --- R/guide-.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/guide-.R b/R/guide-.R index 543d23b03e..f3047a450a 100644 --- a/R/guide-.R +++ b/R/guide-.R @@ -466,7 +466,7 @@ validate_labels <- function(labels) { # 3. When an `old` element is a subclass of the `new` element, that subclass # is preserved. merge_internal_theme <- function(old, new, new_name = caller_arg(new)) { - if (is.null(new)) { + if (length(new) == 0) { return(old) } # Get non empty names of new theme