Handle legend lazy eval passing

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
 

R/draw_legend.R

@@ -692,32 +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 (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 = utils::modifyList(legend_args, largs, keep.null = TRUE)
-    }
-  }
-  return(legend_args)
-}

R/sanitize_legend.R

@@ -0,0 +1,35 @@
+sanitize_legend = function(legend, legend_args) {
+  if (is.null(legend_args[["x"]])) {
+
+    # Normalize legend to a list
+    largs = if (is.null(legend)) {
+      list(x = "right!")
+    } else if (is.character(legend)) {
+      list(x = legend)
+    } else if (is.list(legend)) {
+      # 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"
+      }
+      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"
+      }
+      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)
+  }
+
+  legend_args
+}

R/tinyplot.R

@@ -641,11 +641,15 @@ tinyplot.default = function(
     theme = NULL,
     ...) {
 
+  # 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())
 
@@ -909,6 +913,7 @@ tinyplot.default = function(
   #
   ## legends -----
   #
+  # browser()
 
   # legend labels
   ncolors = length(col)
@@ -943,30 +948,23 @@ 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
   }
 
   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"
     }
   }
 
-  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)
@@ -1399,6 +1397,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")) {

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)

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")

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),