Clean up theme addition

[clauswilke]

Here is an attempt at fixing #3039. Recall: That issue shows that adding theme elements piecewise behaves differently from adding them all at once. The problem is that when adding a theme to a plot object, the current implementation pulls missing info from the default theme, but this is not the case when adding a theme to a theme. In my mind, this behavior is inconsistent. The addition should behave the same in both cases. The solution is to delete the plot-specific update function.

This is the reprex from #3039, which now works just fine:

library(ggplot2)

p <- ggplot(mtcars, aes(wt, mpg)) + geom_point()
p1 <- p + theme_light() + 
  theme(axis.line.x = element_line(color = "blue")) +
  theme(axis.ticks.x = element_line(color = "red"))

p2 <- p + theme_light() +
  theme(axis.line.x = element_line(color = "blue"),
        axis.ticks.x = element_line(color = "red"))

identical(p1$theme, p2$theme)
#> [1] TRUE

p1

p2

^{Created on 2019-10-14 by the reprex package (v0.3.0)}

However, this is potentially a breaking change (and in fact, some unit tests currently break), because partially updating root elements of a theme doesn't work anymore:
(Update: This has now been fixed, see below.)

library(ggplot2)

# this now breaks but works in current ggplot2 by magically pulling info from default theme
ggplot(iris, aes(Sepal.Length, Sepal.Width)) +
  geom_point() +
  theme(text = element_text(color = "red"))
#> Error in FUN(X[[i]], ...): Theme element 'text' has NULL property: family, face, size, hjust, vjust, angle, lineheight, margin, debug

# this still works
ggplot(iris, aes(Sepal.Length, Sepal.Width)) +
  geom_point() +
  theme(axis.title = element_text(color = "red"))

^{Created on 2019-10-14 by the reprex package (v0.3.0)}

I think the new behavior is correct. If we're messing with a root element, we should have to specify it completely. However, if that's considered to be bad, we could also fix the problem at this point in the code:

ggplot2/R/theme.r

Lines 558 to 565 in 115c396

	# If no parents, this is a "root" node. Just return this element.
	if (is.null(pnames)) {
	# Check that all the properties of this element are non-NULL
	nullprops <- vapply(theme[[element]], is.null, logical(1))
	if (any(nullprops)) {
	stop("Theme element '", element, "' has NULL property: ",
	paste(names(nullprops)[nullprops], collapse = ", "))
	}

by pulling values from the ggplot2 default theme where available instead of issuing an error.

[clauswilke]

@hadley @thomasp85 Could you provide feedback on this? Should I proceed along these lines? And should I implement the workaround for NULL values in root elements, as explained at the end?

[clauswilke]

After some more thinking, I decided that filling in root elements from the default theme is probably the right way to go. The required changes to make this happen were much more involved than I expected, but I think overall the code is moving in the right direction and getting cleaner. There's now only one theme addition function, and it consists of only a few lines of code. Also, all the unit tests work again (two made incorrect assumptions under the new model and required fixing), including all visual tests without modification. The following reprex works again also.

library(ggplot2)

ggplot(iris, aes(Sepal.Length, Sepal.Width)) +
  geom_point() +
  theme(text = element_text(color = "red"))

^{Created on 2019-10-14 by the reprex package (v0.3.0)}

[thomasp85]

LGTM

[lock]

This old issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with reprex) and link to this issue. https://reprex.tidyverse.org/