Introduced a builder pattern interface (#275) #809
Conversation
There was a problem hiding this comment.
I'm pretty impressed with this PR. It's not a lot of lines of code for the functionality it provides. I've left some comments on specific parts of the code, but in addition to that I have a couple of general points to make:
-
I feel like you are potentially mixing too concepts here:
- A
StyleBuilder(helper methods to setStylefields) - A
TreeBuilder(the stuff around children)
This isn't necessarily a problem. Just something to bare in mind. Potentially the setting methods could be added directly to
StyleandStyleBuildercould be renamed toTreeBuilder? - A
-
I think shorthand methods are an important part of "builder" functionality. So I think the
.margin()method ought to acceptLengthPercentageand set all four margins to the single specified value (like margin shorthand in CSS) rather than accepting aRect<LengthPercentage>. There should then be separatemargin_left,margin_top, etc methods. And probably alsomargin_x(ormargin_horizontal) and y/vertical too. -
While I'm impressed with the macro, I wonder if it might be worth switching to a code generation solution (a rust script that generates rust code using a string templating library (e.g.
tera)). The advantage of that would be that the same "API definition" could potentially be shared by this builder and future FFI crates which will require very similar builders (See draft C API and wasm API PRs).What do you think? We could also potentially land the macro-based solution and then switch to codegen later.
src/style/builder.rs
Outdated
| #[derive(Debug, Clone, Default)] | ||
| pub struct NodeIdRef(Rc<Cell<Option<NodeId>>>); | ||
|
|
||
| impl NodeIdRef { | ||
| /// Create an empty [`NodeIdRef`]. | ||
| pub fn new() -> Self { | ||
| Self(Rc::new(Cell::new(None))) | ||
| } | ||
|
|
||
| /// Set the [`NodeId`]. | ||
| fn set(&self, node_id: NodeId) { | ||
| self.0.set(Some(node_id)); | ||
| } | ||
|
|
||
| /// Get a copy of the inner [`NodeId`], if any is present. | ||
| pub fn get(&self) -> Option<NodeId> { | ||
| self.0.get() | ||
| } | ||
| } |
There was a problem hiding this comment.
I might be missing something, but I think this would work with a simple mutable reference instead of the Rc<Cell<>>?
There was a problem hiding this comment.
We need Rc<Cell<_>> here to provide multiple ownership via the clone method and interior mutability. This is required because the call site needs to own the NodeIdRef, but also needs to be able to pass it into the builder, which then modifies the content of the Cell<_>. Let me know if that makes sense, happy to implement this with a mutable reference if you have a specific way this can be achieved!
There was a problem hiding this comment.
@simongarnier this appears to work (compiles at least) 32d90c2 (you could also get rid of the NodeIdRef type and use &mut Option<NodeId> directly if you wanted).
src/style/builder.rs
Outdated
| $( | ||
| $(#[cfg($($cfg)+)])? | ||
| $field: Option<$type>, | ||
| )* |
There was a problem hiding this comment.
Rather than having Option<T> fields for all Style fields, why not just have a single style: Style field here? It could be initialised with Style::default() and then written to directly.
There was a problem hiding this comment.
Agreed, doing this also removes the need for having a build_style method.
simongarnier
force-pushed
the
builder-pattern-interface
branch
from
e4325c6 to
eee08c1
Compare
The first approach I took was to create a I do think
I was wondering about which shorthand should be implement, maybe I can get some inspiration from what CSS offers in terms of shorthand. I'll do a pass to add the margin ones and see what I can find on the CSS side.
I think code generation could be use instead of a macro for this use case, however I don't know how that works on the documentation end. Code generation can probably be done before the docs are generated. Like you said, I don't think this is a one way door and this can be switched to code generation when the need for it arises, i.e. to generate bindings from a single API definition. Let me know what you think, I'll focus on shorthand methods for now. Thanks for the review by the way! |
simongarnier
force-pushed
the
builder-pattern-interface
branch
from
3a19e75 to
9439948
Compare
Objective
Fixes #275 by providing a builder pattern interface to create nested styles which can then be "materialize" (made concrete? not sure of the lingo to use here) into tree nodes by passing a
TaffyTreeinto thebuildmethod of the builder.Might be good to update the RELEASE.md since this adds a new API to interact with Taffy. Happy to add this to this PR if required!
Context
gen_builder!, taking suggestions for a better name) to generate the builder. This macro takes a name for the resulting struct and a list ofStylefields that should be available on the builder. Each field can also be marked with acfgentry, useful for conditional compilation based on features.widthandheightand 2 constructors,columnandrowthat are presets for the appropriateFlexDirection.StyleBuildershould take in references to otherStyleBuilderas children. This avoids having to build children style in advance and has the advantage of being consistent across root and child styles.Feedback wanted
NodeIds back to the caller for child nodes. In the end, I landed on taking inspiration from theNodeRefsolution mentioned in Provide a builder pattern interface to the construction layout tree #275. The main difference is that I'm using aCell<_>instead of aRefCell<_>to hold theNodeId, sinceNodeIdimplementcopy. I'm looking for feedback here regarding how this is implemented and also if another pattern for result feedback would be desired over this.