From dc8a039600cec7c95d78a8dcaf35fbfcacfbdc08 Mon Sep 17 00:00:00 2001 From: Curtis Wensley Date: Wed, 22 May 2024 13:06:08 -0700 Subject: [PATCH] Mac: Fix some memory leaks - NotificationCenter observers are now added during OnLoad, removed during OnUnLoad - Use weak references for handlers in cell views --- src/Eto.Mac/Forms/Cells/CellHandler.cs | 7 +- .../Forms/Cells/CheckBoxCellHandler.cs | 9 +- .../Forms/Cells/ComboBoxCellHandler.cs | 9 +- src/Eto.Mac/Forms/Cells/CustomCellHandler.cs | 24 +++-- src/Eto.Mac/Forms/Cells/TextBoxCellHandler.cs | 30 ++++-- src/Eto.Mac/Forms/Controls/GridViewHandler.cs | 4 +- .../Forms/Controls/TreeGridViewHandler.cs | 4 +- src/Eto.Mac/Forms/MacBase.cs | 91 +++++++++++++++---- src/Eto.Mac/Forms/MacView.cs | 4 + 9 files changed, 140 insertions(+), 42 deletions(-) diff --git a/src/Eto.Mac/Forms/Cells/CellHandler.cs b/src/Eto.Mac/Forms/Cells/CellHandler.cs index d177fbde68..bf5e950bcc 100644 --- a/src/Eto.Mac/Forms/Cells/CellHandler.cs +++ b/src/Eto.Mac/Forms/Cells/CellHandler.cs @@ -84,7 +84,12 @@ public override bool ResignFirstResponder() public abstract class CellHandler : MacObject, Cell.IHandler, ICellHandler where TWidget: Cell { - public IDataColumnHandler ColumnHandler { get; set; } + WeakReference _columnHandler; + public IDataColumnHandler ColumnHandler + { + get => _columnHandler?.Target as IDataColumnHandler; + set => _columnHandler = new WeakReference(value); + } public virtual bool Editable { get; set; } diff --git a/src/Eto.Mac/Forms/Cells/CheckBoxCellHandler.cs b/src/Eto.Mac/Forms/Cells/CheckBoxCellHandler.cs index a5f632fbc9..b95cb64ac0 100644 --- a/src/Eto.Mac/Forms/Cells/CheckBoxCellHandler.cs +++ b/src/Eto.Mac/Forms/Cells/CheckBoxCellHandler.cs @@ -107,15 +107,18 @@ public override NSView GetViewForItem(NSTableView tableView, NSTableColumn table var col = Array.IndexOf(tableView.TableColumns(), tableColumn); view.Activated += (sender, e) => { + var colHandler = ColumnHandler; + if (colHandler == null) + return; var control = (CellView)sender; var r = (int)control.Tag; var item = getItem(control.Item, r); - var ee = MacConversions.CreateCellEventArgs(ColumnHandler.Widget, tableView, r, col, item); - ColumnHandler.DataViewHandler.OnCellEditing(ee); + var ee = MacConversions.CreateCellEventArgs(colHandler.Widget, tableView, r, col, item); + colHandler.DataViewHandler?.OnCellEditing(ee); SetObjectValue(item, control.ObjectValue); control.ObjectValue = GetObjectValue(item); - ColumnHandler.DataViewHandler.OnCellEdited(ee); + colHandler.DataViewHandler?.OnCellEdited(ee); }; } view.Tag = row; diff --git a/src/Eto.Mac/Forms/Cells/ComboBoxCellHandler.cs b/src/Eto.Mac/Forms/Cells/ComboBoxCellHandler.cs index 2390ea8394..d7099e54ee 100644 --- a/src/Eto.Mac/Forms/Cells/ComboBoxCellHandler.cs +++ b/src/Eto.Mac/Forms/Cells/ComboBoxCellHandler.cs @@ -241,14 +241,17 @@ public override NSView GetViewForItem(NSTableView tableView, NSTableColumn table }; view.Activated += (sender, e) => { + var colHandler = ColumnHandler; + if (colHandler == null) + return; var control = (CellView)sender; var r = (int)control.Tag; var item = getItem(control.Item, r); - var cellArgs = MacConversions.CreateCellEventArgs(ColumnHandler.Widget, tableView, r, col, item); - ColumnHandler.DataViewHandler.OnCellEditing(cellArgs); + var cellArgs = MacConversions.CreateCellEventArgs(colHandler.Widget, tableView, r, col, item); + colHandler.DataViewHandler?.OnCellEditing(cellArgs); SetObjectValue(item, control.ObjectValue); - ColumnHandler.DataViewHandler.OnCellEdited(cellArgs); + colHandler.DataViewHandler?.OnCellEdited(cellArgs); control.ObjectValue = GetObjectValue(item); }; view.Bind(enabledBinding, tableColumn, "editable", null); diff --git a/src/Eto.Mac/Forms/Cells/CustomCellHandler.cs b/src/Eto.Mac/Forms/Cells/CustomCellHandler.cs index b3f840d9b7..03137a7a2f 100644 --- a/src/Eto.Mac/Forms/Cells/CustomCellHandler.cs +++ b/src/Eto.Mac/Forms/Cells/CustomCellHandler.cs @@ -45,10 +45,16 @@ public override nfloat GetPreferredWidth(object value, CGSize cellSize, int row, public class EtoCustomCellView : NSTableCellView { - public CustomCellHandler Handler { get; set;} + WeakReference _handler; + public CustomCellHandler Handler + { + get => _handler?.Target as CustomCellHandler; + set => _handler = new WeakReference(value); + } + public MutableCellEventArgs Args { get; set; } - public Control EtoControl { get; set; } + public Control EtoControl => Args.Control; public bool DrawsBackground { get; set; } @@ -93,13 +99,16 @@ private void ControlLostFocus(object sender, EventArgs e) if (losingFocus) return; - losingFocus = true; var h = Handler; + if (h == null) + return; + + losingFocus = true; var ee = MacConversions.CreateCellEventArgs(h.ColumnHandler.Widget, null, Args.Row, Args.Column, Args.Item); - if (!h.ColumnHandler.DataViewHandler.IsCancellingEdit) + if (h.ColumnHandler.DataViewHandler?.IsCancellingEdit == false) { h.Callback.OnCommitEdit(h.Widget, Args); - h.ColumnHandler.DataViewHandler.OnCellEdited(ee); + h.ColumnHandler.DataViewHandler?.OnCellEdited(ee); } else { @@ -111,9 +120,11 @@ private void ControlLostFocus(object sender, EventArgs e) private void ControlGotFocus(object sender, EventArgs e) { var h = Handler; + if (h == null) + return; h.Callback.OnBeginEdit(h.Widget, Args); var ee = MacConversions.CreateCellEventArgs(h.ColumnHandler.Widget, null, Args.Row, Args.Column, Args.Item); - h.ColumnHandler.DataViewHandler.OnCellEditing(ee); + h.ColumnHandler.DataViewHandler?.OnCellEditing(ee); } public override void Layout() @@ -168,7 +179,6 @@ public override NSView GetViewForItem(NSTableView tableView, NSTableColumn table { Handler = this, Args = args, - EtoControl = control, Identifier = identifier, AutoresizingMask = NSViewResizingMask.HeightSizable | NSViewResizingMask.WidthSizable }; diff --git a/src/Eto.Mac/Forms/Cells/TextBoxCellHandler.cs b/src/Eto.Mac/Forms/Cells/TextBoxCellHandler.cs index 82f439d6bd..a5f9b732f5 100644 --- a/src/Eto.Mac/Forms/Cells/TextBoxCellHandler.cs +++ b/src/Eto.Mac/Forms/Cells/TextBoxCellHandler.cs @@ -129,37 +129,55 @@ public override NSView GetViewForItem(NSTableView tableView, NSTableColumn table var col = Array.IndexOf(tableView.TableColumns(), tableColumn); view.BecameFirstResponder += (sender, e) => { + var colHandler = ColumnHandler; + if (colHandler == null) + return; + var table = colHandler.DataViewHandler?.Table; + if (table == null) + return; var control = (CellView)sender; var r = (int)control.Tag; var item = getItem(control.Item, r); - var ee = MacConversions.CreateCellEventArgs(ColumnHandler.Widget, tableView, r, col, item); - ColumnHandler.DataViewHandler.OnCellEditing(ee); + var ee = MacConversions.CreateCellEventArgs(colHandler.Widget, table, r, col, item); + colHandler.DataViewHandler?.OnCellEditing(ee); }; view.EditingEnded += (sender, e) => { + var colHandler = ColumnHandler; + if (colHandler == null) + return; + var table = colHandler.DataViewHandler?.Table; + if (table == null) + return; var notification = (NSNotification)sender; var control = (CellView)notification.Object; var r = (int)control.Tag; var item = getItem(control.Item, r); SetObjectValue(item, control.ObjectValue); - var ee = MacConversions.CreateCellEventArgs(ColumnHandler.Widget, tableView, r, col, item); - ColumnHandler.DataViewHandler.OnCellEdited(ee); + var ee = MacConversions.CreateCellEventArgs(colHandler.Widget, table, r, col, item); + colHandler.DataViewHandler?.OnCellEdited(ee); control.ObjectValue = GetObjectValue(item) ?? new NSString(string.Empty); }; bool isResigning = false; view.ResignedFirstResponder += (sender, e) => { + var colHandler = ColumnHandler; + if (colHandler == null) + return; if (isResigning) return; + var table = colHandler.DataViewHandler?.Table; + if (table == null) + return; isResigning = true; var control = (CellView)sender; var r = (int)control.Tag; var item = getItem(control.Item, r); SetObjectValue(item, control.ObjectValue); - var ee = MacConversions.CreateCellEventArgs(ColumnHandler.Widget, tableView, r, col, item); - ColumnHandler.DataViewHandler.OnCellEdited(ee); + var ee = MacConversions.CreateCellEventArgs(colHandler.Widget, table, r, col, item); + colHandler.DataViewHandler?.OnCellEdited(ee); isResigning = false; }; view.Bind(editableBinding, tableColumn, "editable", null); diff --git a/src/Eto.Mac/Forms/Controls/GridViewHandler.cs b/src/Eto.Mac/Forms/Controls/GridViewHandler.cs index 061cfa98bc..397ebc980d 100644 --- a/src/Eto.Mac/Forms/Controls/GridViewHandler.cs +++ b/src/Eto.Mac/Forms/Controls/GridViewHandler.cs @@ -76,8 +76,8 @@ public override void MouseDown(NSEvent theEvent) public EtoTableView(GridViewHandler handler) { FocusRingType = NSFocusRingType.None; - DataSource = new EtoTableViewDataSource { Handler = handler }; - Delegate = new EtoTableDelegate { Handler = handler }; + WeakDataSource = new EtoTableViewDataSource { Handler = handler }; + WeakDelegate = new EtoTableDelegate { Handler = handler }; ColumnAutoresizingStyle = NSTableViewColumnAutoresizingStyle.Uniform; SetDraggingSourceOperationMask(NSDragOperation.All, true); SetDraggingSourceOperationMask(NSDragOperation.All, false); diff --git a/src/Eto.Mac/Forms/Controls/TreeGridViewHandler.cs b/src/Eto.Mac/Forms/Controls/TreeGridViewHandler.cs index 530cb0bbbe..a04b24a8bf 100644 --- a/src/Eto.Mac/Forms/Controls/TreeGridViewHandler.cs +++ b/src/Eto.Mac/Forms/Controls/TreeGridViewHandler.cs @@ -646,8 +646,8 @@ public override void MouseDown(NSEvent theEvent) public EtoOutlineView(TreeGridViewHandler handler) { - Delegate = new EtoOutlineDelegate { Handler = handler }; - DataSource = new EtoDataSource { Handler = handler }; + WeakDelegate = new EtoOutlineDelegate { Handler = handler }; + WeakDataSource = new EtoDataSource { Handler = handler }; //HeaderView = null, AutoresizesOutlineColumn = false; //AllowsColumnResizing = false, diff --git a/src/Eto.Mac/Forms/MacBase.cs b/src/Eto.Mac/Forms/MacBase.cs index 7697322591..411aff2743 100644 --- a/src/Eto.Mac/Forms/MacBase.cs +++ b/src/Eto.Mac/Forms/MacBase.cs @@ -17,12 +17,20 @@ public interface IMacControlHandler void InvalidateMeasure(); } + + public enum ObserverType + { + Control, + NotificationCenter + } [Register("ObserverHelper")] public class ObserverHelper : NSObject { - bool isNotification; - bool isControl; + bool hasNotification; + bool hasControl; + + public ObserverType Type { get; set; } public Action Action { get; set; } @@ -50,25 +58,33 @@ public override void ObserveValue(NSString keyPath, NSObject ofObject, NSDiction { Action(new ObserverActionEventArgs(this, null)); } - - public void AddToNotificationCenter() + + public void Register() + { + if (Type == ObserverType.NotificationCenter) + AddToNotificationCenter(); + else + AddToControl(); + } + + void AddToNotificationCenter() { var c = Control; - if (!isNotification && c != null) + if (!hasNotification && c != null) { NSNotificationCenter.DefaultCenter.AddObserver(this, selPerformAction, KeyPath, c); - isNotification = true; + hasNotification = true; } } - public void AddToControl() + void AddToControl() { var c = Control; - if (!isControl && c != null) + if (!hasControl && c != null) { //Console.WriteLine ("{0}: 3. Adding observer! {1}, {2}", ((IRef)this.Handler).WidgetID, this.GetType (), Control.GetHashCode ()); c.AddObserver(this, KeyPath, NSKeyValueObservingOptions.New, IntPtr.Zero); - isControl = true; + hasControl = true; } } @@ -77,17 +93,17 @@ public void AddToControl() public void Remove() { // we use the handle here to remove as it may have been GC'd but we still need to remove it! - if (isNotification) + if (hasNotification) { NSNotificationCenter.DefaultCenter.RemoveObserver(this); - isNotification = false; + hasNotification = false; } - if (isControl) + if (hasControl) { //Console.WriteLine ("{0}: 4. Removing observer! {1}, {2}", ((IRef)this.Handler).WidgetID, Handler.GetType (), Control.GetHashCode ()); Messaging.void_objc_msgSend_IntPtr_IntPtr(ControlHandle, selRemoveObserverForKeyPath_Handle, Handle, KeyPath.Handle); - isControl = false; + hasControl = false; } } @@ -147,6 +163,12 @@ public abstract class MacBase : WidgetHandler + /// Return true to delay notification center observers, then use + /// when appropriate to register them, and when removed (usually on OnUnload). + /// + internal virtual bool DelayRegisterNotificationCenter => false; + protected override void Initialize() { base.Initialize(); @@ -193,7 +215,6 @@ public bool HasMethod(IntPtr selector, object control) return ObjCExtensions.GetInstanceMethod(classHandle, selector) != IntPtr.Zero; } - public NSObject AddObserver(NSString key, Action action, NSObject control) { if (observers == null) @@ -202,12 +223,14 @@ public NSObject AddObserver(NSString key, Action action } var observer = new ObserverHelper { + Type = ObserverType.NotificationCenter, Action = action, KeyPath = key, ControlHandle = control.Handle, Handler = this }; - observer.AddToNotificationCenter(); + if (!DelayRegisterNotificationCenter) + observer.Register(); observers.Add(observer); return observer; } @@ -220,16 +243,50 @@ public void AddControlObserver(NSString key, Action act } var observer = new ObserverHelper { + Type = ObserverType.Control, Action = action, KeyPath = key, ControlHandle = control.Handle, Handler = this }; - observer.AddToControl(); + observer.Register(); observers.Add(observer); } protected override void Dispose(bool disposing) + { + RemoveAllObservers(); + + base.Dispose(disposing); + } + + internal void RegisterDelayedNotifications() + { + if (observers != null) + { + for (int i = 0; i < observers.Count; i++) + { + var observer = observers[i]; + // this will only register if not already + observer.Register(); + } + } + } + + internal void RemoveNotificationCenterObservers() + { + if (observers != null) + { + for (int i = 0; i < observers.Count; i++) + { + var observer = observers[i]; + if (observer.Type == ObserverType.NotificationCenter) + observer.Remove(); + } + } + } + + internal void RemoveAllObservers() { if (observers != null) { @@ -240,8 +297,6 @@ protected override void Dispose(bool disposing) } observers = null; } - - base.Dispose(disposing); } } } diff --git a/src/Eto.Mac/Forms/MacView.cs b/src/Eto.Mac/Forms/MacView.cs index bac74458bb..edefe6ed0e 100644 --- a/src/Eto.Mac/Forms/MacView.cs +++ b/src/Eto.Mac/Forms/MacView.cs @@ -642,6 +642,8 @@ public abstract partial class MacView : MacObject< public virtual IEnumerable VisualControls => Enumerable.Empty(); + internal override bool DelayRegisterNotificationCenter => true; + protected virtual Size DefaultMinimumSize => Size.Empty; public virtual Size MinimumSize @@ -1201,6 +1203,7 @@ public virtual void OnPreLoad(EventArgs e) public virtual void OnLoad(EventArgs e) { + RegisterDelayedNotifications(); if (Widget.VisualParent?.Loaded != false && !(Widget is Window)) { // adding dynamically or loading without a parent (e.g. embedding into a native app) @@ -1224,6 +1227,7 @@ public virtual void OnLoadComplete(EventArgs e) public virtual void OnUnLoad(EventArgs e) { mouseDelegate?.FireMouseLeaveIfNeeded(false); + RemoveNotificationCenterObservers(); } public virtual void OnKeyDown(KeyEventArgs e) => Callback.OnKeyDown(Widget, e);