fix(core): 🐛 resolve crash with class impl having multiple children a…

…nd potential node disposal issue.

CHANGELOG.md

@@ -25,10 +25,11 @@ Please only add new entries below the [Unreleased](#unreleased---releasedate) he
 
 ## [@Unreleased] - @ReleaseDate
 
-
 ### Fixed
 
 - **core**: Setting the theme before running the app results in the tree being constructed twice. (#637, @M-Adoo)
+- **core**: Resolve a crash occurring in a class implementation with multiple children. (#637 @M-Adoo)
+- **core**: Nodes created by a class implementation may not be disposed of when switching to another class. (#637 @M-Adoo)
 
 ## [0.4.0-alpha.12] - 2024-10-09
 

Cargo.toml

@@ -52,7 +52,8 @@ fontdb = "0.22.0"
 futures = "0.3.26"
 guillotiere = "0.6.0"
 image = { version = "0.24.5", default-features = false }
-indextree = "4.5.0"
+# Version 4.7.2 of the iterator contains a bug. We are awaiting the release of patch #117 to address this issue.
+indextree = { git = "https://github.com/saschagrunert/indextree.git", rev = "f75bdbb1dfbd5c63519fabedd0127dcbc8130052"}
 log = "0.4.14"
 lyon_algorithms = "1.0.1"
 lyon_geom = "1.0.1"

core/src/builtin_widgets/class.rs

@@ -34,6 +34,7 @@
 use std::cell::UnsafeCell;
 
 use ribir_algo::Sc;
+use smallvec::{SmallVec, smallvec};
 use widget_id::RenderQueryable;
 
 use crate::{
@@ -212,13 +213,15 @@ impl ClassChild {
   fn update(&self, orig: &OrigChild, class: &Class, ctx: &mut BuildCtx) {
     let InnerClassChild { child, child_id, orig_id } = self.inner();
 
-    // Revert back to class node only,  maybe there is information attached on it,
-    // that we need keep.
-    let class_node =
-      std::mem::replace(child_id.get_node_mut(ctx.tree_mut()).unwrap(), Box::new(self.clone()));
+    // Set the widget generated by the class back to the tree node. However, retain
+    // the tree node for reuse with the new ID as it may contain attached
+    // information or be combined with other render widgets.
+    let class_child = self.take_inner();
+    let class_node = std::mem::replace(child_id.get_node_mut(ctx.tree_mut()).unwrap(), class_child);
 
     // Revert to the original child.
     *orig_id.get_node_mut(ctx.tree_mut()).unwrap() = Box::new(orig.clone());
+
     let new_id = class
       .apply_style(Widget::from_id(*orig_id), ctx)
       .build(ctx);
@@ -230,7 +233,9 @@ impl ClassChild {
     });
 
     if new_id != *child_id {
-      // If we modify the tree structure, we must notify the pipe accordingly.
+      // If a pipe widget generates a widget with a class, we place the pipe node
+      // outside of the class node. However, since its widget ID is altered, we must
+      // notify the pipe node accordingly.
       let old_rg = *child_id..=*orig_id;
       let new_rg = new_id..=*orig_id;
       new_id
@@ -248,35 +253,38 @@ impl ClassChild {
       child_id.dispose_subtree(tree);
     }
 
-    if new_id != *orig_id {
-      let mut w = new_id;
-      loop {
-        w.on_widget_mounted(tree);
-        if w == *orig_id {
-          break;
-        }
-        w = w.single_child(tree).unwrap();
+    let mut stack: SmallVec<[WidgetId; 1]> = smallvec![new_id];
+    while let Some(w) = stack.pop() {
+      // Skip the original child subtree as it does not consist of new widgets.
+      if w != *orig_id {
+        w.on_mounted_subtree(tree);
+        stack.extend(w.children(tree).rev());
       }
     }
+
     *child_id = new_id;
 
     tree.mark_dirty(new_id);
   }
 
   #[allow(clippy::mut_from_ref)]
   fn inner(&self) -> &mut InnerClassChild { unsafe { &mut *self.0.get() } }
+
+  fn take_inner(&self) -> Box<dyn RenderQueryable> {
+    std::mem::replace(&mut self.inner().child, Box::new(PureRender(Void)))
+  }
 }
 
 impl OrigChild {
   fn from_id(id: WidgetId, ctx: &mut BuildCtx) -> Self {
-    let mut cls = None;
+    let mut orig = None;
     id.wrap_node(ctx.tree_mut(), |node| {
       let c = OrigChild(Sc::new(UnsafeCell::new(node)));
-      cls = Some(c.clone());
+      orig = Some(c.clone());
       Box::new(c)
     });
 
-    cls.unwrap()
+    orig.unwrap()
   }
 
   fn attach_subscription(&mut self, guard: impl Any) {
@@ -331,7 +339,7 @@ mod tests {
   use super::*;
   use crate::{
     reset_test_env,
-    test_helper::{MockBox, TestWindow, split_value},
+    test_helper::{MockBox, MockMulti, TestWindow, split_value},
   };
 
   #[test]
@@ -341,7 +349,6 @@ mod tests {
     class_names!(MARGIN, SCALE_2X);
 
     let mut theme = Theme::default();
-
     theme
       .classes
       .insert(MARGIN, |w| fn_widget! { @ $w{ margin: EdgeInsets::all(10.) } }.into_widget());
@@ -354,7 +361,6 @@ mod tests {
       }
       .into_widget()
     });
-
     AppCtx::set_app_theme(theme);
 
     let (cls, w_cls) = split_value(MARGIN);
@@ -373,4 +379,81 @@ mod tests {
     wnd.draw_frame();
     wnd.assert_root_size(Size::new(200., 200.));
   }
+
+  #[test]
+  #[should_panic(expected = "on_disposed called")]
+  fn on_disposed_of_class_nodes() {
+    reset_test_env!();
+
+    class_names!(ON_DISPOSED, MARGIN);
+
+    let mut theme = Theme::default();
+    theme.classes.insert(MARGIN, style_class! {
+      margin: EdgeInsets::all(10.)
+    });
+    theme.classes.insert(ON_DISPOSED, |w| {
+      fn_widget! {
+        @MockBox {
+          size: Size::zero(),
+          on_disposed: move |_| panic!("on_disposed called"),
+          @ { w }
+        }
+      }
+      .into_widget()
+    });
+    AppCtx::set_app_theme(theme);
+
+    let (cls, w_cls) = split_value(ON_DISPOSED);
+
+    let mut wnd = TestWindow::new(fn_widget! {
+      @Container {
+        size: Size::new(100., 100.),
+        class: pipe!(*$cls),
+      }
+    });
+
+    wnd.draw_frame();
+    *w_cls.write() = MARGIN;
+    wnd.draw_frame();
+  }
+
+  #[test]
+  fn fix_crash_for_class_impl_may_have_multi_child() {
+    reset_test_env!();
+
+    class_names!(MULTI, SINGLE);
+    let mut theme = Theme::default();
+    theme
+      .classes
+      .insert(SINGLE, style_class! { margin: EdgeInsets::all(10.) });
+    theme.classes.insert(MULTI, |w| {
+      // A class implementation may consist of a complex subtree, and it cannot be
+      // guaranteed to have a single linked structure.
+      fn_widget! {
+        @MockMulti {
+          @MockBox { size: Size::new(100., 100.) }
+          @MockBox { size: Size::new(100., 200.) }
+          @ { w }
+        }
+      }
+      .into_widget()
+    });
+    AppCtx::set_app_theme(theme);
+
+    let (cls, w_cls) = split_value(SINGLE);
+
+    let mut wnd = TestWindow::new(fn_widget! {
+      @Container {
+        size: Size::new(100., 100.),
+        class: pipe!(*$cls),
+      }
+    });
+
+    wnd.draw_frame();
+    wnd.assert_root_size(Size::new(120., 120.));
+
+    *w_cls.write() = MULTI;
+    wnd.draw_frame();
+    wnd.assert_root_size(Size::new(300., 200.));
+  }
 }

core/src/widget_tree/widget_id.rs

@@ -111,7 +111,9 @@ impl WidgetId {
   }
 
   #[inline]
-  pub(crate) fn children(self, tree: &WidgetTree) -> impl Iterator<Item = WidgetId> + '_ {
+  pub(crate) fn children(
+    self, tree: &WidgetTree,
+  ) -> impl DoubleEndedIterator<Item = WidgetId> + '_ {
     // `IndexTree` not check if is a freed id when create iterator, we may iterate
     assert!(!self.is_dropped(tree));
     self.0.children(&tree.arena).map(WidgetId)

macros/src/pipe_macro.rs

@@ -36,7 +36,7 @@ pub fn gen_code(input: TokenStream, refs_ctx: &mut DollarRefsCtx) -> TokenStream
     if refs.used_ctx() {
       quote_spanned! {span =>
         MapPipe::new(
-          ModifiesPipe::new(#upstream.filter(|s| s.contains(ModifyScope::FRAMEWORK)).box_it()),
+          ModifiesPipe::new(#upstream.box_it()),
           {
             #refs
             let _ctx_handle_ಠ_ಠ = ctx!().handle();

macros/src/variable_names.rs

@@ -50,9 +50,6 @@ macro_rules! builtin_member {
   };
 }
 pub static BUILTIN_INFOS: phf::Map<&'static str, BuiltinMember> = phf_map! {
-  // BuiltinObj
-  "lazy_host_id" => builtin_member!{"BuiltinObj", Method, "lazy"},
-  "lazy_id" => builtin_member!{"BuiltinObj", Method, "lazy"},
   // Class
   "class" => builtin_member!{"Class", Field, "class"},
   // MixFlags