Skip to content

Commit 6b74f6f

Browse files
authored
fix(ggplotly): preserve legend ordering (#2495)
* fix: update test expectation * update visual tests * Make merged-legend title order match legend-entry order ggplot2's Guides$merge() breaks ties between same-order guides using a content hash (rlang::hash()), which has no relation to aesthetic declaration order. A recent rlang release changed that hash, flipping the tie-break and silently reordering combined legend titles (e.g. "factor(vs)<br />factor(cyl)" became "factor(cyl)<br />factor(vs)"). Legend entries themselves are named independently, always in aesthetic declaration order (layers2traces.R's discreteScales), so title order and entry order could now disagree. Reorder guides in get_gdefs_ggproto() by (explicit guide order, aesthetic declaration order) so the title always agrees with the entries it labels, regardless of upstream hashing changes. * fix: replace deprecated .Label/.Names structure() names in tests Silences the R CMD check NOTE about deprecated special names in structure() calls. * Revert changes to visual tests * Update other visual tests * Update news
1 parent 2e51828 commit 6b74f6f

7 files changed

Lines changed: 33 additions & 10 deletions

File tree

NEWS.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@
99

1010
* Closed #2483: `save_image()` no longer embeds Windows file paths directly into Python source passed to `reticulate`, fixing static image export with Kaleido on Windows.
1111

12+
* #2495: `ggplotly()` now keeps merged legend titles in sync with their legend entries, fixing a case where an upstream change in `{rlang}`'s hashing could silently reorder combined legend titles (e.g. `factor(vs)<br />factor(cyl)`) out of step with the entries they label.
13+
1214

1315
# plotly 4.12.0
1416

R/ggplotly.R

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1592,6 +1592,23 @@ get_gdefs_ggproto <- function(scales, theme, plot, layers, layer_data) {
15921592
if (length(guides$guides) > 0) {
15931593
guides$merge()
15941594
guides$process_layers(layers, layer_data)
1595+
# guides$merge() sorts guides by their explicit `order` (guide_legend(order=)),
1596+
# but breaks ties (the common case, since `order` defaults to 0/unset for
1597+
# everyone) using a content hash that bears no relation to aesthetic
1598+
# declaration order (ggplot2's `Guides$merge()`, via rlang::hash()).
1599+
# layers2traces() names legend entries in aesthetic declaration order (see
1600+
# `discreteScales`), so on ties, guides must be put back in that same
1601+
# order for the combined legend title (built below) to agree with the
1602+
# entries it labels -- while still respecting any explicit `order`.
1603+
guide_order <- vapply(guides$params, function(p) {
1604+
o <- p$order %||% 0
1605+
if (identical(o, 0)) 99L else as.integer(o)
1606+
}, integer(1))
1607+
declaration_order <- match(guides$aesthetics, aesthetics)
1608+
ord <- order(guide_order, declaration_order)
1609+
guides$guides <- guides$guides[ord]
1610+
guides$params <- guides$params[ord]
1611+
guides$aesthetics <- guides$aesthetics[ord]
15951612
}
15961613
# Add old legend/colorbar classes to guide params so that ggplotly() code
15971614
# can continue to work the same way it always has

tests/testthat/_snaps/ggplot-size/size-global-scaling.svg

Lines changed: 1 addition & 1 deletion
Loading

tests/testthat/_snaps/ggplot-theme/theme-marker-default.svg

Lines changed: 1 addition & 1 deletion
Loading

tests/testthat/test-ggplot-facets.R

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,9 @@ test_that("6 facets becomes 6 panels", {
66
barley <- structure(list(
77
yield = c(27, 48.86667, 27.43334, 39.93333, 32.96667, 28.96667, 43.06666, 55.2, 28.76667, 38.13333, 29.13333, 29.66667, 35.13333, 47.33333, 25.76667, 40.46667, 29.66667, 25.7, 39.9, 50.23333, 26.13333, 41.33333, 23.03333, 26.3, 36.56666, 63.8333, 43.76667, 46.93333, 29.76667, 33.93333, 43.26667, 58.1, 28.7, 45.66667, 32.16667, 33.6, 36.6, 65.7667, 30.36667, 48.56666, 24.93334, 28.1, 32.76667, 48.56666, 29.86667, 41.6, 34.7, 32, 24.66667, 46.76667, 22.6, 44.1, 19.7, 33.06666, 39.3, 58.8, 29.46667, 49.86667, 34.46667, 31.6, 26.9, 33.46667, 34.36666, 32.96667, 22.13333, 22.56667, 36.8, 37.73333, 35.13333, 26.16667, 14.43333, 25.86667, 27.43334, 38.5, 35.03333, 20.63333, 16.63333, 22.23333, 26.8, 37.4, 38.83333, 32.06666, 32.23333, 22.46667, 29.06667, 49.2333, 46.63333, 41.83333, 20.63333, 30.6, 26.43334, 42.2, 43.53334, 34.33333, 19.46667, 22.7, 25.56667, 44.7, 47, 30.53333, 19.9, 22.5, 28.06667, 36.03333, 43.2, 25.23333, 26.76667, 31.36667, 30, 41.26667, 44.23333, 32.13333, 15.23333, 27.36667, 38, 58.16667, 47.16667, 35.9, 20.66667, 29.33333),
88
variety = structure(c(3L, 3L, 3L, 3L, 3L, 3L, 7L, 7L, 7L, 7L, 7L, 7L, 1L, 1L, 1L, 1L, 1L, 1L, 5L, 5L, 5L, 5L, 5L, 5L, 10L, 10L, 10L, 10L, 10L, 10L, 8L, 8L, 8L, 8L, 8L, 8L, 2L, 2L, 2L, 2L, 2L, 2L, 6L, 6L, 6L, 6L, 6L, 6L, 4L, 4L, 4L, 4L, 4L, 4L, 9L, 9L, 9L, 9L, 9L, 9L, 3L, 3L, 3L, 3L, 3L, 3L, 7L, 7L, 7L, 7L, 7L, 7L, 1L, 1L, 1L, 1L, 1L, 1L, 5L, 5L, 5L, 5L, 5L, 5L, 10L, 10L, 10L, 10L, 10L, 10L, 8L, 8L, 8L, 8L, 8L, 8L, 2L, 2L, 2L, 2L, 2L, 2L, 6L, 6L, 6L, 6L, 6L, 6L, 4L, 4L, 4L, 4L, 4L, 4L, 9L, 9L, 9L, 9L, 9L, 9L),
9-
.Label = c("Svansota", "No. 462", "Manchuria", "No. 475", "Velvet", "Peatland", "Glabron", "No. 457", "Wisconsin No. 38", "Trebi"), class = "factor"),
10-
year = structure(c(2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L), .Label = c("1932", "1931"), class = "factor"),
11-
site = structure(c(3L, 6L, 4L, 5L, 1L, 2L, 3L, 6L, 4L, 5L, 1L, 2L, 3L, 6L, 4L, 5L, 1L, 2L, 3L, 6L, 4L, 5L, 1L, 2L, 3L, 6L, 4L, 5L, 1L, 2L, 3L, 6L, 4L, 5L, 1L, 2L, 3L, 6L, 4L, 5L, 1L, 2L, 3L, 6L, 4L, 5L, 1L, 2L, 3L, 6L, 4L, 5L, 1L, 2L, 3L, 6L, 4L, 5L, 1L, 2L, 3L, 6L, 4L, 5L, 1L, 2L, 3L, 6L, 4L, 5L, 1L, 2L, 3L, 6L, 4L, 5L, 1L, 2L, 3L, 6L, 4L, 5L, 1L, 2L, 3L, 6L, 4L, 5L, 1L, 2L, 3L, 6L, 4L, 5L, 1L, 2L, 3L, 6L, 4L, 5L, 1L, 2L, 3L, 6L, 4L, 5L, 1L, 2L, 3L, 6L, 4L, 5L, 1L, 2L, 3L, 6L, 4L, 5L, 1L, 2L), .Label = c("Grand Rapids", "Duluth", "University Farm", "Morris", "Crookston", "Waseca"), class = "factor")),
9+
levels = c("Svansota", "No. 462", "Manchuria", "No. 475", "Velvet", "Peatland", "Glabron", "No. 457", "Wisconsin No. 38", "Trebi"), class = "factor"),
10+
year = structure(c(2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 2L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L, 1L), levels = c("1932", "1931"), class = "factor"),
11+
site = structure(c(3L, 6L, 4L, 5L, 1L, 2L, 3L, 6L, 4L, 5L, 1L, 2L, 3L, 6L, 4L, 5L, 1L, 2L, 3L, 6L, 4L, 5L, 1L, 2L, 3L, 6L, 4L, 5L, 1L, 2L, 3L, 6L, 4L, 5L, 1L, 2L, 3L, 6L, 4L, 5L, 1L, 2L, 3L, 6L, 4L, 5L, 1L, 2L, 3L, 6L, 4L, 5L, 1L, 2L, 3L, 6L, 4L, 5L, 1L, 2L, 3L, 6L, 4L, 5L, 1L, 2L, 3L, 6L, 4L, 5L, 1L, 2L, 3L, 6L, 4L, 5L, 1L, 2L, 3L, 6L, 4L, 5L, 1L, 2L, 3L, 6L, 4L, 5L, 1L, 2L, 3L, 6L, 4L, 5L, 1L, 2L, 3L, 6L, 4L, 5L, 1L, 2L, 3L, 6L, 4L, 5L, 1L, 2L, 3L, 6L, 4L, 5L, 1L, 2L, 3L, 6L, 4L, 5L, 1L, 2L), levels = c("Grand Rapids", "Duluth", "University Farm", "Morris", "Crookston", "Waseca"), class = "factor")),
1212
row.names = c("1", "2", "3", "4", "5", "6", "7", "8", "9", "10", "11", "12", "13", "14", "15", "16", "17", "18", "19", "20", "21", "22", "23", "24", "25", "26", "27", "28", "29", "30", "31", "32", "33", "34", "35", "36", "37", "38", "39", "40", "41", "42", "43", "44", "45", "46", "47", "48", "49", "50", "51", "52", "53", "54", "55", "56", "57", "58", "59", "60", "61", "62", "63", "64", "65", "66", "67", "68", "69", "70", "71", "72", "73", "74", "75", "76", "77", "78", "79", "80", "81", "82", "83", "84", "85", "86", "87", "88", "89", "90", "91", "92", "93", "94", "95", "96", "97", "98", "99", "100", "101", "102", "103", "104", "105", "106", "107", "108", "109", "110", "111", "112", "113", "114", "115", "116", "117", "118", "119", "120"),
1313
class = "data.frame"
1414
)

tests/testthat/test-ggplot-legend.R

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11

2-
32
expect_traces <- function(gg, n.traces, name){
43
stopifnot(is.numeric(n.traces))
54
L <- expect_doppelganger_built(gg, paste0("legend-", name))
@@ -28,9 +27,11 @@ test_that("Discrete colour and shape get merged into one legend", {
2827
expect_identical(
2928
nms, paste0("(", d$vs, ",", d$cyl, ")")
3029
)
30+
# legend title segments must appear in the same order as the aesthetics
31+
# within each entry's name (e.g. "(vs_value,cyl_value)" above), since both
32+
# describe the same combined legend
3133
legend_title <- info$layout$legend$title$text
32-
expect_match(legend_title, "^factor\\(vs\\)")
33-
expect_match(legend_title, "factor\\(cyl\\)$")
34+
expect_identical(legend_title, "factor(vs)<br />factor(cyl)")
3435
})
3536

3637

@@ -89,8 +90,11 @@ test_that("legend can be manipulated via guides(aes = guide_xxx())", {
8990
theme(axis.text.x = element_text(angle = 90))
9091

9192
info <- expect_doppelganger_built(gg, "respect-guides")
92-
93+
9394
expect_equivalent(sum(sapply(info$data, "[[", "showlegend")), 4)
95+
# explicit guide `order` must be respected, even though it disagrees with
96+
# the shape-before-color order the aesthetics were declared in above
97+
expect_identical(info$layout$legend$title$text, "Period<br />")
9498
})
9599

96100
p <- ggplot(mtcars, aes(x = mpg, y = wt, color = factor(vs))) +

tests/testthat/test-group2NA.R

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ test_that("group2NA() yields the correct result", {
6363
id = c(27L, NA, 3L, 8L, 9L, 18L, 19L, 20L, 21L, 26L, 28L, 32L, 1L, 2L, 30L,
6464
NA, 4L, 6L, 10L, 11L, 5L, 7L, 12L, 13L, 14L, 15L, 16L, 17L, 22L,
6565
23L, 24L, 25L, 29L, 31L)),
66-
.Names = c("mpg", "cyl", "disp", "hp", "drat", "wt", "qsec", "vs", "am", "gear", "carb", "id"),
66+
names = c("mpg", "cyl", "disp", "hp", "drat", "wt", "qsec", "vs", "am", "gear", "carb", "id"),
6767
class = "data.frame",
6868
row.names = c(NA, 34L))
6969

0 commit comments

Comments
 (0)