Fix after_scale() with plot mappings

[yutannihilation]

Fix #4260

This feels a bit uneasy to repeat defaults(layer$mapping, plot$mapping) everywhere, but I don't come up with a nice idea... Any suggestions are welcome. This pull request moves defaults(layer$mapping, plot$mapping) to setup_layer() so that we don't need to do it in individual methods that refer to the mapping.

devtools::load_all("~/repo/ggplot2")
#> Loading ggplot2

ggplot(mpg, aes(class, hwy, colour = class, fill = after_scale(alpha(colour, 0.4)))) +
  geom_boxplot()

^{Created on 2020-11-15 by the reprex package (v0.3.0)}

[has2k1]

This feels a bit uneasy to repeat defaults(layer$mapping, plot$mapping) everywhere, but I don't come up with a nice idea... Any suggestions are welcome.

This is against the grain of the mainly functional style in the codebase, but somewhere in a "setup" function of the layer you could resolve the effective mapping for each layer, kind of similar to how a copy of the data is generated for each layer at the start of the plot build.

if (self$inherit.aes) {
   self$mapping <- defaults(self$mapping, plot$mapping)
}

[yutannihilation]

Thanks, it sounds nice, but I'm not yet sure when is safe to modify the layer...

[has2k1]

Thanks, it sounds nice, but I'm not yet sure when is safe to modify the layer...

Here

ggplot2/R/plot-build.r

Lines 34 to 35 in a132727

	layers <- plot$layers
	layer_data <- lapply(layers, function(y) y$layer_data(plot$data))

Just like there is certainty about the data at that point, there is also certainty about the aesthetics.

[clauswilke]

I'm not sure I fully understand the problem, but would a fix fit into the setup_layer() function?

ggplot2/R/layer.r

Lines 205 to 207 in a132727

	setup_layer = function(self, data, plot) {
	data
	},

See e.g. what we do for sf-specific layers:

ggplot2/R/layer-sf.R

Lines 35 to 71 in a132727

	setup_layer = function(self, data, plot) {
	# process generic layer setup first
	data <- ggproto_parent(Layer, self)$setup_layer(data, plot)
	# automatically determine the name of the geometry column
	# and add the mapping if it doesn't exist
	if ((isTRUE(self$inherit.aes) && is.null(self$mapping$geometry) && is.null(plot$mapping$geometry)) ||
	(!isTRUE(self$inherit.aes) && is.null(self$mapping$geometry))) {
	if (is_sf(data)) {
	geometry_col <- attr(data, "sf_column")
	self$mapping$geometry <- sym(geometry_col)
	}
	}
	# automatically determine the legend type
	if (is.null(self$legend_key_type)) {
	# first, set default value in case downstream tests fail
	self$geom_params$legend <- "polygon"
	# now check if the type should not be polygon
	if (!is.null(self$mapping$geometry) && quo_is_symbol(self$mapping$geometry)) {
	geometry_column <- as_name(self$mapping$geometry)
	if (inherits(data[[geometry_column]], "sfc")) {
	sf_type <- detect_sf_type(data[[geometry_column]])
	if (sf_type == "point") {
	self$geom_params$legend <- "point"
	} else if (sf_type == "line") {
	self$geom_params$legend <- "line"
	}
	}
	}
	} else {
	self$geom_params$legend <- self$legend_key_type
	}
	data
	}
	)

We could just finalize the layer mapping in that function and then remove this code from map_statistic():

ggplot2/R/layer.r

Lines 290 to 296 in a132727

	# Assemble aesthetics from layer, plot and stat mappings
	aesthetics <- self$mapping
	if (self$inherit.aes) {
	aesthetics <- defaults(aesthetics, plot$mapping)
	}
	aesthetics <- defaults(aesthetics, self$stat$default_aes)
	aesthetics <- compact(aesthetics)

[has2k1]

but would a fix fit into the setup_layer() function?

Yes it would. setup_layer for sf resolves my concern about keeping methods as pure functions.

[yutannihilation]

What I'm not confident is, whether there is really no case (including extension packages) when we need to distinguish inherited mappings and layer's original mappings. But, considering we already do tweak the mappings in setup_layer(), I now feel it might be okay. Thanks for the suggestion.

[yutannihilation]

This isn't a very big problem, but, interestingly, adding these lines in setup_layer() breaks LayerSf. Since defaults() doesn't preserve the class of uneval, $<-.uneval method won't be triggered when self$mapping$geometry <- sym(geometry_col).

[thomasp85]

@yutannihilation where are we on this? Your last comment indicates that it is still not done?

[yutannihilation]

Ah, sorry I forgot to add a comment, but the concern in my last comment was addressed by this commit.

08de90d

So, I think this is ready now (except for the conflict).

[thomasp85]

LGTM

[yutannihilation]

Thanks, I'll merge this after confirming the CI passes