Calculate constant descender heights, regardless of label content.

[clauswilke]

This pull request fixes #2288, by calculating descender height in titleGrob() on a fixed string rather than based on the specific label string handed to titleGrob().

This commit causes subtle changes to all ggplot2 output, because it changes the height of axis tick labels and of legend titles as well as plot titles, plot subtitles, etc. I have looked through all visual tests and I didn't see any breakage. I also rebuilt my book with this patch and found no problems. However, the spacing between x-axis tick labels and the x-axis title, or between legend title and legend, is now arguably a little too large, and some small tweaks to themes might be warranted.

In a similar vein, with this fix, the default vgap value calculated here and here is arguably a little too large. A coefficient of 0.4 might work better.

I didn't accept the visual tests because for some subset of tests the svgs on my computer have never agreed with the ones in the repository.

[clauswilke]

I have updated the themes. Visual changes are minimal at the default font size but larger for larger or smaller font sizes, because all fixed lengths have been replaced by relative lengths.

I also fixed a bug with excessive spacing underneath multi-line titles for legends. (The spacing is now based on the font size specified in the theme, and therefore doesn't care about multiple lines.) For guide_colorbar(), this touches some of the same code as pull request #2415. If either one of these pull requests can be integrated with the main codebase then I'm happy to rebase the other one appropriately.

Some examples of figures made with these changes:

# default theme
p <- ggplot(iris, aes(Sepal.Width, Sepal.Length, color = Species, fill = Petal.Width)) + 
  geom_point(pch = 21)
p

# default theme w/ large font
p + theme_gray(18)

# default theme w/ small font
p + theme_gray(7)

# other themes
cowplot::plot_grid(p + theme_bw(), p + theme_minimal(),
                   p + theme_linedraw(), p + theme_dark(), scale = .95)

The same plot with current ggplot2 for comparison. The main difference can be seen in the spacing between the legend title and the legend key for the colorbar legend.

Finally, I've changed the font size of the subtitle and caption, to unify font sizes used. Here is the result.

p + labs(title = "Properties of Iris flowers",
         subtitle = "Iris setosa is distinct from both I. versicolor and I. virginica",
         caption = "Source: Ronald Fisher, 1936")

[clauswilke]

One more example, proper spacing in multi-line legend titles:

ggplot(iris, aes(Sepal.Width, Sepal.Length, color = Species, fill = Petal.Width)) + 
  geom_point(pch = 21) + scale_fill_continuous(name = "The\nPetal\nWidth")

For comparison, in the current ggplot2 the output of the above code is the following:

[hadley]

@clauswilke do you mind having another go with the dev version of vdiffr? (You'll need to run vdiffr::manage_cases() and then confirm that each change is ok)

[hadley]

Please let me know if anything is unclear - the visual tests are a bit more work, but they're really valuable for preventing regressions that are otherwise hard to test for.

[clauswilke]

Yes, I completely agree with you on visual tests. I just installed the latest version of ggplot2 and vdiffr and I managed to successfully pass all current visual tests. So I should be able to fix them for this patch.

[hadley]

Great news!

[clauswilke]

Yay for visual testing. I've found a problem I need to fix. Descender heights are not applied to vertical facet strips. I'll need to do some digging to figure out where the fault lies.

ggplot(iris, aes(Sepal.Length, Sepal.Width)) + geom_point() + facet_grid("dummy strip" ~ Species)

(Notice how the vertical strip is thinner than the horizontal strips are.)

[karawoo]

I've only looked quickly but I think the problem is here:

ggplot2/R/margins.R

Line 75 in d0a2929

text_width <- unit(1, "grobwidth", text_grob) + sin(angle / 180 * pi) * descent

The angle of the "dummy strip" text is 270, making sin(angle / 180 * pi) -1. So the descent details are actually making the strip narrower when the strip width is calculated in ggstrip(). This actually seems like a bug regardless of your changes, @clauswilke:

library("ggplot2")

ggplot(iris, aes(Sepal.Length, Sepal.Width)) +
  geom_point() +
  facet_grid("dummy strip" ~ Species) # vertical strip is still narrower

Created on 2018-04-30 by the reprex package (v0.2.0).

[hadley]

@karawoo good catch!!

[clauswilke]

@karawoo Thanks! I was suspecting this line, but I'm not very good with trigonometry in my head. :-) I'll find a fix.

[clauswilke]

I have fixed the issue (a simple abs() fixed it), updated the vdiffr templates, and merged with the current ggplot2 master.

All tests pass on my system:

> devtools::test()
Loading ggplot2
Loading required package: testthat
Testing ggplot2
✔ | OK F W S | Context
✔ |  1       | Adding plot elements
✔ |  9       | test-aes-calculated.r
✔ |  9       | Aesthetics (grouping) [0.7 s]
✔ | 10       | Aes - setting values [0.6 s]
✔ | 30       | Creating aesthetic mappings [1.5 s]
✔ | 10       | annotate [0.2 s]
✔ | 10       | Plot building [0.2 s]
✔ |  1       | coord_sf [1.1 s]
✔ |  3       | test-coord-.r
✔ |  2       | coord_cartesian [0.3 s]
✔ |  1       | coord_map [0.4 s]
✔ | 19       | coord_polar [3.7 s]
✔ |  7       | coord_train [0.2 s]
✔ |  3       | coord_trans [1.2 s]
✔ |  4       | Data [0.3 s]
✔ | 23       | Empty data [0.7 s]
✔ | 47       | Facetting [1.4 s]
✔ | 21       | Facet Labels [0.9 s]
✔ | 38       | Facetting (layout) [0.4 s]
✔ | 33       | Facetting (mapping) [0.6 s]
✔ | 10       | Facet Strips [2.3 s]
✔ |  2       | Fortify [0.1 s]
✔ | 72       | function-args [0.5 s]
✔ | 13       | geom_boxplot [1.1 s]
✔ | 42       | geom_dotplot [8.5 s]
✔ |  3       | geom_freqpoly
✔ |  3       | geom_hex [0.1 s]
✔ |  5       | geom-hline-vline-abline [1.3 s]
✔ | 10       | geom-path [1.4 s]
✔ |  2       | geom-polygon [0.9 s]
✔ | 10       | geom-raster [2.7 s]
✔ |  1       | geom_ribbon
✔ | 10       | geom_rule [0.3 s]
✔ |  2       | geom-sf [1.3 s]
✔ |  1       | geom_smooth
✔ |  4       | geom_text
✔ |  4       | geom_tile
✔ | 18       | geom_violin [4.2 s]
✔ |  1       | ggproto
✔ | 26       | ggsave [0.6 s]
✔ |  2       | Grid utilites
✔ | 26       | Guides [9.5 s]
✔ | 10       | Labels
✔ |  8       | Layer
✔ | 53       | Munch [0.1 s]
✔ | 60       | plot summary API [0.5 s]
✔ |  2       | position_dodge [0.1 s]
✔ |  7       | position_dodge2 [0.3 s]
✔ |  8       | position_stack [0.2 s]
✔ |  6       | qplot
✔ |  5       | range
✔ | 15       | sanitise_dim
✔ |  8       | scale_date [1.9 s]
✔ | 10       | scale_date [0.3 s]
✔ | 16       | scale_discrete [0.4 s]
✔ |  1       | scale_gradient
✔ | 10       | scale_manual [0.3 s]
✔ | 100       | Scales: breaks and labels [3.4 s]
✔ | 50       | Scales [0.7 s]
✔ |  5       | sec-axis [0.4 s]
✔ | 36       | stat_bin/stat_count [1.3 s]
✔ |  7       | stat_bin2d [0.2 s]
✔ |  5       | stat_density
✔ |  4       | stat_density_2d
✔ |  1       | stat_hex
✔ | 24       | stat_sum [1.6 s]
✔ |  7       | stat_function
✔ |  4       | Stats
✔ | 104       | Themes [6.7 s]
✔ | 20       | Utilities
✔ |  3       | Viridis [0.1 s]
✔ |  1       | zzz

══ Results ═══════════════════════════════════════════════════════════════════════════════════════════════════
Duration: 69.0 s

OK:       1138
Failed:   0
Warnings: 0
Skipped:  0

[hadley]

Thanks!

(For future reference, no need to include the tests results because (a) I trust you and (b) travis will check too)

[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/