Skip to content
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

Fix interaction of RadioButton, shared bindings and hanging GC refs #10252

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from
+74 −70
Draft

Fix interaction of RadioButton, shared bindings and hanging GC refs #10252

Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
06014f2
Move from Hashtable to Dictionary
h3xds1nz Jan 7, 2025
df27e74
Move from ArrayList to List<WeakReference>
h3xds1nz Jan 7, 2025
101de4a
Use generic WeakReference<RadioButton>
h3xds1nz Jan 7, 2025
16fe3b3
Remove locks as its thread-bound instance
h3xds1nz Jan 7, 2025
510c3df
Count downwards like regular people, don't branch
h3xds1nz Jan 7, 2025
c1ddc5f
Imho the standard number of RadioButtons is at least 2
h3xds1nz Jan 7, 2025
2bdd976
Add t_ prefix according to conventions
h3xds1nz Jan 7, 2025
87d803c
Use optimized lookup/create for Register
h3xds1nz Jan 7, 2025
bc6ca5e
Final cleanup
h3xds1nz Jan 10, 2025
b18bc5a
Fix #2995 and allow binding instance sharing under same visual root
h3xds1nz Jan 10, 2025
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
144 changes: 74 additions & 70 deletions src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Controls/RadioButton.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Collections;
using System.Windows.Data;
using System.Windows.Input;
using System.ComponentModel;
using System.Runtime.InteropServices;
using System.Windows.Automation.Peers;
using System.Windows.Controls.Primitives;
using System.Windows.Input;
using System.Windows.Media;
using MS.Internal.Telemetry.PresentationFramework;

// Disable CS3001: Warning as Error: not CLS-compliant
Expand Down Expand Up @@ -67,65 +68,53 @@ private static void OnGroupNameChanged(DependencyObject d, DependencyPropertyCha

private static void Register(string groupName, RadioButton radioButton)
{
if (_groupNameToElements == null)
_groupNameToElements = new Hashtable(1);
t_groupNameToElements ??= new Dictionary<string, List<WeakReference<RadioButton>>>(1);

lock (_groupNameToElements)
ref List<WeakReference<RadioButton>> elements = ref CollectionsMarshal.GetValueRefOrAddDefault(t_groupNameToElements, groupName, out bool exists);
if (!exists)
{
// Create new collection
elements = new List<WeakReference<RadioButton>>(2);
}
else
{
ArrayList elements = (ArrayList)_groupNameToElements[groupName];
// There were some elements there, remove dead ones
PurgeDead(elements, null);
}

if (elements == null)
{
elements = new ArrayList(1);
_groupNameToElements[groupName] = elements;
}
else
{
// There were some elements there, remove dead ones
PurgeDead(elements, null);
}
elements.Add(new WeakReference<RadioButton>(radioButton));

elements.Add(new WeakReference(radioButton));
}
_currentlyRegisteredGroupName.SetValue(radioButton, groupName);
}

private static void Unregister(string groupName, RadioButton radioButton)
{
if (_groupNameToElements == null)
Debug.Assert(t_groupNameToElements is not null, "Unregister was called before Register");

if (t_groupNameToElements is null)
return;

lock (_groupNameToElements)
// Get all elements bound to this key and remove this element
if (t_groupNameToElements.TryGetValue(groupName, out List<WeakReference<RadioButton>> elements))
{
// Get all elements bound to this key and remove this element
ArrayList elements = (ArrayList)_groupNameToElements[groupName];
PurgeDead(elements, radioButton);

if (elements != null)
{
PurgeDead(elements, radioButton);
if (elements.Count == 0)
{
_groupNameToElements.Remove(groupName);
}
}
// If the group has zero elements, remove it
if (elements.Count == 0)
t_groupNameToElements.Remove(groupName);
}

_currentlyRegisteredGroupName.SetValue(radioButton, null);
}

private static void PurgeDead(ArrayList elements, object elementToRemove)
private static void PurgeDead(List<WeakReference<RadioButton>> elements, RadioButton elementToRemove)
{
for (int i = 0; i < elements.Count; )
for (int i = elements.Count - 1; i >= 0; i--)
{
WeakReference weakReference = (WeakReference)elements[i];
object element = weakReference.Target;
if (element == null || element == elementToRemove)
if (!elements[i].TryGetTarget(out RadioButton element) || element == elementToRemove)
{
elements.RemoveAt(i);
}
else
{
i++;
}
}
}

Expand All @@ -134,45 +123,62 @@ private void UpdateRadioButtonGroup()
string groupName = GroupName;
if (!string.IsNullOrEmpty(groupName))
{
Visual rootScope = KeyboardNavigation.GetVisualRoot(this);
if (_groupNameToElements == null)
_groupNameToElements = new Hashtable(1);
lock (_groupNameToElements)
t_groupNameToElements ??= new Dictionary<string, List<WeakReference<RadioButton>>>(1);

// Get all elements bound to this key
if (t_groupNameToElements.TryGetValue(groupName, out List<WeakReference<RadioButton>> elements))
{
// Get all elements bound to this key and remove this element
ArrayList elements = (ArrayList)_groupNameToElements[groupName];
for (int i = 0; i < elements.Count; )
for (int i = elements.Count - 1; i >= 0; i--)
{
WeakReference weakReference = (WeakReference)elements[i];
RadioButton rb = weakReference.Target as RadioButton;
if (rb == null)
// Either remove the dead element or uncheck if we're checked
if (elements[i].TryGetTarget(out RadioButton radioButton))
{
// Remove dead instances
elements.RemoveAt(i);
// Uncheck all checked RadioButtons but this one
if (radioButton != this && radioButton.IsChecked is true)
{
DependencyObject rootScope = InputElement.GetRootVisual(this);
DependencyObject otherRoot = InputElement.GetRootVisual(radioButton);

// If elements have the same group name but the visual roots are different, we still treat them
// as unique since we want to promote reuse of group names to make them easier to work with.
if (rootScope != otherRoot)
continue;

// Allow binding sharing under the same visual root
BindingExpression rootBindingSource = GetBindingExpression(IsCheckedProperty);
BindingExpression otherBindingSource = radioButton.GetBindingExpression(IsCheckedProperty);

if (rootBindingSource is not null && otherBindingSource is not null &&
rootBindingSource.SourceItem == otherBindingSource.SourceItem &&
rootBindingSource.SourcePropertyName == otherBindingSource.SourcePropertyName)
continue;

radioButton.UncheckRadioButton();
}
}
else
{
// Uncheck all checked RadioButtons different from the current one
if (rb != this && (rb.IsChecked == true) && rootScope == KeyboardNavigation.GetVisualRoot(rb))
rb.UncheckRadioButton();
i++;
// Remove dead instances
elements.RemoveAt(i);
}
}
}
}
else // Logical parent should be the group
{
DependencyObject parent = this.Parent;
if (parent != null)
DependencyObject parent = Parent;
if (parent is not null)
{
// Traverse logical children
IEnumerable children = LogicalTreeHelper.GetChildren(parent);
IEnumerator itor = children.GetEnumerator();
while (itor.MoveNext())
{
RadioButton rb = itor.Current as RadioButton;
if (rb != null && rb != this && string.IsNullOrEmpty(rb.GroupName) && (rb.IsChecked == true))
rb.UncheckRadioButton();
if (itor.Current is RadioButton radioButton && radioButton.IsChecked is true &&
radioButton != this && string.IsNullOrEmpty(radioButton.GroupName))
{
radioButton.UncheckRadioButton();
}
}
}
}
Expand All @@ -195,7 +201,7 @@ private void UncheckRadioButton()
"GroupName",
typeof(string),
typeof(RadioButton),
new FrameworkPropertyMetadata(String.Empty, new PropertyChangedCallback(OnGroupNameChanged)));
new FrameworkPropertyMetadata(string.Empty, new PropertyChangedCallback(OnGroupNameChanged)));

/// <summary>
/// GroupName determine mutually excusive radiobutton groups
Expand All @@ -222,9 +228,9 @@ public string GroupName
/// <summary>
/// Creates AutomationPeer (<see cref="UIElement.OnCreateAutomationPeer"/>)
/// </summary>
protected override System.Windows.Automation.Peers.AutomationPeer OnCreateAutomationPeer()
protected override AutomationPeer OnCreateAutomationPeer()
{
return new System.Windows.Automation.Peers.RadioButtonAutomationPeer(this);
return new RadioButtonAutomationPeer(this);
}

/// <summary>
Expand Down Expand Up @@ -264,10 +270,6 @@ protected override void OnAccessKey(AccessKeyEventArgs e)

#endregion

#region Accessibility

#endregion Accessibility

#region DTypeThemeStyleKey

// Returns the DependencyObjectType for the registered ThemeStyleKey's default
Expand All @@ -277,13 +279,15 @@ internal override DependencyObjectType DTypeThemeStyleKey
get { return _dType; }
}

private static DependencyObjectType _dType;
private static readonly DependencyObjectType _dType;

#endregion DTypeThemeStyleKey

#region private data

[ThreadStatic] private static Hashtable _groupNameToElements;
[ThreadStatic]
private static Dictionary<string, List<WeakReference<RadioButton>>> t_groupNameToElements;

private static readonly UncommonField<string> _currentlyRegisteredGroupName = new UncommonField<string>();

#endregion private data
Expand Down