Skip to content

Commit

Permalink
Ensure complete themes never inherit defaults
Browse files Browse the repository at this point in the history
Fills in missing theme_void() components revealed by the fix.

Closes #2080. Fixes #2058. Fixes #2079. Written by @has2k1
  • Loading branch information
hadley committed Nov 1, 2017
1 parent 28b387b commit 6770641
Show file tree
Hide file tree
Showing 7 changed files with 88 additions and 47 deletions.
3 changes: 3 additions & 0 deletions NEWS.md
@@ -1,5 +1,8 @@
# ggplot2 2.2.1.9000

* Complete themes now always override all elements of the default theme
(@has2k1, #2058, #2079)

* Alternative syntax for calculated aesthetics. Instead of using
`aes(y = ..count..)` you can (and should!) now use `aes(y = calc(count))`.
`calc()` is a real function with documentation which hopefully will make
Expand Down
4 changes: 3 additions & 1 deletion R/labeller.r
Expand Up @@ -554,7 +554,9 @@ build_strip <- function(label_df, labeller, theme, horizontal) {
clip = "on"
)

theme$strip.text.y$angle <- adjust_angle(theme$strip.text.y$angle)
if (inherits(theme$strip.text.y, "element_text")) {
theme$strip.text.y$angle <- adjust_angle(theme$strip.text.y$angle)
}

grobs_left <- grobs

Expand Down
45 changes: 32 additions & 13 deletions R/theme-defaults.r
Expand Up @@ -221,7 +221,7 @@ theme_linedraw <- function(base_size = 11, base_family = "",
base_line_size = base_size / 22,
base_rect_size = base_size / 22) {
half_line <- base_size / 2

# Starts with theme_bw and then modify some parts
# = replace all greys with pure black or white
theme_bw(
Expand Down Expand Up @@ -260,7 +260,7 @@ theme_light <- function(base_size = 11, base_family = "",
base_line_size = base_size / 22,
base_rect_size = base_size / 22) {
half_line <- base_size / 2

# Starts with theme_grey and then modify some parts
theme_grey(
base_size = base_size,
Expand Down Expand Up @@ -302,7 +302,7 @@ theme_dark <- function(base_size = 11, base_family = "",
base_line_size = base_size / 22,
base_rect_size = base_size / 22) {
half_line <- base_size / 2

# Starts with theme_grey and then modify some parts
theme_grey(
base_size = base_size,
Expand Down Expand Up @@ -398,10 +398,9 @@ theme_void <- function(base_size = 11, base_family = "",
base_line_size = base_size / 22,
base_rect_size = base_size / 22) {
half_line <- base_size / 2


# Only keep indispensable text: legend and plot titles
theme(
# Use only inherited elements and make almost everything blank
# Only keep indispensable text
line = element_blank(),
rect = element_blank(),
text = element_text(
Expand All @@ -410,27 +409,47 @@ theme_void <- function(base_size = 11, base_family = "",
lineheight = 0.9, hjust = 0.5, vjust = 0.5, angle = 0,
margin = margin(), debug = FALSE
),
axis.text = element_blank(),
axis.title = element_blank(),
axis.text = element_blank(),
axis.title = element_blank(),
axis.ticks.length = unit(0, "pt"),
legend.key.size = unit(1.2, "lines"),
legend.position = "right",
legend.text = element_text(size = rel(0.8)),
legend.title = element_text(hjust = 0),
strip.text = element_text(
size = rel(0.8),
margin = margin(half_line, half_line, half_line, half_line)
),
strip.text = element_text(size = rel(0.8)),
strip.switch.pad.grid = unit(0.1, "cm"),
strip.switch.pad.wrap = unit(0.1, "cm"),
panel.ontop = FALSE,
panel.spacing = unit(half_line, "pt"),
plot.margin = unit(c(0, 0, 0, 0), "lines"),
plot.title = element_text(
size = rel(1.2),
hjust = 0, vjust = 1,
margin = margin(t = half_line * 1.2)
),
plot.subtitle = element_text(
size = rel(0.9),
hjust = 0, vjust = 1,
margin = margin(t = half_line * 0.9)
),
plot.caption = element_text(
size = rel(0.9),
hjust = 1, vjust = 1,
margin = margin(t = half_line * 0.9)
),

complete = TRUE
)
}


#' @export
#' @rdname ggtheme
theme_test <- function(base_size = 11, base_family = "",
base_line_size = base_size / 22,
base_rect_size = base_size / 22) {
half_line <- base_size / 2

theme(
line = element_line(
colour = "black", size = base_line_size,
Expand Down
15 changes: 11 additions & 4 deletions R/theme.r
Expand Up @@ -413,10 +413,17 @@ theme <- function(line,
)
}

is_theme_complete <- function(x) isTRUE(attr(x, "complete"))


# Combine plot defaults with current theme to get complete theme for a plot
plot_theme <- function(x) {
defaults(x$theme, theme_get())
plot_theme <- function(x, default = theme_get()) {
theme <- x$theme
if (is_theme_complete(theme)) {
theme
} else {
defaults(theme, default)
}
}

#' Modify properties of an element in a theme object
Expand Down Expand Up @@ -456,7 +463,7 @@ add_theme <- function(t1, t2, t2name) {
}

# If either theme is complete, then the combined theme is complete
attr(t1, "complete") <- attr(t1, "complete") || attr(t2, "complete")
attr(t1, "complete") <- is_theme_complete(t1) || is_theme_complete(t2)
t1
}

Expand Down Expand Up @@ -486,7 +493,7 @@ add_theme <- function(t1, t2, t2name) {
update_theme <- function(oldtheme, newtheme) {
# If the newtheme is a complete one, don't bother searching
# the default theme -- just replace everything with newtheme
if (attr(newtheme, "complete"))
if (is_theme_complete(newtheme))
return(newtheme)

# These are elements in newtheme that aren't already set in oldtheme.
Expand Down
28 changes: 14 additions & 14 deletions tests/figs/themes/theme-void-large.svg
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
28 changes: 14 additions & 14 deletions tests/figs/themes/theme-void.svg
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
12 changes: 11 additions & 1 deletion tests/testthat/test-theme.r
Expand Up @@ -224,6 +224,16 @@ test_that("Elements can be merged", {
)
})

test_that("complete plot themes shouldn't inherit from default", {
default_theme <- theme_gray() + theme(axis.text.x = element_text(colour = "red"))
base <- qplot(1, 1)

ptheme <- plot_theme(base + theme(axis.text.x = element_text(colour = "blue")), default_theme)
expect_equal(ptheme$axis.text.x$colour, "blue")

ptheme <- plot_theme(base + theme_void(), default_theme)
expect_null(ptheme$axis.text.x)
})

# Visual tests ------------------------------------------------------------

Expand Down Expand Up @@ -315,4 +325,4 @@ test_that("axes can be styled independently", {
axis.line.y.right = element_line(colour = 'yellow')
)
vdiffr::expect_doppelganger("axes_styling", plot)
})
})

0 comments on commit 6770641

Please sign in to comment.