Share context code via default trait implementations. #974
Conversation
This is motivated by a desire to avoid some confusion with a future patch that adds a RootState struct to unify some of the other stuff shared between various contexts.
I think it makes sense for show_context_menu to only be available in EventCtx, but for the window menu I can imagine needing it in lifecycle and update (and I need it in update in runebender)
This adds a new type, RootState that holds onto all of the app/window-scoped state that is shared between all non-paint contexts.
xStrom
added
S-needs-review
|
This is definitely another approach. One of my main motivations for using a macro, despite the downsides, is to make the docs clear; each context's docs will list all the available methods in one place; it also avoids introducing too many new types. I do think we will want some traits, though, so maybe it makes sense to do it all this way? not a clear call. |
cmyr
force-pushed
the
common-context-state
branch
from
f1d1650
to
6713aa7
Compare
cmyr
force-pushed
the
common-context-state
branch
from
6713aa7
to
74fa6a9
Compare
|
Another thing I'd been thinking about: even if we have traits that we use for different context behaviours, my thinking was that those would just be additive; all the methods would still be defined on the contexts, and the methods on the traits would just forward to those, so that you wouldn't even really need to know about the traits until you needed them; they'd be like a bonus thing. Again, this is just how I'd been thinking about things.. |
cmyr
force-pushed
the
common-context-state
branch
2 times, most recently
from
8fc5863
to
ce8a1b2
Compare
|
One point in favor of traits over macros is that it can enable code reuse in druid users. This is something that actually came up a little while ago, although I don't remember the exact details: I have some code in a widget that wants to check a condition and possibly submit a command, and I want to run this code in both |
|
@jneem yeah that same issue is in the |
|
fwiw I did not close this intentionally. I definitely agree that traits would be nice as a way to describe some common behaviours, but I'm not sure we want them to be the basis of how contexts are defined; but really the big thing I'm thinking about are docs, and making it easy to see what methods are available on what contexts. |
This is an alternative approach to #972 which aims to reduce copied code. Here, instead of macros, I implement it using traits.
Code inside of macros can't be formatted using
cargo fmt. This means that over time the code formatting will be all over the place and will make it harder to read the code. Alternatively we would have to spend time manually checking formatting during code reviews, which seems almost worse.I've also had some really annoying debugging experiences with macros due to compiler errors and line numbers not being meaningful. I'm not well versed in Rust macros so I can't say for sure if this will apply here, but I can say that I've had the issue with other macros.
Thus I propose we solve the issue without using macros. One way I can think of is using default trait implementations, whcih I did in this PR. That solves the problems that macros introduce. Traits also allow for more powerful function signatures in applications where the function is willing to take any context that implements a specific method.
The trait approach may have a bit of downside. The documentation for the context structs won't contain trait method docs. I've done a bit of googling but not thorough. Other people are having the same issue, but it's unclear if it's already possible to inline the docs to the struct, or if that's something that might become possible in a future version of
cargo doc.PS. This PR still needs some cleanup, but I wanted to get the general idea out there as it is working already.