Skip to content

Commit 1cede18

Browse files
authored
Make sure mapping is not stateful (#4475)
1 parent c9adeed commit 1cede18

File tree

7 files changed

+34
-18
lines changed

7 files changed

+34
-18
lines changed

R/guide-bins.R

+2-2
Original file line numberDiff line numberDiff line change
@@ -194,7 +194,7 @@ guide_merge.bins <- function(guide, new_guide) {
194194
guide_geom.bins <- function(guide, layers, default_mapping) {
195195
# arrange common data for vertical and horizontal guide
196196
guide$geoms <- lapply(layers, function(layer) {
197-
matched <- matched_aes(layer, guide, default_mapping)
197+
matched <- matched_aes(layer, guide)
198198

199199
# check if this layer should be included
200200
include <- include_layer_in_guide(layer, matched)
@@ -208,7 +208,7 @@ guide_geom.bins <- function(guide, layers, default_mapping) {
208208
n <- vapply(layer$aes_params, length, integer(1))
209209
params <- layer$aes_params[n == 1]
210210

211-
aesthetics <- layer$mapping
211+
aesthetics <- layer$computed_mapping
212212
modifiers <- aesthetics[is_scaled_aes(aesthetics) | is_staged_aes(aesthetics)]
213213

214214
data <- tryCatch(

R/guide-colorbar.r

+1-1
Original file line numberDiff line numberDiff line change
@@ -248,7 +248,7 @@ guide_merge.colorbar <- function(guide, new_guide) {
248248
guide_geom.colorbar <- function(guide, layers, default_mapping) {
249249
# Layers that use this guide
250250
guide_layers <- lapply(layers, function(layer) {
251-
matched <- matched_aes(layer, guide, default_mapping)
251+
matched <- matched_aes(layer, guide)
252252

253253
if (length(matched) == 0) {
254254
# This layer does not use this guide

R/guide-legend.r

+2-2
Original file line numberDiff line numberDiff line change
@@ -247,7 +247,7 @@ guide_merge.legend <- function(guide, new_guide) {
247247
guide_geom.legend <- function(guide, layers, default_mapping) {
248248
# arrange common data for vertical and horizontal guide
249249
guide$geoms <- lapply(layers, function(layer) {
250-
matched <- matched_aes(layer, guide, default_mapping)
250+
matched <- matched_aes(layer, guide)
251251

252252
# check if this layer should be included
253253
include <- include_layer_in_guide(layer, matched)
@@ -261,7 +261,7 @@ guide_geom.legend <- function(guide, layers, default_mapping) {
261261
n <- vapply(layer$aes_params, length, integer(1))
262262
params <- layer$aes_params[n == 1]
263263

264-
aesthetics <- layer$mapping
264+
aesthetics <- layer$computed_mapping
265265
modifiers <- aesthetics[is_scaled_aes(aesthetics) | is_staged_aes(aesthetics)]
266266

267267
data <- tryCatch(

R/guides-.r

+3-2
Original file line numberDiff line numberDiff line change
@@ -244,6 +244,7 @@ guides_merge <- function(gdefs) {
244244
}
245245

246246
# process layer information
247+
# TODO: `default_mapping` is unused internally but kept for backwards compitability until guide rewrite
247248
guides_geom <- function(gdefs, layers, default_mapping) {
248249
compact(lapply(gdefs, guide_geom, layers, default_mapping))
249250
}
@@ -372,8 +373,8 @@ guide_gengrob <- function(guide, theme) UseMethod("guide_gengrob")
372373

373374
# Helpers -----------------------------------------------------------------
374375

375-
matched_aes <- function(layer, guide, defaults) {
376-
all <- names(c(layer$mapping, if (layer$inherit.aes) defaults, layer$stat$default_aes))
376+
matched_aes <- function(layer, guide) {
377+
all <- names(c(layer$computed_mapping, layer$stat$default_aes))
377378
geom <- c(layer$geom$required_aes, names(layer$geom$default_aes))
378379
matched <- intersect(intersect(all, geom), names(guide$key))
379380
matched <- setdiff(matched, names(layer$geom_params))

R/layer-sf.R

+6-5
Original file line numberDiff line numberDiff line change
@@ -38,11 +38,12 @@ LayerSf <- ggproto("LayerSf", Layer,
3838

3939
# automatically determine the name of the geometry column
4040
# and add the mapping if it doesn't exist
41-
if ((isTRUE(self$inherit.aes) && is.null(self$mapping$geometry) && is.null(plot$mapping$geometry)) ||
42-
(!isTRUE(self$inherit.aes) && is.null(self$mapping$geometry))) {
41+
if ((isTRUE(self$inherit.aes) && is.null(self$computed_mapping$geometry) &&
42+
is.null(plot$computed_mapping$geometry)) ||
43+
(!isTRUE(self$inherit.aes) && is.null(self$computed_mapping$geometry))) {
4344
if (is_sf(data)) {
4445
geometry_col <- attr(data, "sf_column")
45-
self$mapping$geometry <- sym(geometry_col)
46+
self$computed_mapping$geometry <- sym(geometry_col)
4647
}
4748
}
4849

@@ -52,8 +53,8 @@ LayerSf <- ggproto("LayerSf", Layer,
5253
self$geom_params$legend <- "polygon"
5354

5455
# now check if the type should not be polygon
55-
if (!is.null(self$mapping$geometry) && quo_is_symbol(self$mapping$geometry)) {
56-
geometry_column <- as_name(self$mapping$geometry)
56+
if (!is.null(self$computed_mapping$geometry) && quo_is_symbol(self$computed_mapping$geometry)) {
57+
geometry_column <- as_name(self$computed_mapping$geometry)
5758
if (inherits(data[[geometry_column]], "sfc")) {
5859
sf_type <- detect_sf_type(data[[geometry_column]])
5960
if (sf_type == "point") {

R/layer.r

+8-5
Original file line numberDiff line numberDiff line change
@@ -174,6 +174,7 @@ Layer <- ggproto("Layer", NULL,
174174
# calculated before use
175175
computed_geom_params = NULL,
176176
computed_stat_params = NULL,
177+
computed_mapping = NULL,
177178

178179
data = NULL,
179180
aes_params = NULL,
@@ -211,16 +212,18 @@ Layer <- ggproto("Layer", NULL,
211212
setup_layer = function(self, data, plot) {
212213
# For annotation geoms, it is useful to be able to ignore the default aes
213214
if (isTRUE(self$inherit.aes)) {
214-
self$mapping <- defaults(self$mapping, plot$mapping)
215+
self$computed_mapping <- defaults(self$mapping, plot$mapping)
215216
# defaults() strips class, but it needs to be preserved for now
216-
class(self$mapping) <- "uneval"
217+
class(self$computed_mapping) <- "uneval"
218+
} else {
219+
self$computed_mapping <- self$mapping
217220
}
218221

219222
data
220223
},
221224

222225
compute_aesthetics = function(self, data, plot) {
223-
aesthetics <- self$mapping
226+
aesthetics <- self$computed_mapping
224227

225228
# Drop aesthetics that are set or calculated
226229
set <- names(aesthetics) %in% names(self$aes_params)
@@ -296,7 +299,7 @@ Layer <- ggproto("Layer", NULL,
296299
data <- rename_aes(data)
297300

298301
# Assemble aesthetics from layer, plot and stat mappings
299-
aesthetics <- self$mapping
302+
aesthetics <- self$computed_mapping
300303
aesthetics <- defaults(aesthetics, self$stat$default_aes)
301304
aesthetics <- compact(aesthetics)
302305

@@ -362,7 +365,7 @@ Layer <- ggproto("Layer", NULL,
362365
# Combine aesthetics, defaults, & params
363366
if (empty(data)) return(data)
364367

365-
aesthetics <- self$mapping
368+
aesthetics <- self$computed_mapping
366369
modifiers <- aesthetics[is_scaled_aes(aesthetics) | is_staged_aes(aesthetics)]
367370

368371
self$geom$use_defaults(data, self$aes_params, modifiers)

tests/testthat/test-layer.r

+12-1
Original file line numberDiff line numberDiff line change
@@ -68,11 +68,22 @@ test_that("layers are stateless except for the computed params", {
6868
p <- ggplot(df) +
6969
geom_col(aes(x = x, y = y), width = 0.8, fill = "red")
7070
col_layer <- as.list(p$layers[[1]])
71-
stateless_names <- setdiff(names(col_layer), c("computed_geom_params", "computed_stat_params"))
71+
stateless_names <- setdiff(names(col_layer), c("computed_geom_params", "computed_stat_params", "computed_mapping"))
7272
invisible(ggplotGrob(p))
7373
expect_identical(as.list(p$layers[[1]])[stateless_names], col_layer[stateless_names])
7474
})
7575

76+
test_that("inherit.aes works", {
77+
df <- data.frame(x = 1:10, y = 1:10)
78+
p1 <- ggplot(df, aes(y = y)) +
79+
geom_col(aes(x = x), inherit.aes = TRUE)
80+
p2 <- ggplot(df, aes(colour = y)) +
81+
geom_col(aes(x = x, y = y), inherit.aes = FALSE)
82+
invisible(ggplotGrob(p1))
83+
invisible(ggplotGrob(p2))
84+
expect_identical(p1$layers[[1]]$computed_mapping, p2$layers[[1]]$computed_mapping)
85+
})
86+
7687
# Data extraction ---------------------------------------------------------
7788

7889
test_that("layer_data returns a data.frame", {

0 commit comments

Comments
 (0)