Issue #303 margin adjustment for multi-line labels and titles

[vincentarelbundock]

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]

I might be misunderstanding, but why calculate in inches (csi) instead of user coords (cxy)?

Where does 0.75 come from? Is it borrowed from one of the (hard-coded) based plot scaling factors? Or, just eye-balling?

[vincentarelbundock]

just eye-balling. Maybe we can come up with a better rule.

[grantmcdermott]

TBH I'm a little nervous about this. But I'm also not going to be able to look at it again for at least another fortnight (and probably a bit longer), since I've got some major work deadlines to finish before heading out on vacation next week.

Having said that, the plots generally look good to me. If you're comfortable with the current progress and would like to merge as-is---with the idea that we could fix some of these remaining idiosyncrasies in a later PR---then I can get onboard with that. Otherwise, happy to sync up again in March.

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