diff --git a/CHANGELOG.md b/CHANGELOG.md index 1e6410f..0526c3e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 [92]: https://github.com/khonsulabs/gooey/issues/92 +### Fixed + +- A memory leak has been fixed that prevented the underlying widget tree of each + window from being dropped. This was caused by a reference counting cycle, and + has been fixed by switching `MountedWidget` to use a weak reference internally + and having the window hold the strong reference to the tree. + ### Added - `Validations::validate_result` attaches a `Dynamic>` to the diff --git a/Cargo.lock b/Cargo.lock index 59a3572..a641324 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -106,7 +106,7 @@ dependencies = [ [[package]] name = "appit" version = "0.1.1" -source = "git+https://github.com/khonsulabs/appit#8ca300682c83d1bd6c698bc13ba7011b4be0ca8c" +source = "git+https://github.com/khonsulabs/appit#1ff4b7b2a6f93e2c719787c76e585e6c2ecf290c" dependencies = [ "winit", ] @@ -1179,7 +1179,7 @@ checksum = "e2db585e1d738fc771bf08a151420d3ed193d9d895a36df7f6f8a9456b911ddc" [[package]] name = "kludgine" version = "0.6.1" -source = "git+https://github.com/khonsulabs/kludgine#8327335a103ad4ba09093aa201c007d43b4a48d9" +source = "git+https://github.com/khonsulabs/kludgine#e76fcc5c7a2ac96edcd51ee635d228cc9295d7ce" dependencies = [ "ahash", "alot", diff --git a/src/context.rs b/src/context.rs index 0aa0fb0..8cae0e4 100644 --- a/src/context.rs +++ b/src/context.rs @@ -22,6 +22,7 @@ use crate::styles::components::{ Opacity, TextSize, WidgetBackground, }; use crate::styles::{ComponentDefinition, Styles, Theme, ThemePair}; +use crate::tree::Tree; use crate::utils::IgnorePoison; use crate::value::{IntoValue, Value}; use crate::widget::{ @@ -162,7 +163,7 @@ impl<'context, 'window> EventContext<'context, 'window> { } pub(crate) fn hover(&mut self, location: Point) { - let changes = self.current_node.tree.hover(Some(&self.current_node)); + let changes = self.tree.hover(Some(&self.current_node)); for unhovered in changes.unhovered { let mut context = self.for_other(&unhovered); unhovered.lock().as_widget().unhover(&mut context); @@ -187,7 +188,7 @@ impl<'context, 'window> EventContext<'context, 'window> { } pub(crate) fn clear_hover(&mut self) { - let changes = self.current_node.tree.hover(None); + let changes = self.tree.hover(None); assert!(changes.hovered.is_empty()); for old_hover in changes.unhovered { @@ -201,16 +202,13 @@ impl<'context, 'window> EventContext<'context, 'window> { fn apply_pending_activation(&mut self) { let mut activation_changes = 0; while activation_changes < Self::MAX_PENDING_CHANGE_CYCLES { - let active = self - .pending_state - .active - .and_then(|w| self.current_node.tree.widget(w)); - if self.current_node.tree.active_widget() == active.as_ref().map(|w| w.node_id) { + let active = self.pending_state.active.and_then(|w| self.tree.widget(w)); + if self.tree.active_widget() == active.as_ref().map(|w| w.node_id) { break; } activation_changes += 1; - let new = match self.current_node.tree.activate(active.as_ref()) { + let new = match self.tree.activate(active.as_ref()) { Ok(old) => { if let Some(old) = old { let mut old_context = self.for_other(&old); @@ -221,10 +219,7 @@ impl<'context, 'window> EventContext<'context, 'window> { Err(()) => false, }; if new { - let active = self - .pending_state - .active - .and_then(|w| self.current_node.tree.widget(w)); + let active = self.pending_state.active.and_then(|w| self.tree.widget(w)); if let Some(active) = &active { active .lock() @@ -246,11 +241,8 @@ impl<'context, 'window> EventContext<'context, 'window> { fn apply_pending_focus(&mut self) { let mut focus_changes = 0; while focus_changes < Self::MAX_PENDING_CHANGE_CYCLES { - let focus = self - .pending_state - .focus - .and_then(|w| self.current_node.tree.widget(w)); - if self.current_node.tree.focused_widget() == focus.as_ref().map(|w| w.node_id) { + let focus = self.pending_state.focus.and_then(|w| self.tree.widget(w)); + if self.tree.focused_widget() == focus.as_ref().map(|w| w.node_id) { break; } focus_changes += 1; @@ -270,7 +262,7 @@ impl<'context, 'window> EventContext<'context, 'window> { break self.next_focus_after(focus, self.pending_state.focus_is_advancing); } }); - let new = match self.current_node.tree.focus(self.pending_state.focus) { + let new = match self.tree.focus(self.pending_state.focus) { Ok(old) => { if let Some(old_widget) = old { let mut old_context = self.for_other(&old_widget); @@ -280,7 +272,7 @@ impl<'context, 'window> EventContext<'context, 'window> { } else { // This widget is rejecting the focus change. drop(old_context); - let _result = self.current_node.tree.focus(Some(old_widget.id())); + let _result = self.tree.focus(Some(old_widget.id())); self.pending_state.focus = Some(old_widget.id()); break; } @@ -290,11 +282,7 @@ impl<'context, 'window> EventContext<'context, 'window> { Err(()) => false, }; if new { - if let Some(focus) = self - .pending_state - .focus - .and_then(|w| self.current_node.tree.widget(w)) - { + if let Some(focus) = self.pending_state.focus.and_then(|w| self.tree.widget(w)) { focus.lock().as_widget().focus(&mut self.for_other(&focus)); } } else { @@ -319,8 +307,8 @@ impl<'context, 'window> EventContext<'context, 'window> { self.apply_pending_focus(); // Check that our hover widget still exists. If not, we should try to find a new one. - if let Some(hover) = self.current_node.tree.hovered_widget() { - if self.current_node.tree.widget_from_node(hover).is_none() { + if let Some(hover) = self.tree.hovered_widget() { + if self.tree.widget_from_node(hover).is_none() { self.update_hovered_widget(); } } @@ -329,7 +317,7 @@ impl<'context, 'window> EventContext<'context, 'window> { pub(crate) fn update_hovered_widget(&mut self) { self.cursor.widget = None; if let Some(location) = self.cursor.location { - for widget in self.current_node.tree.widgets_under_point(location) { + for widget in self.tree.widgets_under_point(location) { let mut widget_context = self.for_other(&widget); let Some(widget_layout) = widget_context.last_layout() else { continue; @@ -666,9 +654,7 @@ impl<'context, 'window, 'clip, 'gfx, 'pass> GraphicsContext<'context, 'window, ' "redraw called without set_widget_layout" ); - self.current_node - .tree - .note_widget_rendered(self.current_node.node_id); + self.tree.note_widget_rendered(self.current_node.node_id); let widget = self.current_node.clone(); let mut widget = widget.lock(); if !widget.as_widget().full_control_redraw() { @@ -826,10 +812,7 @@ pub trait AsEventContext<'window> { #[must_use] fn push_child(&mut self, child: WidgetInstance) -> MountedWidget { let mut context = self.as_event_context(); - let pushed_widget = context - .current_node - .tree - .push_boxed(child, Some(&context.current_node)); + let pushed_widget = context.tree.push_boxed(child, Some(&context.current_node)); pushed_widget .lock() .as_widget() @@ -844,10 +827,7 @@ pub trait AsEventContext<'window> { .lock() .as_widget() .unmounted(&mut context.for_other(child)); - context - .current_node - .tree - .remove_child(child, &context.current_node); + context.tree.remove_child(child, &context.current_node); } } @@ -869,6 +849,7 @@ impl<'window> AsEventContext<'window> for GraphicsContext<'_, 'window, '_, '_, ' /// specific widget. pub struct WidgetContext<'context, 'window> { current_node: MountedWidget, + pub(crate) tree: Tree, redraw_status: &'context InvalidationStatus, window: &'context mut RunningWindow<'window>, theme: Cow<'context, ThemePair>, @@ -891,18 +872,18 @@ impl<'context, 'window> WidgetContext<'context, 'window> { kludgine: window.handle(), redraw_status: redraw_status.clone(), }); + let tree = current_node.tree(); Self { pending_state: PendingState::Owned(PendingWidgetState { - focus: current_node - .tree + focus: tree .focused_widget() - .and_then(|id| current_node.tree.widget_from_node(id).map(|w| w.id())), - active: current_node - .tree + .and_then(|id| tree.widget_from_node(id).map(|w| w.id())), + active: tree .active_widget() - .and_then(|id| current_node.tree.widget_from_node(id).map(|w| w.id())), + .and_then(|id| tree.widget_from_node(id).map(|w| w.id())), focus_is_advancing: false, }), + tree, effective_styles: current_node.effective_styles(), cache: WidgetCacheKey { theme_mode, @@ -919,6 +900,7 @@ impl<'context, 'window> WidgetContext<'context, 'window> { /// Returns a new instance that borrows from `self`. pub fn borrowed(&mut self) -> WidgetContext<'_, 'window> { WidgetContext { + tree: self.tree.clone(), current_node: self.current_node.clone(), redraw_status: self.redraw_status, window: &mut *self.window, @@ -958,6 +940,7 @@ impl<'context, 'window> WidgetContext<'context, 'window> { enabled: current_node.enabled(&self.handle()), }, current_node, + tree: self.tree.clone(), redraw_status: self.redraw_status, window: &mut *self.window, theme, @@ -970,8 +953,7 @@ impl<'context, 'window> WidgetContext<'context, 'window> { /// Returns true if `possible_parent` is in this widget's parent list. #[must_use] pub fn is_child_of(&self, possible_parent: &WidgetInstance) -> bool { - self.current_node - .tree + self.tree .is_child(self.current_node.node_id, possible_parent) } @@ -1107,7 +1089,7 @@ impl<'context, 'window> WidgetContext<'context, 'window> { /// for more information. #[must_use] pub fn is_default(&self) -> bool { - self.current_node.tree.default_widget() == Some(self.current_node.node_id) + self.tree.default_widget() == Some(self.current_node.node_id) } /// Returns true if this widget is the target to activate when the user @@ -1118,7 +1100,7 @@ impl<'context, 'window> WidgetContext<'context, 'window> { /// for more information. #[must_use] pub fn is_escape(&self) -> bool { - self.current_node.tree.escape_widget() == Some(self.current_node.node_id) + self.tree.escape_widget() == Some(self.current_node.node_id) } /// Returns the widget this context is for. @@ -1329,7 +1311,7 @@ impl ManageWidget for WidgetId { type Managed = Option; fn manage(&self, context: &WidgetContext<'_, '_>) -> Self::Managed { - context.current_node.tree.widget(*self) + context.tree.widget(*self) } } @@ -1337,7 +1319,7 @@ impl ManageWidget for WidgetInstance { type Managed = Option; fn manage(&self, context: &WidgetContext<'_, '_>) -> Self::Managed { - context.current_node.tree.widget(self.id()) + context.tree.widget(self.id()) } } @@ -1346,7 +1328,7 @@ impl ManageWidget for WidgetRef { fn manage(&self, context: &WidgetContext<'_, '_>) -> Self::Managed { match self { - WidgetRef::Unmounted(instance) => context.current_node.tree.widget(instance.id()), + WidgetRef::Unmounted(instance) => context.tree.widget(instance.id()), WidgetRef::Mounted(instance) => Some(instance.clone()), } } diff --git a/src/tree.rs b/src/tree.rs index 74860b6..1a03005 100644 --- a/src/tree.rs +++ b/src/tree.rs @@ -1,5 +1,5 @@ use std::mem; -use std::sync::{Arc, Mutex}; +use std::sync::{Arc, Mutex, Weak}; use ahash::AHashMap; use alot::{LotId, Lots}; @@ -63,7 +63,7 @@ impl Tree { MountedWidget { node_id, widget, - tree: self.clone(), + tree: WeakTree(Arc::downgrade(&self.data)), } } @@ -421,7 +421,7 @@ impl TreeData { Some(MountedWidget { node_id, widget: self.nodes[node_id].widget.clone(), - tree: tree.clone(), + tree: WeakTree(Arc::downgrade(&tree.data)), }) } @@ -429,7 +429,7 @@ impl TreeData { Some(MountedWidget { node_id, widget: self.nodes.get(node_id)?.widget.clone(), - tree: tree.clone(), + tree: WeakTree(Arc::downgrade(&tree.data)), }) } @@ -621,3 +621,12 @@ struct CachedLayoutQuery { constraints: Size, size: Size, } + +#[derive(Clone, Debug)] +pub struct WeakTree(Weak>); + +impl WeakTree { + pub fn upgrade(&self) -> Option { + self.0.upgrade().map(|data| Tree { data }) + } +} diff --git a/src/widget.rs b/src/widget.rs index d1d3ecf..a54c985 100644 --- a/src/widget.rs +++ b/src/widget.rs @@ -34,7 +34,7 @@ use crate::styles::{ ComponentDefinition, ContainerLevel, Dimension, DimensionRange, Edges, IntoComponentValue, IntoDynamicComponentValue, Styles, ThemePair, VisualOrder, }; -use crate::tree::Tree; +use crate::tree::{Tree, WeakTree}; use crate::utils::IgnorePoison; use crate::value::{Dynamic, Generation, IntoDynamic, IntoValue, Validation, Value}; use crate::widgets::checkbox::{Checkable, CheckboxState}; @@ -1693,7 +1693,7 @@ where pub struct MountedWidget { pub(crate) node_id: LotId, pub(crate) widget: WidgetInstance, - pub(crate) tree: Tree, + pub(crate) tree: WeakTree, } impl Debug for MountedWidget { @@ -1703,6 +1703,10 @@ impl Debug for MountedWidget { } impl MountedWidget { + pub(crate) fn tree(&self) -> Tree { + self.tree.upgrade().expect("tree missing") + } + /// Locks the widget for exclusive access. Locking widgets should only be /// done for brief moments of time when you are certain no deadlocks can /// occur due to other widget locks being held. @@ -1713,11 +1717,14 @@ impl MountedWidget { /// Invalidates this widget. pub fn invalidate(&self) { - self.tree.invalidate(self.node_id, false); + let Some(tree) = self.tree.upgrade() else { + return; + }; + tree.invalidate(self.node_id, false); } pub(crate) fn set_layout(&self, rect: Rect) { - self.tree.set_layout(self.node_id, rect); + self.tree().set_layout(self.node_id, rect); } /// Returns the unique id of this widget instance. @@ -1740,7 +1747,7 @@ impl MountedWidget { pub fn next_focus(&self) -> Option { self.widget .next_focus() - .and_then(|next_focus| self.tree.widget(next_focus)) + .and_then(|next_focus| self.tree.upgrade()?.widget(next_focus)) } /// Returns the widget to focus before this widget. @@ -1749,7 +1756,7 @@ impl MountedWidget { /// automatically using [`MakeWidget::with_next_focus()`]. #[must_use] pub fn previous_focus(&self) -> Option { - self.tree.previous_focus(self.id()) + self.tree.upgrade()?.previous_focus(self.id()) } /// Returns the next or previous focus target, if one was set using @@ -1766,85 +1773,89 @@ impl MountedWidget { /// Returns the region that the widget was last rendered at. #[must_use] pub fn last_layout(&self) -> Option> { - self.tree.layout(self.node_id) + self.tree.upgrade()?.layout(self.node_id) } /// Returns the effective styles for the current tree. #[must_use] pub fn effective_styles(&self) -> Styles { - self.tree.effective_styles(self.node_id) + self.tree().effective_styles(self.node_id) } /// Returns true if this widget is the currently active widget. #[must_use] pub fn active(&self) -> bool { - self.tree.active_widget() == Some(self.node_id) + self.tree().active_widget() == Some(self.node_id) } pub(crate) fn enabled(&self, handle: &WindowHandle) -> bool { - self.tree.is_enabled(self.node_id, handle) + self.tree().is_enabled(self.node_id, handle) } /// Returns true if this widget is currently the hovered widget. #[must_use] pub fn hovered(&self) -> bool { - self.tree.is_hovered(self.node_id) + self.tree().is_hovered(self.node_id) } /// Returns true if this widget that is directly beneath the cursor. #[must_use] pub fn primary_hover(&self) -> bool { - self.tree.hovered_widget() == Some(self.node_id) + self.tree().hovered_widget() == Some(self.node_id) } /// Returns true if this widget is the currently focused widget. #[must_use] pub fn focused(&self) -> bool { - self.tree.focused_widget() == Some(self.node_id) + self.tree().focused_widget() == Some(self.node_id) } /// Returns the parent of this widget. #[must_use] pub fn parent(&self) -> Option { - self.tree - .parent(self.node_id) - .and_then(|id| self.tree.widget_from_node(id)) + let tree = self.tree.upgrade()?; + + tree.parent(self.node_id) + .and_then(|id| tree.widget_from_node(id)) } /// Returns true if this node has a parent. #[must_use] pub fn has_parent(&self) -> bool { - self.tree.parent(self.node_id).is_some() + let Some(tree) = self.tree.upgrade() else { + return false; + }; + tree.parent(self.node_id).is_some() } pub(crate) fn attach_styles(&self, styles: Value) { - self.tree.attach_styles(self.node_id, styles); + self.tree().attach_styles(self.node_id, styles); } pub(crate) fn attach_theme(&self, theme: Value) { - self.tree.attach_theme(self.node_id, theme); + self.tree().attach_theme(self.node_id, theme); } pub(crate) fn attach_theme_mode(&self, theme: Value) { - self.tree.attach_theme_mode(self.node_id, theme); + self.tree().attach_theme_mode(self.node_id, theme); } pub(crate) fn overidden_theme( &self, ) -> (Styles, Option>, Option>) { - self.tree.overriden_theme(self.node_id) + self.tree().overriden_theme(self.node_id) } pub(crate) fn begin_layout(&self, constraints: Size) -> Option> { - self.tree.begin_layout(self.node_id, constraints) + self.tree().begin_layout(self.node_id, constraints) } pub(crate) fn persist_layout(&self, constraints: Size, size: Size) { - self.tree.persist_layout(self.node_id, constraints, size); + self.tree().persist_layout(self.node_id, constraints, size); } pub(crate) fn visually_ordered_children(&self, order: VisualOrder) -> Vec { - self.tree.visually_ordered_children(self.node_id, order) + self.tree().visually_ordered_children(self.node_id, order) } } @@ -2302,7 +2313,7 @@ impl WidgetId { /// Finds this widget mounted in this window, if present. #[must_use] pub fn find_in(self, context: &WidgetContext<'_, '_>) -> Option { - context.widget().tree.widget(self) + context.tree.widget(self) } } diff --git a/src/window.rs b/src/window.rs index 9a88e05..6903882 100644 --- a/src/window.rs +++ b/src/window.rs @@ -400,6 +400,7 @@ pub trait WindowBehavior: Sized + 'static { struct GooeyWindow { behavior: T, + tree: Tree, root: MountedWidget, contents: Drawing, should_close: bool, @@ -445,11 +446,11 @@ where kludgine: &mut Kludgine, ) { if is_pressed { - if let Some(default) = widget.and_then(|id| self.root.tree.widget_from_node(id)) { + if let Some(default) = widget.and_then(|id| self.tree.widget_from_node(id)) { if let Some(previously_active) = self .keyboard_activated .take() - .and_then(|id| self.root.tree.widget(id)) + .and_then(|id| self.tree.widget(id)) { EventContext::new( WidgetContext::new( @@ -481,7 +482,7 @@ where } else if let Some(keyboard_activated) = self .keyboard_activated .take() - .and_then(|id| self.root.tree.widget(id)) + .and_then(|id| self.tree.widget(id)) { EventContext::new( WidgetContext::new( @@ -509,7 +510,7 @@ where let mut padding = Edges::::default(); loop { - let Some(managed) = self.root.tree.widget(root_or_child.id()) else { + let Some(managed) = self.tree.widget(root_or_child.id()) else { break; }; @@ -681,7 +682,8 @@ where &mut RunningWindow::new(window, &gooey, &focused, &occluded, &inner_size), context.user, ); - let root = Tree::default().push_boxed(behavior.make_root(), None); + let tree = Tree::default(); + let root = tree.push_boxed(behavior.make_root(), None); let (current_theme, theme) = match theme { Value::Constant(theme) => (theme, None), @@ -691,6 +693,7 @@ where Self { behavior, root, + tree, contents: Drawing::default(), should_close: false, cursor: CursorState { @@ -731,8 +734,7 @@ where self.redraw_status.refresh_received(); graphics.reset_text_attributes(); - self.root - .tree + self.tree .new_frame(self.redraw_status.invalidations().drain()); let resizable = window.winit().is_resizable(); @@ -941,8 +943,8 @@ where input: KeyEvent, is_synthetic: bool, ) { - let target = self.root.tree.focused_widget().unwrap_or(self.root.node_id); - let Some(target) = self.root.tree.widget_from_node(target) else { + let target = self.tree.focused_widget().unwrap_or(self.root.node_id); + let Some(target) = self.tree.widget_from_node(target) else { return; }; let mut window = RunningWindow::new( @@ -987,12 +989,8 @@ where if input.state.is_pressed() { let reverse = window.modifiers().state().shift_key(); - let target = self.root.tree.focused_widget().unwrap_or(self.root.node_id); - let target = self - .root - .tree - .widget_from_node(target) - .expect("missing widget"); + let target = self.tree.focused_widget().unwrap_or(self.root.node_id); + let target = self.tree.widget_from_node(target).expect("missing widget"); let mut target = EventContext::new( WidgetContext::new( target, @@ -1015,7 +1013,7 @@ where Key::Named(NamedKey::Enter) => { self.keyboard_activate_widget( input.state.is_pressed(), - self.root.tree.default_widget(), + self.tree.default_widget(), &mut window, kludgine, ); @@ -1023,7 +1021,7 @@ where Key::Named(NamedKey::Escape) => { self.keyboard_activate_widget( input.state.is_pressed(), - self.root.tree.escape_widget(), + self.tree.escape_widget(), &mut window, kludgine, ); @@ -1050,16 +1048,10 @@ where phase: TouchPhase, ) { let widget = self - .root .tree .hovered_widget() - .and_then(|hovered| self.root.tree.widget_from_node(hovered)) - .unwrap_or_else(|| { - self.root - .tree - .widget(self.root.id()) - .expect("missing widget") - }); + .and_then(|hovered| self.tree.widget_from_node(hovered)) + .unwrap_or_else(|| self.tree.widget(self.root.id()).expect("missing widget")); let mut window = RunningWindow::new( window, @@ -1093,16 +1085,10 @@ where ime: Ime, ) { let widget = self - .root .tree .focused_widget() - .and_then(|hovered| self.root.tree.widget_from_node(hovered)) - .unwrap_or_else(|| { - self.root - .tree - .widget(self.root.id()) - .expect("missing widget") - }); + .and_then(|hovered| self.tree.widget_from_node(hovered)) + .unwrap_or_else(|| self.tree.widget(self.root.id()).expect("missing widget")); let mut window = RunningWindow::new( window, &self.gooey, @@ -1160,7 +1146,7 @@ where if let Some(state) = self.mouse_buttons.get(&device_id) { // Mouse Drag for (button, handler) in state { - let Some(handler) = self.root.tree.widget(*handler) else { + let Some(handler) = self.tree.widget(*handler) else { continue; }; let mut context = EventContext::new( @@ -1244,7 +1230,7 @@ where if let (ElementState::Pressed, Some(location), Some(hovered)) = ( state, self.cursor.location, - self.cursor.widget.and_then(|id| self.root.tree.widget(id)), + self.cursor.widget.and_then(|id| self.tree.widget(id)), ) { if let Some(handler) = recursively_handle_event( &mut EventContext::new( @@ -1283,7 +1269,7 @@ where if device_buttons.is_empty() { self.mouse_buttons.remove(&device_id); } - let Some(handler) = self.root.tree.widget(handler) else { + let Some(handler) = self.tree.widget(handler) else { return; }; let cursor_location = self.cursor.location;