Rework dynamic theme logic

[vincentarelbundock]

GM edits:

Closes #303
Closes #479
Closes #573
Closes #574

---

This change fixes dynamic margins when annotation text is missing (NA, NULL, or empty) and when labels contain multiple lines.

pkgload::load_all()
tinytheme("clean")

set.seed(1)
x <- 1:20
y <- x + rnorm(length(x), sd = 2)

op <- par(mfrow = c(2, 2))
on.exit(par(op), add = TRUE)
on.exit(tinytheme(), add = TRUE)

tinyplot(x, y, main = "xlab = NA", xlab = NA, ylab = "Y")
tinyplot(x, y, main = "xlab present", xlab = "X", ylab = "Y")
tinyplot(x, y,
  main = "Multi-line labels",
  xlab = "X line 1\nX line 2", ylab = "Y line 1\nY line 2")
tinyplot(x, y, main = "Multi-line\nmain title", xlab = "X", ylab = "Y")

Internal changes:

• Added text_line_count() to normalize line counting for labels/subtitles (treat missing text as 0 lines).
• Passed main/sub/xlab/ylab into facet margin logic and adjusted margins by line count (shrink for missing text, expand for multi-line text).
• Used the same line-counting logic for sub in legend layout, so sub = NA no longer reserves extra bottom space.

Testing note:

• Added/updated margin snapshots for missing and multi-line labels (including a 2x2 case).
• Regenerated ridge snapshots because previous renders showed extra empty space at the top; the updated margin logic changed panel placement.

[grantmcdermott]

Super, thanks for taking this on. I kept meaning to, but couldn't find the time...

[vincentarelbundock]

Frankly, I'm not sure I can wrap my head entirely around the margins part of the code. But this seems to be working for the cases I investigated.

So I'd say this is OK for review.

[grantmcdermott]

Left a few quick comments while I had a sec.

I also have a slightly more involved refactor in mind for the core dynamic theme margin adjustment logic. tl;dr Right now, we do things a bit back to front; we assign title space up front and then remove it if it is absent, or increase if it has multiple lines per your PR here. I think we could simplify things for these dnymaic themes, by setting margins that assume no titles a priori and then make (multline) space for them if these titles are present. I'll take a crack at this later today, hopefully.

[grantmcdermott]

Thanks for this. I'm completely snowed ATM and didn't get around to implementing the full margin-logic refactor that I mentioned above, i.e.

Right now, we do things a bit back to front; we assign title space up front and then remove it if it is absent, or increase if it has multiple lines per your PR here. I think we could simplify things for these dnymaic themes, by setting margins that assume no titles a priori and then make (multline) space for them if these titles are present.

But... I did at least implement a "dynamic" theme, which is something I've want to do for a while. As the name suggests, this is just a "dynamic" version of the base theme and, I think, a more intuitive foundation for the other dynamic themes to build off. (As opposed to our current logic, where the dynamic themes all build off of the "clean" theme). It's a minor change, but just flagging in case something goes wrong and we need to revert once I push to your branch here.

[vincentarelbundock]

I'm not in a rush at all, and TBH I don't understand this code super well. Probably best to wait.

[grantmcdermott]

Hey bud, I'm going to submit a 0.6.1 release to CRAN shortly, which contains mostly bug fixes.

I'll take another look at this PR afterwards, since I'd like to pair it with #500 as part of some "breaking" aesthetic changes for 0.7.0.

[grantmcdermott]

I've co-opted this PR to push through a larger refactoring of the dynamic theme (margins) logic that I've been meaning to implement for a while.

Specifically, I've inverted the dynamic themes' margin (dynmar) logic, so that these themes start with de minus margin that assumes no titles or labels to begin with, and then additively builds up margin space for elements that are actually present (e.g., titles and whatnot). This "inside-out" logic replaces replaces the old "outside-in" logic which worked in the opposite way (i.e, start with some arbitrarily high margin and subtract for missing elements). The latter approach required a bunch of ad hoc guards (e.g., to protect against subtracting too much space off of a fine margin) and guesswork. I've long been convinced that inverting this logic would provide a more principled/safer approach, so I'm glad to have finally done it (with help from Claude). This also made it possible to address some other long-standing dynamic theme issues.

Here's my + Claude's summary of all of the changes:

• Margins adapt to content presence. xlab = NA / missing main / absent sub no longer leave vestigial whitespace; multi-line labels ("\n") are sized correctly. This builds on the work that @vincentarelbundock originally started with this PR, extended to more cases. (See Themes dynamic margins could be smarter about titles #303)
• Title alignment tracks the y-axis. Previously, long y-tick labels (e.g., month names) inflated mar[2] but main/sub stayed stuck at the uninflated plot-region-left. They now anchor to the actual y-axis line, which is more aesthetically pleasing IMO and matches ggplot's behavior. (See Should main and sub positions adjust in response to themes with dynamic LHS margin? #479)
• las is honored when reserving tick-label space. Themes that set las = 1 (e.g., theme_clean) previously had .whtsbp silently skipped because par("las") reads 0 before the theme's before.plot.new hook fires. Now read from .tpars (with pending-hook merge) so the margin calculation sees the theme's intent.
• xlab shifts down for rotated x-tick labels. Under las = 2/3, draw_title now offsets the xlab line position by .whtsbp[1] so it clears the vertical tick-label column.
• Faceted whtsbp is scaled by cex_fct_adj. Faceted plots render per-facet tick labels smaller; the global whtsbp now scales to match so draw_title anchors track the actual facet y-axis.
• "dynamic" tinytheme The new theme = "dynamic" now serves as the basis for all other dynamic tinythemes.

Some internal cleanups:

• sanitize_legend() lifted to a single up-front call in tinyplot.default. Downstream code reads legend_args[["x"]] directly instead of re-parsing the raw legend argument. Removed now-redundant calls in build_legend_args and prepare_legend_multi. Single source of the "right!" default.
• Unreachable dynmar_side() fallbacks removed from draw_facet_window (–38 lines). add = TRUE early-returns, so dynmar_computed is guaranteed non-NULL whenever the dynmar branch runs.
• .tpars in the dynmar block merges pending before.plot.new hook values, making user overrides (e.g., tinytheme(..., las = 2)) visible synchronously to dynmar_side() before plot.new() fires.

MWE

That's a lot of text. Seeing is believing, so here are a couple snapshot diffs that demonstrate some of these ideas:

• https://github.com/grantmcdermott/tinyplot/pull/549/changes#diff-65965c559ef36aa48309a2b1745a5cadc821681e0f2009610a5e4d22d92949cc
• https://github.com/grantmcdermott/tinyplot/pull/549/changes#diff-6587a5f6bfd62b867cbe00244748ff46b5d4bf60dad79917aede7f06f964295c

[grantmcdermott]

@vincentarelbundock @zeileis Please give this a test if you get a chance. The basic idea is that dynamic themes should give better consistency and reduce whitespace where possible. I'd like to merge this before we tackle any of the other open PRs.

[vincentarelbundock]

This seems like an excellent change, congrats @grantmcdermott !

The screenshots look fantastic.

[grantmcdermott]

@zeileis no pressure either way, but LMK if you'd like time to review this, or are happy for me to merge.

[zeileis]

Sorry for joining in so late. I didn't have much time to play around with this but had a look whether this version improves on a problem I noticed with the current CRAN version last week. Namely certain expressions in the axis labels are not rendered correctly:

tinytheme("clean2")
tinyplot(1:10, ylab = expression(Precipitation~"["~mm^{1/2}~"]"))

And it took me a while to figure out that I could only address this with setting mar in the tinytheme() along with dynmar = FALSE as below. Doing it with tpar() was ineffective. E.g., this works:

tinytheme("clean2", mar = c(3.5, 3.7, 0.5, 0.5), dynmar = FALSE)
tinyplot(1:10, ylab = expression(Precipitation~"["~mm^{1/2}~"]"))

Additionally, I noticed that in the development version of this PR, setting dynmar = FALSE by itself appears to fall back to zero margins which I found rather unintuitive:

tinytheme("clean2", dynmar = FALSE)
tinyplot(1:10, ylab = expression(Precipitation~"["~mm^{1/2}~"]"))

The same happens when using tpar() after tinytheme().