Skip to content

Conversation

@lgaborini
Copy link

A tiny PR that builds upon #5967.

Now the names can be used by get_layer_data() and get_layer_grob() in place of the layer index.
Only scalars are accepted, just like the current behavior.

If a name is supplied, it is used, but the layer must exist.
If both the name and the index are supplied (either explicitly or as a default argument), the name takes precedence.

The names must be unique (this is currently enforced by #5967 — should revisit whether this is no longer the case).

devtools::load_all(".")
p <- ggplot(head(mpg), aes(displ, hwy)) +
  geom_point(name = "foo") +
  geom_line(name = "bar")

names(p$layers)
#> [1] "foo" "bar"

get_layer_data(p, i = 2L)
#>     x  y PANEL group flipped_aes colour linewidth linetype alpha
#> 1 1.8 29     1    -1       FALSE  black       0.5        1    NA
#> 2 1.8 29     1    -1       FALSE  black       0.5        1    NA
#> 3 2.0 31     1    -1       FALSE  black       0.5        1    NA
#> 4 2.0 30     1    -1       FALSE  black       0.5        1    NA
#> 5 2.8 26     1    -1       FALSE  black       0.5        1    NA
#> 6 2.8 26     1    -1       FALSE  black       0.5        1    NA
get_layer_data(p, name = "bar")
#>     x  y PANEL group flipped_aes colour linewidth linetype alpha
#> 1 1.8 29     1    -1       FALSE  black       0.5        1    NA
#> 2 1.8 29     1    -1       FALSE  black       0.5        1    NA
#> 3 2.0 31     1    -1       FALSE  black       0.5        1    NA
#> 4 2.0 30     1    -1       FALSE  black       0.5        1    NA
#> 5 2.8 26     1    -1       FALSE  black       0.5        1    NA
#> 6 2.8 26     1    -1       FALSE  black       0.5        1    NA

# silently use the name
get_layer_data(p, i = 1L, name = "bar")
#>     x  y PANEL group flipped_aes colour linewidth linetype alpha
#> 1 1.8 29     1    -1       FALSE  black       0.5        1    NA
#> 2 1.8 29     1    -1       FALSE  black       0.5        1    NA
#> 3 2.0 31     1    -1       FALSE  black       0.5        1    NA
#> 4 2.0 30     1    -1       FALSE  black       0.5        1    NA
#> 5 2.8 26     1    -1       FALSE  black       0.5        1    NA
#> 6 2.8 26     1    -1       FALSE  black       0.5        1    NA

# no match by name: error

get_layer_data(p, name = "none")
#> Error in `get_layer_data()`:
#> ! `name` must be one of "foo" or "bar", not "none".

# no match by subscript: error (uglier)

get_layer_data(p, i = 4L)
#> Error in ggplot_build(plot)@data[[idx]]: subscript out of bounds

# Similarly for get_layer_grob()

get_layer_grob(p, i = 2L)
#> [[1]]
#> polyline[GRID.polyline.1]
get_layer_grob(p, name = "bar")
#> [[1]]
#> polyline[GRID.polyline.2]

Perhaps it's better to make i and name mutually exclusive, like id and number for recipes::tidy.recipe()?

Copy link
Collaborator

@teunbrand teunbrand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR! I agree that being able to use the name to get layer data/grob would be useful. I think it'd make more sense to merge to name approach in this PR with i. Using vctrs::vec_as_location2() to construct the index would handle both the integer and named cases.

@lgaborini
Copy link
Author

lgaborini commented Nov 4, 2025

Thanks, I didn't know that helper existed!

Now the parameter i is a bit inconsistent with get_panel_scales() since it indexes into panels instead of layers.
Probably not an issue until the panels get names too, or layers become panel-dependent.

Copy link
Collaborator

@teunbrand teunbrand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for these updates, I like the direction of the PR!

Now the parameter i is a bit inconsistent with get_panel_scales() since it indexes into panels instead of layers.

I agree that it is unlikely to be an issue.

Comment on lines +238 to +244
# name falls back to index
expect_snapshot_error(
get_layer_data(p, i ="none")
)
expect_snapshot_error(
get_layer_data(p, i = 4L)
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is good that these errors are thrown, but I don't think ggplot2 needs to test for them. They are outside ggplot2's control as they are thrown by the vctrs package. If the message changes, that doesn't imply that ggplot2 needs to change.

Comment on lines +257 to +262
expect_snapshot_error(
get_layer_grob(p, i ="none")
)
expect_snapshot_error(
get_layer_grob(p, i = 4L)
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise, I think we don't need these tests.

Comment on lines +233 to +236
expect_identical(
get_layer_data(p, i = "bar"),
get_layer_data(p, i = 2L)
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the intent of this test is good. However, I think the test would also pass if the index was mismatched because the layer data of both layers would be the same.

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