Add function for registering of new theme elements. #3678

clauswilke · 2019-12-18T22:17:03Z

This closes #3677.

library(ggplot2)

# define a new coord that includes a panel annotation
coord_annotate <- function(label = "panel annotation") {
  ggproto(NULL, CoordCartesian,
          limits = list(x = NULL, y = NULL),
          expand = TRUE,
          default = FALSE,
          clip = "on",
          render_fg = function(panel_params, theme) {
            element_render(theme, "panel.annotation", label = label)
          }
  )
}

# register a new theme element `panel.annotation`
register_theme_elements(
  panel.annotation = element_text(color = "blue", hjust = 0.95, vjust = 0.05),
  element_tree = list(panel.annotation = el_def("element_text", "text"))
)

# change default theme, this now creates a warning of a missing element,
# but things will work fine
theme_set(theme_bw())
#> Warning: New theme missing the following elements: panel.annotation

df <- data.frame(x = 1:3, y = 1:3)
ggplot(df, aes(x, y)) +
  geom_point() +
  coord_annotate("annotation in blue")

# revert default to original ggplot2 settings, but don't touch currently active theme
register_theme_elements(reset_all = TRUE, modify_current = FALSE)

ggplot(df, aes(x, y)) +
  geom_point() +
  coord_annotate("annotation in blue")
#> Theme element `panel.annotation` missing

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

tidyverse#3677

clauswilke · 2019-12-18T22:19:00Z

And another reprex, changing the order of things (change theme default first, then register new element).

library(ggplot2)

# define a new coord that includes a panel annotation
coord_annotate <- function(label = "panel annotation") {
  ggproto(NULL, CoordCartesian,
          limits = list(x = NULL, y = NULL),
          expand = TRUE,
          default = FALSE,
          clip = "on",
          render_fg = function(panel_params, theme) {
            element_render(theme, "panel.annotation", label = label)
          }
  )
}

# change default theme
theme_set(theme_bw())

# register a new theme element `panel.annotation`
register_theme_elements(
  panel.annotation = element_text(color = "blue", hjust = 0.95, vjust = 0.05),
  element_tree = list(panel.annotation = el_def("element_text", "text"))
)

df <- data.frame(x = 1:3, y = 1:3)
ggplot(df, aes(x, y)) +
  geom_point() +
  coord_annotate("annotation in blue")

# revert default to original ggplot2 settings, including currently active theme
register_theme_elements(reset_all = TRUE)

ggplot(df, aes(x, y)) +
  geom_point() +
  coord_annotate("annotation in blue")
#> Theme element `panel.annotation` missing

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

clauswilke · 2019-12-18T22:27:34Z

I realize this still needs a news item and some unit tests, but I'd first like to get some feedback. Am I on the right track?

clauswilke · 2019-12-18T22:32:26Z

One side-effect of this PR is that you can now completely nuke the currently active theme and still things will generally work.

library(ggplot2)

theme_set(theme(text = element_blank()))
#> Warning: New theme missing the following elements: line, rect,
#> title, aspect.ratio, axis.title, axis.title.x, axis.title.x.top,
#> axis.title.x.bottom, axis.title.y, axis.title.y.left, axis.title.y.right,
#> axis.text, axis.text.x, axis.text.x.top, axis.text.x.bottom, axis.text.y,
#> axis.text.y.left, axis.text.y.right, axis.ticks, axis.ticks.x,
#> axis.ticks.x.top, axis.ticks.x.bottom, axis.ticks.y, axis.ticks.y.left,
#> axis.ticks.y.right, axis.ticks.length, axis.ticks.length.x,
#> axis.ticks.length.x.top, axis.ticks.length.x.bottom, axis.ticks.length.y,
#> axis.ticks.length.y.left, axis.ticks.length.y.right, axis.line,
#> axis.line.x, axis.line.x.top, axis.line.x.bottom, axis.line.y,
#> axis.line.y.left, axis.line.y.right, legend.background, legend.margin,
#> legend.spacing, legend.spacing.x, legend.spacing.y, legend.key,
#> legend.key.size, legend.key.height, legend.key.width, legend.text,
#> legend.text.align, legend.title, legend.title.align, legend.position,
#> legend.direction, legend.justification, legend.box, legend.box.just,
#> legend.box.margin, legend.box.background, legend.box.spacing,
#> panel.background, panel.border, panel.spacing, panel.spacing.x,
#> panel.spacing.y, panel.grid, panel.grid.major, panel.grid.minor,
#> panel.grid.major.x, panel.grid.major.y, panel.grid.minor.x,
#> panel.grid.minor.y, panel.ontop, plot.background, plot.title,
#> plot.title.position, plot.subtitle, plot.caption, plot.caption.position,
#> plot.tag, plot.tag.position, plot.margin, strip.background,
#> strip.background.x, strip.background.y, strip.placement, strip.text,
#> strip.text.x, strip.text.y, strip.switch.pad.grid and strip.switch.pad.wrap

df <- data.frame(x = 1:3, y = 1:3)
ggplot(df, aes(x, y)) +
  geom_point()

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

thomasp85 · 2019-12-19T07:35:21Z

In general, I think a warning should only be emitted if an element both isn't defined and doesn't have a defined parent.

In the example above with panel.annotation the element is set to inherit from text, which is defined in the new theme so it should just inherit directly from that.

I think we can enforce that newly registered theme elements must inherit from an existing element and thus ensure that it works with any valid complete theme

clauswilke · 2019-12-19T15:15:01Z

Just so we're on the same page: The warning is carried over from the current ggplot2 behavior. See reprex below, with ggplot2 3.2.1. I'm happy to remove the warning, as I think it generates a lot of false positives on cases that ggplot2 can recover from just fine, and with the new PR even more so.

I'm also happy to add in a new warning as long as the logic is straightforward. In either case, though, the logic you describe would imply that the majority of cases that currently throw a warning wouldn't anymore. Is this your intended outcome?

library(ggplot2)

t <- theme_gray()
t$axis.text.x <- NULL

theme_set(t)
#> Warning: New theme missing the following elements: axis.text.x

df <- data.frame(x = 1:3, y = 1:3)
ggplot(df, aes(x, y)) +
  geom_point()

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

Session info

devtools::session_info()
#> ─ Session info ──────────────────────────────────────────────────────────
#>  setting  value                       
#>  version  R version 3.6.0 (2019-04-26)
#>  os       macOS Mojave 10.14.5        
#>  system   x86_64, darwin15.6.0        
#>  ui       X11                         
#>  language (EN)                        
#>  collate  en_US.UTF-8                 
#>  ctype    en_US.UTF-8                 
#>  tz       America/Chicago             
#>  date     2019-12-19                  
#> 
#> ─ Packages ──────────────────────────────────────────────────────────────
#>  package     * version    date       lib source        
#>  assertthat    0.2.1      2019-03-21 [1] CRAN (R 3.6.0)
#>  backports     1.1.5      2019-10-02 [1] CRAN (R 3.6.0)
#>  callr         3.3.2      2019-09-22 [1] CRAN (R 3.6.0)
#>  cli           1.1.0      2019-03-19 [1] CRAN (R 3.6.0)
#>  colorspace    1.4-1      2019-03-18 [1] CRAN (R 3.6.0)
#>  crayon        1.3.4      2017-09-16 [1] CRAN (R 3.6.0)
#>  curl          4.1        2019-09-16 [1] CRAN (R 3.6.0)
#>  desc          1.2.0      2018-05-01 [1] CRAN (R 3.6.0)
#>  devtools      2.0.2      2019-04-08 [1] CRAN (R 3.6.0)
#>  digest        0.6.23     2019-11-23 [1] CRAN (R 3.6.0)
#>  dplyr         0.8.3      2019-07-04 [1] CRAN (R 3.6.0)
#>  evaluate      0.14       2019-05-28 [1] CRAN (R 3.6.0)
#>  farver        2.0.1      2019-11-13 [1] CRAN (R 3.6.0)
#>  fs            1.3.1      2019-05-06 [1] CRAN (R 3.6.0)
#>  ggplot2     * 3.2.1      2019-08-10 [1] CRAN (R 3.6.0)
#>  glue          1.3.1      2019-03-12 [1] CRAN (R 3.6.0)
#>  gtable        0.3.0      2019-03-25 [1] CRAN (R 3.6.0)
#>  highr         0.8        2019-03-20 [1] CRAN (R 3.6.0)
#>  htmltools     0.3.6      2017-04-28 [1] CRAN (R 3.6.0)
#>  httr          1.4.1      2019-08-05 [1] CRAN (R 3.6.0)
#>  knitr         1.25       2019-09-18 [1] CRAN (R 3.6.0)
#>  labeling      0.3        2014-08-23 [1] CRAN (R 3.6.0)
#>  lazyeval      0.2.2      2019-03-15 [1] CRAN (R 3.6.0)
#>  lifecycle     0.1.0      2019-08-01 [1] CRAN (R 3.6.0)
#>  magrittr      1.5        2014-11-22 [1] CRAN (R 3.6.0)
#>  memoise       1.1.0      2017-04-21 [1] CRAN (R 3.6.0)
#>  mime          0.7        2019-06-11 [1] CRAN (R 3.6.0)
#>  munsell       0.5.0      2018-06-12 [1] CRAN (R 3.6.0)
#>  pillar        1.4.2      2019-06-29 [1] CRAN (R 3.6.0)
#>  pkgbuild      1.0.6      2019-10-09 [1] CRAN (R 3.6.0)
#>  pkgconfig     2.0.3      2019-09-22 [1] CRAN (R 3.6.0)
#>  pkgload       1.0.2      2018-10-29 [1] CRAN (R 3.6.0)
#>  prettyunits   1.0.2      2015-07-13 [1] CRAN (R 3.6.0)
#>  processx      3.4.1      2019-07-18 [1] CRAN (R 3.6.0)
#>  ps            1.3.0      2018-12-21 [1] CRAN (R 3.6.0)
#>  purrr         0.3.3      2019-10-18 [1] CRAN (R 3.6.0)
#>  R6            2.4.1      2019-11-12 [1] CRAN (R 3.6.0)
#>  Rcpp          1.0.3      2019-11-08 [1] CRAN (R 3.6.0)
#>  remotes       2.1.0      2019-06-24 [1] CRAN (R 3.6.0)
#>  rlang         0.4.2      2019-11-23 [1] CRAN (R 3.6.0)
#>  rmarkdown     1.15       2019-08-21 [1] CRAN (R 3.6.0)
#>  rprojroot     1.3-2      2018-01-03 [1] CRAN (R 3.6.0)
#>  scales        1.1.0.9000 2019-12-18 [1] local         
#>  sessioninfo   1.1.1      2018-11-05 [1] CRAN (R 3.6.0)
#>  stringi       1.4.3      2019-03-12 [1] CRAN (R 3.6.0)
#>  stringr       1.4.0      2019-02-10 [1] CRAN (R 3.6.0)
#>  testthat      2.3.1      2019-12-01 [1] CRAN (R 3.6.0)
#>  tibble        2.1.3      2019-06-06 [1] CRAN (R 3.6.0)
#>  tidyselect    0.2.5      2018-10-11 [1] CRAN (R 3.6.0)
#>  usethis       1.5.0      2019-04-07 [1] CRAN (R 3.6.0)
#>  withr         2.1.2      2018-03-15 [1] CRAN (R 3.6.0)
#>  xfun          0.9        2019-08-21 [1] CRAN (R 3.6.0)
#>  xml2          1.2.2      2019-08-09 [1] CRAN (R 3.6.0)
#>  yaml          2.2.0      2018-07-25 [1] CRAN (R 3.6.0)
#> 
#> [1] /Library/Frameworks/R.framework/Versions/3.6/Resources/library

thomasp85 · 2019-12-20T11:49:32Z

Ah - I wasn't aware of that warning in the current code. I think we should remove it. The inheritance setup specifically allows for NULL as long as it has a valid parent so warning against it seems weird.

But I think you should enforce that new theme elements always inherit from something, e.g. text, or line etc. I don't think I understand the intend of the complete argument. Is that to allow new root elements? If so I think it should default to FALSE. I also think modify_current should default to FALSE. Lastly the reset functionality seems like something that should exist in its own function rather than be part of this..?

Basically I think this would be a function solemnly for extension developers. They should not rely on what the current theme is, so modifying that instead of the default (which is fixed now, right?) is bad. Because they don't know the look of the current theme it is better to encourage them to inherit from an existing element as they are more likely to get a fitting look

clauswilke · 2019-12-20T20:46:28Z

@thomasp85 You raise a number of points. I'll try to address them all, one by one, in order.

Ah - I wasn't aware of that warning in the current code. I think we should remove it. The inheritance setup specifically allows for NULL as long as it has a valid parent so warning against it seems weird.

It's a little more complicated than that. In the latest code, an explicit NULL means "inherit everything from parent", but an entirely missing element means "look first whether the element is defined in the default, and use that one if available". I had to code it this way so that existing themes play nicely with newly defined theme elements. The warning here checks for entirely missing elements, so it de facto checks for elements that would be filled in from the underlying default. I could reword the warning accordingly (e.g.: "New theme missing the following elements: ... These elements will be replaced by their ggplot2 defaults."). But I'm also good with removing it altogether. (We don't warn when such a theme is applied to a plot, we just do it.)

But I think you should enforce that new theme elements always inherit from something, e.g. text, or line etc.

There are plenty of elements that don't inherit from anything, such as legend.position, and I think extension developers should be able to create those. Also, I think there is a good use case for replacing root elements. E.g., I have considered adding a root element textbox that behaves like text but has additional parameter settings for a box around the text, such as background color, corner radius, etc.

I don't think I understand the intend of the complete argument. Is that to allow new root elements? If so I think it should default to FALSE.

I think complete is entirely misunderstood by everybody, because it is used for two separate purposes. Here, it is being used to indicate that all elements should inherit from blank, as described in this recent comment I made on the ggplot2 book: hadley/ggplot2-book#171 (comment)
It's also how all themes defined in the ggplot2 code base are written, see e.g. here:

ggplot2/R/theme-defaults.r

Lines 403 to 419 in e6cbcc1

    
           theme_bw( 
        
             base_size = base_size, 
        
             base_family = base_family, 
        
             base_line_size = base_line_size, 
        
             base_rect_size = base_rect_size 
        
           ) %+replace% 
        
             theme( 
        
               axis.ticks      = element_blank(), 
        
               legend.background = element_blank(), 
        
               legend.key        = element_blank(), 
        
               panel.background  = element_blank(), 
        
               panel.border      = element_blank(), 
        
               strip.background  = element_blank(), 
        
               plot.background   = element_blank(), 
        
               complete = TRUE 
        
             )

I would like to keep the default as TRUE because it's a better choice for base themes and yet nobody will know to set it to TRUE.

I also think modify_current should default to FALSE.

I don't have a strong opinion on this, but I played through my head a couple of scenarios of how I would use this functionality and I found it unlikely that I'd ever want to register a new theme element but not also add it to the currently active theme. Either way, I'm happy to set it to FALSE and let people make the decision to explicitly set it to TRUE.

Lastly the reset functionality seems like something that should exist in its own function rather than be part of this..?

Sure, fine by me. Do you have a suggested name? reset_theme_settings()? theme_reset_all()? unregister_theme_elements()?

Basically I think this would be a function solemnly for extension developers. They should not rely on what the current theme is, so modifying that instead of the default (which is fixed now, right?) is bad.

I'm not exactly sure what you mean by "is fixed now". It is still possible to define an individual theme with its own element tree, and it is also possible to make that theme the currently active theme via theme_set() or theme_update(). It's just not something we'd necessarily encourage people to do. This is a design decision suggested by @hadley early on in the process:
#2741 (review)

Are you suggesting that we undo that decision and only allow changes to the element tree via register_theme_elements()? I could go either way on this, but I will say that the ability to modify the element tree for individual themes has advantages, in particular when developing such features.

clauswilke · 2019-12-22T20:47:48Z

@thomasp85 To summarize how I'll move forward:

Delete warning for missing theme elements in theme_set(), theme_update(), etc.
Make the reset to defaults a separate function, called reset_theme_settings().
Delete the modify_current argument.
Remove the element tree from the theme itself.
(more a reminder to myself) Add a function that returns the current element tree. I'm thinking about naming it get_element_tree().

clauswilke · 2019-12-25T02:24:28Z

I think I've addressed all the points. Here is a reprex demonstrating the current API.

library(ggplot2)

# define a new coord that includes a panel annotation
coord_annotate <- function(label = "panel annotation") {
  ggproto(NULL, CoordCartesian,
          limits = list(x = NULL, y = NULL),
          expand = TRUE,
          default = FALSE,
          clip = "on",
          render_fg = function(panel_params, theme) {
            element_render(theme, "panel.annotation", label = label)
          }
  )
}

# change default theme
theme_set(theme_bw())

# register a new theme element `panel.annotation`
register_theme_elements(
  panel.annotation = element_text(color = "blue", hjust = 0.95, vjust = 0.05),
  element_tree = list(panel.annotation = el_def("element_text", "text"))
)

df <- data.frame(x = 1:3, y = 1:3)
ggplot(df, aes(x, y)) +
  geom_point() +
  coord_annotate("annotation in blue")

reset_theme_settings(reset_current = FALSE)

ggplot(df, aes(x, y)) +
  geom_point() +
  coord_annotate("annotation in blue")
#> Theme element `panel.annotation` missing

# no more warnings when themes with missing elements are set via theme_set()
theme_set(theme())

# ggplot recovers and pulls missing elements from default
# (note we had previously set the current theme to `theme_bw()`)
ggplot(df, aes(x, y)) +
  geom_point()

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

thomasp85

LGTM

Add functions specifically for registering of new theme elements. Fixes

76031c5

tidyverse#3677

clauswilke mentioned this pull request Dec 18, 2019

Add default theme separately from currently active theme #3677

Closed

clauswilke requested a review from thomasp85 December 18, 2019 22:34

clauswilke added this to the ggplot2 3.3.0 milestone Dec 18, 2019

clauswilke mentioned this pull request Dec 25, 2019

Enable User Defined Theme Elements #2540

Closed

clauswilke added 3 commits December 24, 2019 19:10

remove element tree from theme.

9b8af9e

minor fixes

7541d67

add one more regression test

6f9207c

thomasp85 approved these changes Jan 3, 2020

View reviewed changes

clauswilke merged commit 47a0618 into tidyverse:master Jan 3, 2020

clauswilke deleted the issue-3677-default-theme branch January 3, 2020 16:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add function for registering of new theme elements. #3678

Add function for registering of new theme elements. #3678

Uh oh!

clauswilke commented Dec 18, 2019

Uh oh!

clauswilke commented Dec 18, 2019

Uh oh!

clauswilke commented Dec 18, 2019

Uh oh!

clauswilke commented Dec 18, 2019

Uh oh!

thomasp85 commented Dec 19, 2019

Uh oh!

clauswilke commented Dec 19, 2019 •

edited

Loading

Uh oh!

thomasp85 commented Dec 20, 2019

Uh oh!

clauswilke commented Dec 20, 2019

Uh oh!

clauswilke commented Dec 22, 2019

Uh oh!

clauswilke commented Dec 25, 2019

Uh oh!

thomasp85 left a comment

Uh oh!

Uh oh!

Add function for registering of new theme elements. #3678

Add function for registering of new theme elements. #3678

Uh oh!

Conversation

clauswilke commented Dec 18, 2019

Uh oh!

clauswilke commented Dec 18, 2019

Uh oh!

clauswilke commented Dec 18, 2019

Uh oh!

clauswilke commented Dec 18, 2019

Uh oh!

thomasp85 commented Dec 19, 2019

Uh oh!

clauswilke commented Dec 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thomasp85 commented Dec 20, 2019

Uh oh!

clauswilke commented Dec 20, 2019

Uh oh!

clauswilke commented Dec 22, 2019

Uh oh!

clauswilke commented Dec 25, 2019

Uh oh!

thomasp85 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

clauswilke commented Dec 19, 2019 •

edited

Loading