Skip to content

Commit

Permalink
fix(combobox): [iOS] Fix memory leak
Browse files Browse the repository at this point in the history
  • Loading branch information
@jeromelaban
jeromelaban committed Sep 29, 2020
1 parent 40fedb4 commit c85f2a3
Showing 1 changed file with 54 additions and 43 deletions.
97 changes: 54 additions & 43 deletions src/Uno.UI/UI/Xaml/Controls/ComboBox/ComboBox.cs
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
using System;
#nullable enable

using System;
using System.Collections.Generic;
using System.Text;
using System.Windows.Input;
Expand Down Expand Up @@ -40,21 +42,21 @@ namespace Windows.UI.Xaml.Controls
// Temporarily inheriting from ListViewBase instead of Selector to leverage existing selection and virtualization code
public partial class ComboBox : ListViewBase // TODO: Selector
{
public event EventHandler<object> DropDownClosed;
public event EventHandler<object> DropDownOpened;
public event EventHandler<object>? DropDownClosed;
public event EventHandler<object>? DropDownOpened;

private bool _areItemTemplatesForwarded = false;

private IPopup _popup;
private Border _popupBorder;
private ContentPresenter _contentPresenter;
private TextBlock _placeholderTextBlock;
private ContentPresenter _headerContentPresenter;
private IPopup? _popup;
private Border? _popupBorder;
private ContentPresenter? _contentPresenter;
private TextBlock? _placeholderTextBlock;
private ContentPresenter? _headerContentPresenter;

/// <summary>
/// The 'inline' parent view of the selected item within the dropdown list. This is only set if SelectedItem is a view type.
/// </summary>
private ManagedWeakReference _selectionParentInDropdown;
private ManagedWeakReference? _selectionParentInDropdown;

public ComboBox()
{
Expand Down Expand Up @@ -99,21 +101,25 @@ protected override void OnApplyTemplate()
{
_contentPresenter.SynchronizeContentWithOuterTemplatedParent = false;

var thisRef = (this as IWeakReferenceProvider).WeakReference;
_contentPresenter.DataContextChanged += (snd, evt) =>
{
// The ContentPresenter will automatically clear its local DataContext
// on first load.
//
// When there's no selection, this will cause this ContentPresenter to
// received the same DataContext as the ComboBox itself, which could
// lead to strange result or errors.
//
// See comments in ContentPresenter.ResetDataContextOnFirstLoad() method.
// Fixed in this PR: https://github.com/unoplatform/uno/pull/1465
if (evt.NewValue != null && SelectedItem == null)
if (thisRef.Target is ComboBox that)
{
_contentPresenter.DataContext = null; // Remove problematic inherited DataContext
// The ContentPresenter will automatically clear its local DataContext
// on first load.
//
// When there's no selection, this will cause this ContentPresenter to
// received the same DataContext as the ComboBox itself, which could
// lead to strange result or errors.
//
// See comments in ContentPresenter.ResetDataContextOnFirstLoad() method.
// Fixed in this PR: https://github.com/unoplatform/uno/pull/1465
if (evt.NewValue != null && that.SelectedItem == null && that._contentPresenter != null)
{
that._contentPresenter.DataContext = null; // Remove problematic inherited DataContext
}
}
};

Expand Down Expand Up @@ -335,7 +341,9 @@ private void RestoreSelectedItem(_View selectionView)
#endif

// Sanity check, ensure parent is still valid (ComboBoxItem may have been recycled)
if (comboBoxItem?.Content == selectionView && selectionView.GetVisualTreeParent() != dropdownParent)
if (dropdownParent != null
&& comboBoxItem?.Content == selectionView
&& selectionView.GetVisualTreeParent() != dropdownParent)
{
dropdownParent.AddChild(selectionView);
}
Expand All @@ -354,15 +362,15 @@ protected override void OnIsEnabledChanged(bool oldValue, bool newValue)

partial void OnIsDropDownOpenChangedPartial(bool oldIsDropDownOpen, bool newIsDropDownOpen)
{
// This method will load the itempresenter children
if (_popup != null)
{
// This method will load the itempresenter children
#if __ANDROID__
SetItemsPresenter((_popup.Child as ViewGroup).FindFirstChild<ItemsPresenter>());
SetItemsPresenter((_popup.Child as ViewGroup).FindFirstChild<ItemsPresenter>());
#elif __IOS__ || __MACOS__
SetItemsPresenter(_popup.Child.FindFirstChild<ItemsPresenter>());
SetItemsPresenter(_popup.Child.FindFirstChild<ItemsPresenter>());
#endif

if (_popup != null)
{
_popup.IsOpen = newIsDropDownOpen;
}

Expand Down Expand Up @@ -450,19 +458,22 @@ internal Brush LightDismissOverlayBackground

private class DropDownLayouter : PopupBase.IDynamicPopupLayouter
{
private readonly ComboBox _combo;
private readonly PopupBase _popup;
private ManagedWeakReference _combo;
private ManagedWeakReference _popup;

private ComboBox? Combo => _combo.Target as ComboBox;
private PopupBase? Popup => _popup.Target as Popup;

public DropDownLayouter(ComboBox combo, PopupBase popup)
{
_combo = combo;
_popup = popup;
_combo = (combo as IWeakReferenceProvider).WeakReference;
_popup = (popup as IWeakReferenceProvider).WeakReference;
}

/// <inheritdoc />
public Size Measure(Size available, Size visibleSize)
{
if (!(_popup.Child is FrameworkElement child))
if (!(Popup?.Child is FrameworkElement child) || Combo == null)
{
return new Size();
}
Expand All @@ -476,7 +487,7 @@ public Size Measure(Size available, Size visibleSize)
// MaxWidth
// MaxHeight

if (_combo.IsPopupFullscreen)
if (Combo.IsPopupFullscreen)
{
// Size : Note we set both Min and Max to match the UWP behavior which alter only those
// properties. The MinHeight is not set to allow the the root child control to specificy
Expand All @@ -489,10 +500,10 @@ public Size Measure(Size available, Size visibleSize)
{
// Set the popup child as max 9 x the height of the combo
// (UWP seams to actually limiting to 9 visible items ... which is not necessarily the 9 x the combo height)
var maxHeight = Math.Min(visibleSize.Height, Math.Min(_combo.MaxDropDownHeight, _combo.ActualHeight * _itemsToShow));
var maxHeight = Math.Min(visibleSize.Height, Math.Min(Combo.MaxDropDownHeight, Combo.ActualHeight * _itemsToShow));

child.MinHeight = _combo.ActualHeight;
child.MinWidth = _combo.ActualWidth;
child.MinHeight = Combo.ActualHeight;
child.MinWidth = Combo.ActualWidth;
child.MaxHeight = maxHeight;
child.MaxWidth = visibleSize.Width;
}
Expand All @@ -507,12 +518,12 @@ public Size Measure(Size available, Size visibleSize)
/// <inheritdoc />
public void Arrange(Size finalSize, Rect visibleBounds, Size desiredSize, Point? upperLeftLocation)
{
if (!(_popup.Child is FrameworkElement child))
if (!(Popup?.Child is FrameworkElement child) || Combo == null)
{
return;
}

if (_combo.IsPopupFullscreen)
if (Combo.IsPopupFullscreen)
{
Point getChildLocation()
{
Expand All @@ -539,7 +550,7 @@ Point getChildLocation()
return;
}

var comboRect = _combo.GetAbsoluteBoundsRect();
var comboRect = Combo.GetAbsoluteBoundsRect();
var frame = new Rect(comboRect.Location, desiredSize.AtMost(visibleBounds.Size));

// On windows, the popup is Y-aligned accordingly to the selected item in order to keep
Expand All @@ -551,14 +562,14 @@ Point getChildLocation()
// which might not be ready at this point (we could try a 2-pass arrange), and to scroll into view to make it visible.
// So for now we only rely on the SelectedIndex and make a highly improvable vertical alignment based on it.

var itemsCount = _combo.NumberOfItems;
var selectedIndex = _combo.SelectedIndex;
var itemsCount = Combo.NumberOfItems;
var selectedIndex = Combo.SelectedIndex;
if (selectedIndex < 0 && itemsCount > 0)
{
selectedIndex = itemsCount / 2;
}

var placement = Uno.UI.Xaml.Controls.ComboBox.GetDropDownPreferredPlacement(_combo);
var placement = Uno.UI.Xaml.Controls.ComboBox.GetDropDownPreferredPlacement(Combo);
var stickyThreshold = Math.Max(1, Math.Min(4, (itemsCount / 2) - 1));
switch (placement)
{
Expand All @@ -573,7 +584,7 @@ Point getChildLocation()
// As we don't scroll into view to the selected item, this case seems awkward if the selected item
// is not directly visible (i.e. without scrolling) when the drop-down appears.
// So if we detect that we should had to scroll to make it visible, we don't try to appear above!
&& (itemsCount <= _itemsToShow && frame.Height < (_combo.ActualHeight * _itemsToShow) - 3):
&& (itemsCount <= _itemsToShow && frame.Height < (Combo.ActualHeight * _itemsToShow) - 3):

frame.Y = comboRect.Bottom - frame.Height;
break;
Expand Down

0 comments on commit c85f2a3

Please sign in to comment.