Recursive style deletion on Element removal by ian-hon · Pull Request #11 · voxell-tech/fynix

ian-hon · 2026-04-15T17:51:14Z

Closes #9
Closes #8

nixonyh

Just some fly by review.

nixonyh · 2026-04-17T02:16:17Z

        let Some(parent) = self.styles.get_mut(&parent_id) else {
            return;
        };



Nice, I think we could add a

debug_assert!(parent.children.iter().any(|c| c.is_none()));

Just to make sure we're not overriding any existing child ids.

Co-authored-by: Nixon <43715558+nixonyh@users.noreply.github.com>

nixonyh

Overall looks good! Thanks for writing the tests 😺 . I have added some comments that can be addressed in a separate PR.

nixonyh · 2026-04-27T02:16:47Z

+            None
+        };

        // restore to old value


nit: I like comments to begin with capital and ends with fullstop xd (same goes to others too)

nixonyh · 2026-04-27T02:19:36Z

    /// Creates element `E`, applies the current style chain to it,
    /// stores it, and returns its [`ElementId`].
+    ///
+    /// Elements created with `add` dont own any styles, so their


nit: typo

Suggested change

/// Elements created with `add` dont own any styles, so their

/// Elements created with `add` don't own any styles, so their

nixonyh · 2026-04-27T03:33:59Z

+            if let Some(meta) = metas.remove(id)
+                && let Some(type_meta) =
+                    type_metas.get_slot(meta.slot)
+            {
+                (type_meta.for_each_child_mut_fn)(
+                    elements,
+                    id,
+                    &mut |child_id, elements| {
+                        remove_recursive(
+                            child_id,
+                            metas,
+                            type_metas,
+                            elements,
+                            id_generator,
+                        );
+                    },
+                );
+
+                elements.dyn_remove_by_slot(meta.slot, id);
+                id_generator.recycle(*id);


Just noting that we might need some debug safe guard to make sure meta points to an actual element. Though, error like these shudn't happen anyway..

nixonyh · 2026-04-27T03:34:52Z

    /// Removes a committed style node and recycles its [`StyleId`].
    ///
    /// Returns `true` if the node was present and removed.
    pub fn delete(&mut self, id: &StyleId) -> bool {


Will need to remove this or make this private, it will disrupt the tree deletion flow.

nixonyh · 2026-04-27T03:35:29Z

    }

+    /// Recursively deletes a style and its children
+    pub fn delete_tree(&mut self, id: &StyleId) {


Suggested change

pub fn delete_tree(&mut self, id: &StyleId) {

pub fn remove(&mut self, id: &StyleId) {

Probably rename this to remove and embed a fn remove_recursive similar to Elements.

nixonyh · 2026-04-27T03:38:26Z

+        for c in children {
+            let Some(c) = c else {
+                continue;
+            };


Suggested change

for c in children {

let Some(c) = c else {

continue;

};

for c in children.iter().filter_map(|c| c.as_ref()) {

nixonyh · 2026-04-27T03:41:14Z

+    /// Treat the style's children as a (sort-of) binary tree.
+    /// `children[0]` is one depth deeper (inner scope).
+    /// `children[1]` is same depth, subsequent style node.
+    children: [Option<StyleId>; 2],


I know this data structure was my suggestion, but after reading more, maybe this would be a better fit as they are both different thing after all.

Suggested change

/// Treat the style's children as a (sort-of) binary tree.

/// `children[0]` is one depth deeper (inner scope).

/// `children[1]` is same depth, subsequent style node.

children: [Option<StyleId>; 2],

adjacent_child: Option<StyleId>,

nested_child: Option<StyleId>,

nixonyh · 2026-04-27T03:44:50Z

+        let pre_commit_id = self.parent_style_id;
+
+        let mut element = self.create_element::<E>(true);
+
+        let pre_fn_id = self.parent_style_id;
+
+        // styleId from create_element
+        // else, current uncomited style if is None
+        let first_inner_style_id = self
+            .parent_style_id
+            .filter(|id| Some(*id) != pre_commit_id)
+            .unwrap_or_else(|| self.fynix.styles.current_id());

        f(&mut element, self);

-        // Restore parent style id from before the closure.
-        self.parent_style_id = parent_style_id;
+        // If self.parent_style_id has changed, that means a
+        // style has been committed inside the closure.
+        // It's ID would be that of `first_inner_style_id`,
+        // so our primary style is set to that.
+        let primary_style = if self.parent_style_id != pre_commit_id {
+            Some(first_inner_style_id)
+        } else {
+            None
+        };
+
+        // restore to old value
+        self.parent_style_id = pre_fn_id;

-        self.fynix.elements.add(element)
+        let id = self.fynix.elements.add(element);
+
+        if let Some(style_id) = primary_style
+            && let Some(meta) = self.fynix.elements.metas.get_mut(&id)
+        {
+            meta.primary_style = Some(style_id);
+        }
+
+        id


Note: I still think we can do a better job here instead of relying on parent_style_id. Cuz it feels a little fragile (IMO) (?)

Recursive style deletion on Element removal

4d41779

nixonyh self-requested a review April 16, 2026 02:14

nixonyh reviewed Apr 16, 2026

View reviewed changes

Comment thread crates/fynix/src/style.rs

Comment thread crates/fynix/src/style.rs Outdated

ian-hon added 2 commits April 17, 2026 01:51

Removed continuing_scope flag from context

0fc444e

Applied suggestions from @nixonyh

bc47659

nixonyh reviewed Apr 17, 2026

View reviewed changes

ian-hon added 4 commits April 22, 2026 13:44

Made style children deterministic

771329a

Merge branch 'main' into ian/element-deletion

8db345a

Recursively removes element children

378725e

Updated non-heap alloc approach

8e94995

ian-hon force-pushed the ian/element-deletion branch from a0e0c73 to 8e94995 Compare April 22, 2026 11:23

ian-hon commented Apr 22, 2026

View reviewed changes

Comment thread crates/fynix/src/element.rs Outdated

nixonyh reviewed Apr 22, 2026

View reviewed changes

Comment thread crates/fynix/src/element.rs Outdated

Comment thread crates/fynix/src/element.rs Outdated

Comment thread crates/fynix/src/element.rs Outdated

nixonyh and others added 5 commits April 23, 2026 11:22

Add scope to ElementTable to unlock recursive pattern

e24f4db

Update element remove docstring

3897ccb

Co-authored-by: Nixon <43715558+nixonyh@users.noreply.github.com>

Merge branch 'main' into ian/element-deletion

7eb515c

Minor fix

90c2be9

Fixed broken style propagation

c40113c

nixonyh approved these changes Apr 27, 2026

View reviewed changes

nixonyh reviewed Apr 27, 2026

View reviewed changes

ian-hon merged commit 2db1510 into main Apr 27, 2026
7 checks passed

ian-hon deleted the ian/element-deletion branch April 27, 2026 09:31

	/// Elements created with `add` dont own any styles, so their
	/// Elements created with `add` don't own any styles, so their

	pub fn delete_tree(&mut self, id: &StyleId) {
	pub fn remove(&mut self, id: &StyleId) {

Uh oh!

Conversation

ian-hon commented Apr 15, 2026 • edited by nixonyh Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nixonyh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nixonyh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ian-hon commented Apr 15, 2026 •

edited by nixonyh

Loading