Adding Layout slot to ggplot object

[jtlandis]

Creates a pull request for issue #5077

cleaner pull request for #5575

[teunbrand]

Thanks for the PR! I've given this some thought and I've come to agree that it would be useful for developers to implement their own Layout strategies. However, I've come to doubt that exporting create_layout() is the best approach for this, and the reasons are as follows:

• If create_layout() is exported as an S3 generic, it is exposed to users as well as developers, so you'd have to go through the usual hassle of input validation (e.g. making a default method that raises errors) and output validation (making sure the layout that is returned is valid).
• Moreover, it can create a weird web of interactions if ggplot_build.my_sublass does not use, or uses a homebrewed version of, create_layout(), or does not know how to deal wth a custom layout.
• Would it not be easier, for ggplot2 itself and extension developers, to have the layout as a slot in the plot object that defaults to the default Layout ggproto object? That way you could use the ggplot_build.my_class method to assign a custom layout and you could call NextMethod() to not bother reimplementing the whole method.

[jtlandis]

Lets game it out a bit, so instead of exporting create_layout as a S3 generic, we have the ggplot2 hold the exported Layout ggproto object.

Lets then assume the create_layout stays internal but looks something like

create_layout <- function(layer = Layer, facet, coord) {
  ggproto(NULL, layer, facet = facet, coord = coord)
}

and in practice, it would be called like so in ggplot_build.ggplot

ggplot_build.ggplot <- function(plot){
  ...
  layout <- create_layout(plot$layout, plot$facet, plot$coord)
  ...
}

and then developers who may want to extend the layout may make their own ggplot_build generic like so

ggplot_build.my_class <- function(plot){
   plot$layout <- ggproto("SubClassLayout", plot$layout, ...)
   NextMethod()
  }

I could see this working. I think this would be more reliable than create_layout as an S3 generic. So long as developers return the layout back to the ggplot object it should make multiple subclassing work as well.

I do wonder what effect this may have on packages that already have their own ggplot_build method. gganimate is the main one that comes to mind as an example but I'm unsure of how many packages this might affect.

[teunbrand]

I do wonder what effect this may have on packages that already have their own ggplot_build method

I'm sure we can make a backward compatible change that will keep the packages running even if they haven't updated their build methods, though they might have some compatibility issues with extension packages that do implement their own layouts. I think keeping the layout argument optional might preserve backward compatibility best.

create_layout <- function(facet, coord, layout = NULL) {
  ggproto(NULL, layout %||% Layout, facet = facet, coord = coord)
}

Then the rest of what you layed out seems to pan out as it should. I did a quick search and it seems that there are ~5-ish packages that have redefined the create_layout method. The compatibility issues then seem managable.

[jtlandis]

Shall I close this PR and open a new one? or just make the commits here?

[teunbrand]

Whatever is most convenient to you, sometimes a fix requires some iteration to get it right :)

[jtlandis]

New updates:

• create_layout is not exported and arguments changed to (facet, coord, layout = NULL)
• Added a check for the layout prior to passing to ggproto to ensure we inherit from Layout
• testthat tests are reverted back to their original form.
• ggplot.default has been changed to include layout = Layout within its structure.

[teunbrand]

Aside from the one suggestion I think this makes more sense now. I'd still would want @thomasp85's eyes on this as well, just to make sure I'm not out of my depth here. Have you also checked that this PR is sufficient for your purposes?

[jtlandis]

This PR should be sufficient for my purposes with ggside and producing a long needed update. Thank you for reviewing!

[teunbrand]

@thomasp85 A brief summary of this PR is that we allow extensions to implement a custom Layout class embedded in the ggplot object. The create_layout() function then uses this layout over the default Layout, but is backwards compatible when no Layout class is embedded in the ggplot object. Do you think we might include it in the milestone?

[thomasp85]

LGTM

[teunbrand]

@jtlandis Can I ask you to add a news bullet and rename this PR to something that reflects the contents more closely?

[jtlandis]

@teunbrand With your approval, I also made an edit to the ggplot object in which we wrap Layout in a ggproto call. The motivation to this is to keep the base Layout object safe from anyone who may modify $layout on the ggplot2 object out of curiosity

[teunbrand]

Yes, good call. In theory anyone can mess around with the exported ggproto classes, including Layout, on a whim, but this is less risky. Could you still edit the title of this PR to something like 'Add Layout slot to ggplot object` (or whatever phrasing you like)? That would make the commit message more descriptive for the merge.

[teunbrand]

Thanks you for the contribution!