fix on_id widget refcount leak (-> memory leak)

src/bin/test/main.rs

@@ -103,7 +103,7 @@ impl Client {
                 .width(rest(3)),
         )
             .span(Dir::RIGHT)
-            .add_static(&mut ui);
+            .add(&mut ui);
 
         let span_test = (
             rrect.color(Color::GREEN).width(100),
@@ -114,9 +114,10 @@ impl Client {
             rrect.color(Color::RED).width(100),
         )
             .span(Dir::LEFT)
-            .add_static(&mut ui);
+            .add(&mut ui);
 
-        let span_add = Span::empty(Dir::RIGHT).add_static(&mut ui);
+        let span_add = Span::empty(Dir::RIGHT).add(&mut ui);
+        let span_add_ = span_add.clone();
 
         let add_button = rect(Color::LIME)
             .radius(30)
@@ -125,22 +126,21 @@ impl Client {
                     .ui
                     .add(image(include_bytes!("assets/sungals.png")).center())
                     .any();
-                ctx.ui[span_add].children.push(child);
+                ctx.ui[&span_add_].children.push(child);
             })
             .sized((150, 150))
             .align(Align::BOT_RIGHT);
 
+        let span_add_ = span_add.clone();
         let del_button = rect(Color::RED)
             .radius(30)
             .on(CursorSense::click(), move |ctx: &mut Client, _| {
-                ctx.ui[span_add].children.pop();
+                ctx.ui[&span_add_].children.pop();
             })
             .sized((150, 150))
             .align(Align::BOT_LEFT);
 
-        let span_add_test = (span_add, add_button, del_button)
+        let span_add_test = (span_add, add_button, del_button).stack().add(&mut ui);
-            .stack()
-            .add_static(&mut ui);
 
         let btext = |content| wtext(content).size(30);
 
@@ -163,10 +163,10 @@ impl Client {
             wtext("pretty cool right?").size(50),
         )
             .span(Dir::DOWN)
-            .add_static(&mut ui);
+            .add(&mut ui);
 
-        let texts = Span::empty(Dir::DOWN).gap(10).add_static(&mut ui);
+        let texts = Span::empty(Dir::DOWN).gap(10).add(&mut ui);
-        let msg_area = texts.scroll().masked().background(rect(Color::SKY));
+        let msg_area = texts.clone().scroll().masked().background(rect(Color::SKY));
         let add_text = wtext("add")
             .editable(false)
             .text_align(Align::LEFT)
@@ -183,7 +183,7 @@ impl Client {
                 let msg_box = text
                     .background(rect(Color::WHITE.darker(0.5)))
                     .add(&mut client.ui);
-                client.ui[texts].children.push(msg_box.any());
+                client.ui[&texts].children.push(msg_box.any());
             })
             .add(&mut ui);
         let text_edit_scroll = (
@@ -207,14 +207,15 @@ impl Client {
                 .align(Align::BOT),
         )
             .span(Dir::DOWN)
-            .add_static(&mut ui);
+            .add(&mut ui);
 
-        let main = pad_test.pad(10).add_static(&mut ui);
+        let main = pad_test.clone().pad(10).add(&mut ui);
 
-        let switch_button = |color, to, label| {
+        let switch_button = |color, to: WidgetId, label| {
+            let main_ = main.clone();
             let rect = rect(color)
                 .id_on(CursorSense::click(), move |id, ui: &mut Ui, _| {
-                    ui[main].inner.set_static(to);
+                    ui[&main_.clone()].inner = to.clone();
                     ui[id].color = color.darker(0.3);
                 })
                 .edit_on(

src/layout/event.rs

@@ -69,8 +69,10 @@ impl<W: WidgetLike<Tag>, Tag> Eventable<W::Widget, Tag> for W {
         W::Widget: Widget,
     {
         self.with_id(move |ui, id| {
-            let id2 = id.clone();
+            // needed so that this widget can actually be dropped
-            id.on(event, move |ctx, pos| f(&id2, ctx, pos)).add(ui)
+            let id2 = id.weak();
+            id.on(event, move |ctx, pos| f(&id2.strong(), ctx, pos))
+                .add(ui)
         })
     }
 

src/layout/id.rs

@@ -1,12 +1,4 @@
-use std::{
+use std::{any::TypeId, marker::PhantomData, sync::mpsc::Sender};
-    any::TypeId,
-    marker::PhantomData,
-    sync::{
-        Arc,
-        atomic::{AtomicBool, Ordering},
-        mpsc::Sender,
-    },
-};
 
 use crate::{
     layout::{Ui, WidgetLike},
@@ -29,7 +21,15 @@ pub struct WidgetId<W = AnyWidget> {
     pub(super) id: Id,
     counter: RefCounter,
     send: Sender<Id>,
-    is_static: Arc<AtomicBool>,
+    _pd: PhantomData<W>,
+}
+
+#[repr(C)]
+pub struct WeakWidgetId<W = AnyWidget> {
+    pub(super) ty: TypeId,
+    pub(super) id: Id,
+    counter: RefCounter,
+    send: Sender<Id>,
     _pd: PhantomData<W>,
 }
 
@@ -39,21 +39,6 @@ impl<W> PartialEq for WidgetId<W> {
     }
 }
 
-/// A WidgetId for a static widget that cannot be removed from a Ui.
-/// Useful because ergonomic clones don't exist yet so you can easily use these in closures.
-/// Do not use this if you want the widget to be freeable.
-///
-/// This is currently not perfectly efficient and just creates new WidgetIds every time it's used,
-/// but they don't send drop messages to Ui.
-/// Ideally I'd have an enum or something that lets you use either, but that doesn't seem worth it
-/// right now; it's good enough and relatively cheap.
-#[repr(C)]
-pub struct StaticWidgetId<W = AnyWidget> {
-    pub(super) ty: TypeId,
-    pub(super) id: Id,
-    _pd: PhantomData<W>,
-}
-
 impl<W> std::fmt::Debug for WidgetId<W> {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         self.id.fmt(f)
@@ -67,20 +52,18 @@ impl<W> Clone for WidgetId<W> {
             ty: self.ty,
             counter: self.counter.clone(),
             send: self.send.clone(),
-            is_static: self.is_static.clone(),
             _pd: PhantomData,
         }
     }
 }
 
 impl<W> WidgetId<W> {
-    pub(super) fn new(id: Id, ty: TypeId, send: Sender<Id>, is_static: bool) -> Self {
+    pub(super) fn new(id: Id, ty: TypeId, send: Sender<Id>) -> Self {
         Self {
             ty,
             id,
             counter: RefCounter::new(),
             send,
-            is_static: Arc::new(is_static.into()),
             _pd: PhantomData,
         }
     }
@@ -107,29 +90,47 @@ impl<W> WidgetId<W> {
         self.counter.refs()
     }
 
-    pub fn into_static(self) -> StaticWidgetId<W> {
+    pub fn weak(&self) -> WeakWidgetId<W> {
-        self.is_static.store(true, Ordering::Release);
+        let Self {
-        StaticWidgetId {
+            ty,
-            ty: self.ty,
+            id,
-            id: self.id,
+            ref counter,
-            _pd: PhantomData,
+            ref send,
+            _pd,
+        } = *self;
+        WeakWidgetId {
+            ty,
+            id,
+            counter: counter.quiet_clone(),
+            send: send.clone(),
+            _pd,
         }
     }
 }
 
-impl WidgetId {
+impl<W> WeakWidgetId<W> {
-    pub fn set_static<W>(&mut self, other: StaticWidgetId<W>) {
+    /// should guarantee that widget is still valid to prevent indexing failures
-        let send = self.send.clone();
+    pub(crate) fn strong(&self) -> WidgetId<W> {
-        drop(std::mem::replace(
+        let Self {
-            self,
+            ty,
-            Self::new(other.id, self.ty, send, true),
+            id,
-        ));
+            ref counter,
+            ref send,
+            _pd,
+        } = *self;
+        WidgetId {
+            ty,
+            id,
+            counter: counter.clone(),
+            send: send.clone(),
+            _pd,
+        }
     }
 }
 
 impl<W> Drop for WidgetId<W> {
     fn drop(&mut self) {
-        if self.counter.drop() && !self.is_static.load(Ordering::Acquire) {
+        if self.counter.drop() {
             let _ = self.send.send(self.id);
         }
     }
@@ -158,31 +159,6 @@ impl<W: 'static, F: FnOnce(&mut Ui) -> WidgetId<W>> WidgetLike<IdFnTag> for F {
     }
 }
 
-impl<W> StaticWidgetId<W> {
-    pub fn to_id(&self, send: &Sender<Id>) -> WidgetId<W> {
-        WidgetId::new(self.id, self.ty, send.clone(), true)
-    }
-    pub fn any(self) -> StaticWidgetId<AnyWidget> {
-        // SAFETY: self is repr(C)
-        unsafe { std::mem::transmute(self) }
-    }
-}
-
-impl<W: 'static> WidgetLike<IdTag> for StaticWidgetId<W> {
-    type Widget = W;
-    fn add(self, ui: &mut Ui) -> WidgetId<W> {
-        self.id(&ui.send)
-    }
-}
-
-impl<W> Clone for StaticWidgetId<W> {
-    fn clone(&self) -> Self {
-        *self
-    }
-}
-
-impl<W> Copy for StaticWidgetId<W> {}
-
 pub trait WidgetIdLike<W> {
     fn id(self, send: &Sender<Id>) -> WidgetId<W>;
 }
@@ -193,12 +169,6 @@ impl<W> WidgetIdLike<W> for &WidgetId<W> {
     }
 }
 
-impl<W> WidgetIdLike<W> for StaticWidgetId<W> {
-    fn id(self, send: &Sender<Id>) -> WidgetId<W> {
-        self.to_id(send)
-    }
-}
-
 pub trait IdLike<W> {
     fn id(&self) -> Id;
 }
@@ -208,9 +178,3 @@ impl<W> IdLike<W> for WidgetId<W> {
         self.id
     }
 }
-
-impl<W> IdLike<W> for StaticWidgetId<W> {
-    fn id(&self) -> Id {
-        self.id
-    }
-}

src/layout/ui.rs

@@ -3,8 +3,8 @@ use image::DynamicImage;
 use crate::{
     core::{TextEdit, TextEditCtx},
     layout::{
-        Event, EventFn, EventModule, IdLike, PainterData, PixelRegion, StaticWidgetId,
+        Event, EventFn, EventModule, IdLike, PainterData, PixelRegion, TextureHandle, Vec2, Widget,
-        TextureHandle, Vec2, Widget, WidgetId, WidgetInstance, WidgetLike,
+        WidgetId, WidgetInstance, WidgetLike,
     },
     util::{HashSet, Id},
 };
@@ -30,14 +30,6 @@ impl Ui {
         w.add(self)
     }
 
-    pub fn add_static<W: Widget, Tag>(
-        &mut self,
-        w: impl WidgetLike<Tag, Widget = W>,
-    ) -> StaticWidgetId<W> {
-        let id = w.add(self);
-        id.into_static()
-    }
-
     /// useful for debugging
     pub fn set_label<W>(&mut self, id: &WidgetId<W>, label: String) {
         self.data.widgets.data_mut(&id.id).unwrap().label = label;
@@ -79,7 +71,6 @@ impl Ui {
             self.data.widgets.reserve(),
             TypeId::of::<W>(),
             self.send.clone(),
-            false,
         )
     }
 
@@ -215,21 +206,6 @@ impl<W: Widget> IndexMut<&WidgetId<W>> for Ui {
     }
 }
 
-impl<W: Widget> Index<StaticWidgetId<W>> for Ui {
-    type Output = W;
-
-    fn index(&self, id: StaticWidgetId<W>) -> &Self::Output {
-        self.data.widgets.get(&id).unwrap()
-    }
-}
-
-impl<W: Widget> IndexMut<StaticWidgetId<W>> for Ui {
-    fn index_mut(&mut self, id: StaticWidgetId<W>) -> &mut Self::Output {
-        self.updates.insert(id.id);
-        self.data.widgets.get_mut(&id).unwrap()
-    }
-}
-
 impl dyn Widget {
     pub fn as_any(&self) -> &dyn Any {
         self

src/layout/widget.rs

@@ -1,6 +1,6 @@
 use crate::{
     core::WidgetPtr,
-    layout::{Len, Painter, SizeCtx, StaticWidgetId, Ui, WidgetId, WidgetIdFn},
+    layout::{Len, Painter, SizeCtx, Ui, WidgetId, WidgetIdFn},
 };
 
 use std::{any::Any, marker::PhantomData};
@@ -26,7 +26,9 @@ pub struct FnTag;
 
 pub trait WidgetLike<Tag> {
     type Widget: 'static;
+
     fn add(self, ui: &mut Ui) -> WidgetId<Self::Widget>;
+
     fn with_id<W2>(
         self,
         f: impl FnOnce(&mut Ui, WidgetId<Self::Widget>) -> WidgetId<W2>,
@@ -39,18 +41,14 @@ pub trait WidgetLike<Tag> {
             f(ui, id)
         }
     }
-    fn add_static(self, ui: &mut Ui) -> StaticWidgetId<Self::Widget>
+
-    where
-        Self: Sized,
-    {
-        self.add(ui).into_static()
-    }
     fn set_root(self, ui: &mut Ui)
     where
         Self: Sized,
     {
         ui.set_root(self);
     }
+
     fn set_ptr(self, ptr: &WidgetId<WidgetPtr>, ui: &mut Ui)
     where
         Self: Sized,

src/util/refcount.rs

@@ -17,6 +17,9 @@ impl RefCounter {
         let refs = self.0.fetch_sub(1, Ordering::Release);
         refs == 0
     }
+    pub fn quiet_clone(&self) -> Self {
+        Self(self.0.clone())
+    }
 }
 
 impl Clone for RefCounter {