fix(core): 🐛 marking the widget as dirty may result in using the wron…

…g ID

core/src/builtin_widgets/global_anchor.rs

@@ -298,7 +298,7 @@ impl<'c> ComposeChild<'c> for GlobalAnchor {
       }
     }
     .into_widget()
-    .on_build(move |id| id.dirty_on(modifies))
+    .dirty_on(modifies)
   }
 }
 

core/src/builtin_widgets/keep_alive.rs

@@ -30,11 +30,11 @@ impl<'c> ComposeChild<'c> for KeepAlive {
     fn_widget! {
       let mut w = FatObj::new(child);
       { this.silent().wid = Some($w.track_id()); }
-      let modifies = this.raw_modifies();
       w
       .into_widget()
+      .dirty_on(this.raw_modifies())
       .try_unwrap_state_and_attach(this)
-      .on_build(|id| id.dirty_on(modifies))
+
     }
     .into_widget()
   }

core/src/builtin_widgets/smooth_layout.rs

@@ -274,35 +274,41 @@ macro_rules! impl_compose_child {
       type Child = Widget<'c>;
 
       fn compose_child(this: impl StateWriter<Value = Self>, child: Self::Child) -> Widget<'c> {
+        let track = TrackWidgetId::default();
+        let id = track.track_id();
+        let ctx = BuildCtx::get();
+        let marker = ctx.tree().dirty_marker();
+        let window = ctx.window();
+
+        // The animation utilizes the smooth value for a seamless visual transition.
+        // Throughout the animation, we must monitor if the widget has been altered by
+        // external factors. If any modifications occur, we must initiate a forced
+        // layout to ensure the animation transitions to a new and accurate layout
+        // result.
         let inner = this.read().0.clone_writer();
-        WrapRender::combine_child(this, child).on_build(move |id| {
-          let ctx = BuildCtx::get();
-          let marker = ctx.tree().dirty_marker();
-          let window = ctx.window();
-
-          // The animation utilizes the smooth value for a seamless visual transition.
-          // Throughout the animation, we must monitor if the widget has been altered by
-          // external factors. If any modifications occur, we must initiate a forced
-          // layout to ensure the animation transitions to a new and accurate layout
-          // result.
-          let h = inner
-            .raw_modifies()
-            .filter(|b| b.contains(ModifyScope::FRAMEWORK))
-            .subscribe(move |_| {
-              let inner = inner.clone_writer();
-              let marker = marker.clone();
-              window.once_before_layout(move || {
-                if marker.is_dirty(id) {
-                  inner.set_force_layout(true)
-                }
-                if $dirty_self {
-                  marker.mark(id);
-                }
-              })
+        let h = inner
+          .raw_modifies()
+          .filter(|b| b.contains(ModifyScope::FRAMEWORK))
+          .subscribe(move |_| {
+            let inner = inner.clone_writer();
+            let marker = marker.clone();
+            let id = id.get().unwrap();
+            window.once_before_layout(move || {
+              if marker.is_dirty(id) {
+                inner.set_force_layout(true)
+              }
+              if $dirty_self {
+                marker.mark(id);
+              }
             })
-            .unsubscribe_when_dropped();
-          id.attach_anonymous_data(h, BuildCtx::get_mut().tree_mut());
-        })
+          })
+          .unsubscribe_when_dropped();
+        let child = track
+          .with_child(child)
+          .into_widget()
+          .attach_anonymous_data(h);
+
+        WrapRender::combine_child(this, child)
       }
     }
   };

core/src/state.rs

@@ -398,8 +398,9 @@ where
         Ok(r) => ReaderRender(r).into_widget(),
         Err(s) => {
           let modifies = s.raw_modifies();
-          let w = ReaderRender(s.clone_reader()).into_widget();
-          w.on_build(move |id| id.dirty_on(modifies))
+          ReaderRender(s.clone_reader())
+            .into_widget()
+            .dirty_on(modifies)
         }
       },
     }

core/src/widget.rs

@@ -1,11 +1,12 @@
-use std::cell::RefCell;
 #[doc(hidden)]
 pub use std::{
   any::{Any, TypeId},
   marker::PhantomData,
   ops::Deref,
 };
+use std::{cell::RefCell, convert::Infallible};
 
+use ops::box_it::CloneableBoxOp;
 use ribir_algo::Sc;
 use smallvec::SmallVec;
 use widget_id::RenderQueryable;
@@ -200,6 +201,33 @@ impl<'w> Widget<'w> {
     Widget(InnerWidget::Lazy(LazyNode::new(lazy)))
   }
 
+  /// Subscribe to the modified `upstream` to mark the widget as dirty when the
+  /// `upstream` emits a modify event containing `ModifyScope::FRAMEWORK`.
+  ///
+  /// # Panic
+  /// This method only works within a build process; otherwise, it will
+  /// result in a panic.
+  pub fn dirty_on(self, upstream: CloneableBoxOp<'static, ModifyScope, Infallible>) -> Self {
+    let track = TrackWidgetId::default();
+    let id = track.track_id();
+
+    let tree = BuildCtx::get_mut().tree_mut();
+    let marker = tree.dirty_marker();
+    let h = upstream
+      .filter(|b| b.contains(ModifyScope::FRAMEWORK))
+      .subscribe(move |_| {
+        if let Some(id) = id.get() {
+          marker.mark(id);
+        }
+      })
+      .unsubscribe_when_dropped();
+
+    track
+      .with_child(self)
+      .into_widget()
+      .attach_anonymous_data(h)
+  }
+
   pub(crate) fn from_render(r: Box<dyn RenderQueryable>) -> Widget<'static> {
     Widget(InnerWidget::Node(Node::Leaf(Box::new(|| BuildCtx::get_mut().alloc(r)))))
   }

core/src/widget_tree/widget_id.rs

@@ -1,7 +1,6 @@
-use std::{convert::Infallible, rc::Rc};
+use std::rc::Rc;
 
 use indextree::{Node, NodeId};
-use rxrust::ops::box_it::CloneableBoxOp;
 use smallvec::{SmallVec, smallvec};
 
 use super::*;
@@ -70,25 +69,6 @@ impl WidgetId {
     tree.arena.get(self.0).map(|n| &**n.get())
   }
 
-  /// Subscribe to the modified `upstream` to mark the widget as dirty when the
-  /// `upstream` emits a modify event containing `ModifyScope::FRAMEWORK`.
-  ///
-  /// # Panic
-  /// This method only works within a build process; otherwise, it will
-  /// result in a panic.
-  pub fn dirty_on(self, upstream: CloneableBoxOp<'static, ModifyScope, Infallible>) {
-    let tree = BuildCtx::get_mut().tree_mut();
-    let marker = tree.dirty_marker();
-    let h = upstream
-      .filter(|b| b.contains(ModifyScope::FRAMEWORK))
-      .subscribe(move |_| {
-        marker.mark(self);
-      })
-      .unsubscribe_when_dropped();
-
-    self.attach_anonymous_data(h, tree);
-  }
-
   pub(crate) fn get_node_mut(self, tree: &mut WidgetTree) -> Option<&mut Box<dyn RenderQueryable>> {
     tree.arena.get_mut(self.0).map(|n| n.get_mut())
   }

core/src/wrap_render.rs

@@ -29,24 +29,27 @@ pub trait WrapRender {
 
   fn get_transform(&self, host: &dyn Render) -> Option<Transform> { host.get_transform() }
 
-  fn combine_child(this: impl StateWriter<Value = Self>, child: Widget) -> Widget
+  fn combine_child(this: impl StateWriter<Value = Self>, mut child: Widget) -> Widget
   where
     Self: Sized + 'static,
   {
-    child.on_build(move |id| {
-      let tree = BuildCtx::get_mut().tree_mut();
-      id.wrap_node(tree, |r| match this.try_into_value() {
-        Ok(this) => Box::new(RenderPair { wrapper: Box::new(this), host: r }),
-        Err(this) => {
-          let reader = match this.into_reader() {
-            Ok(r) => r,
-            Err(s) => {
-              id.dirty_on(s.raw_modifies());
-              s.clone_reader()
-            }
-          };
-          Box::new(RenderPair { wrapper: Box::new(reader), host: r })
-        }
+    let wrapper: Box<dyn WrapRender> = match this.try_into_value() {
+      Ok(this) => Box::new(this),
+      Err(this) => {
+        let reader = match this.into_reader() {
+          Ok(r) => r,
+          Err(s) => {
+            child = child.dirty_on(s.raw_modifies());
+            s.clone_reader()
+          }
+        };
+        Box::new(reader)
+      }
+    };
+
+    child.on_build(|id| {
+      id.wrap_node(BuildCtx::get_mut().tree_mut(), move |r| {
+        Box::new(RenderPair { wrapper, host: r })
       });
     })
   }

docs/en/understanding_ribir/without_dsl.md

@@ -123,12 +123,14 @@ Note that while widgets created with `Declare` can be configured with all built-
 ```rust
 use ribir::prelude::*;
 
-let mut btn = Radio::declarer().finish();
-
-let m = btn.get_margin_widget().clone_writer();
-let btn = btn
-  .on_tap(move |_| m.write().margin = EdgeInsets::all(10.0))
-  .into_widget();
+fn radio_btn() -> Widget<'static> {
+  let mut btn = Radio::declarer().finish();
+
+  let m = btn.get_margin_widget().clone_writer();
+  btn
+    .on_tap(move |_| m.write().margin = EdgeInsets::all(10.0))
+    .into_widget()
+}
 ```
 
 ## Composing child widgets

docs/zh/understanding_ribir/without_dsl.md

@@ -121,12 +121,14 @@ let _row = Row::declarer()
 ```rust
 use ribir::prelude::*;
 
-let mut btn = Radio::declarer().finish();
-
-let m = btn.get_margin_widget().clone_writer();
-let btn = btn
-  .on_tap(move |_| m.write().margin = EdgeInsets::all(10.0))
-  .into_widget();
+fn radio_btn() -> Widget<'static> {
+  let mut btn = Radio::declarer().finish();
+
+  let m = btn.get_margin_widget().clone_writer();
+  btn
+    .on_tap(move |_| m.write().margin = EdgeInsets::all(10.0))
+    .into_widget()
+}
 ```
 
 ## 子 widget 的组合

widgets/src/layout/expanded.rs

@@ -57,8 +57,7 @@ impl<'c> ComposeChild<'c> for Expanded {
     let data: Box<dyn Query> = match this.try_into_value() {
       Ok(this) => Box::new(Queryable(this)),
       Err(this) => {
-        let modifies = this.raw_modifies();
-        child = child.on_build(|id| id.dirty_on(modifies));
+        child = child.dirty_on(this.raw_modifies());
         Box::new(this)
       }
     };