From ab22fd7f9c0fe17e31e57b32f4ec6a04939353e4 Mon Sep 17 00:00:00 2001 From: TheDestroyer19 <17345449+TheDestroyer19@users.noreply.github.com> Date: Sat, 11 Jun 2022 11:19:44 -0700 Subject: [PATCH] Improve Errors (#152) * Added checks for child_index out of bounds * slightly better descriptions * updated RELEASES.md * Changed TaffyError back to Error * split error type into two * fixed error in git merge * updated RELEASES.md file * cargo fmt * moved errors into error module, and gave them better names * Split InvalidChild::InvalidNode into two * updated RELEASES.md * cargo fmt * more changes to RELEASES.md Co-authored-by: Alice Cecile --- RELEASES.md | 4 ++- examples/basic.rs | 2 +- examples/nested.rs | 2 +- src/error.rs | 57 ++++++++++++++++++++++++++++++++++ src/lib.rs | 29 +---------------- src/node.rs | 77 +++++++++++++++++++++++++++++----------------- src/prelude.rs | 1 - 7 files changed, 112 insertions(+), 60 deletions(-) create mode 100644 src/error.rs diff --git a/RELEASES.md b/RELEASES.md index 3ef21d4e1..fc4ed9d04 100644 --- a/RELEASES.md +++ b/RELEASES.md @@ -4,7 +4,7 @@ ### 0.2.0 Added -- Nothing yet +- Added `taffy::error::InvalidChild` Error type ### 0.2.0 Changed @@ -12,6 +12,8 @@ - renamed `taffy::forest::Forest.new-node(..)` `taffy::forest::Forest.new_with_children(..)` - renamed `taffy::node::Taffy.new-node(..)` -> `taffy::node::Taffy.new_with_children(..)` - renamed `taffy::style::Style` -> `taffy::style::FlexboxLayout` to more precicely indicate its purpose +- renamed `taffy::Error` -> `taffy::error::InvalidNode` +- `taffy::Taffy::remove_child_at_index`, `taffy::Taffy::replace_child_at_index`, and `taffy::Taffy::child_at_index` now return `taffy::InvalidChild::ChildIndexOutOfBounds` instead of panicing ### 0.2.0 Fixed diff --git a/examples/basic.rs b/examples/basic.rs index f5576a723..751477ee9 100644 --- a/examples/basic.rs +++ b/examples/basic.rs @@ -1,6 +1,6 @@ use taffy::prelude::*; -fn main() -> Result<(), Error> { +fn main() -> Result<(), taffy::error::InvalidNode> { let mut taffy = Taffy::new(); let child = taffy.new_with_children( diff --git a/examples/nested.rs b/examples/nested.rs index ba185c028..53fb7e491 100644 --- a/examples/nested.rs +++ b/examples/nested.rs @@ -1,6 +1,6 @@ use taffy::prelude::*; -fn main() -> Result<(), Error> { +fn main() -> Result<(), taffy::error::InvalidNode> { let mut taffy = Taffy::new(); // left diff --git a/src/error.rs b/src/error.rs new file mode 100644 index 000000000..30a2e8d94 --- /dev/null +++ b/src/error.rs @@ -0,0 +1,57 @@ +//! The Error types produced by Taffy. +#[cfg(feature = "std")] +use core::fmt::{Display, Formatter, Result}; + +use crate::node::Node; + +/// The [`Node`] was not found in the [`Taffy`](crate::Taffy) instance +#[derive(Debug)] +pub struct InvalidNode(pub Node); + +#[cfg(feature = "std")] +impl Display for InvalidNode { + fn fmt(&self, f: &mut Formatter) -> Result { + write!(f, "Node {:?} is not in the Taffy instance", self.0) + } +} + +#[cfg(feature = "std")] +impl std::error::Error for InvalidNode {} + +/// An error that occurs while trying to access or modify a [`Node`]'s children by index. +#[derive(Debug)] +pub enum InvalidChild { + /// The parent [`Node`] does not have a child at `child_index`. It only has `child_count` children + ChildIndexOutOfBounds { + /// The parent node whose child was being looked up + parent: Node, + /// The index that was looked up + child_index: usize, + /// The total number of children the parent has + child_count: usize, + }, + /// The parent [`Node`] was not found in the [`Taffy`](crate::Taffy) instance. + InvalidParentNode(Node), + /// The child [`Node`] was not found in the [`Taffy`](crate::Taffy) instance. + InvalidChildNode(Node), +} + +#[cfg(feature = "std")] +impl Display for InvalidChild { + fn fmt(&self, f: &mut Formatter) -> Result { + match self { + InvalidChild::ChildIndexOutOfBounds { parent, child_index, child_count } => write!( + f, + "Index (is {}) should be < child_count ({}) for parent node {:?}", + child_index, child_count, parent + ), + InvalidChild::InvalidParentNode(parent) => { + write!(f, "Parent Node {:?} is not in the Taffy instance", parent) + } + InvalidChild::InvalidChildNode(child) => write!(f, "Child Node {:?} is not in the Taffy instance", child), + } + } +} + +#[cfg(feature = "std")] +impl std::error::Error for InvalidChild {} diff --git a/src/lib.rs b/src/lib.rs index 074a1dd07..b10b07041 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -11,6 +11,7 @@ extern crate alloc; #[cfg(feature = "serde")] extern crate serde; +pub mod error; pub mod geometry; pub mod layout; pub mod node; @@ -25,31 +26,3 @@ mod indexmap; mod sys; pub use crate::node::Taffy; - -#[cfg(feature = "std")] -use core::fmt::{Display, Formatter, Result}; - -/// An error that can occur when performing layout -#[derive(Debug)] -pub enum Error { - /// The [`Node`](node::Node) was invalid - InvalidNode(node::Node), -} - -#[cfg(feature = "std")] -impl Display for Error { - fn fmt(&self, f: &mut Formatter) -> Result { - match *self { - Error::InvalidNode(ref node) => write!(f, "Invalid node {:?}", node), - } - } -} - -#[cfg(feature = "std")] -impl std::error::Error for Error { - fn description(&self) -> &str { - match *self { - Error::InvalidNode(_) => "The node is not part of the Taffy instance", - } - } -} diff --git a/src/node.rs b/src/node.rs index 97bfd1a40..ee56bc770 100644 --- a/src/node.rs +++ b/src/node.rs @@ -1,6 +1,7 @@ //! UI [`Node`] types and related data structures. //! //! Layouts are composed of multiple nodes, which live in a forest-like data structure. +use crate::error; use crate::forest::Forest; use crate::geometry::Size; use crate::layout::Layout; @@ -9,7 +10,6 @@ use crate::style::FlexboxLayout; #[cfg(any(feature = "std", feature = "alloc"))] use crate::sys::Box; use crate::sys::{new_map_with_capacity, ChildrenVec, Map, Vec}; -use crate::Error; use core::sync::atomic::{AtomicUsize, Ordering}; /// A function that can be applied to a `Size` to obtain a `Size` @@ -87,15 +87,15 @@ impl Taffy { } /// Returns the `NodeId` of the provided node within the forest - fn find_node(&self, node: Node) -> Result { + fn find_node(&self, node: Node) -> Result { match self.nodes_to_ids.get(&node) { Some(id) => Ok(*id), - None => Err(Error::InvalidNode(node)), + None => Err(error::InvalidNode(node)), } } /// Adds a new leaf node, which does not have any children - pub fn new_leaf(&mut self, style: FlexboxLayout, measure: MeasureFunc) -> Result { + pub fn new_leaf(&mut self, style: FlexboxLayout, measure: MeasureFunc) -> Result { let node = self.allocate_node(); let id = self.forest.new_leaf(style, measure); self.add_node(node, id); @@ -103,10 +103,12 @@ impl Taffy { } /// Adds a new node, which may have any number of `children` - pub fn new_with_children(&mut self, style: FlexboxLayout, children: &[Node]) -> Result { + pub fn new_with_children(&mut self, style: FlexboxLayout, children: &[Node]) -> Result { let node = self.allocate_node(); - let children = - children.iter().map(|child| self.find_node(*child)).collect::, Error>>()?; + let children = children + .iter() + .map(|child| self.find_node(*child)) + .collect::, error::InvalidNode>>()?; let id = self.forest.new_with_children(style, children); self.add_node(node, id); Ok(node) @@ -124,7 +126,7 @@ impl Taffy { /// Remove a specific [`Node`] from the tree /// /// Its [`Id`] is marked as invalid. Returns the id of the node removed. - pub fn remove(&mut self, node: Node) -> Result { + pub fn remove(&mut self, node: Node) -> Result { let id = self.find_node(node)?; self.nodes_to_ids.remove(&node); @@ -140,7 +142,7 @@ impl Taffy { } /// Sets the [`MeasureFunc`] of the associated node - pub fn set_measure(&mut self, node: Node, measure: Option) -> Result<(), Error> { + pub fn set_measure(&mut self, node: Node, measure: Option) -> Result<(), error::InvalidNode> { let id = self.find_node(node)?; self.forest.nodes[id].measure = measure; self.forest.mark_dirty(id); @@ -148,7 +150,7 @@ impl Taffy { } /// Adds a `child` [`Node`] under the supplied `parent` - pub fn add_child(&mut self, parent: Node, child: Node) -> Result<(), Error> { + pub fn add_child(&mut self, parent: Node, child: Node) -> Result<(), error::InvalidNode> { let node_id = self.find_node(parent)?; let child_id = self.find_node(child)?; @@ -157,7 +159,7 @@ impl Taffy { } /// Directly sets the `children` of the supplied `parent` - pub fn set_children(&mut self, parent: Node, children: &[Node]) -> Result<(), Error> { + pub fn set_children(&mut self, parent: Node, children: &[Node]) -> Result<(), error::InvalidNode> { let node_id = self.find_node(parent)?; let children_id = children.iter().map(|child| self.find_node(*child)).collect::, _>>()?; @@ -179,7 +181,7 @@ impl Taffy { /// Removes the `child` of the parent `node` /// /// The child is not removed from the forest entirely, it is simply no longer attached to its previous parent. - pub fn remove_child(&mut self, parent: Node, child: Node) -> Result { + pub fn remove_child(&mut self, parent: Node, child: Node) -> Result { let node_id = self.find_node(parent)?; let child_id = self.find_node(child)?; @@ -190,9 +192,13 @@ impl Taffy { /// Removes the child at the given `index` from the `parent` /// /// The child is not removed from the forest entirely, it is simply no longer attached to its previous parent. - pub fn remove_child_at_index(&mut self, parent: Node, child_index: usize) -> Result { - let node_id = self.find_node(parent)?; - // TODO: index check + pub fn remove_child_at_index(&mut self, parent: Node, child_index: usize) -> Result { + let node_id = self.find_node(parent).map_err(|e| error::InvalidChild::InvalidParentNode(e.0))?; + + let child_count = self.forest.children[node_id].len(); + if child_index >= child_count { + return Err(error::InvalidChild::ChildIndexOutOfBounds { parent, child_index, child_count }); + } let prev_id = self.forest.remove_child_at_index(node_id, child_index); Ok(self.ids_to_nodes[&prev_id]) @@ -201,10 +207,19 @@ impl Taffy { /// Replaces the child at the given `child_index` from the `parent` node with the new `child` node /// /// The child is not removed from the forest entirely, it is simply no longer attached to its previous parent. - pub fn replace_child_at_index(&mut self, parent: Node, child_index: usize, new_child: Node) -> Result { - let node_id = self.find_node(parent)?; - let child_id = self.find_node(new_child)?; + pub fn replace_child_at_index( + &mut self, + parent: Node, + child_index: usize, + new_child: Node, + ) -> Result { + let node_id = self.find_node(parent).map_err(|e| error::InvalidChild::InvalidParentNode(e.0))?; + let child_id = self.find_node(new_child).map_err(|e| error::InvalidChild::InvalidChildNode(e.0))?; // TODO: index check + let child_count = self.forest.children[node_id].len(); + if child_index >= child_count { + return Err(error::InvalidChild::ChildIndexOutOfBounds { parent, child_index, child_count }); + } self.forest.parents[child_id].push(node_id); let old_child = core::mem::replace(&mut self.forest.children[node_id][child_index], child_id); @@ -216,25 +231,31 @@ impl Taffy { } /// Returns the child [`Node`] of the parent `node` at the provided `child_index` - pub fn child_at_index(&self, parent: Node, child_index: usize) -> Result { - let id = self.find_node(parent)?; + pub fn child_at_index(&self, parent: Node, child_index: usize) -> Result { + let id = self.find_node(parent).map_err(|e| error::InvalidChild::InvalidParentNode(e.0))?; + + let child_count = self.forest.children[id].len(); + if child_index >= child_count { + return Err(error::InvalidChild::ChildIndexOutOfBounds { parent, child_index, child_count }); + } + Ok(self.ids_to_nodes[&self.forest.children[id][child_index]]) } /// Returns the number of children of the `parent` [`Node`] - pub fn child_count(&self, parent: Node) -> Result { + pub fn child_count(&self, parent: Node) -> Result { let id = self.find_node(parent)?; Ok(self.forest.children[id].len()) } /// Returns a list of children that belong to the [`Parent`] - pub fn children(&self, parent: Node) -> Result, Error> { + pub fn children(&self, parent: Node) -> Result, error::InvalidNode> { let id = self.find_node(parent)?; Ok(self.forest.children[id].iter().map(|child| self.ids_to_nodes[child]).collect()) } /// Sets the [`Style`] of the provided `node` - pub fn set_style(&mut self, node: Node, style: FlexboxLayout) -> Result<(), Error> { + pub fn set_style(&mut self, node: Node, style: FlexboxLayout) -> Result<(), error::InvalidNode> { let id = self.find_node(node)?; self.forest.nodes[id].style = style; self.forest.mark_dirty(id); @@ -242,32 +263,32 @@ impl Taffy { } /// Gets the [`Style`] of the provided `node` - pub fn style(&self, node: Node) -> Result<&FlexboxLayout, Error> { + pub fn style(&self, node: Node) -> Result<&FlexboxLayout, error::InvalidNode> { let id = self.find_node(node)?; Ok(&self.forest.nodes[id].style) } /// Return this node layout relative to its parent - pub fn layout(&self, node: Node) -> Result<&Layout, Error> { + pub fn layout(&self, node: Node) -> Result<&Layout, error::InvalidNode> { let id = self.find_node(node)?; Ok(&self.forest.nodes[id].layout) } /// Marks the layout computation of this node and its children as outdated - pub fn mark_dirty(&mut self, node: Node) -> Result<(), Error> { + pub fn mark_dirty(&mut self, node: Node) -> Result<(), error::InvalidNode> { let id = self.find_node(node)?; self.forest.mark_dirty(id); Ok(()) } /// Indicates whether the layout of this node (and its children) need to be recomputed - pub fn dirty(&self, node: Node) -> Result { + pub fn dirty(&self, node: Node) -> Result { let id = self.find_node(node)?; Ok(self.forest.nodes[id].is_dirty) } /// Updates the stored layout of the provided `node` and its children - pub fn compute_layout(&mut self, node: Node, size: Size) -> Result<(), Error> { + pub fn compute_layout(&mut self, node: Node, size: Size) -> Result<(), error::InvalidNode> { let id = self.find_node(node)?; self.forest.compute_layout(id, size); Ok(()) diff --git a/src/prelude.rs b/src/prelude.rs index 297daa9d4..107baab24 100644 --- a/src/prelude.rs +++ b/src/prelude.rs @@ -9,5 +9,4 @@ pub use crate::{ AlignContent, AlignItems, AlignSelf, Dimension, Display, FlexDirection, FlexWrap, FlexboxLayout, JustifyContent, PositionType, }, - Error, };