Convert guides to ggproto#4879
Conversation
Sync branches
Merge branch 'main' of https://github.com/tidyverse/ggplot2 into tidyverse-main2 # Conflicts: # R/guides-.r # R/guides-axis.r
|
Hi @thomasp85, do you think you want to course-correct this WIP-PR at some point (whenever is convenient for you, I know these are busy times with the conference and all) or should I continue with the next guide? I was thinking about |
|
I'll take a look next week as conference gets behind me. Thanks for reminding me |
thomasp85
left a comment
There was a problem hiding this comment.
I would like to challenge you to think about a way where we can make the guide implementations themselves stateless. Not for the purity of heart, but more as a way to make the implementation more strict and understandable. Making it stateless requires you to think about the input and output of each function call more clearly and I think this will benefit us going forward. If you'd like to discuss this in more detail let me know and I'll set up a meeting
|
Thanks for taking a look, Thomas!
My intuition here would then be to tinker with the |
|
Yes exactly. Having a clear expected output of a method makes it so much easier to build up intuition for what it is supposed to do. You may also think about having a |
Merge branch 'main' of https://github.com/tidyverse/ggplot2 into tidyverse-main # Conflicts: # R/coord-cartesian-.r # R/guides-axis.r # man/ggplot2-ggproto.Rd
|
Maybe instead of testing we should make sure we have a very clear understanding of which values in the guides and guidelist are susceptible to change during a rendering pass, and then make sure that changes in these values does not leak into a new rendering pass... |
|
This is the sort of situation I am eager to avoid #4475 |
| # `guides` is the only initially mutable field. | ||
| # It gets populated as a user adds `+ guides(...)` to a plot by the | ||
| # `Guides$add()` method. | ||
| guides = list(), |
There was a problem hiding this comment.
This should be the only mutable field up to the point of rendering
| # Setup routine for resolving and validating guides based on paired scales. | ||
| # | ||
| # The output of the setup is a child `Guides` class with two additional | ||
| # mutable fields, both of which are parallel to the child's `Guides$guides` | ||
| # field. | ||
| # | ||
| # 1. The child's `Guides$params` manages all parameters of a guide that may | ||
| # need to be updated during subsequent steps. This ensures that we never need | ||
| # to update the `Guide` itself and risk reference class shenanigans. | ||
| # | ||
| # 2. The child's `Guides$aesthetics` holds the aesthetic name of the scale | ||
| # that spawned the guide. The `Coord`'s own build methods need this to | ||
| # correctly pick the primary and secondary guides. |
There was a problem hiding this comment.
I tried to clarify here a bit that two other fields are also mutable
| ggproto( | ||
| NULL, self, | ||
| guides = new_guides, | ||
| # Extract the guide's params to manage separately | ||
| params = lapply(new_guides, `[[`, "params"), | ||
| aesthetics = aesthetics | ||
| ) |
There was a problem hiding this comment.
Here are these mutable fields added to a child ggproto object and the original guide's parameters are copied. If the user has provided a guide directly, e.g. x = guide_axis() instead of x = "axis", the Guides class has never touched the guide_axis() other than to read out available_aes and such.
| ggproto( | ||
| NULL, super, | ||
| params = params, | ||
| elements = elems, | ||
| available_aes = available_aes | ||
| ) |
There was a problem hiding this comment.
The guide constructor never updates the class with a user's input. It takes the user's input and fuses it to the guide's defaults, which lives on in a child instead of modifying the parent.
|
I've reordered the |
thomasp85
left a comment
There was a problem hiding this comment.
Okay let's do this :-)
But maybe wait until Monday with merging just to avoid the "never push to prod on a friday" situation :-)
This comment was marked as resolved.
This comment was marked as resolved.
|
Good call to wait after the weekend, I just caught a bug that I should've caught before :) |
|
I don't think GitHub picks up fix keywords in comments so you may want to add them to the first message instead |
This is a WIP PR addressing #3329.
Since the aim is a substantial overhaul of the guide system, it would be good to gather some feedback along the way. I was hoping we could align on some vision early on, which would make subsequent steps easier. For now, I made a first step by making a parent
Guideclass andGuideAxisto get the ball rolling. This step is small enough that it wouldn't matter if we decide on a totally different direction.A few comments from my side that aren't in the code itself:
GuideAxisis visually identical to the previous axis guide.GuideNoneto get this to work.'Using non-position guides for position scales results in an informative error', which should be resolved whenguide_legend()is also converted.The type of feedback I was hoping to get is (not limited to) the following:
draw-method (previously theguide_gengrob()method), I just pass around the entireparamslist to every function that might use it. I don't know whether it is a bad thing, but if it might be useful if there was some elegant way to pass just the parameters it needs, without resorting to very verbose code.Some magic github incantations:
Fix #4879, Fix #4364, Fix #4930, Fix #2728, Fix #4650, Fix #4958, Fix #5152