From 8bd9f31f032e72c601f63096785de152a978f53b Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Tue, 4 Mar 2025 15:19:19 +0100 Subject: [PATCH 01/22] add S7 as import --- DESCRIPTION | 1 + 1 file changed, 1 insertion(+) diff --git a/DESCRIPTION b/DESCRIPTION index 753e7dd49a..0a33c39ea4 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -39,6 +39,7 @@ Imports: isoband, lifecycle (> 1.0.1), rlang (>= 1.1.0), + S7, scales (>= 1.3.0), stats, vctrs (>= 0.6.0), From d54e46444f736aba3abc53fc5777b28d1e17180b Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Wed, 5 Mar 2025 11:18:20 +0100 Subject: [PATCH 02/22] custom properties for theme elements --- DESCRIPTION | 1 + R/properties.R | 102 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 103 insertions(+) create mode 100644 R/properties.R diff --git a/DESCRIPTION b/DESCRIPTION index 0a33c39ea4..a632b6fcf6 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -176,6 +176,7 @@ Collate: 'grob-dotstack.R' 'grob-null.R' 'grouping.R' + 'properties.R' 'theme-elements.R' 'guide-.R' 'guide-axis.R' diff --git a/R/properties.R b/R/properties.R new file mode 100644 index 0000000000..be857393b0 --- /dev/null +++ b/R/properties.R @@ -0,0 +1,102 @@ + +property_boolean <- function(allow_null = FALSE, default = TRUE) { + class <- S7::class_logical + class <- if (allow_null) S7::new_union(class, NULL) else class + validator <- function(value) { + if ((allow_null && is.null(value)) || is_bool(value)) { + return(character()) + } + "must be a boolean" + } + S7::new_property( + class = class, + validator = validator, + default = default + ) +} + +property_choice <- function(options, allow_null = FALSE, default = NULL) { + force(options) + class <- S7::class_character + class <- if (allow_null) S7::new_union(class, NULL) else class + validator <- function(value) { + if (allow_null && is.null(value)) { + return(character()) + } + if (!is_string(value)) { + return(as_cli("must be a string, not {.obj_type_friendly {value}}")) + } + if (value %in% options) { + return(character()) + } + as_cli("must be one of {.or {.val {options}}}") + } + S7::new_property( + class = class, + validator = validator, + default = default + ) +} + +element_props <- list( + fill = S7::new_property( + S7::new_union(S7::class_character, S7::new_S3_class("GridPattern"), S7::class_logical, NULL), + default = NULL + ), + colour = S7::new_property( + S7::new_union(S7::class_character, S7::class_logical, NULL), + default = NULL + ), + family = S7::new_property( + S7::new_union(S7::class_character, NULL), + default = NULL + ), + hjust = S7::new_property( + S7::new_union(S7::class_numeric, NULL), + default = NULL + ), + vjust = S7::new_property( + S7::new_union(S7::class_numeric, NULL), + default = NULL + ), + angle = S7::new_property( + S7::new_union(S7::class_numeric, NULL), + default = NULL + ), + size = S7::new_property( + S7::new_union(S7::class_numeric, NULL), + default = NULL + ), + lineheight = S7::new_property( + S7::new_union(S7::class_numeric, NULL), + default = NULL + ), + margin = S7::new_property( + S7::new_union(S7::new_S3_class("margin"), NULL), + default = NULL + ), + face = property_choice(c("plain", "bold", "italic", "oblique", "bold.italic"), allow_null = TRUE), + linewidth = S7::new_property( + S7::new_union(S7::class_numeric, NULL), + default = NULL + ), + linetype = S7::new_property( + S7::new_union(S7::class_numeric, S7::class_character, NULL), + default = NULL + ), + lineend = property_choice(c("round", "butt", "square"), allow_null = TRUE), + shape = S7::new_property( + S7::new_union(S7::class_numeric, S7::class_character, NULL), + default = NULL + ), + arrow = S7::new_property( + S7::new_union(S7::new_S3_class("arrow"), S7::class_logical, NULL), + default = NULL + ), + arrow.fill = S7::new_property( + S7::new_union(S7::class_character, S7::class_logical, NULL), + default = NULL + ), + debug = property_boolean(allow_null = TRUE, default = NULL), + inherit.blank = property_boolean(default = FALSE) +) From 91302e0c21e835cabb258f60b6c0bffdfe346601 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Wed, 5 Mar 2025 11:18:57 +0100 Subject: [PATCH 03/22] convert theme elements to S7 classes --- NAMESPACE | 2 +- R/theme-elements.R | 245 +++++++++++++++++++++++++-------------------- R/theme.R | 4 +- man/element.Rd | 3 + 4 files changed, 142 insertions(+), 112 deletions(-) diff --git a/NAMESPACE b/NAMESPACE index b58765ecc1..5e4384df4e 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -106,7 +106,6 @@ S3method(predictdf,default) S3method(predictdf,glm) S3method(predictdf,locfit) S3method(predictdf,loess) -S3method(print,element) S3method(print,ggplot) S3method(print,ggplot2_bins) S3method(print,ggproto) @@ -344,6 +343,7 @@ export(draw_key_vline) export(draw_key_vpath) export(dup_axis) export(el_def) +export(element) export(element_blank) export(element_geom) export(element_grob) diff --git a/R/theme-elements.R b/R/theme-elements.R index c3b6ded319..c479aa6e3a 100644 --- a/R/theme-elements.R +++ b/R/theme-elements.R @@ -81,55 +81,63 @@ NULL #' @export #' @rdname element -element_blank <- function() { - structure( - list(), - class = c("element_blank", "element") - ) -} +element <- S7::new_class("element", abstract = TRUE) #' @export #' @rdname element -element_rect <- function(fill = NULL, colour = NULL, linewidth = NULL, - linetype = NULL, color = NULL, inherit.blank = FALSE, size = deprecated()) { +element_blank <- S7::new_class("element_blank", parent = element) - if (lifecycle::is_present(size)) { - deprecate_soft0("3.4.0", "element_rect(size)", "element_rect(linewidth)") - linewidth <- size - } +#' @include properties.R - if (!is.null(color)) colour <- color - structure( - list(fill = fill, colour = colour, linewidth = linewidth, linetype = linetype, - inherit.blank = inherit.blank), - class = c("element_rect", "element") - ) -} +#' @export +#' @rdname element +element_rect <- S7::new_class( + "element_rect", parent = element, + properties = element_props[c("fill", "colour", "linewidth", "linetype", "inherit.blank")], + constructor = function(fill = NULL, colour = NULL, linewidth = NULL, + linetype = NULL, color = NULL, inherit.blank = FALSE, + size = deprecated()){ + if (lifecycle::is_present(size)) { + deprecate_soft0("3.4.0", "element_rect(size)", "element_rect(linewidth)") + linewidth <- size + } + S7::new_object( + S7::S7_object(), + fill = fill, colour = color %||% colour, + linewidth = linewidth, linetype = linetype, + inherit.blank = inherit.blank + ) + } +) #' @export #' @rdname element #' @param lineend Line end Line end style (round, butt, square) #' @param arrow Arrow specification, as created by [grid::arrow()] -element_line <- function(colour = NULL, linewidth = NULL, linetype = NULL, - lineend = NULL, color = NULL, arrow = NULL, arrow.fill = NULL, - inherit.blank = FALSE, size = deprecated()) { - - if (lifecycle::is_present(size)) { - deprecate_soft0("3.4.0", "element_line(size)", "element_line(linewidth)") - linewidth <- size +element_line <- S7::new_class( + "element_line", parent = element, + properties = element_props[c( + "colour", "linewidth", "linetype", "lineend", "arrow", "arrow.fill", + "inherit.blank" + )], + constructor = function(colour = NULL, linewidth = NULL, linetype = NULL, + lineend = NULL, color = NULL, arrow = NULL, + arrow.fill = NULL, inherit.blank = FALSE, size = deprecated()) { + if (lifecycle::is_present(size)) { + deprecate_soft0("3.4.0", "element_line(size)", "element_line(linewidth)") + linewidth <- size + } + colour <- color %||% colour + S7::new_object( + S7::S7_object(), + colour = colour, + linewidth = linewidth, linetype = linetype, lineend = lineend, + arrow = arrow %||% FALSE, + arrow.fill = arrow.fill %||% colour, + inherit.blank = inherit.blank + ) } - - colour <- color %||% colour - arrow.fill <- arrow.fill %||% colour - arrow <- arrow %||% FALSE - - structure( - list(colour = colour, linewidth = linewidth, linetype = linetype, lineend = lineend, - arrow = arrow, arrow.fill = arrow.fill, inherit.blank = inherit.blank), - class = c("element_line", "element") - ) -} - +) #' @param family Font family #' @param face Font face ("plain", "italic", "bold", "bold.italic") @@ -145,93 +153,116 @@ element_line <- function(colour = NULL, linewidth = NULL, linetype = NULL, #' is anchored. #' @export #' @rdname element -element_text <- function(family = NULL, face = NULL, colour = NULL, - size = NULL, hjust = NULL, vjust = NULL, angle = NULL, lineheight = NULL, - color = NULL, margin = NULL, debug = NULL, inherit.blank = FALSE) { - - if (!is.null(color)) colour <- color - - n <- max( - length(family), length(face), length(colour), length(size), - length(hjust), length(vjust), length(angle), length(lineheight) - ) - if (n > 1) { - cli::cli_warn(c( - "Vectorized input to {.fn element_text} is not officially supported.", - "i" = "Results may be unexpected or may change in future versions of ggplot2." - )) - } - - - structure( - list(family = family, face = face, colour = colour, size = size, +element_text <- S7::new_class( + "element_text", parent = element, + properties = element_props[c( + "family", "face", "colour", "size", "hjust", "vjust", "angle", "lineheight", + "margin", "debug", "inherit.blank" + )], + constructor = function(family = NULL, face = NULL, colour = NULL, + size = NULL, hjust = NULL, vjust = NULL, angle = NULL, + lineheight = NULL, color = NULL, margin = NULL, + debug = NULL, inherit.blank = FALSE) { + n <- max( + length(family), length(face), length(colour), length(size), + length(hjust), length(vjust), length(angle), length(lineheight) + ) + if (n > 1) { + cli::cli_warn(c( + "Vectorized input to {.fn element_text} is not officially supported.", + "i" = "Results may be unexpected or may change in future versions of ggplot2." + )) + } + + colour <- color %||% colour + S7::new_object( + S7::S7_object(), + family = family, face = face, colour = colour, size = size, hjust = hjust, vjust = vjust, angle = angle, lineheight = lineheight, - margin = margin, debug = debug, inherit.blank = inherit.blank), - class = c("element_text", "element") - ) -} + margin = margin, debug = debug, inherit.blank = inherit.blank + ) + } +) #' @export #' @rdname element -element_polygon <- function(fill = NULL, colour = NULL, linewidth = NULL, - linetype = NULL, color = NULL, - inherit.blank = FALSE) { - structure( - list( +element_polygon <- S7::new_class( + "element_polygon", parent = element, + properties = element_props[c( + "fill", "colour", "linewidth", "linetype", "inherit.blank" + )], + constructor = function(fill = NULL, colour = NULL, linewidth = NULL, + linetype = NULL, color = NULL, inherit.blank = FALSE) { + colour <- color %||% colour + S7::new_object( + S7::S7_object(), fill = fill, colour = color %||% colour, linewidth = linewidth, linetype = linetype, inherit.blank = inherit.blank - ), - class = c("element_polygon", "element") - ) -} + ) + } +) #' @export #' @rdname element -element_point <- function(colour = NULL, shape = NULL, size = NULL, fill = NULL, - stroke = NULL, color = NULL, inherit.blank = FALSE) { - structure( - list( +element_point <- S7::new_class( + "element_point", parent = element, + properties = rename( + element_props[c( + "colour", "shape", "size", "fill", "linewidth", "inherit.blank" + )], + c("linewidth" = "stroke") + ), + constructor = function(colour = NULL, shape = NULL, size = NULL, fill = NULL, + stroke = NULL, color = NULL, inherit.blank = FALSE) { + S7::new_object( + S7::S7_object(), colour = color %||% colour, fill = fill, shape = shape, size = size, stroke = stroke, inherit.blank = inherit.blank - ), - class = c("element_point", "element") - ) -} + ) + } +) #' @param ink Foreground colour. #' @param paper Background colour. #' @param accent Accent colour. #' @export #' @rdname element -element_geom <- function( - # colours - ink = NULL, paper = NULL, accent = NULL, - # linewidth - linewidth = NULL, borderwidth = NULL, - # linetype - linetype = NULL, bordertype = NULL, - # text - family = NULL, fontsize = NULL, - # points - pointsize = NULL, pointshape = NULL) { - - if (!is.null(fontsize)) { - fontsize <- fontsize / .pt - } - - structure( - list( - ink = ink, - paper = paper, - accent = accent, +element_geom <- S7::new_class( + "element_geom", parent = element, + properties = list( + ink = element_props$colour, + paper = element_props$colour, + accent = element_props$colour, + linewidth = element_props$linewidth, + borderwidth = element_props$linewidth, + linetype = element_props$linetype, + bordertype = element_props$linetype, + family = element_props$family, + fontsize = element_props$size, + pointsize = element_props$size, + pointshape = element_props$shape + ), + constructor = function( + ink = NULL, paper = NULL, accent = NULL, + linewidth = NULL, borderwidth = NULL, + linetype = NULL, bordertype = NULL, + family = NULL, fontsize = NULL, + pointsize = NULL, pointshape = NULL) { + + if (!is.null(fontsize)) { + fontsize <- fontsize / .pt + } + + S7::new_object( + S7::S7_object(), + ink = ink, paper = paper, accent = accent, linewidth = linewidth, borderwidth = borderwidth, linetype = linetype, bordertype = bordertype, family = family, fontsize = fontsize, pointsize = pointsize, pointshape = pointshape - ), - class = c("element_geom", "element") - ) -} + ) + } +) .default_geom_element <- element_geom( ink = "black", paper = "white", accent = "#3366FF", @@ -243,11 +274,7 @@ element_geom <- function( #' @export #' @rdname is_tests -is.theme_element <- function(x) inherits(x, "element") - -#' @export -print.element <- function(x, ...) utils::str(x) - +is.theme_element <- function(x) S7::S7_inherits(x, element) #' @param x A single number specifying size relative to parent element. #' @rdname element diff --git a/R/theme.R b/R/theme.R index dfe986fc62..1c16c85656 100644 --- a/R/theme.R +++ b/R/theme.R @@ -543,8 +543,8 @@ theme <- function(..., # If complete theme set all non-blank elements to inherit from blanks if (complete) { elements <- lapply(elements, function(el) { - if (is.theme_element(el) && !inherits(el, "element_blank")) { - el$inherit.blank <- TRUE + if (is.theme_element(el) && S7::prop_exists(el, "inherit.blank")) { + S7::prop(el, "inherit.blank") <- TRUE } el }) diff --git a/man/element.Rd b/man/element.Rd index 99e56f0e94..72d8c2b601 100644 --- a/man/element.Rd +++ b/man/element.Rd @@ -1,6 +1,7 @@ % Generated by roxygen2: do not edit by hand % Please edit documentation in R/theme-elements.R, R/margins.R \name{element} +\alias{element} \alias{element_blank} \alias{element_rect} \alias{element_line} @@ -14,6 +15,8 @@ \alias{margin_auto} \title{Theme elements} \usage{ +element() + element_blank() element_rect( From 8c199a16232fd491702aa234739094aa7c632030 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Wed, 5 Mar 2025 13:30:38 +0100 Subject: [PATCH 04/22] adapt element definitions --- R/theme-elements.R | 183 +++++++++++++++++---------------- man/register_theme_elements.Rd | 6 +- 2 files changed, 98 insertions(+), 91 deletions(-) diff --git a/R/theme-elements.R b/R/theme-elements.R index c479aa6e3a..5e3a016e69 100644 --- a/R/theme-elements.R +++ b/R/theme-elements.R @@ -483,7 +483,7 @@ element_grob.element_point <- function(element, x = 0.5, y = 0.5, colour = NULL, #' # plot panels. To do so, it registers a new theme element `ggxyz.panel.annotation` #' register_theme_elements( #' ggxyz.panel.annotation = element_text(color = "blue", hjust = 0.95, vjust = 0.05), -#' element_tree = list(ggxyz.panel.annotation = el_def("element_text", "text")) +#' element_tree = list(ggxyz.panel.annotation = el_def(element_text, "text")) #' ) #' #' # Now the package can define a new coord that includes a panel annotation @@ -602,8 +602,8 @@ check_element_tree <- function(x, arg = caller_arg(x), call = caller_env()) { #' @details #' The function `el_def()` is used to define new or modified element types and #' element inheritance relationships for the element tree. -#' @param class The name of the element class. Examples are "element_line" or -#' "element_text" or "unit", or one of the two reserved keywords "character" or +#' @param class The name of the element class. Examples are `element_line` or +#' `element_text` or "unit", or one of the two reserved keywords "character" or #' "margin". The reserved keyword "character" implies a character #' or numeric vector, not a class called "character". The keyword #' "margin" implies a unit vector of length 4, as created by [margin()]. @@ -622,43 +622,43 @@ el_def <- function(class = NULL, inherit = NULL, description = NULL) { # among them. It should not be read from directly, since users may modify the # current element tree stored in ggplot_global$element_tree .element_tree <- list( - line = el_def("element_line"), - rect = el_def("element_rect"), - text = el_def("element_text"), - point = el_def("element_point"), - polygon = el_def("element_polygon"), - geom = el_def("element_geom"), - title = el_def("element_text", "text"), + line = el_def(element_line), + rect = el_def(element_rect), + text = el_def(element_text), + point = el_def(element_point), + polygon = el_def(element_polygon), + geom = el_def(element_geom), + title = el_def(element_text, "text"), spacing = el_def("unit"), margins = el_def(c("margin", "unit")), - axis.line = el_def("element_line", "line"), - axis.text = el_def("element_text", "text"), - axis.title = el_def("element_text", "title"), - axis.ticks = el_def("element_line", "line"), + axis.line = el_def(element_line, "line"), + axis.text = el_def(element_text, "text"), + axis.title = el_def(element_text, "title"), + axis.ticks = el_def(element_line, "line"), legend.key.size = el_def(c("unit", "rel"), "spacing"), - panel.grid = el_def("element_line", "line"), - panel.grid.major = el_def("element_line", "panel.grid"), - panel.grid.minor = el_def("element_line", "panel.grid"), - strip.text = el_def("element_text", "text"), - - axis.line.x = el_def("element_line", "axis.line"), - axis.line.x.top = el_def("element_line", "axis.line.x"), - axis.line.x.bottom = el_def("element_line", "axis.line.x"), - axis.line.y = el_def("element_line", "axis.line"), - axis.line.y.left = el_def("element_line", "axis.line.y"), - axis.line.y.right = el_def("element_line", "axis.line.y"), - axis.line.theta = el_def("element_line", "axis.line.x"), - axis.line.r = el_def("element_line", "axis.line.y"), - - axis.text.x = el_def("element_text", "axis.text"), - axis.text.x.top = el_def("element_text", "axis.text.x"), - axis.text.x.bottom = el_def("element_text", "axis.text.x"), - axis.text.y = el_def("element_text", "axis.text"), - axis.text.y.left = el_def("element_text", "axis.text.y"), - axis.text.y.right = el_def("element_text", "axis.text.y"), - axis.text.theta = el_def("element_text", "axis.text.x"), - axis.text.r = el_def("element_text", "axis.text.y"), + panel.grid = el_def(element_line, "line"), + panel.grid.major = el_def(element_line, "panel.grid"), + panel.grid.minor = el_def(element_line, "panel.grid"), + strip.text = el_def(element_text, "text"), + + axis.line.x = el_def(element_line, "axis.line"), + axis.line.x.top = el_def(element_line, "axis.line.x"), + axis.line.x.bottom = el_def(element_line, "axis.line.x"), + axis.line.y = el_def(element_line, "axis.line"), + axis.line.y.left = el_def(element_line, "axis.line.y"), + axis.line.y.right = el_def(element_line, "axis.line.y"), + axis.line.theta = el_def(element_line, "axis.line.x"), + axis.line.r = el_def(element_line, "axis.line.y"), + + axis.text.x = el_def(element_text, "axis.text"), + axis.text.x.top = el_def(element_text, "axis.text.x"), + axis.text.x.bottom = el_def(element_text, "axis.text.x"), + axis.text.y = el_def(element_text, "axis.text"), + axis.text.y.left = el_def(element_text, "axis.text.y"), + axis.text.y.right = el_def(element_text, "axis.text.y"), + axis.text.theta = el_def(element_text, "axis.text.x"), + axis.text.r = el_def(element_text, "axis.text.y"), axis.ticks.length = el_def(c("unit", "rel"), "spacing"), axis.ticks.length.x = el_def(c("unit", "rel"), "axis.ticks.length"), @@ -670,28 +670,28 @@ el_def <- function(class = NULL, inherit = NULL, description = NULL) { axis.ticks.length.theta = el_def(c("unit", "rel"), "axis.ticks.length.x"), axis.ticks.length.r = el_def(c("unit", "rel"), "axis.ticks.length.y"), - axis.ticks.x = el_def("element_line", "axis.ticks"), - axis.ticks.x.top = el_def("element_line", "axis.ticks.x"), - axis.ticks.x.bottom = el_def("element_line", "axis.ticks.x"), - axis.ticks.y = el_def("element_line", "axis.ticks"), - axis.ticks.y.left = el_def("element_line", "axis.ticks.y"), - axis.ticks.y.right = el_def("element_line", "axis.ticks.y"), - axis.ticks.theta = el_def("element_line", "axis.ticks.x"), - axis.ticks.r = el_def("element_line", "axis.ticks.y"), - - axis.title.x = el_def("element_text", "axis.title"), - axis.title.x.top = el_def("element_text", "axis.title.x"), - axis.title.x.bottom = el_def("element_text", "axis.title.x"), - axis.title.y = el_def("element_text", "axis.title"), - axis.title.y.left = el_def("element_text", "axis.title.y"), - axis.title.y.right = el_def("element_text", "axis.title.y"), - - axis.minor.ticks.x.top = el_def("element_line", "axis.ticks.x.top"), - axis.minor.ticks.x.bottom = el_def("element_line", "axis.ticks.x.bottom"), - axis.minor.ticks.y.left = el_def("element_line", "axis.ticks.y.left"), - axis.minor.ticks.y.right = el_def("element_line", "axis.ticks.y.right"), - axis.minor.ticks.theta = el_def("element_line", "axis.ticks.theta"), - axis.minor.ticks.r = el_def("element_line", "axis.ticks.r"), + axis.ticks.x = el_def(element_line, "axis.ticks"), + axis.ticks.x.top = el_def(element_line, "axis.ticks.x"), + axis.ticks.x.bottom = el_def(element_line, "axis.ticks.x"), + axis.ticks.y = el_def(element_line, "axis.ticks"), + axis.ticks.y.left = el_def(element_line, "axis.ticks.y"), + axis.ticks.y.right = el_def(element_line, "axis.ticks.y"), + axis.ticks.theta = el_def(element_line, "axis.ticks.x"), + axis.ticks.r = el_def(element_line, "axis.ticks.y"), + + axis.title.x = el_def(element_text, "axis.title"), + axis.title.x.top = el_def(element_text, "axis.title.x"), + axis.title.x.bottom = el_def(element_text, "axis.title.x"), + axis.title.y = el_def(element_text, "axis.title"), + axis.title.y.left = el_def(element_text, "axis.title.y"), + axis.title.y.right = el_def(element_text, "axis.title.y"), + + axis.minor.ticks.x.top = el_def(element_line, "axis.ticks.x.top"), + axis.minor.ticks.x.bottom = el_def(element_line, "axis.ticks.x.bottom"), + axis.minor.ticks.y.left = el_def(element_line, "axis.ticks.y.left"), + axis.minor.ticks.y.right = el_def(element_line, "axis.ticks.y.right"), + axis.minor.ticks.theta = el_def(element_line, "axis.ticks.theta"), + axis.minor.ticks.r = el_def(element_line, "axis.ticks.r"), axis.minor.ticks.length = el_def(c("unit", "rel")), axis.minor.ticks.length.x = el_def(c("unit", "rel"), "axis.minor.ticks.length"), @@ -715,25 +715,25 @@ el_def <- function(class = NULL, inherit = NULL, description = NULL) { c("unit", "rel"), c("axis.minor.ticks.length.y", "axis.ticks.length.r") ), - legend.background = el_def("element_rect", "rect"), + legend.background = el_def(element_rect, "rect"), legend.margin = el_def(c("margin", "unit", "rel"), "margins"), legend.spacing = el_def(c("unit", "rel"), "spacing"), legend.spacing.x = el_def(c("unit", "rel"), "legend.spacing"), legend.spacing.y = el_def(c("unit", "rel"), "legend.spacing"), - legend.key = el_def("element_rect", "panel.background"), + legend.key = el_def(element_rect, "panel.background"), legend.key.height = el_def(c("unit", "rel"), "legend.key.size"), legend.key.width = el_def(c("unit", "rel"), "legend.key.size"), legend.key.spacing = el_def(c("unit", "rel"), "spacing"), legend.key.spacing.x = el_def(c("unit", "rel"), "legend.key.spacing"), legend.key.spacing.y = el_def(c("unit", "rel"), "legend.key.spacing"), legend.key.justification = el_def(c("character", "numeric", "integer")), - legend.frame = el_def("element_rect", "rect"), - legend.axis.line = el_def("element_line", "line"), - legend.ticks = el_def("element_line", "legend.axis.line"), + legend.frame = el_def(element_rect, "rect"), + legend.axis.line = el_def(element_line, "line"), + legend.ticks = el_def(element_line, "legend.axis.line"), legend.ticks.length = el_def(c("rel", "unit"), "legend.key.size"), - legend.text = el_def("element_text", "text"), + legend.text = el_def(element_text, "text"), legend.text.position = el_def("character"), - legend.title = el_def("element_text", "title"), + legend.title = el_def(element_text, "title"), legend.title.position = el_def("character"), legend.byrow = el_def("logical"), legend.position = el_def("character"), @@ -767,45 +767,45 @@ el_def <- function(class = NULL, inherit = NULL, description = NULL) { legend.box = el_def("character"), legend.box.just = el_def("character"), legend.box.margin = el_def(c("margin", "unit", "rel"), "margins"), - legend.box.background = el_def("element_rect", "rect"), + legend.box.background = el_def(element_rect, "rect"), legend.box.spacing = el_def(c("unit", "rel"), "spacing"), - panel.background = el_def("element_rect", "rect"), - panel.border = el_def("element_rect", "rect"), + panel.background = el_def(element_rect, "rect"), + panel.border = el_def(element_rect, "rect"), panel.spacing = el_def(c("unit", "rel"), "spacing"), panel.spacing.x = el_def(c("unit", "rel"), "panel.spacing"), panel.spacing.y = el_def(c("unit", "rel"), "panel.spacing"), - panel.grid.major.x = el_def("element_line", "panel.grid.major"), - panel.grid.major.y = el_def("element_line", "panel.grid.major"), - panel.grid.minor.x = el_def("element_line", "panel.grid.minor"), - panel.grid.minor.y = el_def("element_line", "panel.grid.minor"), + panel.grid.major.x = el_def(element_line, "panel.grid.major"), + panel.grid.major.y = el_def(element_line, "panel.grid.major"), + panel.grid.minor.x = el_def(element_line, "panel.grid.minor"), + panel.grid.minor.y = el_def(element_line, "panel.grid.minor"), panel.ontop = el_def("logical"), panel.widths = el_def("unit"), panel.heights = el_def("unit"), - strip.background = el_def("element_rect", "rect"), - strip.background.x = el_def("element_rect", "strip.background"), - strip.background.y = el_def("element_rect", "strip.background"), + strip.background = el_def(element_rect, "rect"), + strip.background.x = el_def(element_rect, "strip.background"), + strip.background.y = el_def(element_rect, "strip.background"), strip.clip = el_def("character"), - strip.text.x = el_def("element_text", "strip.text"), - strip.text.x.top = el_def("element_text", "strip.text.x"), - strip.text.x.bottom = el_def("element_text", "strip.text.x"), - strip.text.y = el_def("element_text", "strip.text"), - strip.text.y.left = el_def("element_text", "strip.text.y"), - strip.text.y.right = el_def("element_text", "strip.text.y"), + strip.text.x = el_def(element_text, "strip.text"), + strip.text.x.top = el_def(element_text, "strip.text.x"), + strip.text.x.bottom = el_def(element_text, "strip.text.x"), + strip.text.y = el_def(element_text, "strip.text"), + strip.text.y.left = el_def(element_text, "strip.text.y"), + strip.text.y.right = el_def(element_text, "strip.text.y"), strip.placement = el_def("character"), strip.placement.x = el_def("character", "strip.placement"), strip.placement.y = el_def("character", "strip.placement"), strip.switch.pad.grid = el_def(c("unit", "rel"), "spacing"), strip.switch.pad.wrap = el_def(c("unit", "rel"), "spacing"), - plot.background = el_def("element_rect", "rect"), - plot.title = el_def("element_text", "title"), + plot.background = el_def(element_rect, "rect"), + plot.title = el_def(element_text, "title"), plot.title.position = el_def("character"), - plot.subtitle = el_def("element_text", "text"), - plot.caption = el_def("element_text", "text"), + plot.subtitle = el_def(element_text, "text"), + plot.caption = el_def(element_text, "text"), plot.caption.position = el_def("character"), - plot.tag = el_def("element_text", "text"), + plot.tag = el_def(element_text, "text"), plot.tag.position = el_def(c("character", "numeric", "integer")), # Need to also accept numbers plot.tag.location = el_def("character"), plot.margin = el_def(c("margin", "unit", "rel"), "margins"), @@ -849,11 +849,18 @@ check_element <- function(el, elname, element_tree, call = caller_env()) { # NULL values for elements are OK if (is.null(el)) return() + class <- eldef$class + if (inherits(class, "S7_class") && S7::S7_inherits(el)) { + if (S7::S7_inherits(el, class) || + (S7::S7_inherits(el, element) && S7::S7_inherits(el, element_blank))) { + return() + } + } - if ("margin" %in% eldef$class) { + if ("margin" %in% class) { if (!is.unit(el) && length(el) == 4) cli::cli_abort("The {.var {elname}} theme element must be a {.cls unit} vector of length 4.", call = call) - } else if (!inherits(el, eldef$class) && !inherits(el, "element_blank")) { + } else if (!inherits(el, class) && !inherits(el, "element_blank")) { cli::cli_abort("The {.var {elname}} theme element must be a {.cls {eldef$class}} object.", call = call) } invisible() diff --git a/man/register_theme_elements.Rd b/man/register_theme_elements.Rd index cdbbb25d70..0cb822e686 100644 --- a/man/register_theme_elements.Rd +++ b/man/register_theme_elements.Rd @@ -27,8 +27,8 @@ a list of named element definitions created with el_def().} \item{reset_current}{If \code{TRUE} (the default), the currently active theme is reset to the default theme.} -\item{class}{The name of the element class. Examples are "element_line" or -"element_text" or "unit", or one of the two reserved keywords "character" or +\item{class}{The name of the element class. Examples are \code{element_line} or +\code{element_text} or "unit", or one of the two reserved keywords "character" or "margin". The reserved keyword "character" implies a character or numeric vector, not a class called "character". The keyword "margin" implies a unit vector of length 4, as created by \code{\link[=margin]{margin()}}.} @@ -76,7 +76,7 @@ element inheritance relationships for the element tree. # plot panels. To do so, it registers a new theme element `ggxyz.panel.annotation` register_theme_elements( ggxyz.panel.annotation = element_text(color = "blue", hjust = 0.95, vjust = 0.05), - element_tree = list(ggxyz.panel.annotation = el_def("element_text", "text")) + element_tree = list(ggxyz.panel.annotation = el_def(element_text, "text")) ) # Now the package can define a new coord that includes a panel annotation From 202b80d36ec06db7984ffab6083c5797576d9e21 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Wed, 5 Mar 2025 14:28:17 +0100 Subject: [PATCH 05/22] convert element_grob to S7 generic --- NAMESPACE | 6 -- R/theme-elements.R | 211 ++++++++++++++++++++++----------------------- R/zzz.R | 1 + 3 files changed, 103 insertions(+), 115 deletions(-) diff --git a/NAMESPACE b/NAMESPACE index 5e4384df4e..0d77b59701 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -18,12 +18,6 @@ S3method(autolayer,default) S3method(autoplot,default) S3method(c,mapped_discrete) S3method(drawDetails,zeroGrob) -S3method(element_grob,element_blank) -S3method(element_grob,element_line) -S3method(element_grob,element_point) -S3method(element_grob,element_polygon) -S3method(element_grob,element_rect) -S3method(element_grob,element_text) S3method(format,ggproto) S3method(format,ggproto_method) S3method(fortify,"NULL") diff --git a/R/theme-elements.R b/R/theme-elements.R index 5e3a016e69..b451a963cc 100644 --- a/R/theme-elements.R +++ b/R/theme-elements.R @@ -321,134 +321,127 @@ element_render <- function(theme, element, ..., name = NULL) { #' usually at least position. See the source code for individual methods. #' @keywords internal #' @export -element_grob <- function(element, ...) { - UseMethod("element_grob") -} +element_grob <- S7::new_generic("element_grob", "element") -#' @export -element_grob.element_blank <- function(element, ...) zeroGrob() +S7::method(element_grob, element_blank) <- function(element, ...) zeroGrob() -#' @export -element_grob.element_rect <- function(element, x = 0.5, y = 0.5, - width = 1, height = 1, - fill = NULL, colour = NULL, linewidth = NULL, linetype = NULL, ..., size = deprecated()) { +S7::method(element_grob, element_rect) <- + function(element, x = 0.5, y = 0.5, width = 1, height = 1, + fill = NULL, colour = NULL, linewidth = NULL, linetype = NULL, + ..., size = deprecated()) { - if (lifecycle::is_present(size)) { - deprecate_soft0("3.4.0", "element_grob.element_rect(size)", "element_grob.element_rect(linewidth)") - linewidth <- size - } + if (lifecycle::is_present(size)) { + deprecate_soft0("3.4.0", "element_grob.element_rect(size)", "element_grob.element_rect(linewidth)") + linewidth <- size + } - # The gp settings can override element_gp - gp <- gg_par(lwd = linewidth, col = colour, fill = fill, lty = linetype) - element_gp <- gg_par(lwd = element$linewidth, col = element$colour, - fill = element$fill, lty = element$linetype) + gp <- gg_par(lwd = linewidth, col = colour, fill = fill, lty = linetype) + element_gp <- gg_par(lwd = element@linewidth, col = element@colour, + fill = element@fill, lty = element@linetype) - rectGrob(x, y, width, height, gp = modify_list(element_gp, gp), ...) -} + rectGrob(x, y, width, height, gp = modify_list(element_gp, gp), ...) + } +S7::method(element_grob, element_text) <- + function(element, label = "", x = NULL, y = NULL, + family = NULL, face = NULL, colour = NULL, size = NULL, + hjust = NULL, vjust = NULL, angle = NULL, lineheight = NULL, + margin = NULL, margin_x = FALSE, margin_y = FALSE, ...) { -#' @export -element_grob.element_text <- function(element, label = "", x = NULL, y = NULL, - family = NULL, face = NULL, colour = NULL, size = NULL, - hjust = NULL, vjust = NULL, angle = NULL, lineheight = NULL, - margin = NULL, margin_x = FALSE, margin_y = FALSE, ...) { + if (is.null(label)) + return(zeroGrob()) - if (is.null(label)) - return(zeroGrob()) + vj <- vjust %||% element@vjust + hj <- hjust %||% element@hjust + margin <- margin %||% element@margin - vj <- vjust %||% element$vjust - hj <- hjust %||% element$hjust - margin <- margin %||% element$margin + angle <- angle %||% element@angle %||% 0 - angle <- angle %||% element$angle %||% 0 + # The gp settings can override element_gp + gp <- gg_par(fontsize = size, col = colour, + fontfamily = family, fontface = face, + lineheight = lineheight) + element_gp <- gg_par(fontsize = element@size, col = element@colour, + fontfamily = element@family, fontface = element@face, + lineheight = element@lineheight) - # The gp settings can override element_gp - gp <- gg_par(fontsize = size, col = colour, - fontfamily = family, fontface = face, - lineheight = lineheight) - element_gp <- gg_par(fontsize = element$size, col = element$colour, - fontfamily = element$family, fontface = element$face, - lineheight = element$lineheight) + titleGrob(label, x, y, hjust = hj, vjust = vj, angle = angle, + gp = modify_list(element_gp, gp), margin = margin, + margin_x = margin_x, margin_y = margin_y, debug = element@debug, ...) + } - titleGrob(label, x, y, hjust = hj, vjust = vj, angle = angle, - gp = modify_list(element_gp, gp), margin = margin, - margin_x = margin_x, margin_y = margin_y, debug = element$debug, ...) -} +S7::method(element_grob, element_line) <- + function(element, x = 0:1, y = 0:1, + colour = NULL, linewidth = NULL, linetype = NULL, lineend = NULL, + arrow.fill = NULL, + default.units = "npc", id.lengths = NULL, ..., size = deprecated()) { + if (lifecycle::is_present(size)) { + deprecate_soft0("3.4.0", "element_grob.element_line(size)", "element_grob.element_line(linewidth)") + linewidth <- size + } + arrow <- if (is.logical(element@arrow) && !element@arrow) { + NULL + } else { + element@arrow + } + if (is.null(arrow)) { + arrow.fill <- colour + element@arrow.fill <- element@colour + } -#' @export -element_grob.element_line <- function(element, x = 0:1, y = 0:1, - colour = NULL, linewidth = NULL, linetype = NULL, lineend = NULL, - arrow.fill = NULL, - default.units = "npc", id.lengths = NULL, ..., size = deprecated()) { - - if (lifecycle::is_present(size)) { - deprecate_soft0("3.4.0", "element_grob.element_line(size)", "element_grob.element_line(linewidth)") - linewidth <- size - } + # The gp settings can override element_gp + gp <- gg_par( + col = colour, fill = arrow.fill %||% colour, + lwd = linewidth, lty = linetype, lineend = lineend + ) + element_gp <- gg_par( + col = element@colour, fill = element@arrow.fill %||% element@colour, + lwd = element@linewidth, lty = element@linetype, + lineend = element@lineend + ) - arrow <- if (is.logical(element$arrow) && !element$arrow) { - NULL - } else { - element$arrow - } - if (is.null(arrow)) { - arrow.fill <- colour - element$arrow.fill <- element$colour + polylineGrob( + x, y, default.units = default.units, + gp = modify_list(element_gp, gp), + id.lengths = id.lengths, arrow = arrow, ... + ) } - # The gp settings can override element_gp - gp <- gg_par( - col = colour, fill = arrow.fill %||% colour, - lwd = linewidth, lty = linetype, lineend = lineend - ) - element_gp <- gg_par( - col = element$colour, fill = element$arrow.fill %||% element$colour, - lwd = element$linewidth, lty = element$linetype, - lineend = element$lineend - ) - - polylineGrob( - x, y, default.units = default.units, - gp = modify_list(element_gp, gp), - id.lengths = id.lengths, arrow = arrow, ... - ) -} - -#' @export -element_grob.element_polygon <- function(element, x = c(0, 0.5, 1, 0.5), - y = c(0.5, 1, 0.5, 0), fill = NULL, - colour = NULL, linewidth = NULL, - linetype = NULL, ..., - id = NULL, id.lengths = NULL, - pathId = NULL, pathId.lengths = NULL) { - - gp <- gg_par(lwd = linewidth, col = colour, fill = fill, lty = linetype) - element_gp <- gg_par(lwd = element$linewidth, col = element$colour, - fill = element$fill, lty = element$linetype) - pathGrob( - x = x, y = y, gp = modify_list(element_gp, gp), ..., - # We swap the id logic so that `id` is always the (super)group id - # (consistent with `polygonGrob()`) and `pathId` always the subgroup id. - pathId = id, pathId.lengths = id.lengths, - id = pathId, id.lengths = pathId.lengths - ) -} +S7::method(element_grob, element_polygon) <- + function(element, x = c(0, 0.5, 1, 0.5), + y = c(0.5, 1, 0.5, 0), fill = NULL, + colour = NULL, linewidth = NULL, + linetype = NULL, ..., + id = NULL, id.lengths = NULL, + pathId = NULL, pathId.lengths = NULL) { + + gp <- gg_par(lwd = linewidth, col = colour, fill = fill, lty = linetype) + element_gp <- gg_par(lwd = element@linewidth, col = element@colour, + fill = element@fill, lty = element@linetype) + pathGrob( + x = x, y = y, gp = modify_list(element_gp, gp), ..., + # We swap the id logic so that `id` is always the (super)group id + # (consistent with `polygonGrob()`) and `pathId` always the subgroup id. + pathId = id, pathId.lengths = id.lengths, + id = pathId, id.lengths = pathId.lengths + ) + } -#' @export -element_grob.element_point <- function(element, x = 0.5, y = 0.5, colour = NULL, - shape = NULL, fill = NULL, size = NULL, - stroke = NULL, ..., - default.units = "npc") { - - gp <- gg_par(col = colour, fill = fill, pointsize = size, stroke = stroke) - element_gp <- gg_par(col = element$colour, fill = element$fill, - pointsize = element$size, stroke = element$stroke) - shape <- translate_shape_string(shape %||% element$shape %||% 19) - pointsGrob(x = x, y = y, pch = shape, gp = modify_list(element_gp, gp), - default.units = default.units, ...) -} +S7::method(element_grob, element_point) <- + function(element, x = 0.5, y = 0.5, colour = NULL, + shape = NULL, fill = NULL, size = NULL, + stroke = NULL, ..., + default.units = "npc") { + + gp <- gg_par(col = colour, fill = fill, pointsize = size, stroke = stroke) + element_gp <- gg_par(col = element@colour, fill = element@fill, + pointsize = element@size, stroke = element@stroke) + shape <- translate_shape_string(shape %||% element@shape %||% 19) + pointsGrob(x = x, y = y, pch = shape, gp = modify_list(element_gp, gp), + default.units = default.units, ...) + } #' Define and register new theme elements #' diff --git a/R/zzz.R b/R/zzz.R index 398cb7d7b6..249d96a1be 100644 --- a/R/zzz.R +++ b/R/zzz.R @@ -30,6 +30,7 @@ on_load( vars <- dplyr::vars } ) +on_load(S7::methods_register()) .onLoad <- function(...) { run_on_load() } From df02b37acd372d5bf0bfaa02fb119079e7fd96db Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Wed, 5 Mar 2025 17:37:27 +0100 Subject: [PATCH 06/22] replace merge_element by S7 --- NAMESPACE | 4 -- R/theme.R | 124 ++++++++++++++++++++----------------------- man/merge_element.Rd | 14 +---- 3 files changed, 60 insertions(+), 82 deletions(-) diff --git a/NAMESPACE b/NAMESPACE index 0d77b59701..04986e15bb 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -87,10 +87,6 @@ S3method(limits,character) S3method(limits,factor) S3method(limits,numeric) S3method(makeContext,dotstackGrob) -S3method(merge_element,default) -S3method(merge_element,element) -S3method(merge_element,element_blank) -S3method(merge_element,margin) S3method(pattern_alpha,GridPattern) S3method(pattern_alpha,GridTilingPattern) S3method(pattern_alpha,default) diff --git a/R/theme.R b/R/theme.R index 1c16c85656..3478c45b7c 100644 --- a/R/theme.R +++ b/R/theme.R @@ -797,69 +797,63 @@ calc_element <- function(element, theme, verbose = FALSE, skip_blank = FALSE, #' # Adopt size but ignore colour #' merge_element(new, old) #' -merge_element <- function(new, old) { - UseMethod("merge_element") -} +merge_element <- S7::new_generic("merge_element", c("new", "old")) + +S7::method(merge_element, list(S7::class_any, S7::class_any)) <- + function(new, old, ...) { + if (is.null(old) || S7::S7_inherits(old, element_blank)) { + # If old is NULL or element_blank, then just return new + return(new) + } else if (is.null(new) || is.character(new) || is.numeric(new) || is.unit(new) || + is.logical(new)) { + # If new is NULL, or a string, numeric vector, unit, or logical, just return it + return(new) + } -#' @rdname merge_element -#' @export -merge_element.default <- function(new, old) { - if (is.null(old) || inherits(old, "element_blank")) { - # If old is NULL or element_blank, then just return new - return(new) - } else if (is.null(new) || is.character(new) || is.numeric(new) || is.unit(new) || - is.logical(new)) { - # If new is NULL, or a string, numeric vector, unit, or logical, just return it - return(new) + # otherwise we can't merge + cli::cli_abort("No method for merging {.cls {class(new)[1]}} into {.cls {class(old)[1]}}.") } - # otherwise we can't merge - cli::cli_abort("No method for merging {.cls {class(new)[1]}} into {.cls {class(old)[1]}}.") -} - -#' @rdname merge_element -#' @export -merge_element.element_blank <- function(new, old) { - # If new is element_blank, just return it - new -} - -#' @rdname merge_element -#' @export -merge_element.element <- function(new, old) { - if (is.null(old) || inherits(old, "element_blank")) { - # If old is NULL or element_blank, then just return new - return(new) +S7::method(merge_element, list(element_blank, S7::class_any)) <- + function(new, old, ...) { + # If new is element_blank, just return it + new } - # actual merging can only happen if classes match - if (!inherits(new, class(old)[1])) { - cli::cli_abort("Only elements of the same class can be merged.") - } +S7::method(merge_element, list(element, S7::class_any)) <- + function(new, old, ...) { + if (is.null(old) || S7::S7_inherits(old, element_blank)) { + # If old is NULL or element_blank, then just return new + return(new) + } - # Override NULL properties of new with the values in old - # Get logical vector of NULL properties in new - idx <- vapply(new, is.null, logical(1)) - # Get the names of TRUE items - idx <- names(idx[idx]) + # actual merging can only happen if classes match + if (!inherits(new, class(old)[1])) { + cli::cli_abort("Only elements of the same class can be merged.") + } - # Update non-NULL items - new[idx] <- old[idx] + # Override NULL properties of new with the values in old + # Get logical vector of NULL properties in new + idx <- lengths(S7::props(new)) == 0 + # Get the names of TRUE items + idx <- names(idx[idx]) - new + # Update non-NULL items + S7::props(new)[idx] <- S7::props(old, idx) + + new } -#' @rdname merge_element -#' @export -merge_element.margin <- function(new, old) { - if (is.null(old) || inherits(old, "element_blank")) { - return(new) - } - if (anyNA(new)) { - new[is.na(new)] <- old[is.na(new)] +S7::method(merge_element, list(S7::new_S3_class("margin"), S7::class_any)) <- + function(new, old, ...) { + if (is.null(old) || S7::S7_inherits(old, element_blank)) { + return(new) + } + if (anyNA(new)) { + new[is.na(new)] <- old[is.na(new)] + } + new } - new -} #' Combine the properties of two elements #' @@ -871,7 +865,7 @@ merge_element.margin <- function(new, old) { combine_elements <- function(e1, e2) { # If e2 is NULL, nothing to inherit - if (is.null(e2) || inherits(e1, "element_blank")) { + if (is.null(e2) || S7::S7_inherits(e1, element_blank)) { return(e1) } @@ -904,14 +898,14 @@ combine_elements <- function(e1, e2) { } # If neither of e1 or e2 are element_* objects, return e1 - if (!inherits(e1, "element") && !inherits(e2, "element")) { + if (!S7::S7_inherits(e1, element) && !S7::S7_inherits(e2, element)) { return(e1) } # If e2 is element_blank, and e1 inherits blank inherit everything from e2, # otherwise ignore e2 - if (inherits(e2, "element_blank")) { - if (e1$inherit.blank) { + if (S7::S7_inherits(e2, element_blank)) { + if (S7::prop_exists(e1, "inherit.blank") && e1@inherit.blank) { return(e2) } else { return(e1) @@ -919,29 +913,29 @@ combine_elements <- function(e1, e2) { } # If e1 has any NULL properties, inherit them from e2 - n <- names(e1)[vapply(e1, is.null, logical(1))] - e1[n] <- e2[n] + n <- S7::prop_names(e1)[lengths(S7::props(e1)) == 0] + S7::props(e1)[n] <- S7::props(e2)[n] # Calculate relative sizes - if (is.rel(e1$size)) { - e1$size <- e2$size * unclass(e1$size) + if (S7::prop_exists(e1, "size") && is.rel(e1@size)) { + e1@size <- e2@size * unclass(e1@size) } # Calculate relative linewidth - if (is.rel(e1$linewidth)) { - e1$linewidth <- e2$linewidth * unclass(e1$linewidth) + if (S7::prop_exists(e1, "linewidth") && is.rel(e1@linewidth)) { + e1@linewidth <- e2@linewidth * unclass(e1@linewidth) } if (inherits(e1, "element_text")) { - e1$margin <- combine_elements(e1$margin, e2$margin) + e1@margin <- combine_elements(e1@margin, e2@margin) } # If e2 is 'richer' than e1, fill e2 with e1 parameters is_subclass <- !any(inherits(e2, class(e1), which = TRUE) == 0) is_subclass <- is_subclass && length(setdiff(class(e2), class(e1)) > 0) if (is_subclass) { - new <- defaults(e1, e2) - e2[names(new)] <- new + new <- defaults(S7::props(e1), S7::props(e2)) + S7::props(e2)[names(new)] <- new return(e2) } diff --git a/man/merge_element.Rd b/man/merge_element.Rd index ca993eeec3..3060360dc0 100644 --- a/man/merge_element.Rd +++ b/man/merge_element.Rd @@ -2,21 +2,9 @@ % Please edit documentation in R/theme.R \name{merge_element} \alias{merge_element} -\alias{merge_element.default} -\alias{merge_element.element_blank} -\alias{merge_element.element} -\alias{merge_element.margin} \title{Merge a parent element into a child element} \usage{ -merge_element(new, old) - -\method{merge_element}{default}(new, old) - -\method{merge_element}{element_blank}(new, old) - -\method{merge_element}{element}(new, old) - -\method{merge_element}{margin}(new, old) +merge_element(new, old, ...) } \arguments{ \item{new}{The child element in the theme hierarchy} From b038e8f633d8526f6e20ee9bf4549634cc38d565 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Thu, 6 Mar 2025 10:49:48 +0100 Subject: [PATCH 07/22] implement `$.element` for backward compatibility --- NAMESPACE | 6 +++--- R/ggproto.R | 5 +++-- R/theme-elements.R | 8 ++++++++ R/theme.R | 3 ++- 4 files changed, 16 insertions(+), 6 deletions(-) diff --git a/NAMESPACE b/NAMESPACE index 04986e15bb..a88e3f341d 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -1,8 +1,5 @@ # Generated by roxygen2: do not edit by hand -S3method("$",ggproto) -S3method("$",ggproto_parent) -S3method("$",theme) S3method("$<-",uneval) S3method("+",gg) S3method("[",mapped_discrete) @@ -16,6 +13,9 @@ S3method(as.data.frame,mapped_discrete) S3method(as.list,ggproto) S3method(autolayer,default) S3method(autoplot,default) +S3method(base::`$`, ggproto) +S3method(base::`$`, ggproto_parent) +S3method(base::`$`, theme) S3method(c,mapped_discrete) S3method(drawDetails,zeroGrob) S3method(format,ggproto) diff --git a/R/ggproto.R b/R/ggproto.R index 6165a9707d..3de50401c3 100644 --- a/R/ggproto.R +++ b/R/ggproto.R @@ -152,7 +152,8 @@ fetch_ggproto <- function(x, name) { } -#' @export +# Prevents bug described in S7/#390 +#' @rawNamespace S3method(base::`$`, ggproto) `$.ggproto` <- function(x, name) { res <- fetch_ggproto(x, name) if (!is.function(res)) { @@ -162,7 +163,7 @@ fetch_ggproto <- function(x, name) { make_proto_method(x, res, name) } -#' @export +#' @rawNamespace S3method(base::`$`, ggproto_parent) `$.ggproto_parent` <- function(x, name) { res <- fetch_ggproto(.subset2(x, "parent"), name) if (!is.function(res)) { diff --git a/R/theme-elements.R b/R/theme-elements.R index b451a963cc..4118948e07 100644 --- a/R/theme-elements.R +++ b/R/theme-elements.R @@ -83,6 +83,14 @@ NULL #' @rdname element element <- S7::new_class("element", abstract = TRUE) +S7::method(`$`, element) <- + function(x, i) { + if (!S7::prop_exists(x, i)) { + return(NULL) + } + S7::prop(x, i) + } + #' @export #' @rdname element element_blank <- S7::new_class("element_blank", parent = element) diff --git a/R/theme.R b/R/theme.R index 3478c45b7c..d63b126e84 100644 --- a/R/theme.R +++ b/R/theme.R @@ -942,7 +942,8 @@ combine_elements <- function(e1, e2) { e1 } -#' @export +# Prevents bug described in S7/#390 +#' @rawNamespace S3method(base::`$`, theme) `$.theme` <- function(x, ...) { .subset2(x, ...) } From 09eef8dae6931296b0123794e23a1259466ea928 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Thu, 6 Mar 2025 10:56:02 +0100 Subject: [PATCH 08/22] Use S7 properties --- R/coord-sf.R | 8 +-- R/geom-.R | 2 +- R/guide-axis-theta.R | 8 +-- R/guide-axis.R | 10 ++-- R/guide-custom.R | 2 +- R/guide-legend.R | 4 +- R/plot-build.R | 8 +-- R/theme.R | 12 +++- tests/testthat/_snaps/theme.md | 4 +- tests/testthat/test-theme.R | 100 ++++++++++++++++----------------- 10 files changed, 83 insertions(+), 75 deletions(-) diff --git a/R/coord-sf.R b/R/coord-sf.R index d603d57de7..0bcfafa4c3 100644 --- a/R/coord-sf.R +++ b/R/coord-sf.R @@ -334,13 +334,13 @@ CoordSf <- ggproto("CoordSf", CoordCartesian, # we don't draw the graticules if the major panel grid is # turned off - if (inherits(el, "element_blank")) { + if (S7::S7_inherits(el, element_blank)) { grobs <- list(element_render(theme, "panel.background")) } else { line_gp <- gg_par( - col = el$colour, - lwd = el$linewidth, - lty = el$linetype + col = el@colour, + lwd = el@linewidth, + lty = el@linetype ) grobs <- c( list(element_render(theme, "panel.background")), diff --git a/R/geom-.R b/R/geom-.R index 843bd4c11c..52e36b9121 100644 --- a/R/geom-.R +++ b/R/geom-.R @@ -248,7 +248,7 @@ eval_from_theme <- function(aesthetics, theme) { return(aesthetics) } settings <- calc_element("geom", theme) %||% .default_geom_element - lapply(aesthetics[themed], eval_tidy, data = settings) + lapply(aesthetics[themed], eval_tidy, data = S7::props(settings)) } #' Graphical units diff --git a/R/guide-axis-theta.R b/R/guide-axis-theta.R index af96a337b6..492fd53482 100644 --- a/R/guide-axis-theta.R +++ b/R/guide-axis-theta.R @@ -153,7 +153,7 @@ GuideAxisTheta <- ggproto( } offset <- max(unit(0, "pt"), elements$major_length, elements$minor_length) - elements$offset <- offset + max(elements$text$margin %||% unit(0, "pt")) + elements$offset <- offset + max(elements$text@margin %||% unit(0, "pt")) elements }, @@ -197,7 +197,7 @@ GuideAxisTheta <- ggproto( # Resolve text angle if (is.waiver(params$angle) || is.null(params$angle)) { - angle <- elements$text$angle + angle <- elements$text@angle } else { angle <- flip_text_angle(params$angle - rad2deg(key$theta)) } @@ -273,14 +273,14 @@ GuideAxisTheta <- ggproto( # Resolve text angle if (is.waiver(params$angle %||% waiver())) { - angle <- elements$text$angle + angle <- elements$text@angle } else { angle <- flip_text_angle(params$angle - rad2deg(key$theta)) } angle <- key$theta + deg2rad(angle) # Set margin - margin <- rep(max(elements$text$margin), length.out = 4) + margin <- rep(max(elements$text@margin), length.out = 4) # Measure size of each individual label single_labels <- lapply(labels, function(lab) { diff --git a/R/guide-axis.R b/R/guide-axis.R index d445900071..32398bf3bc 100644 --- a/R/guide-axis.R +++ b/R/guide-axis.R @@ -450,13 +450,13 @@ GuideAxis <- ggproto( # rather than dimensions of this axis alone. if (has_labels && params$position %in% c("left", "right")) { where <- layout$l[-c(1, length(layout$l))] - just <- with(elements$text, rotate_just(angle, hjust, vjust))$hjust %||% 0.5 + just <- with(S7::props(elements$text), rotate_just(angle, hjust, vjust))$hjust %||% 0.5 gt <- gtable_add_cols(gt, unit(just, "null"), pos = min(where) - 1) gt <- gtable_add_cols(gt, unit(1 - just, "null"), pos = max(where) + 1) } if (has_labels && params$position %in% c("top", "bottom")) { where <- layout$t[-c(1, length(layout$t))] - just <- with(elements$text, rotate_just(angle, hjust, vjust))$vjust %||% 0.5 + just <- with(S7::props(elements$text), rotate_just(angle, hjust, vjust))$vjust %||% 0.5 gt <- gtable_add_rows(gt, unit(1 - just, "null"), pos = min(where) - 1) gt <- gtable_add_rows(gt, unit(just, "null"), pos = max(where) + 1) } @@ -612,8 +612,8 @@ label_angle_heuristic <- function(element, position, angle) { hjust <- switch(position, left = cosine, right = 1 - cosine, top = 1 - sine, sine) vjust <- switch(position, left = 1 - sine, right = sine, top = 1 - cosine, cosine) - element$angle <- angle %||% element$angle - element$hjust <- hjust %||% element$hjust - element$vjust <- vjust %||% element$vjust + element@angle <- angle %||% element@angle + element@hjust <- hjust %||% element@hjust + element@vjust <- vjust %||% element@vjust element } diff --git a/R/guide-custom.R b/R/guide-custom.R index f602bfc843..1a6d977c7f 100644 --- a/R/guide-custom.R +++ b/R/guide-custom.R @@ -113,7 +113,7 @@ GuideCustom <- ggproto( gt <- self$add_title( gt, title, title_position, - with(elems$title, rotate_just(angle, hjust, vjust)) + with(S7::props(elems$title), rotate_just(angle, hjust, vjust)) ) # Add padding and background diff --git a/R/guide-legend.R b/R/guide-legend.R index c8bf395f0a..b37e210a78 100644 --- a/R/guide-legend.R +++ b/R/guide-legend.R @@ -325,7 +325,7 @@ GuideLegend <- ggproto( # Resolve title. The trick here is to override the main text element, so # that any settings declared in `legend.title` will be honoured but we have # custom defaults for the guide. - margin <- calc_element("text", theme)$margin + margin <- calc_element("text", theme)@margin title <- theme(text = element_text( hjust = 0, vjust = 0.5, margin = position_margin(title_position, margin, gap) @@ -573,7 +573,7 @@ GuideLegend <- ggproto( gt <- self$add_title( gt, grobs$title, elements$title_position, - with(elements$title, rotate_just(angle, hjust, vjust)) + with(S7::props(elements$title), rotate_just(angle, hjust, vjust)) ) gt <- gtable_add_padding(gt, unit(elements$padding, "cm")) diff --git a/R/plot-build.R b/R/plot-build.R index f855dddd78..45cf53571c 100644 --- a/R/plot-build.R +++ b/R/plot-build.R @@ -387,20 +387,20 @@ table_add_tag <- function(table, label, theme) { if (location %in% c("plot", "panel")) { if (!is.numeric(position)) { if (right || left) { - x <- (1 - element$hjust) * width + x <- (1 - element@hjust) * width if (right) { x <- unit(1, "npc") - x } } else { - x <- unit(element$hjust, "npc") + x <- unit(element@hjust, "npc") } if (top || bottom) { - y <- (1 - element$vjust) * height + y <- (1 - element@vjust) * height if (top) { y <- unit(1, "npc") - y } } else { - y <- unit(element$vjust, "npc") + y <- unit(element@vjust, "npc") } } else { x <- unit(position[1], "npc") diff --git a/R/theme.R b/R/theme.R index d63b126e84..dd08ceb926 100644 --- a/R/theme.R +++ b/R/theme.R @@ -746,14 +746,22 @@ calc_element <- function(element, theme, verbose = FALSE, skip_blank = FALSE, if (verbose) cli::cli_inform("nothing (top level)") # Check that all the properties of this element are non-NULL - nullprops <- vapply(el_out, is.null, logical(1)) + if (inherits(el_out, "ggplot2::element")) { + nullprops <- lengths(S7::props(el_out)) == 0 + } else { + nullprops <- vapply(el_out, is.null, logical(1)) + } if (!any(nullprops)) { return(el_out) # no null properties, return element as is } # if we have null properties, try to fill in from ggplot_global$theme_default el_out <- combine_elements(el_out, ggplot_global$theme_default[[element]]) - nullprops <- vapply(el_out, is.null, logical(1)) + if (inherits(el_out, "ggplot2::element")) { + nullprops <- lengths(S7::props(el_out)) == 0 + } else { + nullprops <- vapply(el_out, is.null, logical(1)) + } if (!any(nullprops)) { return(el_out) # no null properties remaining, return element } diff --git a/tests/testthat/_snaps/theme.md b/tests/testthat/_snaps/theme.md index 005e1b2abd..37657e09b5 100644 --- a/tests/testthat/_snaps/theme.md +++ b/tests/testthat/_snaps/theme.md @@ -25,7 +25,7 @@ # incorrect theme specifications throw meaningful errors Can't merge the `line` theme element. - Caused by error in `merge_element()`: + Caused by error in `method(merge_element, list(ggplot2::element, class_any))`: ! Only elements of the same class can be merged. --- @@ -74,7 +74,7 @@ Code merge_element(text_base, rect_base) Condition - Error in `merge_element()`: + Error in `method(merge_element, list(ggplot2::element, class_any))`: ! Only elements of the same class can be merged. # Theme elements are checked during build diff --git a/tests/testthat/test-theme.R b/tests/testthat/test-theme.R index 8d74b4038f..86cb19853f 100644 --- a/tests/testthat/test-theme.R +++ b/tests/testthat/test-theme.R @@ -30,10 +30,10 @@ test_that("modifying theme element properties with + operator works", { # Modifying a root node changes only the specified properties t <- theme_grey() + theme(text = element_text(colour = 'red')) - expect_identical(t$text$colour, 'red') - expect_identical(t$text$family, theme_grey()$text$family) - expect_identical(t$text$face, theme_grey()$text$face) - expect_identical(t$text$size, theme_grey()$text$size) + expect_identical(t$text@colour, 'red') + expect_identical(t$text@family, theme_grey()$text@family) + expect_identical(t$text@face, theme_grey()$text@face) + expect_identical(t$text@size, theme_grey()$text@size) # Descendent is unchanged expect_identical(t$axis.title.x, theme_grey()$axis.title.x) @@ -56,34 +56,34 @@ test_that("adding theme object to ggplot object with + operator works", { ## test with complete theme p <- ggplot(data.frame(x = 1:3), aes(x, x)) + geom_point() + theme_grey() p <- p + theme(axis.title = element_text(size = 20)) - expect_true(p$theme$axis.title$size == 20) + expect_true(p$theme$axis.title@size == 20) # Should update specified properties, but not reset other properties p <- p + theme(text = element_text(colour = 'red')) - expect_true(p$theme$text$colour == 'red') + expect_true(p$theme$text@colour == 'red') tt <- theme_grey()$text - tt$colour <- 'red' - expect_true(tt$inherit.blank) - tt$inherit.blank <- FALSE + tt@colour <- 'red' + expect_true(tt@inherit.blank) + tt@inherit.blank <- FALSE expect_identical(p$theme$text, tt) ## test without complete theme p <- ggplot(data.frame(x = 1:3), aes(x, x)) + geom_point() p <- p + theme(axis.title = element_text(size = 20)) - expect_true(p$theme$axis.title$size == 20) + expect_true(p$theme$axis.title@size == 20) # Should update specified properties, but not reset other properties p <- p + theme(text = element_text(colour = 'red')) - expect_true(p$theme$text$colour == 'red') - expect_null(p$theme$text$family) - expect_null(p$theme$text$face) - expect_null(p$theme$text$size) - expect_null(p$theme$text$hjust) - expect_null(p$theme$text$vjust) - expect_null(p$theme$text$angle) - expect_null(p$theme$text$lineheight) - expect_null(p$theme$text$margin) - expect_null(p$theme$text$debug) + expect_true(p$theme$text@colour == 'red') + expect_null(p$theme$text@family) + expect_null(p$theme$text@face) + expect_null(p$theme$text@size) + expect_null(p$theme$text@hjust) + expect_null(p$theme$text@vjust) + expect_null(p$theme$text@angle) + expect_null(p$theme$text@lineheight) + expect_null(p$theme$text@margin) + expect_null(p$theme$text@debug) ## stepwise addition of partial themes is identical to one-step addition p <- ggplot(data.frame(x = 1:3), aes(x, x)) + geom_point() @@ -123,19 +123,19 @@ test_that("calculating theme element inheritance works", { # Check that properties are passed along from axis.title to axis.title.x e <- calc_element('axis.title.x', t) - expect_identical(e$colour, 'red') - expect_false(is.null(e$family)) - expect_false(is.null(e$face)) - expect_false(is.null(e$size)) + expect_identical(e@colour, 'red') + expect_false(is.null(e@family)) + expect_false(is.null(e@face)) + expect_false(is.null(e@size)) # Check that rel() works for relative sizing, and is applied at each level t <- theme_grey(base_size = 12) + theme(axis.title = element_text(size = rel(0.5))) + theme(axis.title.x = element_text(size = rel(0.5))) e <- calc_element('axis.title', t) - expect_identical(e$size, 6) + expect_identical(e@size, 6) ex <- calc_element('axis.title.x', t) - expect_identical(ex$size, 3) + expect_identical(ex@size, 3) # Check that a theme_blank in a parent node gets passed along to children t <- theme_grey() + theme(text = element_blank()) @@ -175,7 +175,7 @@ test_that("calculating theme element inheritance works", { ) e1 <- calc_element("strip.text.x", t) e2 <- calc_element("text", t) - e2$inherit.blank <- FALSE # b/c inherit.blank = TRUE for complete themes + e2@inherit.blank <- FALSE # b/c inherit.blank = TRUE for complete themes expect_identical(e1, e2) theme <- theme_gray() + @@ -201,18 +201,18 @@ test_that("complete and non-complete themes interact correctly with each other", # But for _element properties_, the one on the right modifies the one on the left. t <- theme_bw() + theme(text = element_text(colour = 'red')) expect_true(attr(t, "complete")) - expect_equal(t$text$colour, 'red') + expect_equal(t$text@colour, 'red') # A complete theme object (like theme_bw) always trumps a non-complete theme object t <- theme(text = element_text(colour = 'red')) + theme_bw() expect_true(attr(t, "complete")) - expect_equal(t$text$colour, theme_bw()$text$colour) + expect_equal(t$text@colour, theme_bw()$text@colour) # Adding two non-complete themes: the one on the right modifies the one on the left. t <- theme(text = element_text(colour = 'blue')) + theme(text = element_text(colour = 'red')) expect_false(attr(t, "complete")) - expect_equal(t$text$colour, 'red') + expect_equal(t$text@colour, 'red') }) test_that("complete and non-complete themes interact correctly with ggplot objects", { @@ -240,14 +240,14 @@ test_that("complete and non-complete themes interact correctly with ggplot objec expect_identical(pt, tt) p <- ggplot_build(base + theme(text = element_text(colour = 'red', face = 'italic'))) - expect_equal(p$plot$theme$text$colour, "red") - expect_equal(p$plot$theme$text$face, "italic") + expect_equal(p$plot$theme$text@colour, "red") + expect_equal(p$plot$theme$text@face, "italic") p <- ggplot_build(base + theme(text = element_text(colour = 'red')) + theme(text = element_text(face = 'italic'))) - expect_equal(p$plot$theme$text$colour, "red") - expect_equal(p$plot$theme$text$face, "italic") + expect_equal(p$plot$theme$text@colour, "red") + expect_equal(p$plot$theme$text@face, "italic") }) test_that("theme(validate=FALSE) means do not check_element", { @@ -327,11 +327,11 @@ test_that("element tree can be modified", { final_theme <- ggplot2:::plot_theme(p, theme_gray()) e1 <- calc_element("blablabla", final_theme) e2 <- calc_element("text", final_theme) - expect_identical(e1$family, e2$family) - expect_identical(e1$face, e2$face) - expect_identical(e1$size, e2$size) - expect_identical(e1$lineheight, e2$lineheight) - expect_identical(e1$colour, "red") # not inherited from element_text + expect_identical(e1@family, e2@family) + expect_identical(e1@face, e2@face) + expect_identical(e1@size, e2@size) + expect_identical(e1@lineheight, e2@lineheight) + expect_identical(e1@colour, "red") # not inherited from element_text # existing elements can be overwritten ed <- el_def("element_rect", "rect") @@ -394,7 +394,7 @@ test_that("complete plot themes shouldn't inherit from default", { base <- ggplot(data.frame(x = 1), aes(x, x)) + geom_point() ptheme <- plot_theme(base + theme(axis.text.x = element_text(colour = "blue")), default_theme) - expect_equal(ptheme$axis.text.x$colour, "blue") + expect_equal(ptheme$axis.text.x@colour, "blue") ptheme <- plot_theme(base + theme_void(), default_theme) expect_null(ptheme$axis.text.x) @@ -429,9 +429,9 @@ test_that("current theme can be updated with new elements", { e1 <- calc_element("abcde", plot_theme(b2)) e2 <- calc_element("text", plot_theme(b2)) - e2$colour <- "blue" - e2$hjust <- 0 - e2$vjust <- 1 + e2@colour <- "blue" + e2@hjust <- 0 + e2@vjust <- 1 expect_identical(e1, e2) reset_theme_settings() @@ -569,7 +569,7 @@ test_that("Element subclasses are inherited", { test <- combine_elements(poor, rich) expect_s3_class(test, "element_rich") expect_equal( - test[c("colour", "linetype", "linewidth")], + S7::props(test)[c("colour", "linetype", "linewidth")], list(colour = "red", linetype = 3, linewidth = 2) ) @@ -577,7 +577,7 @@ test_that("Element subclasses are inherited", { test <- combine_elements(rich, poor) expect_s3_class(test, "element_rich") expect_equal( - test[c("colour", "linetype", "linewidth")], + S7::props(test)[c("colour", "linetype", "linewidth")], list(colour = "red", linetype = 2, linewidth = 2) ) @@ -589,7 +589,7 @@ test_that("Element subclasses are inherited", { test <- combine_elements(sibling, rich) expect_s3_class(test, "element_sibling") expect_equal( - test[c("colour", "linetype", "linewidth")], + S7::props(test)[c("colour", "linetype", "linewidth")], list(colour = "red", linetype = 3, linewidth = 2) ) @@ -597,7 +597,7 @@ test_that("Element subclasses are inherited", { test <- combine_elements(rich, sibling) expect_s3_class(test, "element_rich") expect_equal( - test[c("colour", "linetype", "linewidth")], + S7::props(test)[c("colour", "linetype", "linewidth")], list(colour = "red", linetype = 2, linewidth = 2) ) }) @@ -624,10 +624,10 @@ test_that("header_family is passed on correctly", { td <- theme_dark(base_family = "x", header_family = "y") test <- calc_element("plot.title", td) - expect_equal(test$family, "y") + expect_equal(test@family, "y") test <- calc_element("plot.subtitle", td) - expect_equal(test$family, "x") + expect_equal(test@family, "x") }) test_that("complete_theme completes a theme", { @@ -638,7 +638,7 @@ test_that("complete_theme completes a theme", { # Elements are propagated new <- complete_theme(theme(axis.line = element_line("red")), gray) - expect_equal(new$axis.line$colour, "red") + expect_equal(new$axis.line@colour, "red") # Missing elements are filled in if default theme is incomplete new <- complete_theme(default = theme()) From 62d8db489ce59d06c764c528465362e63db8b968 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Thu, 6 Mar 2025 10:59:07 +0100 Subject: [PATCH 09/22] fix `inherits()` issues --- R/guide-.R | 2 +- R/guide-axis-logticks.R | 2 +- R/guide-axis-theta.R | 6 +++--- R/guide-axis.R | 8 ++++---- R/plot-build.R | 2 +- R/theme-elements.R | 7 +++++-- R/theme.R | 16 ++++++++++++---- tests/testthat/test-theme.R | 10 +++++----- 8 files changed, 32 insertions(+), 21 deletions(-) diff --git a/R/guide-.R b/R/guide-.R index 54cae7c873..813c722b4a 100644 --- a/R/guide-.R +++ b/R/guide-.R @@ -379,7 +379,7 @@ Guide <- ggproto( if (!is.theme_element(elements)) { elements <- elements$ticks } - if (!inherits(elements, "element_line")) { + if (!S7::S7_inherits(elements, element_line)) { return(zeroGrob()) } diff --git a/R/guide-axis-logticks.R b/R/guide-axis-logticks.R index 37273cba06..9da147870d 100644 --- a/R/guide-axis-logticks.R +++ b/R/guide-axis-logticks.R @@ -119,7 +119,7 @@ guide_axis_logticks <- function( allow_null = TRUE ) check_bool(expanded) - check_inherits(short.theme, c("element_blank", "element_line")) + check_inherits(short.theme, c("ggplot2::element_blank", "ggplot2::element_line")) new_guide( available_aes = c("x", "y"), diff --git a/R/guide-axis-theta.R b/R/guide-axis-theta.R index 492fd53482..678b75d2a8 100644 --- a/R/guide-axis-theta.R +++ b/R/guide-axis-theta.R @@ -183,7 +183,7 @@ GuideAxisTheta <- ggproto( build_labels = function(key, elements, params) { - if (inherits(elements$text, "element_blank")) { + if (S7::S7_inherits(elements$text, element_blank)) { return(zeroGrob()) } @@ -267,7 +267,7 @@ GuideAxisTheta <- ggproto( key <- params$key key <- vec_slice(key, !is.na(key$.label) & nzchar(key$.label)) labels <- validate_labels(key$.label) - if (length(labels) == 0 || inherits(elements$text, "element_blank")) { + if (length(labels) == 0 || S7::S7_inherits(elements$text, element_blank)) { return(list(offset = offset)) } @@ -364,7 +364,7 @@ GuideAxisTheta <- ggproto( theta_tickmarks <- function(key, element, length, offset = NULL) { n_breaks <- nrow(key) - if (n_breaks < 1 || inherits(element, "element_blank")) { + if (n_breaks < 1 || S7::S7_inherits(element, element_blank)) { return(zeroGrob()) } diff --git a/R/guide-axis.R b/R/guide-axis.R index 32398bf3bc..4cf00f2a25 100644 --- a/R/guide-axis.R +++ b/R/guide-axis.R @@ -259,10 +259,10 @@ GuideAxis <- ggproto( override_elements = function(params, elements, theme) { elements$text <- label_angle_heuristic(elements$text, params$position, params$angle) - if (inherits(elements$ticks, "element_blank")) { + if (S7::S7_inherits(elements$ticks, element_blank)) { elements$major_length <- unit(0, "cm") } - if (inherits(elements$minor, "element_blank") || isFALSE(params$minor.ticks)) { + if (S7::S7_inherits(elements$minor, element_blank) || isFALSE(params$minor.ticks)) { elements$minor_length <- unit(0, "cm") } return(elements) @@ -379,7 +379,7 @@ GuideAxis <- ggproto( # Ticks major_cm <- convertUnit(elements$major_length, "cm", valueOnly = TRUE) range <- range(0, major_cm) - if (params$minor.ticks && !inherits(elements$minor, "element_blank")) { + if (params$minor.ticks && !S7::S7_inherits(elements$minor, element_blank)) { minor_cm <- convertUnit(elements$minor_length, "cm", valueOnly = TRUE) range <- range(range, minor_cm) } @@ -590,7 +590,7 @@ axis_label_priority_between <- function(x, y) { #' overridden from the user- or theme-supplied element. #' @noRd label_angle_heuristic <- function(element, position, angle) { - if (!inherits(element, "element_text") + if (!S7::S7_inherits(element, element_text) || is.null(position) || is.null(angle %|W|% NULL)) { return(element) diff --git a/R/plot-build.R b/R/plot-build.R index 45cf53571c..40b99d46e5 100644 --- a/R/plot-build.R +++ b/R/plot-build.R @@ -342,7 +342,7 @@ table_add_tag <- function(table, label, theme) { return(table) } element <- calc_element("plot.tag", theme) - if (inherits(element, "element_blank")) { + if (S7::S7_inherits(element, element_blank)) { return(table) } diff --git a/R/theme-elements.R b/R/theme-elements.R index 4118948e07..e8bfba0132 100644 --- a/R/theme-elements.R +++ b/R/theme-elements.R @@ -861,8 +861,11 @@ check_element <- function(el, elname, element_tree, call = caller_env()) { if ("margin" %in% class) { if (!is.unit(el) && length(el) == 4) cli::cli_abort("The {.var {elname}} theme element must be a {.cls unit} vector of length 4.", call = call) - } else if (!inherits(el, class) && !inherits(el, "element_blank")) { - cli::cli_abort("The {.var {elname}} theme element must be a {.cls {eldef$class}} object.", call = call) + } else if (!inherits(el, class) && !S7::S7_inherits(el, element_blank)) { + if (inherits(class, "S7_class")) { + class <- class@name + } + cli::cli_abort("The {.var {elname}} theme element must be a {.cls {class}} object.", call = call) } invisible() } diff --git a/R/theme.R b/R/theme.R index dd08ceb926..de8a7a5fda 100644 --- a/R/theme.R +++ b/R/theme.R @@ -719,7 +719,7 @@ calc_element <- function(element, theme, verbose = FALSE, skip_blank = FALSE, # If result is element_blank, we skip it if `skip_blank` is `TRUE`, # and otherwise we don't inherit anything from parents - if (inherits(el_out, "element_blank")) { + if (S7::S7_inherits(el_out, element_blank)) { if (isTRUE(skip_blank)) { el_out <- NULL } else { @@ -733,9 +733,17 @@ calc_element <- function(element, theme, verbose = FALSE, skip_blank = FALSE, # If the element is defined (and not just inherited), check that # it is of the class specified in element_tree - if (!is.null(el_out) && - !inherits(el_out, element_tree[[element]]$class)) { - cli::cli_abort("Theme element {.var {element}} must have class {.cls {ggplot_global$element_tree[[element]]$class}}.", call = call) + if (!is.null(el_out)) { + class <- element_tree[[element]]$class + if (inherits(class, "S7_class")) { + if (!S7::S7_inherits(el_out, class)) { + cli::cli_abort("Theme element {.var {element}} must have class {.cls {class@name}}.", call = call) + } + } else { + if (!inherits(el_out, class)) { + cli::cli_abort("Theme element {.var {element}} must have class {.cls {ggplot_global$element_tree[[element]]$class}}.", call = call) + } + } } # Get the names of parents from the inheritance tree diff --git a/tests/testthat/test-theme.R b/tests/testthat/test-theme.R index 86cb19853f..a9c38f2ed4 100644 --- a/tests/testthat/test-theme.R +++ b/tests/testthat/test-theme.R @@ -21,8 +21,8 @@ test_that("modifying theme element properties with + operator works", { # Make sure the theme class didn't change or get dropped expect_true(is.theme(t)) # Make sure the element class didn't change or get dropped - expect_true(inherits(t$axis.title.x, "element")) - expect_true(inherits(t$axis.title.x, "element_text")) + expect_s3_class(t$axis.title.x, "ggplot2::element") + expect_s3_class(t$axis.title.x, "ggplot2::element_text") # Modifying an intermediate node works t <- theme_grey() + theme(axis.title = element_text(colour = 'red')) @@ -346,7 +346,7 @@ test_that("element tree can be modified", { test_that("all elements in complete themes have inherit.blank=TRUE", { inherit_blanks <- function(theme) { all(vapply(theme, function(el) { - if (inherits(el, "element") && !inherits(el, "element_blank")) { + if (inherits(el, "element") && !S7::S7_inherits(el, element_blank)) { el$inherit.blank } else { TRUE @@ -642,7 +642,7 @@ test_that("complete_theme completes a theme", { # Missing elements are filled in if default theme is incomplete new <- complete_theme(default = theme()) - expect_s3_class(new$axis.line, "element_blank") + expect_s3_class(new$axis.line, "ggplot2::element_blank") # Registered elements are included register_theme_elements( @@ -650,7 +650,7 @@ test_that("complete_theme completes a theme", { element_tree = list(test = el_def("element_text", "text")) ) new <- complete_theme(default = gray) - expect_s3_class(new$test, "element_text") + expect_s3_class(new$test, "ggplot2::element_text") reset_theme_settings() }) From 27cdb73660c38647072317bf8331ae502f6b6eeb Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Thu, 6 Mar 2025 10:59:55 +0100 Subject: [PATCH 10/22] fix misc issues --- R/guide-.R | 1 + R/guide-legend.R | 4 +++- R/theme-elements.R | 2 +- tests/testthat/test-theme.R | 41 ++++++++++++++++--------------------- 4 files changed, 23 insertions(+), 25 deletions(-) diff --git a/R/guide-.R b/R/guide-.R index 813c722b4a..0b8a339222 100644 --- a/R/guide-.R +++ b/R/guide-.R @@ -376,6 +376,7 @@ Guide <- ggproto( # Renders tickmarks build_ticks = function(key, elements, params, position = params$position, length = elements$ticks_length) { + force(length) if (!is.theme_element(elements)) { elements <- elements$ticks } diff --git a/R/guide-legend.R b/R/guide-legend.R index b37e210a78..5fd148f509 100644 --- a/R/guide-legend.R +++ b/R/guide-legend.R @@ -690,13 +690,15 @@ keep_key_data <- function(key, data, aes, show) { position_margin <- function(position, margin = NULL, gap = unit(0, "pt")) { margin <- margin %||% margin() - switch( + margin <- switch( position, top = replace(margin, 3, margin[3] + gap), bottom = replace(margin, 1, margin[1] + gap), left = replace(margin, 2, margin[2] + gap), right = replace(margin, 4, margin[4] + gap) ) + class(margin) <- union("margin", class(margin)) + margin } # Function implementing backward compatibility with the old way of specifying diff --git a/R/theme-elements.R b/R/theme-elements.R index e8bfba0132..30cc44100a 100644 --- a/R/theme-elements.R +++ b/R/theme-elements.R @@ -858,7 +858,7 @@ check_element <- function(el, elname, element_tree, call = caller_env()) { } } - if ("margin" %in% class) { + if (is.character(class) && "margin" %in% class) { if (!is.unit(el) && length(el) == 4) cli::cli_abort("The {.var {elname}} theme element must be a {.cls unit} vector of length 4.", call = call) } else if (!inherits(el, class) && !S7::S7_inherits(el, element_blank)) { diff --git a/tests/testthat/test-theme.R b/tests/testthat/test-theme.R index a9c38f2ed4..dff6b0dc50 100644 --- a/tests/testthat/test-theme.R +++ b/tests/testthat/test-theme.R @@ -142,12 +142,10 @@ test_that("calculating theme element inheritance works", { expect_identical(calc_element('axis.title.x', t), element_blank()) # Check that inheritance from derived class works - element_dummyrect <- function(dummy) { # like element_rect but w/ dummy argument - structure(list( - fill = NULL, colour = NULL, dummy = dummy, linewidth = NULL, - linetype = NULL, inherit.blank = FALSE - ), class = c("element_dummyrect", "element_rect", "element")) - } + element_dummyrect <- S7::new_class( + "element_dummyrect", parent = element_rect, + properties = c(element_rect@properties, list(dummy = S7::class_any)) + ) e <- calc_element( "panel.background", @@ -160,10 +158,10 @@ test_that("calculating theme element inheritance works", { expect_identical( e, - structure(list( + element_dummyrect( fill = "white", colour = "black", dummy = 5, linewidth = 0.5, linetype = 1, inherit.blank = TRUE # this is true because we're requesting a complete theme - ), class = c("element_dummyrect", "element_rect", "element")) + ) ) # Check that blank elements are skipped in inheritance tree if and only if elements @@ -283,9 +281,10 @@ test_that("theme validation happens at build stage", { test_that("incorrect theme specifications throw meaningful errors", { expect_snapshot_error(add_theme(theme_grey(), theme(line = element_rect()))) expect_snapshot_error(calc_element("line", theme(line = element_rect()))) - register_theme_elements(element_tree = list(test = el_def("element_rect"))) + register_theme_elements(element_tree = list(test = el_def(element_rect))) expect_snapshot_error(calc_element("test", theme_gray() + theme(test = element_rect()))) expect_snapshot_error(set_theme("foo")) + reset_theme_settings() }) test_that("element tree can be modified", { @@ -305,7 +304,7 @@ test_that("element tree can be modified", { # things work once we add a new element to the element tree register_theme_elements( - element_tree = list(blablabla = el_def("element_text", "text")) + element_tree = list(blablabla = el_def(element_text, "text")) ) expect_silent(ggplotGrob(p)) @@ -334,13 +333,13 @@ test_that("element tree can be modified", { expect_identical(e1@colour, "red") # not inherited from element_text # existing elements can be overwritten - ed <- el_def("element_rect", "rect") + ed <- el_def(element_rect, "rect") register_theme_elements( element_tree = list(axis.title = ed) ) expect_identical(get_element_tree()$axis.title, ed) - reset_theme_settings(reset_current = FALSE) # revert back to defaults + reset_theme_settings() # revert back to defaults }) test_that("all elements in complete themes have inherit.blank=TRUE", { @@ -424,7 +423,7 @@ test_that("current theme can be updated with new elements", { # element tree gets merged properly register_theme_elements( abcde = element_text(color = "blue", hjust = 0, vjust = 1), - element_tree = list(abcde = el_def("element_text", "text")) + element_tree = list(abcde = el_def(element_text, "text")) ) e1 <- calc_element("abcde", plot_theme(b2)) @@ -647,7 +646,7 @@ test_that("complete_theme completes a theme", { # Registered elements are included register_theme_elements( test = element_text(), - element_tree = list(test = el_def("element_text", "text")) + element_tree = list(test = el_def(element_text, "text")) ) new <- complete_theme(default = gray) expect_s3_class(new$test, "ggplot2::element_text") @@ -959,15 +958,11 @@ test_that("Legends can on all sides of the plot with custom justification", { }) test_that("Strips can render custom elements", { - element_test <- function(...) { - el <- element_text(...) - class(el) <- c('element_test', 'element_text', 'element') - el - } - element_grob.element_test <- function(element, label = "", x = NULL, y = NULL, ...) { - rectGrob(width = unit(1, "cm"), height = unit(1, "cm")) - } - registerS3method("element_grob", "element_test", element_grob.element_test) + element_test <- S7::new_class("element_test", element_text) + S7::method(element_grob, element_test) <- + function(element, label = "", x = NULL, y = NULL, ...) { + rectGrob(width = unit(1, "cm"), height = unit(1, "cm")) + } df <- data_frame(x = 1:3, y = 1:3, a = letters[1:3]) plot <- ggplot(df, aes(x, y)) + From 3bc0d63ebcfbf16a2857a171131c5a564873ebbd Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Thu, 6 Mar 2025 15:02:47 +0100 Subject: [PATCH 11/22] Revert "implement `$.element` for backward compatibility" This reverts commit b038e8f633d8526f6e20ee9bf4549634cc38d565. --- NAMESPACE | 6 +++--- R/ggproto.R | 5 ++--- R/theme-elements.R | 8 -------- R/theme.R | 3 +-- 4 files changed, 6 insertions(+), 16 deletions(-) diff --git a/NAMESPACE b/NAMESPACE index a88e3f341d..04986e15bb 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -1,5 +1,8 @@ # Generated by roxygen2: do not edit by hand +S3method("$",ggproto) +S3method("$",ggproto_parent) +S3method("$",theme) S3method("$<-",uneval) S3method("+",gg) S3method("[",mapped_discrete) @@ -13,9 +16,6 @@ S3method(as.data.frame,mapped_discrete) S3method(as.list,ggproto) S3method(autolayer,default) S3method(autoplot,default) -S3method(base::`$`, ggproto) -S3method(base::`$`, ggproto_parent) -S3method(base::`$`, theme) S3method(c,mapped_discrete) S3method(drawDetails,zeroGrob) S3method(format,ggproto) diff --git a/R/ggproto.R b/R/ggproto.R index 3de50401c3..6165a9707d 100644 --- a/R/ggproto.R +++ b/R/ggproto.R @@ -152,8 +152,7 @@ fetch_ggproto <- function(x, name) { } -# Prevents bug described in S7/#390 -#' @rawNamespace S3method(base::`$`, ggproto) +#' @export `$.ggproto` <- function(x, name) { res <- fetch_ggproto(x, name) if (!is.function(res)) { @@ -163,7 +162,7 @@ fetch_ggproto <- function(x, name) { make_proto_method(x, res, name) } -#' @rawNamespace S3method(base::`$`, ggproto_parent) +#' @export `$.ggproto_parent` <- function(x, name) { res <- fetch_ggproto(.subset2(x, "parent"), name) if (!is.function(res)) { diff --git a/R/theme-elements.R b/R/theme-elements.R index 30cc44100a..268457d116 100644 --- a/R/theme-elements.R +++ b/R/theme-elements.R @@ -83,14 +83,6 @@ NULL #' @rdname element element <- S7::new_class("element", abstract = TRUE) -S7::method(`$`, element) <- - function(x, i) { - if (!S7::prop_exists(x, i)) { - return(NULL) - } - S7::prop(x, i) - } - #' @export #' @rdname element element_blank <- S7::new_class("element_blank", parent = element) diff --git a/R/theme.R b/R/theme.R index de8a7a5fda..609c1eaf4e 100644 --- a/R/theme.R +++ b/R/theme.R @@ -958,8 +958,7 @@ combine_elements <- function(e1, e2) { e1 } -# Prevents bug described in S7/#390 -#' @rawNamespace S3method(base::`$`, theme) +#' @export `$.theme` <- function(x, ...) { .subset2(x, ...) } From 360c97528e6cbf7a8d05dfaa4de21bb1e6fa0606 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Thu, 6 Mar 2025 15:41:18 +0100 Subject: [PATCH 12/22] don't rely on `element$prop` --- R/save.R | 4 +++- R/theme.R | 15 +++++++++++---- tests/testthat/test-theme.R | 2 +- 3 files changed, 15 insertions(+), 6 deletions(-) diff --git a/R/save.R b/R/save.R index 5e1ef5983a..7a6184e5ba 100644 --- a/R/save.R +++ b/R/save.R @@ -103,7 +103,9 @@ ggsave <- function(filename, plot = get_last_plot(), limitsize = limitsize, dpi = dpi) if (is_null(bg)) { - bg <- calc_element("plot.background", plot_theme(plot))$fill %||% "transparent" + bg <- calc_element("plot.background", plot_theme(plot)) + bg <- if (S7::prop_exists(bg, "fill")) bg@fill else NULL + bg <- bg %||% "transparent" } old_dev <- grDevices::dev.cur() dev(filename = filename, width = dim[1], height = dim[2], bg = bg, ...) diff --git a/R/theme.R b/R/theme.R index 609c1eaf4e..2df99014f8 100644 --- a/R/theme.R +++ b/R/theme.R @@ -779,15 +779,22 @@ calc_element <- function(element, theme, verbose = FALSE, skip_blank = FALSE, # Calculate the parent objects' inheritance if (verbose) cli::cli_inform("{pnames}") + + # once we've started skipping blanks, we continue doing so until the end of the + # recursion; we initiate skipping blanks if we encounter an element that + # doesn't inherit blank. + skip_blank <- skip_blank || + (!is.null(el_out) && + !isTRUE(S7::S7_inherits(el_out) && + S7::prop_exists(el_out, "inherit.blank") && + el_out@inherit.blank)) + parents <- lapply( pnames, calc_element, theme, verbose = verbose, - # once we've started skipping blanks, we continue doing so until the end of the - # recursion; we initiate skipping blanks if we encounter an element that - # doesn't inherit blank. - skip_blank = skip_blank || (!is.null(el_out) && !isTRUE(el_out$inherit.blank)), + skip_blank = skip_blank, call = call ) diff --git a/tests/testthat/test-theme.R b/tests/testthat/test-theme.R index dff6b0dc50..81674c705d 100644 --- a/tests/testthat/test-theme.R +++ b/tests/testthat/test-theme.R @@ -346,7 +346,7 @@ test_that("all elements in complete themes have inherit.blank=TRUE", { inherit_blanks <- function(theme) { all(vapply(theme, function(el) { if (inherits(el, "element") && !S7::S7_inherits(el, element_blank)) { - el$inherit.blank + el@inherit.blank } else { TRUE } From 68fe80bd1ccd8aa48a48528bc365d7259087d5d0 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Fri, 7 Mar 2025 10:02:07 +0100 Subject: [PATCH 13/22] move element properties --- R/properties.R | 69 ++++------------------------------------------ R/theme-elements.R | 23 +++++++++++++++- 2 files changed, 28 insertions(+), 64 deletions(-) diff --git a/R/properties.R b/R/properties.R index be857393b0..493d787f05 100644 --- a/R/properties.R +++ b/R/properties.R @@ -1,4 +1,3 @@ - property_boolean <- function(allow_null = FALSE, default = TRUE) { class <- S7::class_logical class <- if (allow_null) S7::new_union(class, NULL) else class @@ -38,65 +37,9 @@ property_choice <- function(options, allow_null = FALSE, default = NULL) { ) } -element_props <- list( - fill = S7::new_property( - S7::new_union(S7::class_character, S7::new_S3_class("GridPattern"), S7::class_logical, NULL), - default = NULL - ), - colour = S7::new_property( - S7::new_union(S7::class_character, S7::class_logical, NULL), - default = NULL - ), - family = S7::new_property( - S7::new_union(S7::class_character, NULL), - default = NULL - ), - hjust = S7::new_property( - S7::new_union(S7::class_numeric, NULL), - default = NULL - ), - vjust = S7::new_property( - S7::new_union(S7::class_numeric, NULL), - default = NULL - ), - angle = S7::new_property( - S7::new_union(S7::class_numeric, NULL), - default = NULL - ), - size = S7::new_property( - S7::new_union(S7::class_numeric, NULL), - default = NULL - ), - lineheight = S7::new_property( - S7::new_union(S7::class_numeric, NULL), - default = NULL - ), - margin = S7::new_property( - S7::new_union(S7::new_S3_class("margin"), NULL), - default = NULL - ), - face = property_choice(c("plain", "bold", "italic", "oblique", "bold.italic"), allow_null = TRUE), - linewidth = S7::new_property( - S7::new_union(S7::class_numeric, NULL), - default = NULL - ), - linetype = S7::new_property( - S7::new_union(S7::class_numeric, S7::class_character, NULL), - default = NULL - ), - lineend = property_choice(c("round", "butt", "square"), allow_null = TRUE), - shape = S7::new_property( - S7::new_union(S7::class_numeric, S7::class_character, NULL), - default = NULL - ), - arrow = S7::new_property( - S7::new_union(S7::new_S3_class("arrow"), S7::class_logical, NULL), - default = NULL - ), - arrow.fill = S7::new_property( - S7::new_union(S7::class_character, S7::class_logical, NULL), - default = NULL - ), - debug = property_boolean(allow_null = TRUE, default = NULL), - inherit.blank = property_boolean(default = FALSE) -) +property_nullable <- function(class = S7::class_any, ...) { + S7::new_property( + class = S7::new_union(NULL, class), + ... + ) +} diff --git a/R/theme-elements.R b/R/theme-elements.R index 268457d116..540c19fed9 100644 --- a/R/theme-elements.R +++ b/R/theme-elements.R @@ -87,7 +87,28 @@ element <- S7::new_class("element", abstract = TRUE) #' @rdname element element_blank <- S7::new_class("element_blank", parent = element) -#' @include properties.R +# All properties are listed here so they can easily be recycled in the different +# element classes +element_props <- list( + fill = property_nullable(S7::class_character | S7::new_S3_class("GridPattern") | S7::class_logical), + colour = property_nullable(S7::class_character | S7::class_logical), + family = property_nullable(S7::class_character), + hjust = property_nullable(S7::class_numeric), + vjust = property_nullable(S7::class_numeric), + angle = property_nullable(S7::class_numeric), + size = property_nullable(S7::class_numeric), + lineheight = property_nullable(S7::class_numeric), + margin = property_nullable(margin), + face = property_choice(c("plain", "bold", "italic", "oblique", "bold.italic"), allow_null = TRUE), + linewidth = property_nullable(S7::class_numeric), + linetype = property_nullable(S7::class_numeric | S7::class_character), + lineend = property_choice(c("round", "butt", "square"), allow_null = TRUE), + shape = property_nullable(S7::class_numeric | S7::class_character), + arrow = property_nullable(S7::new_S3_class("arrow") | S7::class_logical), + arrow.fill = property_nullable(S7::class_character | S7::class_logical), + debug = property_boolean(allow_null = TRUE, default = NULL), + inherit.blank = property_boolean(default = FALSE) +) #' @export #' @rdname element From 8c3471e1adfe97f8dfdcfc50dd80904e5c530dc4 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Fri, 7 Mar 2025 14:06:24 +0100 Subject: [PATCH 14/22] convert `margin` to S7 --- DESCRIPTION | 2 +- R/geom-label.R | 2 +- R/guide-legend.R | 4 +++- R/margins.R | 22 ++++++++++++++++------ R/properties.R | 13 +++++++++++++ R/theme-elements.R | 3 +++ R/theme.R | 8 ++++---- man/element.Rd | 32 ++++++++++++++++---------------- man/is_tests.Rd | 10 +++++----- 9 files changed, 62 insertions(+), 34 deletions(-) diff --git a/DESCRIPTION b/DESCRIPTION index a632b6fcf6..c734a5f689 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -177,6 +177,7 @@ Collate: 'grob-null.R' 'grouping.R' 'properties.R' + 'margins.R' 'theme-elements.R' 'guide-.R' 'guide-axis.R' @@ -201,7 +202,6 @@ Collate: 'layer-sf.R' 'layout.R' 'limits.R' - 'margins.R' 'performance.R' 'plot-build.R' 'plot-construction.R' diff --git a/R/geom-label.R b/R/geom-label.R index ae21a48df3..73f1d8bedf 100644 --- a/R/geom-label.R +++ b/R/geom-label.R @@ -87,7 +87,7 @@ GeomLabel <- ggproto("GeomLabel", Geom, data <- coord$transform(data, panel_params) data$vjust <- compute_just(data$vjust, data$y, data$x, data$angle) data$hjust <- compute_just(data$hjust, data$x, data$y, data$angle) - if (!is.margin("margin")) { + if (!is.margin(label.padding)) { label.padding <- rep(label.padding, length.out = 4) } diff --git a/R/guide-legend.R b/R/guide-legend.R index 5fd148f509..090dccc0f6 100644 --- a/R/guide-legend.R +++ b/R/guide-legend.R @@ -697,7 +697,9 @@ position_margin <- function(position, margin = NULL, gap = unit(0, "pt")) { left = replace(margin, 2, margin[2] + gap), right = replace(margin, 4, margin[4] + gap) ) - class(margin) <- union("margin", class(margin)) + # We have to manually reconstitute the class because the 'simpleUnit' class + # might be dropped by the replacement operation. + class(margin) <- c("ggplot2::margin", class(margin), "S7_object") margin } diff --git a/R/margins.R b/R/margins.R index 61ea1dab43..643cc95374 100644 --- a/R/margins.R +++ b/R/margins.R @@ -1,13 +1,23 @@ +#' @include properties.R + #' @param t,r,b,l Dimensions of each margin. (To remember order, think trouble). #' @param unit Default units of dimensions. Defaults to "pt" so it #' can be most easily scaled with the text. #' @rdname element #' @export -margin <- function(t = 0, r = 0, b = 0, l = 0, unit = "pt") { - u <- unit(c(t, r, b, l), unit) - class(u) <- c("margin", class(u)) - u -} +margin <- S7::new_class( + "margin", parent = S7::new_S3_class(c("simpleUnit", "unit", "unit_v2")), + constructor = function(t = 0, r = 0, b = 0, l = 0, unit = "pt") { + u <- unit(c(t, r, b, l), unit) + S7::new_object(u) + }, + properties = list( + top = property_index(1L), + right = property_index(2L), + bottom = property_index(3L), + left = property_index(4L) + ) +) #' @rdname element #' @export @@ -23,7 +33,7 @@ margin_auto <- function(t = 0, r = t, b = t, l = r, unit = "pt") { #' @export #' @rdname is_tests -is.margin <- function(x) inherits(x, "margin") +is.margin <- function(x) S7::S7_inherits(x, margin) #' Create a text grob with the proper location and margins #' diff --git a/R/properties.R b/R/properties.R index 493d787f05..d7df34fd0a 100644 --- a/R/properties.R +++ b/R/properties.R @@ -43,3 +43,16 @@ property_nullable <- function(class = S7::class_any, ...) { ... ) } + +property_index <- function(i) { + force(i) + S7::new_property( + getter = function(self) { + self[i] + }, + setter = function(self, value) { + self[i] <- value + self + } + ) +} diff --git a/R/theme-elements.R b/R/theme-elements.R index 540c19fed9..28b28dca94 100644 --- a/R/theme-elements.R +++ b/R/theme-elements.R @@ -87,8 +87,11 @@ element <- S7::new_class("element", abstract = TRUE) #' @rdname element element_blank <- S7::new_class("element_blank", parent = element) + # All properties are listed here so they can easily be recycled in the different # element classes +#' @include properties.R +#' @include margins.R element_props <- list( fill = property_nullable(S7::class_character | S7::new_S3_class("GridPattern") | S7::class_logical), colour = property_nullable(S7::class_character | S7::class_logical), diff --git a/R/theme.R b/R/theme.R index 2df99014f8..05cae4be30 100644 --- a/R/theme.R +++ b/R/theme.R @@ -867,7 +867,7 @@ S7::method(merge_element, list(element, S7::class_any)) <- new } -S7::method(merge_element, list(S7::new_S3_class("margin"), S7::class_any)) <- +S7::method(merge_element, list(margin, S7::class_any)) <- function(new, old, ...) { if (is.null(old) || S7::S7_inherits(old, element_blank)) { return(new) @@ -911,7 +911,7 @@ combine_elements <- function(e1, e2) { return(e1) } - if (inherits(e1, "margin") && inherits(e2, "margin")) { + if (is.margin(e1) && is.margin(e2)) { if (anyNA(e2)) { e2[is.na(e2)] <- unit(0, "pt") } @@ -921,7 +921,7 @@ combine_elements <- function(e1, e2) { } # If neither of e1 or e2 are element_* objects, return e1 - if (!S7::S7_inherits(e1, element) && !S7::S7_inherits(e2, element)) { + if (!is.theme_element(e1) && !is.theme_element(e2)) { return(e1) } @@ -949,7 +949,7 @@ combine_elements <- function(e1, e2) { e1@linewidth <- e2@linewidth * unclass(e1@linewidth) } - if (inherits(e1, "element_text")) { + if (S7::S7_inherits(e1, element_text)) { e1@margin <- combine_elements(e1@margin, e2@margin) } diff --git a/man/element.Rd b/man/element.Rd index 72d8c2b601..7bc21e1c74 100644 --- a/man/element.Rd +++ b/man/element.Rd @@ -1,6 +1,9 @@ % Generated by roxygen2: do not edit by hand -% Please edit documentation in R/theme-elements.R, R/margins.R -\name{element} +% Please edit documentation in R/margins.R, R/theme-elements.R +\name{margin} +\alias{margin} +\alias{margin_part} +\alias{margin_auto} \alias{element} \alias{element_blank} \alias{element_rect} @@ -10,11 +13,14 @@ \alias{element_point} \alias{element_geom} \alias{rel} -\alias{margin} -\alias{margin_part} -\alias{margin_auto} \title{Theme elements} \usage{ +margin(t = 0, r = 0, b = 0, l = 0, unit = "pt") + +margin_part(t = NA, r = NA, b = NA, l = NA, unit = "pt") + +margin_auto(t = 0, r = t, b = t, l = r, unit = "pt") + element() element_blank() @@ -90,14 +96,13 @@ element_geom( ) rel(x) - -margin(t = 0, r = 0, b = 0, l = 0, unit = "pt") - -margin_part(t = NA, r = NA, b = NA, l = NA, unit = "pt") - -margin_auto(t = 0, r = t, b = t, l = r, unit = "pt") } \arguments{ +\item{t, r, b, l}{Dimensions of each margin. (To remember order, think trouble).} + +\item{unit}{Default units of dimensions. Defaults to "pt" so it +can be most easily scaled with the text.} + \item{fill}{Fill colour. \code{fill_alpha()} can be used to set the transparency of the fill.} @@ -154,11 +159,6 @@ is anchored.} \item{accent}{Accent colour.} \item{x}{A single number specifying size relative to parent element.} - -\item{t, r, b, l}{Dimensions of each margin. (To remember order, think trouble).} - -\item{unit}{Default units of dimensions. Defaults to "pt" so it -can be most easily scaled with the text.} } \value{ An S3 object of class \code{element}, \code{rel}, or \code{margin}. diff --git a/man/is_tests.Rd b/man/is_tests.Rd index bcb7bf0683..10fa48db7a 100644 --- a/man/is_tests.Rd +++ b/man/is_tests.Rd @@ -1,7 +1,7 @@ % Generated by roxygen2: do not edit by hand % Please edit documentation in R/ggproto.R, R/aes.R, R/geom-.R, R/coord-.R, -% R/facet-.R, R/stat-.R, R/theme-elements.R, R/guide-.R, R/layer.R, -% R/guides-.R, R/margins.R, R/plot.R, R/position-.R, R/scale-.R, R/theme.R +% R/facet-.R, R/stat-.R, R/margins.R, R/theme-elements.R, R/guide-.R, +% R/layer.R, R/guides-.R, R/plot.R, R/position-.R, R/scale-.R, R/theme.R \name{is.ggproto} \alias{is.ggproto} \alias{is.mapping} @@ -10,11 +10,11 @@ \alias{is.Coord} \alias{is.facet} \alias{is.stat} +\alias{is.margin} \alias{is.theme_element} \alias{is.guide} \alias{is.layer} \alias{is.guides} -\alias{is.margin} \alias{is_tests} \alias{is.ggplot} \alias{is.position} @@ -36,6 +36,8 @@ is.facet(x) is.stat(x) +is.margin(x) + is.theme_element(x) is.guide(x) @@ -44,8 +46,6 @@ is.layer(x) is.guides(x) -is.margin(x) - is.ggplot(x) is.position(x) From 9f69323b4b7111ac5a21098830a38e262bc70288 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Fri, 7 Mar 2025 15:49:59 +0100 Subject: [PATCH 15/22] I liked index properties as an idea but apparently they print badly --- R/margins.R | 8 +------- R/properties.R | 13 ------------- 2 files changed, 1 insertion(+), 20 deletions(-) diff --git a/R/margins.R b/R/margins.R index 643cc95374..46e075d103 100644 --- a/R/margins.R +++ b/R/margins.R @@ -10,13 +10,7 @@ margin <- S7::new_class( constructor = function(t = 0, r = 0, b = 0, l = 0, unit = "pt") { u <- unit(c(t, r, b, l), unit) S7::new_object(u) - }, - properties = list( - top = property_index(1L), - right = property_index(2L), - bottom = property_index(3L), - left = property_index(4L) - ) + } ) #' @rdname element diff --git a/R/properties.R b/R/properties.R index d7df34fd0a..493d787f05 100644 --- a/R/properties.R +++ b/R/properties.R @@ -43,16 +43,3 @@ property_nullable <- function(class = S7::class_any, ...) { ... ) } - -property_index <- function(i) { - force(i) - S7::new_property( - getter = function(self) { - self[i] - }, - setter = function(self, value) { - self[i] <- value - self - } - ) -} From e4b870b2929bb07a382f40fa733a33bc164c2c7b Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Fri, 7 Mar 2025 15:55:01 +0100 Subject: [PATCH 16/22] adapt failing example --- R/plot-construction.R | 10 +++++----- man/ggplot_add.Rd | 10 +++++----- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/R/plot-construction.R b/R/plot-construction.R index cd18fc8310..d8d3234be4 100644 --- a/R/plot-construction.R +++ b/R/plot-construction.R @@ -98,18 +98,18 @@ add_ggplot <- function(p, object, objectname) { #' @export #' @examples #' # making a new method for the generic -#' # in this example, we apply a text element to the text theme setting -#' ggplot_add.element_text <- function(object, plot, object_name) { -#' plot + theme(text = object) +#' # in this example, we apply a colour to the plot background +#' ggplot_add.character <- function(object, plot, object_name) { +#' plot + theme(plot.background = element_rect(fill = object)) #' } #' #' # we can now use `+` to add our object to a plot #' ggplot(mpg, aes(displ, cty)) + #' geom_point() + -#' element_text(colour = "red") +#' "cornsilk" #' #' # clean-up -#' rm(ggplot_add.element_text) +#' rm(ggplot_add.character) ggplot_add <- function(object, plot, object_name) { UseMethod("ggplot_add") } diff --git a/man/ggplot_add.Rd b/man/ggplot_add.Rd index c71d6f863e..2584ba7602 100644 --- a/man/ggplot_add.Rd +++ b/man/ggplot_add.Rd @@ -32,17 +32,17 @@ the plot intact. } \examples{ # making a new method for the generic -# in this example, we apply a text element to the text theme setting -ggplot_add.element_text <- function(object, plot, object_name) { - plot + theme(text = object) +# in this example, we apply a colour to the plot background +ggplot_add.character <- function(object, plot, object_name) { + plot + theme(plot.background = element_rect(fill = object)) } # we can now use `+` to add our object to a plot ggplot(mpg, aes(displ, cty)) + geom_point() + - element_text(colour = "red") + "cornsilk" # clean-up -rm(ggplot_add.element_text) +rm(ggplot_add.character) } \keyword{internal} From b2f143ed80d11858b8a998756223d6c7aa57386a Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Tue, 1 Apr 2025 16:53:43 +0200 Subject: [PATCH 17/22] backport `@` --- NAMESPACE | 1 + R/backports.R | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/NAMESPACE b/NAMESPACE index 99c6e874c6..3725f12aac 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -752,6 +752,7 @@ export(xlim) export(ylab) export(ylim) export(zeroGrob) +if (getRversion() < "4.3.0") importFrom("S7", "@") import(grid) import(gtable) import(rlang) diff --git a/R/backports.R b/R/backports.R index 7ccedc4296..53ab2a6f7e 100644 --- a/R/backports.R +++ b/R/backports.R @@ -15,6 +15,10 @@ if (getRversion() < "3.3") { backport_unit_methods <- function() {} } +# enable usage of @name in package code +#' @rawNamespace if (getRversion() < "4.3.0") importFrom("S7", "@") +NULL + on_load(backport_unit_methods()) unitType <- function(x) { From 98fb83efe8c7d891f501f1e8b43f7492f7b68193 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Wed, 16 Apr 2025 11:14:25 +0200 Subject: [PATCH 18/22] S7-aware `is_theme_element()` --- R/coord-sf.R | 2 +- R/guide-axis-theta.R | 6 +++--- R/guide-axis.R | 8 +++---- R/plot-build.R | 2 +- R/theme-elements.R | 43 ++++++++++++++++++------------------- R/theme.R | 20 ++++++++--------- man/is_tests.Rd | 2 +- tests/testthat/test-theme.R | 2 +- 8 files changed, 42 insertions(+), 43 deletions(-) diff --git a/R/coord-sf.R b/R/coord-sf.R index 0bcfafa4c3..63e5ed4a26 100644 --- a/R/coord-sf.R +++ b/R/coord-sf.R @@ -334,7 +334,7 @@ CoordSf <- ggproto("CoordSf", CoordCartesian, # we don't draw the graticules if the major panel grid is # turned off - if (S7::S7_inherits(el, element_blank)) { + if (is_theme_element(el, "blank")) { grobs <- list(element_render(theme, "panel.background")) } else { line_gp <- gg_par( diff --git a/R/guide-axis-theta.R b/R/guide-axis-theta.R index b67026662b..b75528d347 100644 --- a/R/guide-axis-theta.R +++ b/R/guide-axis-theta.R @@ -184,7 +184,7 @@ GuideAxisTheta <- ggproto( build_labels = function(key, elements, params) { - if (S7::S7_inherits(elements$text, element_blank)) { + if (is_theme_element(elements$text, "blank")) { return(zeroGrob()) } @@ -268,7 +268,7 @@ GuideAxisTheta <- ggproto( key <- params$key key <- vec_slice(key, !is.na(key$.label) & nzchar(key$.label)) labels <- validate_labels(key$.label) - if (length(labels) == 0 || S7::S7_inherits(elements$text, element_blank)) { + if (length(labels) == 0 || is_theme_element(elements$text, "blank")) { return(list(offset = offset)) } @@ -365,7 +365,7 @@ GuideAxisTheta <- ggproto( theta_tickmarks <- function(key, element, length, offset = NULL) { n_breaks <- nrow(key) - if (n_breaks < 1 || S7::S7_inherits(element, element_blank)) { + if (n_breaks < 1 || is_theme_element(element, "blank")) { return(zeroGrob()) } diff --git a/R/guide-axis.R b/R/guide-axis.R index 4cf00f2a25..03dffcaebd 100644 --- a/R/guide-axis.R +++ b/R/guide-axis.R @@ -259,10 +259,10 @@ GuideAxis <- ggproto( override_elements = function(params, elements, theme) { elements$text <- label_angle_heuristic(elements$text, params$position, params$angle) - if (S7::S7_inherits(elements$ticks, element_blank)) { + if (is_theme_element(elements$ticks, "blank")) { elements$major_length <- unit(0, "cm") } - if (S7::S7_inherits(elements$minor, element_blank) || isFALSE(params$minor.ticks)) { + if (is_theme_element(elements$minor, "blank") || isFALSE(params$minor.ticks)) { elements$minor_length <- unit(0, "cm") } return(elements) @@ -379,7 +379,7 @@ GuideAxis <- ggproto( # Ticks major_cm <- convertUnit(elements$major_length, "cm", valueOnly = TRUE) range <- range(0, major_cm) - if (params$minor.ticks && !S7::S7_inherits(elements$minor, element_blank)) { + if (params$minor.ticks && !is_theme_element(elements$minor, "blank")) { minor_cm <- convertUnit(elements$minor_length, "cm", valueOnly = TRUE) range <- range(range, minor_cm) } @@ -590,7 +590,7 @@ axis_label_priority_between <- function(x, y) { #' overridden from the user- or theme-supplied element. #' @noRd label_angle_heuristic <- function(element, position, angle) { - if (!S7::S7_inherits(element, element_text) + if (!is_theme_element(element, "text") || is.null(position) || is.null(angle %|W|% NULL)) { return(element) diff --git a/R/plot-build.R b/R/plot-build.R index 2901f8d57e..e09134fe21 100644 --- a/R/plot-build.R +++ b/R/plot-build.R @@ -348,7 +348,7 @@ table_add_tag <- function(table, label, theme) { return(table) } element <- calc_element("plot.tag", theme) - if (S7::S7_inherits(element, element_blank)) { + if (is_theme_element(element, "blank")) { return(table) } diff --git a/R/theme-elements.R b/R/theme-elements.R index 39319c9352..15751c5319 100644 --- a/R/theme-elements.R +++ b/R/theme-elements.R @@ -207,25 +207,6 @@ element_text <- S7::new_class( } ) -#' @export -#' @param type For testing elements: the type of element to expect. One of -#' `"blank"`, `"rect"`, `"line"` or `"text"`. -#' @rdname is_tests -is_theme_element <- function(x, type = "any") { - switch( - type %||% "any", - any = inherits(x, "element"), - rect = inherits(x, "element_rect"), - line = inherits(x, "element_line"), - text = inherits(x, "element_text"), - blank = inherits(x, "element_blank"), - # TODO: ideally we accept more elements from extensions. We need to - # consider how this will work with S7 classes, where ggplot2 doesn't know - # about the extension's class objects. - FALSE - ) -} - #' @export #' @rdname element element_polygon <- S7::new_class( @@ -322,6 +303,25 @@ element_geom <- S7::new_class( #' @export print.element <- function(x, ...) utils::str(x) +#' @export +#' @param type For testing elements: the type of element to expect. One of +#' `"blank"`, `"rect"`, `"line"`, `"text"`, `"polygon"`, `"point"` or `"geom"`. +#' @rdname is_tests +is_theme_element <- function(x, type = "any") { + switch( + type %||% "any", + any = S7::S7_inherits(x, element), + blank = S7::S7_inherits(x, element_blank), + rect = S7::S7_inherits(x, element_rect), + line = S7::S7_inherits(x, element_line), + text = S7::S7_inherits(x, element_text), + polygon = S7::S7_inherits(x, element_polygon), + point = S7::S7_inherits(x, element_point), + geom = S7::S7_inherits(x, element_geom), + FALSE + ) +} + #' @param x A single number specifying size relative to parent element. #' @rdname element #' @export @@ -890,8 +890,7 @@ check_element <- function(el, elname, element_tree, call = caller_env()) { if (is.null(el)) return() class <- eldef$class if (inherits(class, "S7_class") && S7::S7_inherits(el)) { - if (S7::S7_inherits(el, class) || - (S7::S7_inherits(el, element) && S7::S7_inherits(el, element_blank))) { + if (S7::S7_inherits(el, class) || is_theme_element(el, "blank")) { return() } } @@ -899,7 +898,7 @@ check_element <- function(el, elname, element_tree, call = caller_env()) { if (is.character(class) && "margin" %in% class) { if (!is.unit(el) && length(el) == 4) cli::cli_abort("The {.var {elname}} theme element must be a {.cls unit} vector of length 4.", call = call) - } else if (!inherits(el, class) && !S7::S7_inherits(el, element_blank)) { + } else if (!inherits(el, class) && !is_theme_element(el, "blank")) { if (inherits(class, "S7_class")) { class <- class@name } diff --git a/R/theme.R b/R/theme.R index 128707107e..18f3bf76e4 100644 --- a/R/theme.R +++ b/R/theme.R @@ -751,7 +751,7 @@ calc_element <- function(element, theme, verbose = FALSE, skip_blank = FALSE, # If result is element_blank, we skip it if `skip_blank` is `TRUE`, # and otherwise we don't inherit anything from parents - if (S7::S7_inherits(el_out, element_blank)) { + if (is_theme_element(el_out, "blank")) { if (isTRUE(skip_blank)) { el_out <- NULL } else { @@ -786,7 +786,7 @@ calc_element <- function(element, theme, verbose = FALSE, skip_blank = FALSE, if (verbose) cli::cli_inform("nothing (top level)") # Check that all the properties of this element are non-NULL - if (inherits(el_out, "ggplot2::element")) { + if (is_theme_element(el_out)) { nullprops <- lengths(S7::props(el_out)) == 0 } else { nullprops <- vapply(el_out, is.null, logical(1)) @@ -797,12 +797,12 @@ calc_element <- function(element, theme, verbose = FALSE, skip_blank = FALSE, # if we have null properties, try to fill in from ggplot_global$theme_default el_out <- combine_elements(el_out, ggplot_global$theme_default[[element]]) - if (is.theme_element(el_out)) { + if (is_theme_element(el_out)) { nullprops <- lengths(S7::props(el_out)) == 0 } else { nullprops <- vapply(el_out, is.null, logical(1)) } - if (S7::S7_inherits(el_out, element_geom)) { + if (is_theme_element(el_out, "geom")) { # Geom elements are expected to have NULL fill/colour, so allow these # to be missing nullprops[c("colour", "fill")] <- FALSE @@ -861,7 +861,7 @@ merge_element <- S7::new_generic("merge_element", c("new", "old")) S7::method(merge_element, list(S7::class_any, S7::class_any)) <- function(new, old, ...) { - if (is.null(old) || S7::S7_inherits(old, element_blank)) { + if (is.null(old) || is_theme_element(old, "blank")) { # If old is NULL or element_blank, then just return new return(new) } else if (is.null(new) || is.character(new) || is.numeric(new) || is.unit(new) || @@ -882,7 +882,7 @@ S7::method(merge_element, list(element_blank, S7::class_any)) <- S7::method(merge_element, list(element, S7::class_any)) <- function(new, old, ...) { - if (is.null(old) || S7::S7_inherits(old, element_blank)) { + if (is.null(old) || is_theme_element(old, "blank")) { # If old is NULL or element_blank, then just return new return(new) } @@ -906,7 +906,7 @@ S7::method(merge_element, list(element, S7::class_any)) <- S7::method(merge_element, list(margin, S7::class_any)) <- function(new, old, ...) { - if (is.null(old) || S7::S7_inherits(old, element_blank)) { + if (is.null(old) || is_theme_element(old, "blank")) { return(new) } if (anyNA(new)) { @@ -925,7 +925,7 @@ S7::method(merge_element, list(margin, S7::class_any)) <- combine_elements <- function(e1, e2) { # If e2 is NULL, nothing to inherit - if (is.null(e2) || S7::S7_inherits(e1, element_blank)) { + if (is.null(e2) || is_theme_element(e1, "blank")) { return(e1) } @@ -948,7 +948,7 @@ combine_elements <- function(e1, e2) { return(e1) } - if (is.margin(e1) && is.margin(e2)) { + if (is_margin(e1) && is_margin(e2)) { if (anyNA(e2)) { e2[is.na(e2)] <- unit(0, "pt") } @@ -986,7 +986,7 @@ combine_elements <- function(e1, e2) { e1@linewidth <- e2@linewidth * unclass(e1@linewidth) } - if (S7::S7_inherits(e1, element_text)) { + if (is_theme_element(e1, "text")) { e1@margin <- combine_elements(e1@margin, e2@margin) } diff --git a/man/is_tests.Rd b/man/is_tests.Rd index 801ca7db0c..c3c0630915 100644 --- a/man/is_tests.Rd +++ b/man/is_tests.Rd @@ -70,7 +70,7 @@ is.theme(x) # Deprecated \item{x}{An object to test} \item{type}{For testing elements: the type of element to expect. One of -\code{"blank"}, \code{"rect"}, \code{"line"} or \code{"text"}.} +\code{"blank"}, \code{"rect"}, \code{"line"}, \code{"text"}, \code{"polygon"}, \code{"point"} or \code{"geom"}.} } \description{ Reports wether \code{x} is a type of object diff --git a/tests/testthat/test-theme.R b/tests/testthat/test-theme.R index 8c1c6787b8..8000d25946 100644 --- a/tests/testthat/test-theme.R +++ b/tests/testthat/test-theme.R @@ -345,7 +345,7 @@ test_that("element tree can be modified", { test_that("all elements in complete themes have inherit.blank=TRUE", { inherit_blanks <- function(theme) { all(vapply(theme, function(el) { - if (is_theme_element(el) && !is_theme_element(el, "blank")) { + if (is_theme_element(el) && S7::prop_exists(el, "inherit.blank")) { el@inherit.blank } else { TRUE From f56a504923f7c96a9250485d24b1f55cdee64f57 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Wed, 16 Apr 2025 12:56:59 +0200 Subject: [PATCH 19/22] utility for grabbing props --- R/save.R | 3 +-- R/theme.R | 13 +++++-------- R/utilities.R | 10 ++++++++++ tests/testthat/test-theme.R | 12 +++++------- 4 files changed, 21 insertions(+), 17 deletions(-) diff --git a/R/save.R b/R/save.R index e0d63131af..4ebf9dae3e 100644 --- a/R/save.R +++ b/R/save.R @@ -245,8 +245,7 @@ get_plot_background <- function(plot, bg = NULL, default = "transparent") { return(default) } bg <- calc_element("plot.background", plot_theme(plot)) - bg <- if (S7::prop_exists(bg, "fill")) bg@fill else NULL - bg %||% "transparent" + try_prop(bg, "fill") %||% "transparent" } validate_device <- function(device, filename = NULL, dpi = 300, call = caller_env()) { diff --git a/R/theme.R b/R/theme.R index 18f3bf76e4..3270ded02c 100644 --- a/R/theme.R +++ b/R/theme.R @@ -821,10 +821,7 @@ calc_element <- function(element, theme, verbose = FALSE, skip_blank = FALSE, # recursion; we initiate skipping blanks if we encounter an element that # doesn't inherit blank. skip_blank <- skip_blank || - (!is.null(el_out) && - !isTRUE(S7::S7_inherits(el_out) && - S7::prop_exists(el_out, "inherit.blank") && - el_out@inherit.blank)) + (!is.null(el_out) && !isTRUE(try_prop(el_out, "inherit.blank"))) parents <- lapply( pnames, @@ -964,8 +961,8 @@ combine_elements <- function(e1, e2) { # If e2 is element_blank, and e1 inherits blank inherit everything from e2, # otherwise ignore e2 - if (S7::S7_inherits(e2, element_blank)) { - if (S7::prop_exists(e1, "inherit.blank") && e1@inherit.blank) { + if (is_theme_element(e2, "blank")) { + if (isTRUE(try_prop(e1, "inherit.blank"))) { return(e2) } else { return(e1) @@ -977,12 +974,12 @@ combine_elements <- function(e1, e2) { S7::props(e1)[n] <- S7::props(e2)[n] # Calculate relative sizes - if (S7::prop_exists(e1, "size") && is.rel(e1@size)) { + if (is.rel(try_prop(e1, "size"))) { e1@size <- e2@size * unclass(e1@size) } # Calculate relative linewidth - if (S7::prop_exists(e1, "linewidth") && is.rel(e1@linewidth)) { + if (is.rel(try_prop(e1, "linewidth"))) { e1@linewidth <- e2@linewidth * unclass(e1@linewidth) } diff --git a/R/utilities.R b/R/utilities.R index ff623df1a8..df642f3182 100644 --- a/R/utilities.R +++ b/R/utilities.R @@ -959,3 +959,13 @@ compute_data_size <- function(data, size, default = 0.9, data[[target]] <- res * (default %||% 0.9) data } + +try_prop <- function(object, name, default = NULL) { + if (!S7::S7_inherits(object)) { + return(default) + } + if (!S7::prop_exists(object, name)) { + return(default) + } + S7::prop(object, name) +} diff --git a/tests/testthat/test-theme.R b/tests/testthat/test-theme.R index 8000d25946..e83f69c521 100644 --- a/tests/testthat/test-theme.R +++ b/tests/testthat/test-theme.R @@ -344,13 +344,11 @@ test_that("element tree can be modified", { test_that("all elements in complete themes have inherit.blank=TRUE", { inherit_blanks <- function(theme) { - all(vapply(theme, function(el) { - if (is_theme_element(el) && S7::prop_exists(el, "inherit.blank")) { - el@inherit.blank - } else { - TRUE - } - }, logical(1))) + all(vapply( + theme, try_prop, + name = "inherit.blank", default = TRUE, + logical(1) + )) } expect_true(inherit_blanks(theme_grey())) expect_true(inherit_blanks(theme_bw())) From a7756357e9d46ff6627d90a5157a48e1d0a1cce1 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Wed, 16 Apr 2025 12:58:50 +0200 Subject: [PATCH 20/22] use classic extractors and subassignment --- NAMESPACE | 6 ++++++ R/theme-elements.R | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/NAMESPACE b/NAMESPACE index 3fa5bc812b..4d09ba2414 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -1,15 +1,21 @@ # Generated by roxygen2: do not edit by hand +S3method("$","ggplot2::element") S3method("$",ggproto) S3method("$",ggproto_parent) S3method("$",theme) +S3method("$<-","ggplot2::element") S3method("$<-",uneval) S3method("+",gg) +S3method("[","ggplot2::element") S3method("[",mapped_discrete) S3method("[",uneval) +S3method("[<-","ggplot2::element") S3method("[<-",mapped_discrete) S3method("[<-",uneval) +S3method("[[","ggplot2::element") S3method("[[",ggproto) +S3method("[[<-","ggplot2::element") S3method("[[<-",uneval) S3method(.DollarNames,ggproto) S3method(as.data.frame,mapped_discrete) diff --git a/R/theme-elements.R b/R/theme-elements.R index 15751c5319..5efad37745 100644 --- a/R/theme-elements.R +++ b/R/theme-elements.R @@ -329,6 +329,45 @@ rel <- function(x) { structure(x, class = "rel") } +#' @export +`$.ggplot2::element` <- function(x, i) { + # deprecate_soft0("4.0.0", I("`$i`"), I("`@i`")) + `[[`(S7::props(x), i) +} + +#' @export +`[.ggplot2::element` <- function(x, i) { + # deprecate_soft0("4.0.0", I("`[i]`"), I("`S7::props(, i)`")) + `[`(S7::props(x), i) +} + +#' @export +`[[.ggplot2::element` <- function(x, i) { + # deprecate_soft0("4.0.0", I("`[[i]]`"), I("`S7::prop(, i)`")) + `[[`(S7::props(x), i) +} + +#' @export +`$<-.ggplot2::element` <- function(x, i, value) { + # deprecate_soft0("4.0.0", I("`$i <- value`"), I("`@i <- value`")) + S7::props(x) <- `[[<-`(S7::props(x), i, value) + x +} + +#' @export +`[<-.ggplot2::element` <- function(x, i, value) { + # deprecate_soft0("4.0.0", I("`[i] <- value`"), I("`S7::props()[i] <- value`")) + S7::props(x) <- `[<-`(S7::props(x), i, value) + x +} + +#' @export +`[[<-.ggplot2::element` <- function(x, i, value) { + # deprecate_soft0("4.0.0", I("`[[i]] <- value`"), I("S7::prop(, i) <- value")) + S7::props(x) <- `[[<-`(S7::props(x), i, value) + x +} + #' @export print.rel <- function(x, ...) print(noquote(paste(x, " *", sep = ""))) From b38f3e523f92ba47d39e509ab271627d20cad8b5 Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Wed, 16 Apr 2025 13:14:02 +0200 Subject: [PATCH 21/22] fallback for `register_theme_elements()` --- R/theme-elements.R | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/R/theme-elements.R b/R/theme-elements.R index 5efad37745..7cddb0afd4 100644 --- a/R/theme-elements.R +++ b/R/theme-elements.R @@ -692,6 +692,27 @@ check_element_tree <- function(x, arg = caller_arg(x), call = caller_env()) { #' @keywords internal #' @export el_def <- function(class = NULL, inherit = NULL, description = NULL) { + if (is.character(class) && length(class) == 1) { + # Swap S3 class name for S7 class object + class <- switch( + class, + element = element, + element_blank = element_blank, + element_rect = element_rect, + element_line = element_line, + element_text = element_text, + element_polygon = element_polygon, + element_point = element_point, + element_geom = element_geom, + margin = margin, + class + ) + } + # margins often occur in c("unit", "margin", "rel"), we cannot use the + # S7 class here because we don't support heterogeneous lists + if (is.character(class) && length(class) > 1) { + class[class == "margin"] <- "ggplot2::margin" + } list(class = class, inherit = inherit, description = description) } From aa14c8811073485d46b0d992b2dd838a3451d96b Mon Sep 17 00:00:00 2001 From: Teun van den Brand Date: Wed, 16 Apr 2025 14:37:31 +0200 Subject: [PATCH 22/22] contingencies for `inherits(x, )` on old R versions --- R/theme-elements.R | 37 ++++++++++++++++++++++++++----------- 1 file changed, 26 insertions(+), 11 deletions(-) diff --git a/R/theme-elements.R b/R/theme-elements.R index 7cddb0afd4..a8d99e6466 100644 --- a/R/theme-elements.R +++ b/R/theme-elements.R @@ -948,21 +948,36 @@ check_element <- function(el, elname, element_tree, call = caller_env()) { # NULL values for elements are OK if (is.null(el)) return() + class <- eldef$class - if (inherits(class, "S7_class") && S7::S7_inherits(el)) { - if (S7::S7_inherits(el, class) || is_theme_element(el, "blank")) { + if (inherits(class, "S7_class")) { + inherit_ok <- S7::S7_inherits(el, class) + } else { + inherit_ok <- inherits(el, class) + } + + if (is.character(class) && any(c("margin", "ggplot2::margin") %in% class)) { + if ("rel" %in% class && is.rel(el)) { return() } + if (is.unit(el) && length(el) == 4) { + return() + } + cli::cli_abort( + "The {.var {elname}} theme element must be a {.cls unit} vector of length 4", + call = call + ) } - if (is.character(class) && "margin" %in% class) { - if (!is.unit(el) && length(el) == 4) - cli::cli_abort("The {.var {elname}} theme element must be a {.cls unit} vector of length 4.", call = call) - } else if (!inherits(el, class) && !is_theme_element(el, "blank")) { - if (inherits(class, "S7_class")) { - class <- class@name - } - cli::cli_abort("The {.var {elname}} theme element must be a {.cls {class}} object.", call = call) + # Maybe we should check that `class` is an element class before approving of + # blank elements? + if (inherit_ok || is_theme_element(el, "blank")) { + return() } - invisible() + + class_name <- if (inherits(class, "S7_class")) class@name else class + cli::cli_abort( + "The {.var {elname}} theme element must be a {.cls {class_name}} object.", + call = call + ) }