Change function signature of Geom$draw_layer()

[clauswilke]

Several distinct issues have arisen where Geom$draw_layer() needs more info than it is currently given. Specifically, it looks like it would be good to hand it a new object called layer_params (#3116 (comment)) as well as the theme (#2749 (comment)). Is there anything else that we should add, while we're add it?

The current signature is as follows:

ggplot2/R/geom-.r

Line 74 in 83c8123

draw_layer = function(self, data, params, layout, coord) {

I propose the following signature:

  draw_layer = function(self, data, params, layout, coord, theme, layer_params, ...) 

The ... future-proofs this in case we are still missing something.

@paleolimbot @thomasp85 Are you guys still on board with layer_params as discussed here: #3116 (comment) or has your thinking evolved since then?

We could also now make the function signature

  draw_layer = function(self, data, params, layout, coord, theme, ...) 

and leave the layer_params issue for a future date.

[clauswilke]

I have re-read all the comments from #3116 (comment) down and it looks to me that we never got to a better solution than adding layer_params, though it was still unclear what exactly should go into that object.

[paleolimbot]

I did a PR that implemented layer_params (#3170) but closed it this summer because I thought it was an ugly solution. Hadley also noted this in the original thread (#3116 (comment)).

I think that using the Layout as a home for all the things that layers need access to is the right way to go, but I seem to remember that idea was shot down at some point. It would require few changes...the Layout gains a theme and/or scales field, and then Stats and Geoms can access them at build/draw time. Again, I think I'm in the minority on that one, but it feels so much better to me than a layer_params object with an uncertain definition.

[clauswilke]

I guess we need an executive decision.

@thomasp85 Can you make the call? And should the theme be part of the layout or be handled separately?

[paleolimbot]

(My unqualified two cents is that something like theme_get("key") should be available globally, and that all the scales should be in the Layout or panel_params)

[clauswilke]

My unqualified two cents is that something like theme_get("key") should be available globally

That just makes me deeply uncomfortable. I'd much rather stick the theme into the layout object. I'm fine with sticking scales into the layout also if that looks like the best compromise.

[thomasp85]

Sorry, I need to think some more about this - my head has been with the release for a while. The only thing I can say with a fair amount of certainty is the the Layout object should not be handed off. This is an internal structure and we will regret the day we ever give extension developers access to it

[clauswilke]

Thomas, I'm not sure I understand. The Layout object is already accessible to extension developers, via draw_layer(). We would just be adding more information to it.

[thomasp85]

So it is... but both compute_layer and draw_layer are both not advertised as extension points and I have no problem burning people that implements something weird there using the layout...

The layout is decidedly not a user facing object and should not be passed around. It is neither documented nor intended to be stable. We already have so much development constraint because developers rely on undocumented objects

[teunbrand]

If there is anything the geom drawing bit may need to add in terms of parameters, you can just funnel it through the Layer$computed_geom_params which will become the params argument. We don't need to change the signature for Geom$draw_layer() for this. If there is a good reason for insisting on this approach (better than 'we haven't come up with anything else'), we can keep this issue open. Otherwise, I think this feature request distracts from exploring other solutions to #3116 and it should be closed.