Fix scale- and coord-related regressions (#3566)

* fix failure with censored breaks when labels were specified manually (fixes #3558)

* ensure that ScaleContinuous$is_empty() returns TRUE when there is an NA in the limits (fixes #3560)

* ensure that functional and NA limits are handled properly on discrete and continuous scales (fixes #3448)

Author: paleolimbot

R/guides-axis.r

@@ -93,7 +93,7 @@ guide_train.axis <- function(guide, scale, aesthetic = NULL) {
       }
     }
 
-    guide$key <- ticks
+    guide$key <- ticks[is.finite(ticks[[aesthetic]]), ]
   }
 
   guide$name <- paste0(guide$name, "_", aesthetic)

R/scale-.r

@@ -451,10 +451,9 @@ Scale <- ggproto("Scale", NULL,
     if (is.null(self$limits)) {
       self$range$range
     } else if (is.function(self$limits)) {
-      # if limits is a function, it expects to work in data space
-      self$trans$transform(self$limits(self$trans$inverse(self$range$range)))
+      self$limits(self$range$range)
     } else {
-      ifelse(is.na(self$limits), self$range$range, self$limits)
+      self$limits
     }
   },
 
@@ -542,6 +541,12 @@ ScaleContinuous <- ggproto("ScaleContinuous", Scale,
     self$range$train(x)
   },
 
+  is_empty = function(self) {
+    has_data <- !is.null(self$range$range)
+    has_limits <- is.function(self$limits) || (!is.null(self$limits) && all(is.finite(self$limits)))
+    !has_data && !has_limits
+  },
+
   transform = function(self, x) {
     new_x <- self$trans$transform(x)
     axis <- if ("x" %in% self$aesthetics) "x" else "y"
@@ -563,6 +568,22 @@ ScaleContinuous <- ggproto("ScaleContinuous", Scale,
     self$rescaler(x, from = range)
   },
 
+  get_limits = function(self) {
+    if (self$is_empty()) {
+      return(c(0, 1))
+    }
+
+    if (is.null(self$limits)) {
+      self$range$range
+    } else if (is.function(self$limits)) {
+      # if limits is a function, it expects to work in data space
+      self$trans$transform(self$limits(self$trans$inverse(self$range$range)))
+    } else {
+      # NA limits for a continuous scale mean replace with the min/max of data
+      ifelse(is.na(self$limits), self$range$range, self$limits)
+    }
+  },
+
   dimension = function(self, expand = expansion(0, 0), limits = self$get_limits()) {
     expand_limits_scale(self, expand, limits)
   },

R/scale-view.r

@@ -17,7 +17,6 @@ view_scale_primary <- function(scale, limits = scale$get_limits(),
 
   if(!scale$is_discrete()) {
     breaks <- scale$get_breaks(continuous_range)
-    breaks <- breaks[is.finite(breaks)]
     minor_breaks <- scale$get_breaks_minor(b = breaks, limits = continuous_range)
   } else {
     breaks <- scale$get_breaks(limits)

tests/testthat/test-scale-discrete.R

@@ -81,3 +81,9 @@ test_that("discrete position scales can accept functional limits", {
   scale$train(c("a", "b", "c"))
   expect_identical(scale$get_limits(), c("c", "b", "a"))
 })
+
+test_that("discrete non-position scales can accept functional limits", {
+  scale <- scale_colour_discrete(limits = rev)
+  scale$train(c("a", "b", "c"))
+  expect_identical(scale$get_limits(), c("c", "b", "a"))
+})

tests/testthat/test-scales-breaks-labels.r

@@ -247,6 +247,22 @@ test_that("continuous limits accepts functions", {
   expect_equal(layer_scales(p)$y$get_limits(), c(range(mpg$hwy)[1] - 10, range(mpg$hwy)[2] + 100))
 })
 
+test_that("equal length breaks and labels can be passed to ViewScales with limits", {
+
+  test_scale <- scale_x_continuous(
+    breaks = c(0, 20, 40),
+    labels = c("0", "20", "40"),
+    limits = c(10, 30)
+  )
+
+  expect_identical(test_scale$get_breaks(), c(NA, 20, NA))
+  expect_identical(test_scale$get_labels(), c(c("0", "20", "40")))
+
+  test_view_scale <- view_scale_primary(test_scale)
+  expect_identical(test_view_scale$get_breaks(), c(NA, 20, NA))
+  expect_identical(test_scale$get_labels(), c(c("0", "20", "40")))
+})
+
 # Visual tests ------------------------------------------------------------
 
 test_that("minor breaks draw correctly", {

tests/testthat/test-scales.r

@@ -294,3 +294,28 @@ test_that("multiple aesthetics can be set with one function call", {
   expect_equal(layer_data(p)$colour, c("cyan", "red", "green"))
   expect_equal(layer_data(p)$fill, c("red", "green", "blue"))
 })
+
+test_that("limits with NA are replaced with the min/max of the data for continuous scales", {
+  make_scale <- function(limits = NULL, data = NULL) {
+    scale <- continuous_scale("aesthetic", scale_name = "test", palette = identity, limits = limits)
+    if (!is.null(data)) {
+      scale$train(data)
+    }
+    scale
+  }
+
+  # emptiness
+  expect_true(make_scale()$is_empty())
+  expect_false(make_scale(limits = c(0, 1))$is_empty())
+  expect_true(make_scale(limits = c(0, NA))$is_empty())
+  expect_true(make_scale(limits = c(NA, NA))$is_empty())
+  expect_true(make_scale(limits = c(NA, 0))$is_empty())
+
+  # limits
+  expect_equal(make_scale(data = 1:5)$get_limits(), c(1, 5))
+  expect_equal(make_scale(limits = c(1, 5))$get_limits(), c(1, 5))
+  expect_equal(make_scale(limits = c(NA, NA))$get_limits(), c(0, 1))
+  expect_equal(make_scale(limits = c(NA, NA), data = 1:5)$get_limits(), c(1, 5))
+  expect_equal(make_scale(limits = c(1, NA), data = 1:5)$get_limits(), c(1, 5))
+  expect_equal(make_scale(limits = c(NA, 5), data = 1:5)$get_limits(), c(1, 5))
+})