new handling of custom geom defaults

[matthewstern]

This PR is set to merge into #67. The intention is to isolate the setting and resetting of geom defaults into their own functions that can be called not only in finalize_plot but also by the user as to have our custom geom defaults impact ggplots pre-the finalize stage.

Two new exported functions are created, currently in cmap_ggplot_defaults.R:

• fetch_cmap_gg_defaults returns a named list of all the geom defaults we would seek to overwrite (for replacement later)
• set_cmap_gg_defaults sets geom defaults for all the geoms we plan to overwrite. This function will either overwrite with our defaults, or will take the list outputted by fetch to reset.

The general workflow (which is now implemented in finalize_plot but can also be implemented by the user) is:

g <- some ggplot

g   # display g with standard settings

old <- fetch_cmap_gg_defaults()
set_cmap_gg_defaults()

g   # display g with our preferred settings

set_cmap_gg_defaults(old)

g   # display g with standard settings

To do this, I have created a tibble in cmapplot_constants that contains the geoms to overwrite and their preferred values. Here's where things get tricky. Because the preferred values are calculated based on other other cmapplot values and functions (e.g. ggplot_size_conversion and values from cmapplot_globals such as font families and faces) I needed to add this via .onLoad. This works pretty well, but it's not perfect.

It seems to take the size attributes OK, but it leaves fonts as Arial. In other words, even though fonts are set before the tibble is created, the values taken from cmapplot_globals are still the old ones, maybe because the version of cmapplot_globals available within the .onLoad environment is still the old one with all fonts set to Arial?

This is where I could use some help, probably from @nmpeterson. Am I thinking about this the right way? How would I coax .onLoad to fetch the updated fonts? Is there somewhere besides for .onLoad I should put this?

Here's the test code that I've been using:

g <- ggplot(mtcars, aes(x = mpg, y = disp, label = rownames(mtcars))) +
  geom_line() +
  geom_text() +
  geom_text_lastonly() +
  theme_cmap() +
  coord_cartesian(clip = "off")

# without special defaults
g

# with special defaults
finalize_plot(g)

# without special defaults
g

# with special defaults
old <- fetch_cmap_gg_defaults()
set_cmap_gg_defaults()
g

# without special defaults
set_cmap_gg_defaults(old)
g

If we can get this to work, I still need to:

• add details for all geoms that need to be overwritten (I think just GeomRecessionsText) test GeomRecessionsText - font family seems to not be sticking.
• write documentation for the new functions
• add an arg to finalize_plot to ignore the set and reset of geom defaults if the user wants
• although we have long thought that updating geom_defaults would not apply to finalize_plot's return of an object (and our documentation says this is the case), confirm that this is actually wrong because what finalize returns is a grid object, not a ggobject. If correct, move the geom reset higher in the code and edit the documentation.

[matthewstern]

OK, I think I addressed the issue by building the values right in the new fetch and set functions directly. I think it's working, although the above to do list still applies.

[matthewstern]

This is ready to be tested and maybe merged? My test code is here. I still need to write documentation for this. I can do that documentation in this branch, or back in the label_fonts branch as we continue to work on a consistent approach to font sizing.

[matthewstern]

@nmpeterson thanks for your tweaks. I added documentation and added the purrr imports. check() now passes. However, the additional import of purrr causes the package to throw the following warnings on load:

Warning messages:
1: replacing previous import ‘purrr::list_along’ by ‘rlang::list_along’ when loading ‘cmapplot’
2: replacing previous import ‘purrr::invoke’ by ‘rlang::invoke’ when loading ‘cmapplot’
3: replacing previous import ‘purrr::flatten_raw’ by ‘rlang::flatten_raw’ when loading ‘cmapplot’
4: replacing previous import ‘purrr::modify’ by ‘rlang::modify’ when loading ‘cmapplot’
5: replacing previous import ‘purrr::as_function’ by ‘rlang::as_function’ when loading ‘cmapplot’
6: replacing previous import ‘purrr::flatten_dbl’ by ‘rlang::flatten_dbl’ when loading ‘cmapplot’
7: replacing previous import ‘purrr::flatten_lgl’ by ‘rlang::flatten_lgl’ when loading ‘cmapplot’
8: replacing previous import ‘purrr::flatten_int’ by ‘rlang::flatten_int’ when loading ‘cmapplot’
9: replacing previous import ‘purrr::%@%’ by ‘rlang::%@%’ when loading ‘cmapplot’
10: replacing previous import ‘purrr::flatten_chr’ by ‘rlang::flatten_chr’ when loading ‘cmapplot’
11: replacing previous import ‘purrr::splice’ by ‘rlang::splice’ when loading ‘cmapplot’
12: replacing previous import ‘purrr::flatten’ by ‘rlang::flatten’ when loading ‘cmapplot’
13: replacing previous import ‘purrr::prepend’ by ‘rlang::prepend’ when loading ‘cmapplot’
14: replacing previous import ‘purrr::discard’ by ‘scales::discard’ when loading ‘cmapplot’

I'm not sure how to handle this... all these packages get loaded in the tidyverse without issue...

[nmpeterson]

I don't get that particular error with check(). However, I do get a different one:

> checking examples ... ERROR
  Running examples in ‘cmapplot-Ex.R’ failed
  The error most likely occurred in:
  
  > base::assign(".ptime", proc.time(), pos = "CheckExEnv")
  > ### Name: cmap_geom_defaults
  > ### Title: Fetch and set aesthetic defaults
  > ### Aliases: cmap_geom_defaults fetch_cmap_geom_defaults
  > ###   set_cmap_geom_defaults
  > 
  > ### ** Examples
  > 
  > g <- ggplot(mtcars, aes(x = mpg, y = disp, label = rownames(mtcars))) +
  +   geom_line() +
  +   geom_text() +
  +   geom_text_lastonly() +
  +   theme_minimal() +
  +   coord_cartesian(clip = "off")
  > 
  > # print normally
  > g
  > 
  > # before overwriting defaults, save the current ones
  > old <- fetch_cmap_geom_defaults()
  > 
  > # overwite defaults
  > set_cmap_geom_defaults()
  Geom defaults overridden in the current session for: Line, Text, TextLast, RecessionsText
  > g
  Warning in grid.Call.graphics(C_text, as.graphicsAnnot(x$label), x$x, x$y,  :
    font family 'Arial' not found in PostScript font database
  Warning in grid.Call.graphics(C_text, as.graphicsAnnot(x$label), x$x, x$y,  :
    font family 'Arial' not found in PostScript font database
  Warning in grid.Call.graphics(C_text, as.graphicsAnnot(x$label), x$x, x$y,  :
    font family 'Arial' not found in PostScript font database
  Warning in grid.Call.graphics(C_text, as.graphicsAnnot(x$label), x$x, x$y,  :
    font family 'Arial' not found in PostScript font database
  Warning in grid.Call.graphics(C_text, as.graphicsAnnot(x$label), x$x, x$y,  :
    font family 'Arial' not found in PostScript font database
  Error in grid.Call.graphics(C_text, as.graphicsAnnot(x$label), x$x, x$y,  : 
    invalid font type
  Calls: <Anonymous> ... drawDetails -> drawDetails.text -> grid.Call.graphics
  Execution halted

[matthewstern]

Damn. We really can't run any examples that rely on font changes and expect check to pass, can we? Not sure why this didn't happen to me last time around, but it does now. I guess we just need to wrap that whole example section in /dontrun{} tags? It's frustrating but not the end of the world.

The Purrr error has disappeared for me, too. Must have just been a quirk of the current session I was in.

[nmpeterson]

I think I'm fine with this. The one thing I don't like is that, with these functions exposed, if a user were to run set_cmap_geom_defaults() without first saving the current defaults via fetch_cmap_geom_defaults(), there is no way to revert the change without reloading cmapplot/ggplot2. Or am I missing something?

[matthewstern]

I think I'm fine with this. The one thing I don't like is that, with these functions exposed, if a user were to run set_cmap_geom_defaults() without first saving the current defaults via fetch_cmap_geom_defaults(), there is no way to revert the change without reloading cmapplot/ggplot2. Or am I missing something?

@nmpeterson that's correct. Because the risk of screwing it up is not actually that great (restarting the session/reloading the packages), this did not feel like a fatal flaw. That being said, a safer option would certainly be preferable. I am open to other implementations (e.g. automatic caching of the initial defaults somewhere in a global environment) but I do not have the knowledge of R environments to easily implement this. Not sure if you or anyone else want to take a stab at that?

[nmpeterson]

Any reason not to move the code for fetch_cmap_geom_defaults() into cmapplot.R and assign the results to a new item in cmapplot_globals?

Then set_cmap_geom_defaults() could be a non-exported function with two exported wrapper functions, apply_cmap_geom_defaults() and unapply_cmap_geom_defaults(). The first would act as set_cmap_geom_defaults() currently does with no values argument. The second would run set_cmap_geom_defaults(cmapplot_globals$name_of_saved_defaults_list).

[matthewstern]

That seems reasonable. I'll give it a try.

[nmpeterson]

The CMAP defaults could also be saved there instead of in the function definition, in case anybody wanted to tweak them.

[matthewstern]

OK, I have been able to mostly implement the framework recommended by @nmpeterson. See the above commit 79f2202 description, which documents the changes.

The one thing I was not able to do is incorporate the CMAP defaults into cmapplot_globals. This goes back to the challenge identified in the opening post of this PR:

To do this, I have created a tibble in cmapplot_constants that contains the geoms to overwrite and their preferred values. Here's where things get tricky. Because the preferred values are calculated based on other other cmapplot values and functions (e.g. ggplot_size_conversion and values from cmapplot_globals such as font families and faces) I needed to add this via .onLoad. This works pretty well, but it's not perfect.

It seems to take the size attributes OK, but it leaves fonts as Arial. In other words, even though fonts are set before the tibble is created, the values taken from cmapplot_globals are still the old ones, maybe because the version of cmapplot_globals available within the .onLoad environment is still the old one with all fonts set to Arial?

@nmpeterson want to give this a try? I agree it would be superior to not bury these defaults in apply_cmap_geom_defaults. If not, the cmapplot_globals$geom_changes tibble should be rewritten as a list or vector, as it now only has a single column.

EDIT:
As of commit bd5a980 below, I have addressed the problem by "loading in" the default aes to cmapplot_globals after fonts are set via a new function init_cmap_default_aes. This is still inconvenient from a pkg maintainer perspective (it creates an additional backend location in which theming constants are stored) but from the user perspective, it does keep all values available in cmapplot_globals.

It's worth noting that I think the right way to handle this is to put those expressions directly into cmapplot_globals but to delay the evaluation of them. I believe this is how ggplot gets around allowing a saved plot to change appearance when a default aesthetic is changed: note that class(GeomLine$default_aes) is "uneval". See also delayedassign and eval. But, I'm not sure I have the energy to care.

[nmpeterson]

Do you sleep??

[nmpeterson]

I've tested all of the latest changes and everything seems to be in order. I've added a few comments that don't impact functionality (see above).

[matthewstern]

I've addressed @nmpeterson's comments. Waiting for review from @dlcomeaux before merging into label_fonts.

In other news, sometimes when I run check(), I get a note:

   Non-standard file/directory found at top level:
     'Rplots.pdf'

But I don't see that PDF anywhere in the directory. Curious if others see this?

[dlcomeaux]

This seems to work as expected. Matt, you beat me to it on changing that example that Noel suggested - otherwise, I left one small comment about line widths. Great work.

[dlcomeaux]

I've addressed @nmpeterson's comments. Waiting for review from @dlcomeaux before merging into label_fonts.

In other news, sometimes when I run check(), I get a note:

   Non-standard file/directory found at top level:
     'Rplots.pdf'

But I don't see that PDF anywhere in the directory. Curious if others see this?

I have had that note before. It doesn't happen every time - I have noticed it when I was doing testing on PDFs. Doesn't seem to cause any issues, but I have not found a way to reliably get rid of it (aside from sometimes just restarting the session).