n.breaks are supplied to function-breaks. (#5974)

* deal with NA/NULL breaks earlier

* generalise `trans_support_nbreaks()` --> `support_nbreaks()`

* early exit on zero-range limits

* apply `n.breaks` regarless or whence breaks-function came

* Throw `NA` breaks error in 1 place

* omit outdated comment

* include binned scales

* approve pattern snapshots (re: #6453)

* add news bullet

Author: teunbrand

NEWS.md

@@ -1,5 +1,8 @@
 # ggplot2 (development version)
 
+* In continuous scales, when `breaks` is a function and `n.breaks` is set, the 
+  `n.breaks` will be passed to the `breaks` function. Previously, `n.breaks` 
+  only applied to the default break calculation (@teunbrand, #5972)
 * (internal) New `Facet$draw_panel_content()` method for delegating panel 
   assembly (@Yunuuuu, #6406).
 * Facet gains a new method `setup_panel_params` to interact with the

R/scale-.R

@@ -640,6 +640,12 @@ check_breaks_labels <- function(breaks, labels, call = NULL) {
   if (is.null(breaks) || is.null(labels)) {
     return(invisible())
   }
+  if (identical(breaks, NA)) {
+    cli::cli_abort(
+      "Invalid {.arg breaks} specification. Use {.code NULL}, not {.code NA}.",
+      call = call
+    )
+  }
 
   bad_labels <- is.atomic(breaks) && is.atomic(labels) &&
     length(breaks) != length(labels)
@@ -751,48 +757,38 @@ ScaleContinuous <- ggproto("ScaleContinuous", Scale,
       return(numeric())
     }
     transformation <- self$get_transformation()
+    breaks <- self$breaks %|W|% transformation$breaks
+
+    if (is.null(breaks)) {
+      return(NULL)
+    }
+
     # Ensure limits don't exceed domain (#980)
     domain <- suppressWarnings(transformation$transform(transformation$domain))
     domain <- sort(domain)
     # To avoid NaN causing issues. NaN are dropped by the sort()
     if (length(domain) == 2 && !zero_range(domain)) {
       limits <- oob_squish(limits, domain)
     }
-
-    # Limits in transformed space need to be converted back to data space
-    limits <- transformation$inverse(limits)
-
-    if (is.null(self$breaks)) {
-      return(NULL)
-    }
-
-    if (identical(self$breaks, NA)) {
-      cli::cli_abort(
-        "Invalid {.arg breaks} specification. Use {.code NULL}, not {.code NA}.",
-        call = self$call
-      )
+    if (zero_range(as.numeric(limits))) {
+      return(limits[1])
     }
 
-    # Compute `zero_range()` in transformed space in case `limits` in data space
-    # don't support conversion to numeric (#5304)
-    if (zero_range(as.numeric(transformation$transform(limits)))) {
-      breaks <- limits[1]
-    } else if (is.waiver(self$breaks)) {
-      if (!is.null(self$n.breaks) && trans_support_nbreaks(transformation)) {
-        breaks <- transformation$breaks(limits, self$n.breaks)
+    if (is.function(breaks)) {
+      # Limits in transformed space need to be converted back to data space
+      limits <- transformation$inverse(limits)
+      if (!is.null(self$n.breaks) && support_nbreaks(breaks)) {
+        breaks <- breaks(limits, n = self$n.breaks)
       } else {
+        breaks <- breaks(limits)
         if (!is.null(self$n.breaks)) {
           cli::cli_warn(
-            "Ignoring {.arg n.breaks}. Use a {.cls transform} object that supports setting number of breaks.",
+            "Ignoring {.arg n.breaks}. Use a {.cls transform} object or \\
+            {.arg breaks} function that supports setting number of breaks",
             call = self$call
           )
         }
-        breaks <- transformation$breaks(limits)
       }
-    } else if (is.function(self$breaks)) {
-      breaks <- self$breaks(limits)
-    } else {
-      breaks <- self$breaks
     }
 
     # Breaks in data space need to be converted back to transformed space
@@ -1046,13 +1042,6 @@ ScaleDiscrete <- ggproto("ScaleDiscrete", Scale,
       return(NULL)
     }
 
-    if (identical(self$breaks, NA)) {
-      cli::cli_abort(
-        "Invalid {.arg breaks} specification. Use {.code NULL}, not {.code NA}.",
-        call = self$call
-      )
-    }
-
     if (is.waiver(self$breaks)) {
       breaks <- limits
     } else if (is.function(self$breaks)) {
@@ -1268,14 +1257,9 @@ ScaleBinned <- ggproto("ScaleBinned", Scale,
 
     if (is.null(self$breaks)) {
       return(NULL)
-    } else if (identical(self$breaks, NA)) {
-      cli::cli_abort(
-        "Invalid {.arg breaks} specification. Use {.code NULL}, not {.code NA}.",
-        call = self$call
-      )
     } else if (is.waiver(self$breaks)) {
       if (self$nice.breaks) {
-        if (!is.null(self$n.breaks) && trans_support_nbreaks(transformation)) {
+        if (!is.null(self$n.breaks) && support_nbreaks(transformation$breaks)) {
           breaks <- transformation$breaks(limits, n = self$n.breaks)
         } else {
           if (!is.null(self$n.breaks)) {
@@ -1332,9 +1316,16 @@ ScaleBinned <- ggproto("ScaleBinned", Scale,
         }
       }
     } else if (is.function(self$breaks)) {
-      if ("n.breaks" %in% names(formals(environment(self$breaks)$f))) {
+      fmls <- names(formals(environment(self$breaks)$f))
+      if (any(c("n", "n.breaks") %in% fmls)) {
         n.breaks <- self$n.breaks %||% 5 # same default as trans objects
-        breaks <- self$breaks(limits, n.breaks = n.breaks)
+        # TODO: we should only allow `n` argument and not `n.breaks` to be
+        # consistent with other scales. We should start deprecation at some point.
+        if ("n.breaks" %in% fmls) {
+          breaks <- self$breaks(limits, n.breaks = n.breaks)
+        } else {
+          breaks <- self$breaks(limits, n = n.breaks)
+        }
       } else {
         if (!is.null(self$n.breaks)) {
           cli::cli_warn(
@@ -1439,6 +1430,14 @@ check_transformation <- function(x, transformed, name, arg = NULL, call = NULL)
   cli::cli_warn(msg, call = call)
 }
 
+
+support_nbreaks <- function(fun) {
+  if (inherits(fun, "ggproto_method")) {
+    fun <- environment(fun)$f
+  }
+  "n" %in% fn_fmls_names(fun)
+}
+
 check_continuous_limits <- function(limits, ...,
                                     arg = caller_arg(limits),
                                     call = caller_env()) {
@@ -1449,10 +1448,6 @@ check_continuous_limits <- function(limits, ...,
   check_length(limits, 2L, arg = arg, call = call)
 }
 
-trans_support_nbreaks <- function(trans) {
-  "n" %in% names(formals(trans$breaks))
-}
-
 allow_lambda <- function(x) {
   if (is_formula(x)) as_function(x) else x
 }