From a5176a0d15ee67d29a91cd3f790a81c53bbc9bef Mon Sep 17 00:00:00 2001 From: Adoo Date: Sat, 2 Nov 2024 15:58:49 +0800 Subject: [PATCH] =?UTF-8?q?fix(core):=20=F0=9F=90=9B=20the=20class=20updat?= =?UTF-8?q?e=20context=20is=20not=20clean=20and=20may=20result=20in=20miss?= =?UTF-8?q?ing=20mounted=20widgets?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- CHANGELOG.md | 3 +- core/src/builtin_widgets/class.rs | 178 +++++++++++++++++------------- core/src/widget_tree/widget_id.rs | 24 ++++ 3 files changed, 129 insertions(+), 76 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5c18c5a6a..78f4ff4d2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,7 +31,8 @@ Please only add new entries below the [Unreleased](#unreleased---releasedate) he ### Fixed -- **core**: The `Provider` might be missing in a pipe class. (#pr @M-Adoo) +- **core**: The `Provider` might be missing in a pipe class. (#648 @M-Adoo) +- **core**: The child generated by the class may not be mounted. (#648 @M-Adoo) ## [0.4.0-alpha.14] - 2024-10-30 diff --git a/core/src/builtin_widgets/class.rs b/core/src/builtin_widgets/class.rs index cb103e4cf..5bde4d467 100644 --- a/core/src/builtin_widgets/class.rs +++ b/core/src/builtin_widgets/class.rs @@ -208,21 +208,29 @@ impl ClassChild { fn update(&self, orig: &OrigChild, class: &Class, ctx: &mut BuildCtx) { let InnerClassChild { child, child_id, orig_id } = self.inner(); + eprintln!("old tree\n {}", ctx.tree().display_tree(ctx.tree().root())); + let cls_holder = child_id.place_holder(ctx.tree()); - // 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()); - + // Extract the child from this node, retaining only the external information + // linked from the parent to create a clean context for applying the class. + let child_node = self.take_inner(); + let n_orig = ctx.alloc(Box::new(orig.clone())); let new_id = class - .apply_style(Widget::from_id(*orig_id), ctx) + .apply_style(Widget::from_id(n_orig), ctx) .build(ctx); - let tree = ctx.tree_mut(); + // Place the inner child node within the old ID for disposal, then utilize the + // class node to wrap the new child in the new ID. + // This action should be taken before modifying the `orig_id`, as the `orig_id` + // may be the same as the `child_id`. + let class_node = std::mem::replace(child_id.get_node_mut(tree).unwrap(), child_node); + + // Retain the original widget ID. + let [new, old] = tree.get_many_mut(&[n_orig, *orig_id]); + std::mem::swap(new, old); + n_orig.insert_after(*orig_id, tree); + n_orig.dispose_subtree(tree); + new_id.wrap_node(tree, |node| { *child = node; class_node @@ -242,10 +250,10 @@ impl ClassChild { .borrow_mut() .single_range_replace(&old_rg, &new_rg) }); + cls_holder.replace(new_id, tree); } if orig_id != child_id { - child_id.insert_after(new_id, tree); child_id.dispose_subtree(tree); } @@ -259,8 +267,9 @@ impl ClassChild { } *child_id = new_id; - tree.mark_dirty(new_id); + tree.mark_dirty(*orig_id); + eprintln!("new tree\n {}", ctx.tree().display_tree(ctx.tree().root())); } #[allow(clippy::mut_from_ref)] @@ -337,18 +346,12 @@ mod tests { reset_test_env, test_helper::{MockBox, MockMulti, TestWindow, split_value}, }; + class_names!(MARGIN, BOX_200, EMPTY); - #[test] - fn switch_class() { - reset_test_env!(); - - class_names!(MARGIN, SCALE_2X); - - let mut theme = Theme::default(); - theme - .classes - .insert(MARGIN, style_class!(margin: EdgeInsets::all(10.))); - theme.classes.insert(SCALE_2X, |w| { + fn initd_classes() -> Classes { + let mut classes = Classes::default(); + classes.insert(MARGIN, style_class!(margin: EdgeInsets::all(10.))); + classes.insert(BOX_200, |w| { fn_widget! { @MockBox { size: Size::new(200., 200.), @@ -357,21 +360,28 @@ mod tests { } .into_widget() }); - AppCtx::set_app_theme(theme); + classes + } - let (cls, w_cls) = split_value(MARGIN); + #[test] + fn switch_class() { + reset_test_env!(); + let (cls, w_cls) = split_value(MARGIN); let mut wnd = TestWindow::new(fn_widget! { - @Container { - size: Size::new(100., 100.), - class: pipe!(*$cls), - } + let cls = cls.clone_watcher(); + initd_classes().with_child(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() = SCALE_2X; + *w_cls.write() = BOX_200; wnd.draw_frame(); wnd.assert_root_size(Size::new(200., 200.)); } @@ -381,31 +391,29 @@ mod tests { 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); + class_names!(ON_DISPOSED); let (cls, w_cls) = split_value(ON_DISPOSED); let mut wnd = TestWindow::new(fn_widget! { - @Container { - size: Size::new(100., 100.), - class: pipe!(*$cls), - } + let cls = cls.clone_watcher(); + let mut classes = initd_classes(); + classes.insert(ON_DISPOSED, |w| { + fn_widget! { + @MockBox { + size: Size::zero(), + on_disposed: move |_| panic!("on_disposed called"), + @ { w } + } + } + .into_widget() + }); + classes.with_child(fn_widget! { + @Container { + size: Size::new(100., 100.), + class: pipe!(*$cls), + } + }) }); wnd.draw_frame(); @@ -417,32 +425,29 @@ mod tests { 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); + class_names!(MULTI); - let (cls, w_cls) = split_value(SINGLE); + let (cls, w_cls) = split_value(MARGIN); let mut wnd = TestWindow::new(fn_widget! { - @Container { - size: Size::new(100., 100.), - class: pipe!(*$cls), - } + let cls = cls.clone_watcher(); + let mut classes = initd_classes(); + classes.insert(MULTI, |w| { + fn_widget! { + @MockMulti { + @MockBox { size: Size::new(100., 100.) } + @MockBox { size: Size::new(100., 200.) } + @ { w } + } + } + .into_widget() + }); + classes.with_child(fn_widget! { + @Container { + size: Size::new(100., 100.), + class: pipe!(*$cls), + } + }) }); wnd.draw_frame(); @@ -480,4 +485,27 @@ mod tests { }); wnd.draw_frame(); } + + #[test] + fn fix_not_mounted_class_node() { + reset_test_env!(); + + let (cls, w_cls) = split_value(EMPTY); + let mut wnd = TestWindow::new(fn_widget! { + let cls = cls.clone_watcher(); + initd_classes().with_child(fn_widget! { + @Container { + size: Size::new(100., 100.), + class: pipe!(*$cls), + } + }) + }); + + wnd.draw_frame(); + wnd.assert_root_size(Size::new(100., 100.)); + + *w_cls.write() = BOX_200; + wnd.draw_frame(); + wnd.assert_root_size(Size::new(200., 200.)); + } } diff --git a/core/src/widget_tree/widget_id.rs b/core/src/widget_tree/widget_id.rs index 951c81b75..3ba288a87 100644 --- a/core/src/widget_tree/widget_id.rs +++ b/core/src/widget_tree/widget_id.rs @@ -14,6 +14,13 @@ use crate::{ pub struct WidgetId(pub(crate) NodeId); +// A place holder get from a `WidgetId`, you can use it to insert a widget +// replace the `WidgetId` you get this place holder. +pub(crate) enum PlaceHolder { + PrevSibling(WidgetId), + Parent(WidgetId), +} + pub trait RenderQueryable: Render + Query { fn as_render(&self) -> &dyn Render; } @@ -75,6 +82,14 @@ impl WidgetId { .map(|(a, _)| a) } + pub(crate) fn place_holder(self, tree: &WidgetTree) -> PlaceHolder { + if let Some(prev) = self.prev_sibling(tree) { + PlaceHolder::PrevSibling(prev) + } else { + PlaceHolder::Parent(self.parent(tree).unwrap()) + } + } + pub(crate) fn parent(self, tree: &WidgetTree) -> Option { self.node_feature(tree, |node| node.parent()) } @@ -299,3 +314,12 @@ impl WidgetId { pub(crate) fn queryable(&self, tree: &WidgetTree) -> bool { self.assert_get(tree).queryable() } } + +impl PlaceHolder { + pub(crate) fn replace(self, widget: WidgetId, tree: &mut WidgetTree) { + match self { + PlaceHolder::PrevSibling(prev) => prev.insert_after(widget, tree), + PlaceHolder::Parent(parent) => parent.append(widget, tree), + } + } +}