From 1b827c79fd10258b2dc3d535e82c944e80eda683 Mon Sep 17 00:00:00 2001 From: Grant McDermott Date: Wed, 19 Nov 2025 21:29:00 -0800 Subject: [PATCH 1/9] force legend eval to avoid downstream symbol (promise) issue --- R/tinyplot.R | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/R/tinyplot.R b/R/tinyplot.R index 72fd8b12..7523304b 100644 --- a/R/tinyplot.R +++ b/R/tinyplot.R @@ -641,6 +641,8 @@ tinyplot.default = function( theme = NULL, ...) { + # Force evaluation of legend to avoid downstream symbol (promise) issues + if (!missing(legend)) legend = legend # ## save parameters and calls ----- @@ -909,6 +911,7 @@ tinyplot.default = function( # ## legends ----- # + # browser() # legend labels ncolors = length(col) @@ -1399,6 +1402,7 @@ tinyplot.formula = function( ## placeholder for legend title legend_args = list(x = NULL) + # browser() ## turn facet into a formula if it does not evaluate successfully if (inherits(try(facet, silent = TRUE), "try-error")) { From 45960088d34ed5eb71cb51148c3dd87e85d4c209 Mon Sep 17 00:00:00 2001 From: Grant McDermott Date: Wed, 19 Nov 2025 21:30:36 -0800 Subject: [PATCH 2/9] also need list handler --- R/draw_legend.R | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/R/draw_legend.R b/R/draw_legend.R index b8ddee0e..a5182fbb 100644 --- a/R/draw_legend.R +++ b/R/draw_legend.R @@ -701,6 +701,9 @@ sanitize_legend = function(legend, legend_args) { legend_args[["x"]] = "right!" } else if (is.character(legend)) { legend_args = utils::modifyList(legend_args, list(x = legend)) + } else if (is.list(legend)) { + if (is.null(legend[["x"]])) legend[["x"]] = "right!" + legend_args = modifyList(legend_args, legend, keep.null = TRUE) } else if (class(legend) %in% c("call", "name")) { largs = as.list(legend) if (is.null(largs[["x"]])) { @@ -716,7 +719,7 @@ sanitize_legend = function(legend, legend_args) { } } # Finally, combine with any pre-existing legend args (e.g., title from the by label) - legend_args = utils::modifyList(legend_args, largs, keep.null = TRUE) + legend_args = modifyList(legend_args, largs, keep.null = TRUE) } } return(legend_args) From 8e9c4f84b9642934cc2ee9fffd29257cb97d5954 Mon Sep 17 00:00:00 2001 From: Grant McDermott Date: Wed, 19 Nov 2025 21:36:02 -0800 Subject: [PATCH 3/9] move `sanitize_legend` to own file while we're at it (like other sanitizers) --- R/draw_legend.R | 32 -------------------------------- R/sanitize_legend.R | 29 +++++++++++++++++++++++++++++ 2 files changed, 29 insertions(+), 32 deletions(-) create mode 100644 R/sanitize_legend.R diff --git a/R/draw_legend.R b/R/draw_legend.R index a5182fbb..d97ccc45 100644 --- a/R/draw_legend.R +++ b/R/draw_legend.R @@ -692,35 +692,3 @@ gradient_legend = function( } } - -# sanitize legend (helper function) ---- - -sanitize_legend = function(legend, legend_args) { - if (is.null(legend_args[["x"]])) { - if (is.null(legend)) { - legend_args[["x"]] = "right!" - } else if (is.character(legend)) { - legend_args = utils::modifyList(legend_args, list(x = legend)) - } else if (is.list(legend)) { - if (is.null(legend[["x"]])) legend[["x"]] = "right!" - legend_args = modifyList(legend_args, legend, keep.null = TRUE) - } else if (class(legend) %in% c("call", "name")) { - largs = as.list(legend) - if (is.null(largs[["x"]])) { - lnms = names(largs) - # check second position b/c first will be a symbol - if (is.null(lnms)) { - largs = setNames(largs, c("", "x")) - } else if (length(largs) >= 2 && lnms[2] == "") { - lnms[2] = "x" - largs = setNames(largs, lnms) - } else { - largs[["x"]] = "right!" - } - } - # Finally, combine with any pre-existing legend args (e.g., title from the by label) - legend_args = modifyList(legend_args, largs, keep.null = TRUE) - } - } - return(legend_args) -} diff --git a/R/sanitize_legend.R b/R/sanitize_legend.R new file mode 100644 index 00000000..87369ba9 --- /dev/null +++ b/R/sanitize_legend.R @@ -0,0 +1,29 @@ +sanitize_legend = function(legend, legend_args) { + if (is.null(legend_args[["x"]])) { + if (is.null(legend)) { + legend_args[["x"]] = "right!" + } else if (is.character(legend)) { + legend_args = modifyList(legend_args, list(x = legend)) + } else if (is.list(legend)) { + if (is.null(legend[["x"]])) legend[["x"]] = "right!" + legend_args = modifyList(legend_args, legend, keep.null = TRUE) + } else if (class(legend) %in% c("call", "name")) { + largs = as.list(legend) + if (is.null(largs[["x"]])) { + lnms = names(largs) + # check second position b/c first will be a symbol + if (is.null(lnms)) { + largs = setNames(largs, c("", "x")) + } else if (length(largs) >= 2 && lnms[2] == "") { + lnms[2] = "x" + largs = setNames(largs, lnms) + } else { + largs[["x"]] = "right!" + } + } + # Finally, combine with any pre-existing legend args (e.g., title from the by label) + legend_args = modifyList(legend_args, largs, keep.null = TRUE) + } + } + return(legend_args) +} From 85a8220df52363bddfe1fde62ccc2ebac4b4c3c8 Mon Sep 17 00:00:00 2001 From: Grant McDermott Date: Wed, 19 Nov 2025 22:09:57 -0800 Subject: [PATCH 4/9] fix test errors --- R/sanitize_legend.R | 6 ++++++ R/tinyplot.R | 12 +++++++----- R/title.R | 2 +- 3 files changed, 14 insertions(+), 6 deletions(-) diff --git a/R/sanitize_legend.R b/R/sanitize_legend.R index 87369ba9..5277f326 100644 --- a/R/sanitize_legend.R +++ b/R/sanitize_legend.R @@ -5,6 +5,12 @@ sanitize_legend = function(legend, legend_args) { } else if (is.character(legend)) { legend_args = modifyList(legend_args, list(x = legend)) } else if (is.list(legend)) { + # Check if first element is unnamed and character (position keyword) + if (is.null(names(legend)) || names(legend)[1] == "") { + if (length(legend) >= 1 && is.character(legend[[1]])) { + names(legend)[1] = "x" + } + } if (is.null(legend[["x"]])) legend[["x"]] = "right!" legend_args = modifyList(legend_args, legend, keep.null = TRUE) } else if (class(legend) %in% c("call", "name")) { diff --git a/R/tinyplot.R b/R/tinyplot.R index 7523304b..8fd5f5cf 100644 --- a/R/tinyplot.R +++ b/R/tinyplot.R @@ -641,13 +641,15 @@ tinyplot.default = function( theme = NULL, ...) { - # Force evaluation of legend to avoid downstream symbol (promise) issues - if (!missing(legend)) legend = legend + # Force evaluation of legend if it's a symbol to avoid downstream promise + # issues. Let sanitize_legend handle it + if (!missing(legend) && is.symbol(substitute(legend))) { + legend = legend + } # ## save parameters and calls ----- # - par_first = get_saved_par("first") if (is.null(par_first)) set_saved_par("first", par()) @@ -946,7 +948,7 @@ tinyplot.default = function( } else if (isTRUE(legend)) { legend = NULL } - if (!is.null(legend) && legend == "none") { + if (!is.null(legend) && is.character(legend) && legend == "none") { legend_args[["x"]] = "none" dual_legend = FALSE } @@ -969,7 +971,7 @@ tinyplot.default = function( } } - if ((is.null(legend) || legend != "none" || bubble) && !add) { + if ((is.null(legend) || !is.character(legend) || legend != "none" || bubble) && !add) { if (isFALSE(by_continuous) && (!bubble || dual_legend)) { if (ngrps > 1) { lgnd_labs = if (is.factor(datapoints$by)) levels(datapoints$by) else unique(datapoints$by) diff --git a/R/title.R b/R/title.R index 29b57208..74311d05 100644 --- a/R/title.R +++ b/R/title.R @@ -9,7 +9,7 @@ draw_title = function(main, sub, xlab, ylab, legend, legend_args, opar) { legend_eval = tryCatch(paste0(legend)[[2]], error = function(e) NULL) } - adj_title = !is.null(legend) && (legend == "top!" || (!is.null(legend_args[["x"]]) && legend_args[["x"]] == "top!") || (is.list(legend_eval) && legend_eval[[1]] == "top!")) + adj_title = !is.null(legend) && ((is.character(legend) && legend == "top!") || (!is.null(legend_args[["x"]]) && legend_args[["x"]] == "top!") || (is.list(legend_eval) && legend_eval[[1]] == "top!")) # For the "top!" legend case, bump main title up to make space for the # legend beneath it: Take the normal main title line gap (i.e., 1.7 lines) From 7cf8c64a2208645c3c2334c52d05ad51f44355a3 Mon Sep 17 00:00:00 2001 From: Grant McDermott Date: Wed, 19 Nov 2025 22:20:10 -0800 Subject: [PATCH 5/9] simplify sanitize_legend --- R/sanitize_legend.R | 52 ++++++++++++++++++++++----------------------- 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/R/sanitize_legend.R b/R/sanitize_legend.R index 5277f326..d7951951 100644 --- a/R/sanitize_legend.R +++ b/R/sanitize_legend.R @@ -1,35 +1,35 @@ sanitize_legend = function(legend, legend_args) { if (is.null(legend_args[["x"]])) { - if (is.null(legend)) { - legend_args[["x"]] = "right!" + + # Normalize legend to a list + largs = if (is.null(legend)) { + list(x = "right!") } else if (is.character(legend)) { - legend_args = modifyList(legend_args, list(x = legend)) + list(x = legend) } else if (is.list(legend)) { - # Check if first element is unnamed and character (position keyword) - if (is.null(names(legend)) || names(legend)[1] == "") { - if (length(legend) >= 1 && is.character(legend[[1]])) { - names(legend)[1] = "x" - } + # Handle unnamed first element as position + if (length(legend) >= 1 && is.character(legend[[1]]) && + (is.null(names(legend)) || names(legend)[1] == "")) { + names(legend)[1] = "x" } - if (is.null(legend[["x"]])) legend[["x"]] = "right!" - legend_args = modifyList(legend_args, legend, keep.null = TRUE) - } else if (class(legend) %in% c("call", "name")) { - largs = as.list(legend) - if (is.null(largs[["x"]])) { - lnms = names(largs) - # check second position b/c first will be a symbol - if (is.null(lnms)) { - largs = setNames(largs, c("", "x")) - } else if (length(largs) >= 2 && lnms[2] == "") { - lnms[2] = "x" - largs = setNames(largs, lnms) - } else { - largs[["x"]] = "right!" - } + legend + } else if (inherits(legend, c("call", "name"))) { + # Convert call to list and handle unnamed first arg as position + new_legend = as.list(legend)[-1] # Remove function name + if (length(new_legend) >= 1 && (is.null(names(new_legend)) || names(new_legend)[1] == "")) { + names(new_legend)[1] = "x" } - # Finally, combine with any pre-existing legend args (e.g., title from the by label) - legend_args = modifyList(legend_args, largs, keep.null = TRUE) + new_legend + } else { + list(x = "right!") # Fallback } + + # Ensure position exists + if (is.null(largs[["x"]])) largs[["x"]] = "right!" + + # Merge + legend_args = modifyList(legend_args, largs, keep.null = TRUE) } - return(legend_args) + + legend_args } From 8d05d8198ab9e4a76e06165545e0ff04c49ff649 Mon Sep 17 00:00:00 2001 From: Grant McDermott Date: Wed, 19 Nov 2025 22:36:48 -0800 Subject: [PATCH 6/9] ancilliary simplification --- R/tinyplot.R | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/R/tinyplot.R b/R/tinyplot.R index 8fd5f5cf..7785bcdc 100644 --- a/R/tinyplot.R +++ b/R/tinyplot.R @@ -954,20 +954,13 @@ tinyplot.default = function( } if (null_by) { - if (is.null(legend)) { - # special case: bubble legend, no by legend - if (bubble && !dual_legend) { - legend_args[["title"]] = cex_dep ## rather by_dep? - lgnd_labs = names(bubble_cex) - lgnd_cex = bubble_cex * cex_fct_adj - } else { - legend = "none" - legend_args[["x"]] = "none" - } - } else if (bubble && !dual_legend) { - legend_args[["title"]] = cex_dep ## rather by_dep? - lgnd_labs = names(bubble_cex) - lgnd_cex = bubble_cex * cex_fct_adj + if (bubble && !dual_legend) { + legend_args[["title"]] = cex_dep + lgnd_labs = names(bubble_cex) + lgnd_cex = bubble_cex * cex_fct_adj + } else if (is.null(legend)) { + legend = "none" + legend_args[["x"]] = "none" } } From 195c138a1f2484be50f25d71248ce5d8810321af Mon Sep 17 00:00:00 2001 From: Grant McDermott Date: Wed, 19 Nov 2025 22:41:38 -0800 Subject: [PATCH 7/9] add test --- .../_tinysnapshot/legend_custom_s3.svg | 83 +++++++++++++++++++ inst/tinytest/test-legend.R | 13 +++ 2 files changed, 96 insertions(+) create mode 100644 inst/tinytest/_tinysnapshot/legend_custom_s3.svg diff --git a/inst/tinytest/_tinysnapshot/legend_custom_s3.svg b/inst/tinytest/_tinysnapshot/legend_custom_s3.svg new file mode 100644 index 00000000..142e259f --- /dev/null +++ b/inst/tinytest/_tinysnapshot/legend_custom_s3.svg @@ -0,0 +1,83 @@ + + + + + + + + + + + + + + + +New Title +A +B + + + + + + + +x +y + + + + + + + + + +1.0 +1.2 +1.4 +1.6 +1.8 +2.0 + + + + + + + +1.0 +1.2 +1.4 +1.6 +1.8 +2.0 + + + + + + + + + + + + + diff --git a/inst/tinytest/test-legend.R b/inst/tinytest/test-legend.R index cf72ece3..3818e7ce 100644 --- a/inst/tinytest/test-legend.R +++ b/inst/tinytest/test-legend.R @@ -175,3 +175,16 @@ expect_snapshot_plot(f, label = "legend_lmar_top") # reset par par(op) + + +# custom legend for new tinyplot.foo s3 method +f = function() { + foo = data.frame(x = 1:2, y = 1:2, grp = c("A", "B")) + class(foo) = c("foo", "data.frame") + tinyplot.foo = function(x, ...) { + legend = list(title = 'New Title') + plt(y ~ x | grp, data = x, legend = legend, ...) + } + plt(foo) +} +expect_snapshot_plot(f, label = "legend_custom_s3") \ No newline at end of file From 84d08f44f4cef0bf59d6c2685d60c7a19c7b795f Mon Sep 17 00:00:00 2001 From: Grant McDermott Date: Wed, 19 Nov 2025 22:51:21 -0800 Subject: [PATCH 8/9] test for #513 while we're at it --- .../_tinysnapshot/pointrange_with_hline.svg | 82 +++++++++++++++++++ inst/tinytest/test-type_pointrange.R | 13 ++- 2 files changed, 94 insertions(+), 1 deletion(-) create mode 100644 inst/tinytest/_tinysnapshot/pointrange_with_hline.svg diff --git a/inst/tinytest/_tinysnapshot/pointrange_with_hline.svg b/inst/tinytest/_tinysnapshot/pointrange_with_hline.svg new file mode 100644 index 00000000..d96fa8ae --- /dev/null +++ b/inst/tinytest/_tinysnapshot/pointrange_with_hline.svg @@ -0,0 +1,82 @@ + + + + + + + + + + + + + +x +y + + + + + +(Intercept) +hp +factor(cyl)6 +factor(cyl)8 + + + + + + +-10 +0 +10 +20 +30 + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/inst/tinytest/test-type_pointrange.R b/inst/tinytest/test-type_pointrange.R index ddc22b8a..dad8f77e 100644 --- a/inst/tinytest/test-type_pointrange.R +++ b/inst/tinytest/test-type_pointrange.R @@ -34,8 +34,19 @@ fun = function() { } expect_snapshot_plot(fun, label = "pointrange_errorbar") +# issue 511: adding hline to coefplot +fun = function() { + tinyplot( + y ~ x, ymin = ymin, ymax = ymax, + data = coefs, + type = "pointrange", + theme = "basic" + ) + tinyplot_add(type = "hline", lty = 2) +} +expect_snapshot_plot(fun, label = "pointrange_with_hline") -# Issue #406: dodge pointrage and errorbar +# Issue #406: dodge pointrange and errorbar models = list( "Model A" = lm(mpg ~ wt + cyl, data = mtcars), "Model B" = lm(mpg ~ wt + hp + cyl, data = mtcars), From c373eefd6b331760c6aff1818a8645efef88c0a4 Mon Sep 17 00:00:00 2001 From: Grant McDermott Date: Wed, 19 Nov 2025 22:53:59 -0800 Subject: [PATCH 9/9] news --- NEWS.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/NEWS.md b/NEWS.md index 45dcb16e..2efdb0e4 100644 --- a/NEWS.md +++ b/NEWS.md @@ -37,7 +37,10 @@ where the formatting is also better._ the report. (#512 @grantmcdermott) - Axis limits are now correctly calculated for factor (and character) variables, by coercing to numeric first. We also avoid the redundancy of re-calculating - axis limits for secondary plot layers. (#513 @grantmcdermott) + axis limits for secondary plot layers. (#513 @grantmcdermott) +- Fixed lazy evaluation bug where `legend` passed as a symbol through S3 methods + (e.g., `tinyplot.foo`) would fail. (#515 @grantmcdermott) + ### Documentation