Skip to content

TabControlEx: Fixing potential leaks of ContentPresenter #130

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
punker76 merged 2 commits into from
Aug 4, 2020
Merged

TabControlEx: Fixing potential leaks of ContentPresenter #130

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d64c932
First try to fix wrong cleanup code
batzen Jul 14, 2020
8c0787c
Fixing potential leak of content presenters while ItemContainerGenera…
batzen Jul 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 97 additions & 0 deletions src/ControlzEx.Tests/Controls/TabControlExTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
namespace ControlzEx.Tests.Controls
{
using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.Linq;
using System.Reflection;
using System.Windows.Controls;
using ControlzEx.Controls;
using ControlzEx.Tests.TestClasses;
using NUnit.Framework;

[TestFixture]
public class TabControlExTests
{
[Test]
public void TestAddRemoveInsertWithItemsSource()
{
var items = new ObservableCollection<string>
{
"1",
"2",
"3"
};

var tabControl = new TabControlEx
{
ItemsSource = items
};

using (new TestWindow(tabControl))
{
var itemsPanel = (Panel)tabControl.GetType().GetField("itemsHolder", BindingFlags.Instance | BindingFlags.NonPublic).GetValue(tabControl);

var tabItemsInitial = GetTabItems(tabControl);

Assert.That(tabItemsInitial, Has.Count.EqualTo(3));

Assert.That(itemsPanel.Children.Count, Is.EqualTo(1));

foreach (var tabItem in tabItemsInitial)
{
tabItem.IsSelected = true;
UITestHelper.DoEvents();
}

Assert.That(itemsPanel.Children.Count, Is.EqualTo(3));

items.RemoveAt(1);
items.Insert(1, "2");

UITestHelper.DoEvents();

Assert.That(itemsPanel.Children.Count, Is.EqualTo(2));
}
}

[Test]
public void TestItemContainerGeneratorRefresh()
{
var items = new ObservableCollection<string>
{
"1",
"2",
"3"
};

var tabControl = new TabControlEx
{
ItemsSource = items
};

using (new TestWindow(tabControl))
{
var itemsPanel = (Panel)tabControl.GetType().GetField("itemsHolder", BindingFlags.Instance | BindingFlags.NonPublic).GetValue(tabControl);

var tabItemsInitial = GetTabItems(tabControl);

Assert.That(tabItemsInitial, Has.Count.EqualTo(3));

Assert.That(itemsPanel.Children.Count, Is.EqualTo(1));

tabControl.ItemContainerGenerator.GetType().GetMethod("Refresh", BindingFlags.NonPublic | BindingFlags.Instance).Invoke(tabControl.ItemContainerGenerator, null);

UITestHelper.DoEvents();

Assert.That(itemsPanel.Children.Count, Is.EqualTo(1));
}
}

private static IList<TabItem> GetTabItems(TabControlEx tabControl)
{
return Enumerable.Range(0, tabControl.ItemContainerGenerator.Items.Count).Select(x => tabControl.ItemContainerGenerator.ContainerFromIndex(x))
.Cast<TabItem>()
.ToList();
}
}
}
20 changes: 20 additions & 0 deletions src/ControlzEx.Tests/TestClasses/UITestHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
namespace ControlzEx.Tests.TestClasses
{
using System.Windows.Threading;

public static class UITestHelper
{
public static void DoEvents()
{
var frame = new DispatcherFrame();
Dispatcher.CurrentDispatcher.BeginInvoke(DispatcherPriority.Background,
new DispatcherOperationCallback(
delegate(object f)
{
((DispatcherFrame)f).Continue = false;
return null;
}), frame);
Dispatcher.PushFrame(frame);
}
}
}
2 changes: 1 addition & 1 deletion src/ControlzEx/Automation/Peers/TabItemExAutomationPeer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ protected override List<AutomationPeer> GetChildrenCore()
return headerChildren;
}

var contentHost = parentTabControl.FindChildContentPresenter(tabItem.Content);
var contentHost = parentTabControl.FindChildContentPresenter(tabItem.Content, tabItem);

if (contentHost != null)
{
Expand Down
101 changes: 70 additions & 31 deletions src/ControlzEx/Controls/TabControlEx.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,34 +31,72 @@ namespace ControlzEx.Controls
/// http://stackoverflow.com/a/10210889/920384
/// http://stackoverflow.com/a/7838955/920384
/// </summary>
[TemplatePart(Name = "PART_ItemsHolder", Type = typeof(Panel))]
/// <remarks>
/// We use two attached properties to later recognize the content presenters we generated.
/// We need the OwningItem because the TabItem associated with an item can later change.
///
/// We need the OwningTabItem to reduce the amount of lookups we have to do.
/// </remarks>
[TemplatePart(Name = "PART_HeaderPanel", Type = typeof(Panel))]
[TemplatePart(Name = "PART_ItemsHolder", Type = typeof(Panel))]
public class TabControlEx : TabControl
{
private static readonly MethodInfo updateSelectedContentMethodInfo = typeof(TabControl).GetMethod("UpdateSelectedContent", BindingFlags.NonPublic | BindingFlags.Instance);

private Panel itemsHolder;

/// <summary>Identifies the <see cref="ChildContentVisibility"/> dependency property.</summary>
public static readonly DependencyProperty ChildContentVisibilityProperty
= DependencyProperty.Register(nameof(ChildContentVisibility), typeof(Visibility), typeof(TabControlEx), new PropertyMetadata(Visibility.Visible));

/// <summary>Identifies the <see cref="TabPanelVisibility"/> dependency property.</summary>
public static readonly DependencyProperty TabPanelVisibilityProperty =
DependencyProperty.Register(nameof(TabPanelVisibility), typeof(Visibility), typeof(TabControlEx), new PropertyMetadata(Visibility.Visible));

public static readonly DependencyProperty OwningTabItemProperty = DependencyProperty.RegisterAttached("OwningTabItem", typeof(TabItem), typeof(TabControlEx), new PropertyMetadata(default(TabItem)));

/// <summary>Helper for getting <see cref="OwningTabItemProperty"/> from <paramref name="element"/>.</summary>
/// <param name="element"><see cref="DependencyObject"/> to read <see cref="OwningTabItemProperty"/> from.</param>
/// <returns>OwningTabItem property value.</returns>
[AttachedPropertyBrowsableForType(typeof(ContentPresenter))]
public static TabItem GetOwningTabItem(DependencyObject element)
{
return (TabItem)element.GetValue(OwningTabItemProperty);
}

/// <summary>Helper for setting <see cref="OwningTabItemProperty"/> on <paramref name="element"/>.</summary>
/// <param name="element"><see cref="DependencyObject"/> to set <see cref="OwningTabItemProperty"/> on.</param>
/// <param name="value">OwningTabItem property value.</param>
public static void SetOwningTabItem(DependencyObject element, TabItem value)
{
element.SetValue(OwningTabItemProperty, value);
}

public static readonly DependencyProperty OwningItemProperty = DependencyProperty.RegisterAttached("OwningItem", typeof(object), typeof(TabControlEx), new PropertyMetadata(default(object)));

/// <summary>Helper for setting <see cref="OwningItemProperty"/> on <paramref name="element"/>.</summary>
/// <param name="element"><see cref="DependencyObject"/> to set <see cref="OwningItemProperty"/> on.</param>
/// <param name="value">OwningItem property value.</param>
public static void SetOwningItem(DependencyObject element, object value)
{
element.SetValue(OwningItemProperty, value);
}

/// <summary>Helper for getting <see cref="OwningItemProperty"/> from <paramref name="element"/>.</summary>
/// <param name="element"><see cref="DependencyObject"/> to read <see cref="OwningItemProperty"/> from.</param>
/// <returns>OwningItem property value.</returns>
[AttachedPropertyBrowsableForType(typeof(ContentPresenter))]
public static object GetOwningItem(DependencyObject element)
{
return (object)element.GetValue(OwningItemProperty);
}

/// <summary>Identifies the <see cref="MoveFocusToContentWhenSelectionChanges"/> dependency property.</summary>
Comment thread
punker76 marked this conversation as resolved.
public static readonly DependencyProperty MoveFocusToContentWhenSelectionChangesProperty = DependencyProperty.Register(nameof(MoveFocusToContentWhenSelectionChanges), typeof(bool), typeof(TabControlEx), new PropertyMetadata(default(bool)));

/// <summary>
/// Gets or sets whether keyboard focus should be moved to the content area when the selected item changes.
/// </summary>
public bool MoveFocusToContentWhenSelectionChanges
{
get => (bool)this.GetValue(MoveFocusToContentWhenSelectionChangesProperty);
Expand Down Expand Up @@ -113,28 +151,13 @@ public override void OnApplyTemplate()
this.RefreshItemsHolder();
}

/// <inheritdoc />
protected override void OnItemContainerStyleChanged(Style oldItemContainerStyle, Style newItemContainerStyle)
{
base.OnItemContainerStyleChanged(oldItemContainerStyle, newItemContainerStyle);

this.RefreshItemsHolder();
}

/// <inheritdoc />
protected override void OnItemContainerStyleSelectorChanged(StyleSelector oldItemContainerStyleSelector, StyleSelector newItemContainerStyleSelector)
{
base.OnItemContainerStyleSelectorChanged(oldItemContainerStyleSelector, newItemContainerStyleSelector);

this.RefreshItemsHolder();
}

/// <inheritdoc />
protected override void OnInitialized(EventArgs e)
{
base.OnInitialized(e);

this.ItemContainerGenerator.StatusChanged += this.OnGeneratorStatusChanged;
this.ItemContainerGenerator.ItemsChanged += this.OnGeneratorItemsChanged;
}

/// <inheritdoc />
Expand Down Expand Up @@ -162,7 +185,7 @@ protected override void OnItemsChanged(NotifyCollectionChangedEventArgs e)
{
foreach (var item in e.OldItems)
{
var contentPresenter = this.FindChildContentPresenter(item);
var contentPresenter = this.FindChildContentPresenter(item, null);

if (contentPresenter is null == false)
{
Expand Down Expand Up @@ -304,6 +327,7 @@ private void ClearItemsHolder()
{
var contentPresenter = itemsHolderChild as ContentPresenter;

contentPresenter?.ClearValue(OwningItemProperty);
contentPresenter?.ClearValue(OwningTabItemProperty);
}

Expand Down Expand Up @@ -340,6 +364,17 @@ private void OnGeneratorStatusChanged(object sender, EventArgs e)
this.UpdateSelectedContent();
}

private void OnGeneratorItemsChanged(object sender, ItemsChangedEventArgs e)
{
// We only care about reset.
// Reset, in case of ItemContainerGenerator, is generated when it's refreshed.
// It gets refresh when things like ItemContainerStyleSelector etc. change.
if (e.Action == NotifyCollectionChangedAction.Reset)
{
this.RefreshItemsHolder();
}
}

/// <summary>
/// Generate a ContentPresenter for the selected item and control the visibility of already created presenters.
/// </summary>
Expand All @@ -352,18 +387,18 @@ private void UpdateSelectedContent()

updateSelectedContentMethodInfo.Invoke(this, null);

var selectedItem = this.GetSelectedTabItem();
var selectedTabItem = this.GetSelectedTabItem();

if (selectedItem is null == false)
if (selectedTabItem is null == false)
{
// generate a ContentPresenter if necessary
this.CreateChildContentPresenterIfRequired(selectedItem);
this.CreateChildContentPresenterIfRequired(this.SelectedItem, selectedTabItem);
}

// show the right child
foreach (ContentPresenter contentPresenter in this.itemsHolder.Children)
{
var tabItem = GetOwningTabItem(contentPresenter);
var tabItem = (TabItem)this.ItemContainerGenerator.ContainerFromItem(GetOwningItem(contentPresenter)) ?? GetOwningTabItem(contentPresenter);

// Hide all non selected items and show the selected item
if (tabItem.IsSelected)
Expand Down Expand Up @@ -403,23 +438,23 @@ private void UpdateSelectedContent()
/// <summary>
/// Create the child ContentPresenter for the given item (could be data or a TabItem) if none exists.
/// </summary>
private void CreateChildContentPresenterIfRequired(object item)
private void CreateChildContentPresenterIfRequired(object item, TabItem tabItem)
{
if (item is null)
{
return;
}

// Jump out if we already created a ContentPresenter for this item
if (this.FindChildContentPresenter(item) is null == false)
if (this.FindChildContentPresenter(item, tabItem) is null == false)
{
return;
}

// the actual child to be added
var contentPresenter = new ContentPresenter
{
Content = item is TabItem tabItem ? tabItem.Content : item,
Content = item is TabItem itemAsTabItem ? itemAsTabItem.Content : item,
Visibility = Visibility.Collapsed
};

Expand All @@ -430,6 +465,7 @@ private void CreateChildContentPresenterIfRequired(object item)
throw new Exception("No owning TabItem could be found.");
}

SetOwningItem(contentPresenter, item);
SetOwningTabItem(contentPresenter, owningTabItem);

this.itemsHolder.Children.Add(contentPresenter);
Expand All @@ -438,7 +474,7 @@ private void CreateChildContentPresenterIfRequired(object item)
/// <summary>
/// Find the <see cref="ContentPresenter"/> for the given object. Data could be a TabItem or a piece of data.
/// </summary>
public ContentPresenter FindChildContentPresenter(object item)
public ContentPresenter FindChildContentPresenter(object item, TabItem tabItem)
{
if (item is null)
{
Expand All @@ -450,19 +486,22 @@ public ContentPresenter FindChildContentPresenter(object item)
return null;
}

var tabItem = item as TabItem ?? (TabItem)this.ItemContainerGenerator.ContainerFromItem(item);

var contentPresenters = this.itemsHolder.Children
.OfType<ContentPresenter>();
.OfType<ContentPresenter>()
.ToList();

if (tabItem is null == false)
{
return contentPresenters
.FirstOrDefault(contentPresenter => ReferenceEquals(GetOwningTabItem(contentPresenter), tabItem));
.FirstOrDefault(contentPresenter => ReferenceEquals(GetOwningTabItem(contentPresenter), tabItem))
?? contentPresenters
.FirstOrDefault(contentPresenter => ReferenceEquals(GetOwningItem(contentPresenter), item));
}

return contentPresenters
.FirstOrDefault(contentPresenter => ReferenceEquals(contentPresenter.Content, item));
.FirstOrDefault(contentPresenter => ReferenceEquals(GetOwningItem(contentPresenter), item))
?? contentPresenters
.FirstOrDefault(contentPresenter => ReferenceEquals(contentPresenter.Content, item));
}

private void MoveFocusToContent(ContentPresenter contentPresenter, TabItem tabItem)
Expand Down