From 3f8885efbea81c336d25d6afd9f784e9f01a9a39 Mon Sep 17 00:00:00 2001 From: Jonathan Johnson Date: Fri, 1 Dec 2023 13:31:42 -0800 Subject: [PATCH] Callback handles are now managed Installing a callback now returns a CallbackHandle. All map-style APIs install this handle automatically on the created dynamic, which keeps the callback installed until the dynamic is freed. All other APIs return the handle for the caller to either call persist() or store somewhere. Now, the dynamic system can be used for application-long data with almost no fear of leaking data due to how callbacks are being installed. Technically cycles are still possible by moving clones into the callbacks, so a WeakDynamic type might be worth exposing. --- Cargo.lock | 3 +- Cargo.toml | 3 +- examples/theme.rs | 39 +++++++++-------- examples/tic-tac-toe.rs | 3 +- examples/todo.rs | 12 +++--- src/value.rs | 96 +++++++++++++++++++++++++++++++++-------- src/widgets/progress.rs | 4 +- 7 files changed, 113 insertions(+), 47 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 03038e3..863fb57 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -48,8 +48,7 @@ checksum = "0942ffc6dcaadf03badf6e6a2d0228460359d5e34b57ccdc720b7382dfbd5ec5" [[package]] name = "alot" version = "0.3.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0f1063fbee2ac6d371ffcb55cb443f4d5b469f1b8eace60042878242bb534169" +source = "git+https://github.com/khonsulabs/alot#8868932a271587bdabcd2aeb18aa960fa1b5c79e" [[package]] name = "android-activity" diff --git a/Cargo.toml b/Cargo.toml index 115aa1a..99c082a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -39,7 +39,8 @@ unicode-segmentation = "1.10.1" # [patch."https://github.com/khonsulabs/figures"] # figures = { path = "../figures" } -# [patch.crates-io] +[patch.crates-io] +alot = { git = "https://github.com/khonsulabs/alot" } # kempt = { path = "../objectmap" } [profile.dev.package."*"] diff --git a/examples/theme.rs b/examples/theme.rs index 515f679..5677e35 100644 --- a/examples/theme.rs +++ b/examples/theme.rs @@ -112,14 +112,16 @@ fn main() -> gooey::Result { scheme.build() }, ); - color_scheme.for_each_cloned(move |scheme| { - sources.primary.set(scheme.primary); - sources.secondary.set(scheme.secondary); - sources.tertiary.set(scheme.tertiary); - sources.error.set(scheme.error); - sources.neutral.set(scheme.neutral); - sources.neutral_variant.set(scheme.neutral_variant); - }); + color_scheme + .for_each_cloned(move |scheme| { + sources.primary.set(scheme.primary); + sources.secondary.set(scheme.secondary); + sources.tertiary.set(scheme.tertiary); + sources.error.set(scheme.error); + sources.neutral.set(scheme.neutral); + sources.neutral_variant.set(scheme.neutral_variant); + }) + .persist(); let theme = color_scheme.map_each_cloned(ThemePair::from); let editors = theme_switcher @@ -198,18 +200,21 @@ fn color_editor(color: &Dynamic) -> impl MakeWidget { source.hue = OklabHue::new(hue); color.set(source); } - }); + }) + .persist(); let hue_text = hue.linked_string(); let saturation = color.map_each(|color| color.saturation); - saturation.for_each_cloned({ - let color = color.clone(); - move |saturation| { - let mut source = color.get(); - source.saturation = saturation; - color.set(source); - } - }); + saturation + .for_each_cloned({ + let color = color.clone(); + move |saturation| { + let mut source = color.get(); + source.saturation = saturation; + color.set(source); + } + }) + .persist(); let saturation_text = saturation.linked_string(); hue.slider_between(0., 359.99) diff --git a/examples/tic-tac-toe.rs b/examples/tic-tac-toe.rs index 69b1425..d0159b5 100644 --- a/examples/tic-tac-toe.rs +++ b/examples/tic-tac-toe.rs @@ -189,7 +189,8 @@ fn square(row: usize, column: usize, game: &Dynamic) -> impl MakeWidg if enabled.replace(false).is_some() { label.set(player.to_string()); } - }); + }) + .persist(); }); label diff --git a/examples/todo.rs b/examples/todo.rs index acaf82d..31f6996 100644 --- a/examples/todo.rs +++ b/examples/todo.rs @@ -7,11 +7,13 @@ use gooey::Run; fn main() -> gooey::Result { let tasks = Dynamic::default(); let children = Dynamic::default(); - tasks.for_each(children.with_clone(|children| { - move |tasks: &Vec| { - update_task_widgets(tasks, &mut children.lock()); - } - })); + tasks + .for_each(children.with_clone(|children| { + move |tasks: &Vec| { + update_task_widgets(tasks, &mut children.lock()); + } + })) + .persist(); let task_text = Dynamic::default(); let valid = task_text.map_each(|text: &String| !text.is_empty()); diff --git a/src/value.rs b/src/value.rs index 9d252e0..801f27e 100644 --- a/src/value.rs +++ b/src/value.rs @@ -14,6 +14,8 @@ use std::thread::ThreadId; use std::time::Duration; use ahash::AHashSet; +use alot::{LotId, Lots}; +use intentional::Assert; use kempt::{Map, Sort}; use crate::animation::{AnimationHandle, DynamicTransition, IntoAnimate, LinearInterpolate, Spawn}; @@ -41,6 +43,7 @@ impl Dynamic { readers: 0, wakers: Vec::new(), widgets: AHashSet::new(), + source_callback: None, }), during_callback_state: Mutex::default(), sync: UnwindsafeCondvar::default(), @@ -87,7 +90,8 @@ impl Dynamic { if let Some(update) = t_into_r(t).into() { let _result = r.replace(update); } - }); + }) + .persist(); }); self.with_clone(|t| { @@ -162,6 +166,12 @@ impl Dynamic { }) } + fn set_source(&self, source: CallbackHandle) { + self.state() + .assert("called during initialization") + .source_callback = Some(source); + } + /// Returns a new dynamic that contains the updated contents of this dynamic /// at most once every `period`. #[must_use] @@ -171,7 +181,8 @@ impl Dynamic { { let debounced = Dynamic::new(self.get()); let mut debounce = Debounce::new(debounced.clone(), period); - self.for_each_cloned(move |value| debounce.update(value)); + let callback = self.for_each_cloned(move |value| debounce.update(value)); + debounced.set_source(callback); debounced } @@ -185,7 +196,8 @@ impl Dynamic { { let debounced = Dynamic::new(self.get()); let mut debounce = Debounce::new(debounced.clone(), period).extending(); - self.for_each_cloned(move |value| debounce.update(value)); + let callback = self.for_each_cloned(move |value| debounce.update(value)); + debounced.set_source(callback); debounced } @@ -213,7 +225,7 @@ impl Dynamic { /// Attaches `for_each` to this value so that it is invoked each time the /// value's contents are updated. - pub fn for_each(&self, mut for_each: F) + pub fn for_each(&self, mut for_each: F) -> CallbackHandle where T: Send + 'static, F: for<'a> FnMut(&'a T) + Send + 'static, @@ -221,12 +233,12 @@ impl Dynamic { let this = self.clone(); self.0.for_each(move || { this.map_ref(&mut for_each); - }); + }) } /// Attaches `for_each` to this value and its [`Generation`] so that it is /// invoked each time the value's contents are updated. - pub fn for_each_generational(&self, mut for_each: F) + pub fn for_each_generational(&self, mut for_each: F) -> CallbackHandle where T: Send + 'static, F: for<'a> FnMut(&'a GenerationalValue) + Send + 'static, @@ -234,12 +246,12 @@ impl Dynamic { let this = self.clone(); self.0.for_each(move || { this.map_generational(&mut for_each); - }); + }) } /// Attaches `for_each` to this value so that it is invoked each time the /// value's contents are updated. - pub fn for_each_cloned(&self, mut for_each: F) + pub fn for_each_cloned(&self, mut for_each: F) -> CallbackHandle where T: Clone + Send + 'static, F: FnMut(T) + Send + 'static, @@ -247,7 +259,7 @@ impl Dynamic { let this = self.clone(); self.0.for_each(move || { for_each(this.get()); - }); + }) } /// Attaches `for_each` to this value so that it is invoked each time the @@ -258,7 +270,7 @@ impl Dynamic { T: Send + 'static, F: for<'a> FnMut(&'a T) + Send + 'static, { - self.for_each(for_each); + self.for_each(for_each).persist(); self } @@ -584,7 +596,7 @@ impl Dynamic { E: Display, { let validation = Dynamic::new(Validation::None); - self.for_each({ + let callback = self.for_each({ let validation = validation.clone(); move |value| { validation.set(match check(value) { @@ -593,6 +605,7 @@ impl Dynamic { }); } }); + validation.set_source(callback); validation } } @@ -806,13 +819,16 @@ impl DynamicData { Ok(old) } - pub fn for_each(&self, map: F) + pub fn for_each(&self, map: F) -> CallbackHandle where F: for<'a> FnMut() + Send + 'static, { let state = self.state().expect("deadlocked"); let mut callbacks = state.callbacks.callbacks.lock().ignore_poison(); - callbacks.push(Box::new(map)); + CallbackHandle { + id: Some(callbacks.push(Box::new(map))), + callbacks: state.callbacks.clone(), + } } pub fn map_each(&self, mut map: F) -> Dynamic @@ -824,10 +840,12 @@ impl DynamicData { let mapped_value = Dynamic::new(initial_value); let returned = mapped_value.clone(); - self.for_each(move || { + let callback = self.for_each(move || { mapped_value.set(map()); }); + returned.set_source(callback); + returned } } @@ -855,8 +873,46 @@ impl Display for DeadlockError { } } +/// A handle to a callback installed on a [`Dynamic`]. When dropped, the +/// callback will be uninstalled. +/// +/// To prevent the callback from ever being uninstalled, use +/// [`Self::persist()`]. +#[must_use] +pub struct CallbackHandle { + id: Option, + callbacks: Arc, +} + +impl Debug for CallbackHandle { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("CallbackHandle") + .field("id", &self.id) + .finish_non_exhaustive() + } +} + +impl CallbackHandle { + /// Persists the callback so that it will always be invoked until the + /// dynamic is freed. + pub fn persist(mut self) { + let _id = self.id.take(); + drop(self); + } +} + +impl Drop for CallbackHandle { + fn drop(&mut self) { + if let Some(id) = self.id { + let mut callbacks = self.callbacks.callbacks.lock().ignore_poison(); + callbacks.remove(id); + } + } +} + struct State { wrapped: GenerationalValue, + source_callback: Option, callbacks: Arc, windows: AHashSet, widgets: AHashSet<(WindowHandle, WidgetId)>, @@ -896,7 +952,7 @@ where #[derive(Default)] struct ChangeCallbacksData { - callbacks: Mutex>>, + callbacks: Mutex>>, currently_executing: AtomicBool, } @@ -1617,7 +1673,7 @@ macro_rules! impl_tuple_for_each { } }; ($self:ident $for_each:ident [] [$type:ident $field:tt $var:ident]) => { - $self.$field.for_each(move |field: &$type| $for_each((field,))); + $self.$field.for_each(move |field: &$type| $for_each((field,))).persist(); }; ($self:ident $for_each:ident [] [$($type:ident $field:tt $var:ident),+]) => { let $for_each = Arc::new(Mutex::new($for_each)); @@ -1684,7 +1740,7 @@ macro_rules! impl_tuple_for_each { for_each.lock().ignore_poison(); (for_each)(($(&$avar,)+)); } - })); + })).persist(); }; } @@ -1761,7 +1817,7 @@ macro_rules! impl_tuple_for_each_cloned { } }; ($self:ident $for_each:ident [] [$type:ident $field:tt $var:ident]) => { - $self.$field.for_each(move |field: &$type| $for_each((field.clone(),))); + $self.$field.for_each(move |field: &$type| $for_each((field.clone(),))).persist(); }; ($self:ident $for_each:ident [] [$($type:ident $field:tt $var:ident),+]) => { let $for_each = Arc::new(Mutex::new($for_each)); @@ -1829,7 +1885,7 @@ macro_rules! impl_tuple_for_each_cloned { (for_each)(($($avar,)+)); } } - })); + })).persist(); }; } @@ -2203,6 +2259,7 @@ struct Debounce { delay: Option, buffer: Dynamic, extend: bool, + _callback: Option, } impl Debounce @@ -2216,6 +2273,7 @@ where period, delay: None, extend: false, + _callback: None, } } diff --git a/src/widgets/progress.rs b/src/widgets/progress.rs index d6ec71d..67678ca 100644 --- a/src/widgets/progress.rs +++ b/src/widgets/progress.rs @@ -64,10 +64,10 @@ impl MakeWidgetWithId for ProgressBar { let slider = value.slider().knobless().non_interactive().make_with_id(id); match self.progress { Value::Dynamic(progress) => { - progress.for_each(move |progress| { + let callback = progress.for_each(move |progress| { update_progress_bar(*progress, &mut indeterminant_animation, &start, &end); }); - Data::new_wrapping(progress, slider).make_widget() + Data::new_wrapping((callback, progress), slider).make_widget() } Value::Constant(_) => Data::new_wrapping(indeterminant_animation, slider).make_widget(), }