Skip to content

Add and cleanup new leaf-nodes methods #181

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
alice-i-cecile merged 4 commits into from
Jun 19, 2022
Merged

Add and cleanup new leaf-nodes methods #181

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d461ab7
Rename and add method
Weibye Jun 19, 2022
4805265
formatting
Weibye Jun 19, 2022
2dfce87
Migrate test to use new method
Weibye Jun 19, 2022
90efcc9
Rename arguments `style` -> `layout`
Weibye Jun 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions RELEASES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,11 @@
### 0.2.0 Added

- Added `taffy::error::InvalidChild` Error type
- `taffy::node::Taffy.new_leaf()` which allows the creation of new leaf-nodes without having to supply a measure function

### 0.2.0 Changed

- renamed `taffy::node::Taffy.new_leaf()` -> `taffy::node::Taffy.new_leaf_with_measure()`
- removed the public `Number` type; a more idiomatic `Option<f32>` is used instead
- the associated public `MinMax` and `OrElse` traits have also been removed; these should never have been public
- `Sprawl::remove` now returns a `Result<usize, Error>`, to indicate if the operation was sucessful, and if it was, which ID was invalidated.
Expand Down
28 changes: 19 additions & 9 deletions src/forest.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,9 @@ pub(crate) struct NodeData {
}

impl NodeData {
/// Create the data for a new leaf node
/// Create the data for a new node with a [`MeasureFunc`]
#[must_use]
fn new_leaf(style: FlexboxLayout, measure: MeasureFunc) -> Self {
fn new_with_measure(style: FlexboxLayout, measure: MeasureFunc) -> Self {
Self {
style,
measure: Some(measure),
Expand All @@ -40,7 +40,6 @@ impl NodeData {
}

/// Create the data for a new node
// TODO: why is this different from new_leaf?
#[must_use]
fn new(style: FlexboxLayout) -> Self {
Self {
Expand Down Expand Up @@ -89,22 +88,33 @@ impl Forest {
}
}

/// Adds a new unattached leaf node to the forest, and returns the [`NodeId`] of the new node
pub(crate) fn new_leaf(&mut self, style: FlexboxLayout, measure: MeasureFunc) -> NodeId {
/// Creates and adds a new unattached leaf node to the forest, and returns the [`NodeId`] of the new node
pub(crate) fn new_leaf(&mut self, layout: FlexboxLayout) -> NodeId {
let id = self.nodes.len();
self.nodes.push(NodeData::new_leaf(style, measure));
self.nodes.push(NodeData::new(layout));
self.children.push(new_vec_with_capacity(0));
self.parents.push(new_vec_with_capacity(1));
id
}

/// Adds a new unparented node to the forest with the associated children attached, and returns the [`NodeId`] of the new node
pub(crate) fn new_with_children(&mut self, style: FlexboxLayout, children: ChildrenVec<NodeId>) -> NodeId {
/// Creates and adds a new unattached leaf node to the forest, and returns the [`NodeId`] of the new node
///
/// The node must have a [`MeasureFunc`] supplied
pub(crate) fn new_leaf_with_measure(&mut self, layout: FlexboxLayout, measure: MeasureFunc) -> NodeId {
let id = self.nodes.len();
self.nodes.push(NodeData::new_with_measure(layout, measure));
self.children.push(new_vec_with_capacity(0));
self.parents.push(new_vec_with_capacity(1));
id
}

/// Creates and adds a new unparented node to the forest with the associated children attached, and returns the [`NodeId`] of the new node
pub(crate) fn new_with_children(&mut self, layout: FlexboxLayout, children: ChildrenVec<NodeId>) -> NodeId {
let id = self.nodes.len();
for child in &children {
self.parents[*child].push(id);
}
self.nodes.push(NodeData::new(style));
self.nodes.push(NodeData::new(layout));
self.children.push(children);
self.parents.push(new_vec_with_capacity(1));
id
Expand Down
24 changes: 18 additions & 6 deletions src/node.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,22 +100,34 @@ impl Taffy {
}
}

/// Adds a new leaf node, which does not have any children
pub fn new_leaf(&mut self, style: FlexboxLayout, measure: MeasureFunc) -> Result<Node, error::InvalidNode> {
/// Creates and adds a new leaf node
pub fn new_leaf(&mut self, layout: FlexboxLayout) -> Result<Node, error::InvalidNode> {
let node = self.allocate_node();
let id = self.forest.new_leaf(style, measure);
let id = self.forest.new_leaf(layout);
self.add_node(node, id);
Ok(node)
}

/// Adds a new node, which may have any number of `children`
pub fn new_with_children(&mut self, style: FlexboxLayout, children: &[Node]) -> Result<Node, error::InvalidNode> {
/// Creates and adds a new leaf node with a supplied [`MeasureFunc`]
pub fn new_leaf_with_measure(
&mut self,
layout: FlexboxLayout,
measure: MeasureFunc,
) -> Result<Node, error::InvalidNode> {
let node = self.allocate_node();
let id = self.forest.new_leaf_with_measure(layout, measure);
self.add_node(node, id);
Ok(node)
}

/// Creates and adds a new node, which may have any number of `children`
pub fn new_with_children(&mut self, layout: FlexboxLayout, children: &[Node]) -> Result<Node, error::InvalidNode> {
let node = self.allocate_node();
let children = children
.iter()
.map(|child| self.find_node(*child))
.collect::<Result<ChildrenVec<_>, error::InvalidNode>>()?;
let id = self.forest.new_with_children(style, children);
let id = self.forest.new_with_children(layout, children);
self.add_node(node, id);
Ok(node)
}
Expand Down
37 changes: 16 additions & 21 deletions tests/measure.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ mod measure {
fn measure_root() {
let mut taffy = taffy::node::Taffy::new();
let node = taffy
.new_leaf(
.new_leaf_with_measure(
taffy::style::FlexboxLayout { ..Default::default() },
MeasureFunc::Raw(|constraint| taffy::geometry::Size {
width: constraint.width.unwrap_or(100.0),
Expand All @@ -26,7 +26,7 @@ mod measure {
let mut taffy = taffy::node::Taffy::new();

let child = taffy
.new_leaf(
.new_leaf_with_measure(
taffy::style::FlexboxLayout { ..Default::default() },
MeasureFunc::Raw(|constraint| taffy::geometry::Size {
width: constraint.width.unwrap_or(100.0),
Expand All @@ -49,7 +49,7 @@ mod measure {
fn measure_child_constraint() {
let mut taffy = taffy::node::Taffy::new();
let child = taffy
.new_leaf(
.new_leaf_with_measure(
taffy::style::FlexboxLayout { ..Default::default() },
MeasureFunc::Raw(|constraint| taffy::geometry::Size {
width: constraint.width.unwrap_or(100.0),
Expand Down Expand Up @@ -81,7 +81,7 @@ mod measure {
fn measure_child_constraint_padding_parent() {
let mut taffy = taffy::node::Taffy::new();
let child = taffy
.new_leaf(
.new_leaf_with_measure(
taffy::style::FlexboxLayout { ..Default::default() },
MeasureFunc::Raw(|constraint| taffy::geometry::Size {
width: constraint.width.unwrap_or(100.0),
Expand Down Expand Up @@ -131,7 +131,7 @@ mod measure {
.unwrap();

let child1 = taffy
.new_leaf(
.new_leaf_with_measure(
taffy::style::FlexboxLayout { flex_grow: 1.0, ..Default::default() },
MeasureFunc::Raw(|constraint| taffy::geometry::Size {
width: constraint.width.unwrap_or(10.0),
Expand Down Expand Up @@ -174,7 +174,7 @@ mod measure {
.unwrap();

let child1 = taffy
.new_leaf(
.new_leaf_with_measure(
taffy::style::FlexboxLayout { ..Default::default() },
MeasureFunc::Raw(|constraint| taffy::geometry::Size {
width: constraint.width.unwrap_or(100.0),
Expand Down Expand Up @@ -216,7 +216,7 @@ mod measure {
.unwrap();

let child1 = taffy
.new_leaf(
.new_leaf_with_measure(
taffy::style::FlexboxLayout { flex_grow: 1.0, ..Default::default() },
MeasureFunc::Raw(|constraint| {
let width = constraint.width.unwrap_or(10.0);
Expand Down Expand Up @@ -262,7 +262,7 @@ mod measure {
.unwrap();

let child1 = taffy
.new_leaf(
.new_leaf_with_measure(
taffy::style::FlexboxLayout { ..Default::default() },
MeasureFunc::Raw(|constraint| {
let width = constraint.width.unwrap_or(100.0);
Expand Down Expand Up @@ -294,7 +294,7 @@ mod measure {
let mut taffy = taffy::node::Taffy::new();

let child = taffy
.new_leaf(
.new_leaf_with_measure(
taffy::style::FlexboxLayout { ..Default::default() },
MeasureFunc::Raw(|constraint| {
let height = constraint.height.unwrap_or(50.0);
Expand Down Expand Up @@ -327,7 +327,7 @@ mod measure {
fn width_overrides_measure() {
let mut taffy = taffy::node::Taffy::new();
let child = taffy
.new_leaf(
.new_leaf_with_measure(
taffy::style::FlexboxLayout {
size: taffy::geometry::Size { width: taffy::style::Dimension::Points(50.0), ..Default::default() },
..Default::default()
Expand All @@ -350,7 +350,7 @@ mod measure {
fn height_overrides_measure() {
let mut taffy = taffy::node::Taffy::new();
let child = taffy
.new_leaf(
.new_leaf_with_measure(
taffy::style::FlexboxLayout {
size: taffy::geometry::Size { height: taffy::style::Dimension::Points(50.0), ..Default::default() },
..Default::default()
Expand Down Expand Up @@ -384,7 +384,7 @@ mod measure {
.unwrap();

let child1 = taffy
.new_leaf(
.new_leaf_with_measure(
taffy::style::FlexboxLayout {
flex_basis: taffy::style::Dimension::Points(50.0),
flex_grow: 1.0,
Expand Down Expand Up @@ -422,7 +422,7 @@ mod measure {
fn stretch_overrides_measure() {
let mut taffy = taffy::node::Taffy::new();
let child = taffy
.new_leaf(
.new_leaf_with_measure(
taffy::style::FlexboxLayout { ..Default::default() },
MeasureFunc::Raw(|constraint| taffy::geometry::Size {
width: constraint.width.unwrap_or(50.0),
Expand Down Expand Up @@ -454,7 +454,7 @@ mod measure {
fn measure_absolute_child() {
let mut taffy = taffy::node::Taffy::new();
let child = taffy
.new_leaf(
.new_leaf_with_measure(
taffy::style::FlexboxLayout {
position_type: taffy::style::PositionType::Absolute,
..Default::default()
Expand Down Expand Up @@ -488,12 +488,7 @@ mod measure {
#[test]
fn ignore_invalid_measure() {
let mut taffy = taffy::node::Taffy::new();
let child = taffy
.new_leaf(
taffy::style::FlexboxLayout { flex_grow: 1.0, ..Default::default() },
MeasureFunc::Raw(|_| taffy::geometry::Size { width: 200.0, height: 200.0 }),
)
.unwrap();
let child = taffy.new_leaf(taffy::style::FlexboxLayout { flex_grow: 1.0, ..Default::default() }).unwrap();

let node = taffy
.new_with_children(
Expand Down Expand Up @@ -522,7 +517,7 @@ mod measure {
static NUM_MEASURES: atomic::AtomicU32 = atomic::AtomicU32::new(0);

let grandchild = taffy
.new_leaf(
.new_leaf_with_measure(
taffy::style::FlexboxLayout { ..Default::default() },
MeasureFunc::Raw(|constraint| {
NUM_MEASURES.fetch_add(1, atomic::Ordering::Relaxed);
Expand Down
2 changes: 1 addition & 1 deletion tests/node.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ mod node {
fn set_measure() {
let mut taffy = Taffy::new();
let node = taffy
.new_leaf(FlexboxLayout::default(), MeasureFunc::Raw(|_| Size { width: 200.0, height: 200.0 }))
.new_leaf_with_measure(FlexboxLayout::default(), MeasureFunc::Raw(|_| Size { width: 200.0, height: 200.0 }))
Copy link
Copy Markdown
Collaborator

@alice-i-cecile alice-i-cecile Jun 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be migrated to the new method.

Copy link
Copy Markdown
Collaborator Author

@Weibye Weibye Jun 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would anything with MeasureFunc::Raw(|_| .. ) be a candidate for migration? If the measure func constraints are discarded, it would make sense that it's not used?

Copy link
Copy Markdown
Collaborator Author

@Weibye Weibye Jun 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was this, then two other occurrences of this; however those were specifically testing the MeasureFunc so migrating those does not make sense.

Copy link
Copy Markdown
Collaborator Author

@Weibye Weibye Jun 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: This test is specifically testing the behaviour of first setting one MeasureFunc, then setting a different, hence this should not be changed.

Copy link
Copy Markdown
Collaborator

@alice-i-cecile alice-i-cecile Jun 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, sounds good.

.unwrap();
taffy.compute_layout(node, Size::undefined()).unwrap();
assert_eq!(taffy.layout(node).unwrap().size.width, 200.0);
Expand Down