grantmcdermott · GitHub

added 2 commits

June 19, 2026 15:16
Replace six scattered hardcoded `type == "spineplot"` checks with semantic
flags the type declares for itself. data_spineplot() now writes a
type_axes_hints list (self_axes, rhs_axis, proportional_lim,
legend_fill_from_col, draw_empty_facet) to settings via the same env2env
write-back pattern used by group_offsets. The main pipeline reads these
flags instead of matching the type string.
Sites converted:
- R/lim.R: proportional_lim (suppress axis-break range expansion)
- R/tinyplot.R: self_axes (dynmar tick-row margin, #635) + draw_empty_facet
- R/facet.R: rhs_axis (omar[4] RHS-axis margin, x2; drops the FIXMEs)
- R/legend.R: rhs_axis (outer-legend bump) + legend_fill_from_col (pt.bg)
- R/legend_multi.R: thread type_axes_hints through the multi-legend path
type_axes_hints is threaded through the exported draw_legend() (new NULL
default + @PARAM) -> build_legend_env() -> stored on legend_env. Pure
refactor: all five flags are TRUE for spineplot, NULL elsewhere, so every
site evaluates identically. Verified byte-identical renders across simple,
flip, facet, by-group, outer-legend, and xlab/ylab=NA (#635) cases. Adds a
spineplot_legend_outer_right snapshot to cover the RHS legend-margin path.
The default legend position is already "right!" (outer), so the existing
spineplot_facet_by test exercises the outer_side + rhs_axis legend-margin
branch. A dedicated outer-right test added no new coverage.
Follow-up to the spineplot pilot, addressing three gaps in how types
declare behaviour to the generic pipeline.
Remove the last generic-pipeline read of `type_info`. The grouped
(`y_by`) legend-fill branch in prepare_legend() was guarded by a
hardcoded `type == "spineplot"` check, giving spineplot two parallel
legend-fill mechanisms. It now keys off the `legend_fills_from_col`
hint. The fill resolution itself stays put: `col` is only the resolved
group palette after by_aesthetics(), so it cannot move into
data_spineplot().
Rename `type_axes_hints` to `type_hints`, since several flags
(legend fill, empty-group drawing) are not axes concerns. Flags are
renamed to property-style predicates for a consistent convention:
  self_axes            -> draws_own_axes
  rhs_axis             -> has_rhs_axis
  proportional_lim     -> has_proportional_lim
  legend_fill_from_col -> legend_fills_from_col
Replace the empty-plot type list with a data-driven check. Rather than
asking which type this is, ask whether there is anything to draw:
  has_data = length(ix) > 0 || (length(ixmin) > 0 && length(ixmax) > 0)
Requiring both xmin and xmax isolates the types that carry horizontal
geometry there (rect, segments, histogram, spineplot) from those needing
`x` alongside a y-extent (errorbar, pointrange, ribbon). This drops the
`draw_empty_facet` flag and the hardcoded list together.
That last change also fixes a latent bug: the old list drew a group
whenever the type matched, even with nothing to draw. On grouped and
faceted spineplots this redrew each facet's axes once per empty group
(40 spine_axis() calls where 10 suffice), overplotting the self-drawn
labels so they rendered too heavy. Two snapshots are updated to the
corrected single-draw output.
Faceted plots suppress interior axes when frameless, since without a
frame those axes float into the neighbouring panel and collide with its
labels. The generic pipeline already did this via oxaxis/oyaxis, but
three paths bypassed it.
Introduce a single predicate, draw_facet_axis(), as the one place that
answers "should this facet draw this axis?", and route every decision
site through it. Previously the rule was inlined in facet.R and the
self-drawing types reimplemented (or omitted) it.
- spineplot drew its category axis in every facet. It sets axes = FALSE
  in its data fn, so the generic block is skipped entirely and all axis
  drawing happens in draw_spineplot(). Note the gate reads frame.plot
  from type_info, since data_spineplot() forces the settings copy FALSE
  while preserving the user's value.
- ridge likewise drew its y-axis category labels unconditionally, plus
  the "ridge2" x-axis line. draw_ridge() gains named ifacet and
  facet_window_args params; they were already passed via `...`.
- flipped plots gated args_x on oxaxis and args_y on oyaxis, i.e. by
  argument name rather than by resolved side. Since flip swaps
  xside/yside, a flipped frameless facet grid both duplicated the
  category axis and omitted every interior x-axis -- so panels were read
  against a neighbour's scale. Gating on the actual side fixes both.
Free facets keep per-facet axes, as each panel has its own scale.
Also adds the `axes` facet argument from #661: "all", "outer", or
"none". This was previously only reachable as a side effect of
frame.plot = FALSE, so `facet.args = list(axes = "outer")` now drops
interior axes while keeping frames. Unset preserves existing behaviour.
An explicit "none" overrides the free-facet exemption.
Verified byte-identical renders for framed, free, single-facet,
facet_grid, nrow=1 and flipped-framed cases; only the frameless cases
change.
Faceted spineplots overlapped their category labels with the
neighbouring panel under framed themes (e.g. tinytheme("clean")).
The tick-label width is measured once and added to both omar and fmar,
but the nmar/noma split then hands it to the *outer* margin:
  omar[2] = 6.37   (base + ~2.85 label width)
  noma[2] = 4.77   <- label space, figure left edge only
  nmar[2] = 1.6    <- every facet's own left gap
That is right when only the leftmost facet draws a y axis. Under a framed
theme every facet draws one, into 1.6 lines when the labels need 2.85, so
they spill leftwards into the neighbour. Keep the fmar bump (rather than
releasing it to the outer margin) whenever an interior facet will draw
that axis, decided by the same draw_facet_axis() predicate that governs
the drawing itself, so margin and axis cannot disagree.
Two supporting details:
- data_spineplot() forces frame.plot = FALSE so the pipeline skips the
  box that draw_spineplot() draws itself. draw_facet_window() therefore
  saw FALSE under *both* framed and frameless themes and subtracted the
  label width in both cases. Surface the user's real choice via a new
  `framed` hint and key the margin logic off that.
- The subtraction condition keeps its original frameless/free guards, so
  this can only retain space that was previously released, never release
  more.
Costs some panel width, since the labels now occupy each facet's margin
rather than one shared outer margin. Verified byte-identical renders for
points, boxplot, ridge and frameless spineplot; only framed faceted
spineplots change.
The pt.bg block in build_legend_args() still matched two types by name,
sitting immediately beside the already-migrated legend_fills_from_col
check. Convert both so the generic legend code no longer needs to know
which types exist.
Splitting them out revealed that legend_fills_from_col was conflating two
independent behaviours: deriving the swatch fill from `col`, and drawing
the swatch border in par("fg"). Spineplot wants both -- its tiles abut
with no gap, so a foreground border reads better -- but hexbin wants only
the fill. Hence three hints rather than two:
  legend_fills_from_col          spineplot, hexbin
  legend_fills_from_seq_palette  ridge (a lighter step of the ramp)
  legend_border_fg               spineplot
The border check now runs independently of the fill branch, which is what
lets hexbin reuse the fill behaviour without inheriting the border.
Pure refactor: verified byte-identical legend renders across spineplot
(y_by, y_by + lighten, by, plain), ridge (by, gradient, plain), hexbin
(discrete, count) and area-type controls (barplot, boxplot, rect). Also
confirmed by trace that each type declares exactly the expected hints and
that non-migrated types declare none.
The "which labels will the y axis draw?" cascade was duplicated three
times: once in tinyplot.default()'s dynmar block and twice in
draw_facet_window() (faceted and non-faceted branches). Each copy carried
its own `type == "ridge"` and `type == "boxplot" && flip` checks, making
them the largest remaining source of hardcoded type strings in the
pipeline.
Extract just the label-selection part as y_axis_labels(), returning NULL
when the axis is numeric so each caller keeps its own fallback.
Deliberately narrow: only the three genuinely identical branches move. The
surrounding whtsbp arithmetic is left alone because the three copies have
diverged in ways that are not obviously intentional -- differing las
source (get_tpar vs par), strwidth cex (.cex_axis vs par("cex.axis")),
guards on the computed bump (is.finite vs > 0 vs none), the
degenerate-range catch (present in two of three), and cex_fct_adj scaling.
Harmonising those would move margins on many snapshots and belongs in its
own change; see SCRATCH/facet-margin-slack.md.
The faceted copy also has a free-facet branch that measures every facet
and keeps the widest tick set. That stays inline, now nested under the
NULL fallback so branch precedence is unchanged.
Pure refactor. Verified equivalence of old and new selection over a truth
table including the empty-but-non-NULL ylabs edge case, plus byte-identical
renders across ridge (plain/faceted/ridge theme), flipped and unflipped
boxplot, spineplot, numeric axes, free facets, las = 2 and a degenerate
(zero-range) y axis.
 d8b8809  used NULL as y_axis_labels()'s "not a categorical axis" signal,
but `levels(y)` is itself NULL for a ridge plot over a *numeric* y (e.g.
`am ~ mpg`). The caller then fell through and substituted axisTicks(),
measuring ~0.028 figure units of tick-label width where the original
cascade measured max(strwidth(NULL)) = 0, so the y labels were pushed away
from the axis title.
Wrap the result in a one-element list, so an empty label set stays
distinguishable from "this type doesn't put categories on the y axis".
Caught by the ridge_gradient_facet* and spineplot_facet_by_fancy snapshots.
My earlier equivalence check missed it because every ridge case in the
truth table used a factor y; the numeric-y case is now covered there too.
`axes = "t"` (ticks) wrongly dropped interior facet axis labels, e.g.
  tinyplot(Survived ~ Sex | Class, facet = "by", data = ttnc,
           type = type_spineplot(weights = ttnc$Freq), axes = "t", lwd = 0)
draw_facet_axis() keys the "outer facets only" rule off framing, on the
grounds that an unframed interior axis floats free and collides with the
neighbouring panel. But `frame.plot` is only a proxy for that, and
sanitize_axes() derives it as all(c(xaxt, yaxt) %in% c("s", "a")) -- so
"t" reports FALSE despite drawing tick marks that anchor the labels
perfectly well. Only "l" (labels) and "n" (none) are genuinely bare.
Add facet_axes_framed() as the single place that answers "are interior
tick labels anchored?", and route all three framing derivations through it
(the generic pipeline plus the two self-drawing types, which each computed
this independently and so shared the bug).
Verified the reported call now renders byte-identical to  99b34a3 , i.e.
before the outer-facet rule existed. Call counts confirm the intended
matrix is preserved: per-facet for framed and for axes="t"; outer-only for
axes="labels" and frameless themes (#660); and facet.args = list(axes =
"outer") still forces outer-only under axes="t". No change to the earlier
regression set (8/8 identical).
The deeper fix is to drop `frame.plot` from this decision in favour of an
explicit "would inner axes float?" flag, but that is a wider behaviour
change; noted in SCRATCH/facet-margin-slack.md.

@grantmcdermott

@grantmcdermott grantmcdermott changed the title Centralize type-specific logic via self-declared flags (re: #543) Give custom types control over plot machinery via type_hints

Jul 29, 2026

@grantmcdermott

@grantmcdermott

@grantmcdermott

Closed

This was referenced

Jul 31, 2026

Merged

Merged

Read the original on github.com ↗