Skip to content

Conversation

thomasp85
Copy link
Member

Fixes #3729

This PR catches any errors happening when applying after_scale() modifications to legend keys. If it errors it will simply discard them all with a warning.

It may be done more fine grained, but not without a lot of weird refactoring of the code to accommodate this edge-case. This PR fixes the surprise of the legend disappearing and gives a meaningful warning. If we get a lot of requests for better support (I doubt it) we can revisit it in the future

@thomasp85 thomasp85 added this to the ggplot2 3.3.0 milestone Feb 6, 2020
@yutannihilation
Copy link
Member

Looks good, but let me confirm.

This PR fixes the surprise of the legend disappearing and gives a meaningful warning.

This doesn't fix "the legend disappearing," but fixes the drawing failure; I pointed two problems on #3729 and the second case is not fixed by this PR. Is this what you intended?

set.seed(88)
d <- data.frame(x = runif(9), y = runif(9), g = rep(c("a", "b", "c"), each = 3))

devtools::load_all("~/Documents/repo/ggplot2/")
#> Loading ggplot2

# Case 1: fails with the current master
ggplot(d) +
  geom_point(aes(x, y, colour = stage(g, after_scale = alpha(colour, x))), size = 10)
#> Warning: Failed to apply `after_scale()` modifications to legend

# Case 2: does not fail, but loses the colours
ggplot(d) +
  geom_point(aes(x, y, size = x + y,
                 colour = stage(g, after_scale = ifelse(size > 4, colour, "grey"))))

Created on 2020-02-06 by the reprex package (v0.3.0)

@thomasp85
Copy link
Member Author

That was bad wording on my part. I got confused by the first example showing a plot without a legend...

Yes - this is the correct behaviour...

@yutannihilation
Copy link
Member

Thanks, then I'm approving this PR. I think your concern is valid.

It may be done more fine grained, but not without a lot of weird refactoring of the code to accommodate this edge-case.

We would need to complicate things just to solve edge cases. We might need to consider this a bit deeper in future if many people use this in some extreme ways, but I agree this fix is the best at this point.

@thomasp85 thomasp85 merged commit 2303fd4 into tidyverse:v3.3.0-rc Feb 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants