Give custom types control over plot machinery via type_hints - #637
Merged
Conversation
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.
Collaborator
|
Overall, I think this is a very good path to go on. I agree we should remove the type specific logic from the main functions and put as much as we can in the type-specific functions. The one thing that worries me a bit is not specific to the new flags you just adopted, but relates to our naming conventions. I feel that we haven't been super strict in naming variables internally, and that can make things harder to read and maintain than they should be. For example, This is of course a different issue, but I wanted to raise it here because this PR will likely lead to the creation of several new internal variables. |
|
@grantmcdermott Let me know if you are looking for any kind of feedback from me. Also, not related exclusively to this issue, I've recently made public the project I (and @rhirk) have been working on that's making use of tinyplot: https://github.com/livingingroups/trackframe The custom types are here:
They don't suffer too badly from this issue, but @rhirk had previously built the |
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
marked this pull request as ready for review
July 29, 2026 23:47
grantmcdermott
changed the title
type_hints
There was a problem hiding this comment.
Pull request overview
This PR introduces a new settings$type_hints mechanism so plot types (including custom types) can declare semantic “behavior” flags that the generic plotting pipeline (margins, facet axes, limits, legend keys) can respect, removing brittle type == "..." branching from core machinery. It also adds explicit facet control for dropping redundant interior axes via facet.args = list(axes = "outer").
Changes:
- Added
type_hintsplumbing through the core pipeline (facets, limits, legends) and updated key types (spineplot/ridge/hexbin) to declare the relevant hints. - Implemented
facet.args$axes("all" | "outer" | "none") and centralized axis-drawing logic viadraw_facet_axis(). - Updated documentation (Types vignette, NEWS, Rd files) and refreshed snapshot outputs for the axis/legend behavior changes.
Reviewed changes
Copilot reviewed 12 out of 20 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| vignettes/types.qmd | Expands custom type documentation and documents type_hints (with examples + supported hint list). |
| R/zzz.R | Adds type_hints to the internal settings key list. |
| R/type_spineplot.R | Declares spineplot type_hints and aligns self-drawn axes with facet axis rules. |
| R/type_ridge.R | Declares ridge type_hints and gates self-drawn axes per facet axis rules. |
| R/type_hexbin.R | Declares legend swatch behavior via type_hints for hexbin. |
| R/tinyplot.R | Introduces type_hints slot in settings, routes it through facet/legend machinery, and refactors empty-plot detection. |
| R/lim.R | Switches proportional-limit behavior from type == "spineplot" to type_hints$has_proportional_lim. |
| R/legend.R | Switches legend margin/swatch special-cases to type_hints and threads type_hints through legend env. |
| R/legend_multi.R | Threads type_hints into multi-legend preparation/drawing. |
| R/facet.R | Adds facet.args$axes validation and centralizes per-facet axis decisions (draw_facet_axis()) + related margin logic. |
| NEWS.md | Announces type_hints and new facet axes control; notes related bug fixes. |
| man/type_ridge.Rd | Syncs docs/examples with updated ridge roxygen. |
| man/tinyplot.Rd | Documents facet.args$axes behavior. |
| man/facet.Rd | Updates signature to include type_hints. |
| man/draw_legend.Rd | Documents new type_hints argument. |
| man/build_legend_env.Rd | Updates signature to include type_hints. |
| inst/tinytest/_tinysnapshot/spineplot_facet_by.svg | Snapshot refresh for spineplot facet axis changes. |
| inst/tinytest/_tinysnapshot/spineplot_facet_by_fancy.svg | Snapshot refresh for spineplot facet axis changes (fancy theme). |
| inst/tinytest/_tinysnapshot/ridge_gradient_facet_theme_ridge2.svg | Snapshot refresh for ridge facet axis changes. |
| altdoc/pkgdown.yml | Updates site build metadata (altdoc/pandoc versions + last built time). |
Files not reviewed (5)
- man/build_legend_env.Rd: Generated file
- man/draw_legend.Rd: Generated file
- man/facet.Rd: Generated file
- man/tinyplot.Rd: Generated file
- man/type_ridge.Rd: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This was referenced Jul 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
NOTE: Edited to reflect updated content.
Fixes #543
Fixes #661 (incidentally)
Towards #660
Moves
type == "..."branching/exceptions out of the main code files (tinyplot.R,facet.R,legend.R,lim.R) and into dedicated flags that the individual type constructors can access and declare themselves. The core idea is that any bespoke behaviour should be controllable from the type constuctors themselves, rather than relying on brittle, hard-coded logic in the main pipeline. We had already done this for most types, but there were (are) some vestigial cases left in the code; mostly related to "tricky" types like spineplots and ridge plots.The main mechanism is a new
settings$type_hintsslot with a set of known flags ("hints") that users can access and set as part of the type'sdata_*()function. Right now, there are several known hints, which are documentedinternally (here), as well as in the updated Types vignette.
Note:
type_hintscould be considered a sister slot totype_info. They do similar(ish) things---and are both escape hatches for custom behaviour---but are ultimately targeting separate issues. As I wrote in the Types vignette: "type_hintsspecifies behaviour you want the plot machinery to respect, whiletype_infois used for passing bespoke rendering payload fromdata_<type>()through todraw_<type>()(e.g., coordinates, precomputed breaks, and so on)."Again, both mechanisms are accessible from the individual
type_*constructors, so this effectively makes them available to custom types too. So, this should addess @katrinabrock's core ask to have "the same toolbox that you as maintainers have". Hopefully, the updated Types vignette documentation also makes clear how to use and implement them.Collateral benefits and bug fixes
While not the original target of this PR, I ended up implementing some other FRs and squashing a few other bugs. The most important of which are:
facet.args = list(axes "outer")top-level argument for suppressing the drawing of redundant facet axes. (Remove redundant x/y-axis in facets #661)Out of scope / future PRs
nameimpacts behavior, consider alternatives to hardcoded types intinyplot::tinyplot#543 point 4), e.g.tinytype_register()mirroringtinytheme_register().tinyplot.formula()(tinyplot.RL1847-49) for density/histogram/barplot labeling that I haven't addressed here. It's harder to migrate because it runs beforesanitize_type()normalises thetypeinput, so it has to handle both the convenience string and the constructor object, and notype_hintsexist yet for it to read.facet.args(axes = "outer")argument works in tandem with the existingframe.plot = FALSElogic; i.e., they both suppress the inner facet axes. But we should arguably drop the old behaviour as less principled; would be a breaking change, though.