Fixed reference cyle in ManagedWidget

This commit is contained in:
Jonathan Johnson 2023-12-22 09:51:06 -08:00
parent 564b9e5a96
commit 6b33b0f686
No known key found for this signature in database
GPG key ID: A66D6A34D6620579
6 changed files with 113 additions and 118 deletions

View file

@ -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<Result<T,E>>` to the

4
Cargo.lock generated
View file

@ -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",

View file

@ -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<Px>) {
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<MountedWidget>;
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<MountedWidget>;
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()),
}
}

View file

@ -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<ConstraintLimit>,
size: Size<UPx>,
}
#[derive(Clone, Debug)]
pub struct WeakTree(Weak<Mutex<TreeData>>);
impl WeakTree {
pub fn upgrade(&self) -> Option<Tree> {
self.0.upgrade().map(|data| Tree { data })
}
}

View file

@ -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<Px>) {
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<MountedWidget> {
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<MountedWidget> {
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<Rect<Px>> {
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<MountedWidget> {
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<Styles>) {
self.tree.attach_styles(self.node_id, styles);
self.tree().attach_styles(self.node_id, styles);
}
pub(crate) fn attach_theme(&self, theme: Value<ThemePair>) {
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<ThemeMode>) {
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<Value<ThemePair>>, Option<Value<ThemeMode>>) {
self.tree.overriden_theme(self.node_id)
self.tree().overriden_theme(self.node_id)
}
pub(crate) fn begin_layout(&self, constraints: Size<ConstraintLimit>) -> Option<Size<UPx>> {
self.tree.begin_layout(self.node_id, constraints)
self.tree().begin_layout(self.node_id, constraints)
}
pub(crate) fn persist_layout(&self, constraints: Size<ConstraintLimit>, size: Size<UPx>) {
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<MountedWidget> {
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<MountedWidget> {
context.widget().tree.widget(self)
context.tree.widget(self)
}
}

View file

@ -400,6 +400,7 @@ pub trait WindowBehavior: Sized + 'static {
struct GooeyWindow<T> {
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::<Px>::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;