From a3ca32bc0c1d4ae5de28975b8b0eec9ae6245fa1 Mon Sep 17 00:00:00 2001 From: Jeremy Kuhne Date: Fri, 23 Feb 2024 14:33:03 -0800 Subject: [PATCH] Add convenience / performance related methods to ListViewItemCollection / ControlCollection ListViewItemCollection doesn't have an AddRange that takes an IEnumerable, which necessitates unnecessary array allocations when adding from a collection. It also doesn't implement a generic IList which makes typed usage and LINQ usage difficult. This adds that. Also add IEnumerable to ControlCollection to address the LINQ scenario. Control has IList, but indexed setting throws. I don't want to add IList until we've had time to evaluate the implications of implicitly doing the public steps with the collection that replicate what those setters should be doing. Also tweak ArrangedElementCollection so the static empty collection can't be written to. This doesn't fix the ambiguity of `[..]`, but it does avoid the need for it in some cases. --- .../src/GlobalSuppressions.cs | 2 + .../src/PublicAPI.Unshipped.txt | 2 + .../Forms/Control.ControlCollection.cs | 55 ++++---- .../src/System/Windows/Forms/Control.cs | 10 +- .../ListView.ListViewItemCollection.cs | 132 ++++++++---------- ...ipPanel.ToolStripPanelControlCollection.cs | 11 +- .../Forms/Layout/ArrangedElementCollection.cs | 26 ++-- 7 files changed, 112 insertions(+), 126 deletions(-) diff --git a/src/System.Windows.Forms/src/GlobalSuppressions.cs b/src/System.Windows.Forms/src/GlobalSuppressions.cs index 3291098e324..8c0bbac0793 100644 --- a/src/System.Windows.Forms/src/GlobalSuppressions.cs +++ b/src/System.Windows.Forms/src/GlobalSuppressions.cs @@ -18,6 +18,8 @@ [assembly: SuppressMessage("Naming", "CA1725:Parameter names should match base declaration", Justification = "Public API", Scope = "member", Target = "~M:System.Windows.Forms.FontDialog.RunDialog(System.IntPtr)~System.Boolean")] [assembly: SuppressMessage("Naming", "CA1725:Parameter names should match base declaration", Justification = "Public API", Scope = "member", Target = "~M:System.Windows.Forms.ImageListStreamer.GetObjectData(System.Runtime.Serialization.SerializationInfo,System.Runtime.Serialization.StreamingContext)")] [assembly: SuppressMessage("Naming", "CA1725:Parameter names should match base declaration", Justification = "Public API", Scope = "member", Target = "~M:System.Windows.Forms.Control.EndInvoke(System.IAsyncResult)~System.Object")] +[assembly: SuppressMessage("Naming", "CA1725:Parameter names should match base declaration", Justification = "Public API", Scope = "member", Target = "~M:System.Windows.Forms.Control.ControlCollection.IndexOf(System.Windows.Forms.Control)~System.Int32")] +[assembly: SuppressMessage("Naming", "CA1725:Parameter names should match base declaration", Justification = "Public API", Scope = "member", Target = "~M:System.Windows.Forms.ListView.ListViewItemCollection.CopyTo(System.Array,System.Int32)")] [assembly: SuppressMessage("Style", "IDE1006:Naming Styles", Justification = "Public API", Scope = "member", Target = "~F:System.Windows.Forms.FontDialog.EventApply")] [assembly: SuppressMessage("Style", "IDE1006:Naming Styles", Justification = "Public API", Scope = "member", Target = "~F:System.Windows.Forms.FileDialog.EventFileOk")] [assembly: SuppressMessage("Performance", "CA1815:Override equals and operator equals on value types", Justification = "Public API", Scope = "type", Target = "~T:System.Windows.Forms.ImeModeConversion")] diff --git a/src/System.Windows.Forms/src/PublicAPI.Unshipped.txt b/src/System.Windows.Forms/src/PublicAPI.Unshipped.txt index e69de29bb2d..a507f942c72 100644 --- a/src/System.Windows.Forms/src/PublicAPI.Unshipped.txt +++ b/src/System.Windows.Forms/src/PublicAPI.Unshipped.txt @@ -0,0 +1,2 @@ +System.Windows.Forms.ListView.ListViewItemCollection.AddRange(System.Collections.Generic.IEnumerable! collection) -> void +System.Windows.Forms.ListView.ListViewItemCollection.CopyTo(System.Windows.Forms.ListViewItem![]! array, int arrayIndex) -> void \ No newline at end of file diff --git a/src/System.Windows.Forms/src/System/Windows/Forms/Control.ControlCollection.cs b/src/System.Windows.Forms/src/System/Windows/Forms/Control.ControlCollection.cs index 13c0e246d8b..9d0caef6c8c 100644 --- a/src/System.Windows.Forms/src/System/Windows/Forms/Control.ControlCollection.cs +++ b/src/System.Windows.Forms/src/System/Windows/Forms/Control.ControlCollection.cs @@ -10,10 +10,10 @@ namespace System.Windows.Forms; public partial class Control { /// - /// Collection of controls... + /// Collection of controls. /// [ListBindable(false)] - public partial class ControlCollection : ArrangedElementCollection, IList, ICloneable + public partial class ControlCollection : ArrangedElementCollection, IList, IEnumerable, ICloneable { /// A caching mechanism for key accessor /// We use an index here rather than control so that we don't have lifetime @@ -29,10 +29,7 @@ public ControlCollection(Control owner) /// /// Returns true if the collection contains an item with the specified key, false otherwise. /// - public virtual bool ContainsKey(string? key) - { - return IsValidIndex(IndexOfKey(key)); - } + public virtual bool ContainsKey(string? key) => IsValidIndex(IndexOfKey(key)); /// /// Adds a child control to this control. The control becomes the last control in @@ -156,11 +153,23 @@ object ICloneable.Clone() { // Use CreateControlInstance so we get the same type of ControlCollection, but whack the // owner so adding controls to this new collection does not affect the control we cloned from. - ControlCollection ccOther = Owner.CreateControlsInstance(); + ControlCollection clone = Owner.CreateControlsInstance(); // We add using InnerList to prevent unnecessary parent cycle checks, etc. - ccOther.InnerList.AddRange(InnerList); - return ccOther; + if (clone.InnerList is List list) + { + list.AddRange(InnerList); + } + else + { + Debug.Fail("Unexpected InnerList type"); + foreach (Control c in this) + { + clone.Add(c); + } + } + + return clone; } public bool Contains(Control? control) => ((IList)InnerList).Contains(control); @@ -224,10 +233,7 @@ private static void FindInternal(string key, bool searchAllChildren, ControlColl } } - public override IEnumerator GetEnumerator() - { - return new ControlCollectionEnumerator(this); - } + public override IEnumerator GetEnumerator() => new ControlCollectionEnumerator(this); public int IndexOf(Control? control) => ((IList)InnerList).IndexOf(control); @@ -269,10 +275,7 @@ public virtual int IndexOfKey(string? key) /// /// Determines if the index is valid for the collection. /// - private bool IsValidIndex(int index) - { - return ((index >= 0) && (index < Count)); - } + private bool IsValidIndex(int index) => (index >= 0) && (index < Count); /// /// Who owns this control collection. @@ -285,10 +288,9 @@ private bool IsValidIndex(int index) /// public virtual void Remove(Control? value) { - // Sanity check parameter if (value is null) { - return; // Don't do anything + return; } if (value.ParentInternal == Owner) @@ -317,10 +319,7 @@ void IList.Remove(object? control) } } - public void RemoveAt(int index) - { - Remove(this[index]); - } + public void RemoveAt(int index) => Remove(this[index]); /// /// Removes the child control with the specified key. @@ -394,7 +393,7 @@ public virtual void Clear() /// Retrieves the index of the specified child control in this array. An ArgumentException /// is thrown if child is not parented to this Control. /// - public int GetChildIndex(Control child) => GetChildIndex(child, true); + public int GetChildIndex(Control child) => GetChildIndex(child, throwException: true); /// /// Retrieves the index of the specified child control in this array. An ArgumentException @@ -444,5 +443,13 @@ internal virtual void SetChildIndexInternal(Control child, int newIndex) /// public virtual void SetChildIndex(Control child, int newIndex) => SetChildIndexInternal(child, newIndex); + + IEnumerator IEnumerable.GetEnumerator() + { + foreach (Control control in InnerList) + { + yield return control; + } + } } } diff --git a/src/System.Windows.Forms/src/System/Windows/Forms/Control.cs b/src/System.Windows.Forms/src/System/Windows/Forms/Control.cs index 13aab3c9f86..367f68c5246 100644 --- a/src/System.Windows.Forms/src/System/Windows/Forms/Control.cs +++ b/src/System.Windows.Forms/src/System/Windows/Forms/Control.cs @@ -4773,20 +4773,14 @@ public bool Contains([NotNullWhen(true)] Control? ctl) /// should not call base.CreateAccessibilityObject. /// [EditorBrowsable(EditorBrowsableState.Advanced)] - protected virtual AccessibleObject CreateAccessibilityInstance() - { - return new ControlAccessibleObject(this); - } + protected virtual AccessibleObject CreateAccessibilityInstance() => new ControlAccessibleObject(this); /// /// Constructs the new instance of the Controls collection objects. Subclasses /// should not call base.CreateControlsInstance. /// [EditorBrowsable(EditorBrowsableState.Advanced)] - protected virtual ControlCollection CreateControlsInstance() - { - return new ControlCollection(this); - } + protected virtual ControlCollection CreateControlsInstance() => new ControlCollection(this); /// /// Creates a Graphics for this control. The control's brush, font, foreground diff --git a/src/System.Windows.Forms/src/System/Windows/Forms/Controls/ListView/ListView.ListViewItemCollection.cs b/src/System.Windows.Forms/src/System/Windows/Forms/Controls/ListView/ListView.ListViewItemCollection.cs index 6af6c9273e5..af90d07dacb 100644 --- a/src/System.Windows.Forms/src/System/Windows/Forms/Controls/ListView/ListView.ListViewItemCollection.cs +++ b/src/System.Windows.Forms/src/System/Windows/Forms/Controls/ListView/ListView.ListViewItemCollection.cs @@ -12,14 +12,14 @@ public partial class ListView /// Represents the collection of items in a ListView or ListViewGroup /// [ListBindable(false)] - public partial class ListViewItemCollection : IList + public partial class ListViewItemCollection : IList, IList { /// A caching mechanism for key accessor /// We use an index here rather than control so that we don't have lifetime /// issues by holding on to extra references. - private int lastAccessedIndex = -1; + private int _lastAccessedIndex = -1; - private readonly IInnerList innerList; + private readonly IInnerList _innerList; public ListViewItemCollection(ListView owner) { @@ -27,66 +27,30 @@ public ListViewItemCollection(ListView owner) // In Whidbey this constructor is a no-op. // initialize the inner list w/ a dummy list. - innerList = new ListViewNativeItemCollection(owner); + _innerList = new ListViewNativeItemCollection(owner); } internal ListViewItemCollection(IInnerList innerList) { Debug.Assert(innerList is not null, "Can't pass in null innerList"); - this.innerList = innerList; + _innerList = innerList; } - private IInnerList InnerList - { - get - { - return innerList; - } - } + private IInnerList InnerList => _innerList; /// /// Returns the total number of items within the list view. /// [Browsable(false)] - public int Count - { - get - { - return InnerList.Count; - } - } + public int Count => InnerList.Count; - object ICollection.SyncRoot - { - get - { - return this; - } - } + object ICollection.SyncRoot => this; - bool ICollection.IsSynchronized - { - get - { - return true; - } - } + bool ICollection.IsSynchronized => true; - bool IList.IsFixedSize - { - get - { - return false; - } - } + bool IList.IsFixedSize => false; - public bool IsReadOnly - { - get - { - return false; - } - } + public bool IsReadOnly => false; /// /// Returns the ListViewItem at the given index. @@ -111,10 +75,7 @@ public virtual ListViewItem this[int index] object? IList.this[int index] { - get - { - return this[index]; - } + get => this[index]; set { this[index] = value is ListViewItem item @@ -152,10 +113,7 @@ public virtual ListViewItem? this[string key] /// the correct sorted position, or, if no sorting is set, at the end /// of the list. /// - public virtual ListViewItem Add(string? text) - { - return Add(text, -1); - } + public virtual ListViewItem Add(string? text) => Add(text, -1); int IList.Add(object? item) { @@ -195,8 +153,6 @@ public virtual ListViewItem Add(ListViewItem value) return value; } - // <-- NEW ADD OVERLOADS IN WHIDBEY - /// /// Add an item to the ListView. The item will be inserted either in /// the correct sorted position, or, if no sorting is set, at the end @@ -239,8 +195,6 @@ public virtual ListViewItem Add(string? key, string? text, int imageIndex) return item; } - // END - NEW ADD OVERLOADS IN WHIDBEY --> - public void AddRange(params ListViewItem[] items) { ArgumentNullException.ThrowIfNull(items); @@ -260,10 +214,7 @@ public void AddRange(ListViewItemCollection items) /// /// Removes all items from the list view. /// - public virtual void Clear() - { - InnerList.Clear(); - } + public virtual void Clear() => InnerList.Clear(); public bool Contains(ListViewItem item) { @@ -294,10 +245,10 @@ public ListViewItem[] Find(string key, bool searchAllSubItems) { key.ThrowIfNullOrEmptyWithMessage(SR.FindKeyMayNotBeEmptyOrNull); - List foundItems = new(); + List foundItems = []; FindInternal(key, searchAllSubItems, this, foundItems); - return foundItems.ToArray(); + return [.. foundItems]; } /// @@ -369,11 +320,11 @@ public virtual int IndexOfKey(string? key) } // step 1 - check the last cached item - if (IsValidIndex(lastAccessedIndex)) + if (IsValidIndex(_lastAccessedIndex)) { - if (WindowsFormsUtils.SafeCompareStrings(this[lastAccessedIndex].Name, key, /* ignoreCase = */ true)) + if (WindowsFormsUtils.SafeCompareStrings(this[_lastAccessedIndex].Name, key, /* ignoreCase = */ true)) { - return lastAccessedIndex; + return _lastAccessedIndex; } } @@ -382,13 +333,13 @@ public virtual int IndexOfKey(string? key) { if (WindowsFormsUtils.SafeCompareStrings(this[i].Name, key, /* ignoreCase = */ true)) { - lastAccessedIndex = i; + _lastAccessedIndex = i; return i; } } // step 3 - we didn't find it. Invalidate the last accessed index and return -1. - lastAccessedIndex = -1; + _lastAccessedIndex = -1; return -1; } @@ -431,8 +382,6 @@ void IList.Insert(int index, object? item) } } - // <-- NEW INSERT OVERLOADS IN WHIDBEY - public ListViewItem Insert(int index, string? text, string? imageKey) => Insert(index, new ListViewItem(text, imageKey)); @@ -448,8 +397,6 @@ public virtual ListViewItem Insert(int index, string? key, string? text, int ima Name = key }); - // END - NEW INSERT OVERLOADS IN WHIDBEY --> - /// /// Removes an item from the ListView /// @@ -488,5 +435,42 @@ void IList.Remove(object? item) Remove(listViewItem); } } + + void IList.Insert(int index, ListViewItem item) => Insert(index, item); + + void ICollection.Add(ListViewItem item) => Add(item); + + public void CopyTo(ListViewItem[] array, int arrayIndex) => CopyTo((Array)array, arrayIndex); + + bool ICollection.Remove(ListViewItem item) + { + if (Contains(item)) + { + Remove(item); + return true; + } + + return false; + } + + IEnumerator IEnumerable.GetEnumerator() + { + var enumerator = GetEnumerator(); + while (enumerator.MoveNext()) + { + if (enumerator.Current is ListViewItem item) + { + yield return item; + } + } + } + + public void AddRange(IEnumerable collection) + { + foreach (ListViewItem item in collection) + { + Add(item); + } + } } } diff --git a/src/System.Windows.Forms/src/System/Windows/Forms/Controls/ToolStrips/ToolStripPanel.ToolStripPanelControlCollection.cs b/src/System.Windows.Forms/src/System/Windows/Forms/Controls/ToolStrips/ToolStripPanel.ToolStripPanelControlCollection.cs index 64a600d8260..4dca59f6e22 100644 --- a/src/System.Windows.Forms/src/System/Windows/Forms/Controls/ToolStrips/ToolStripPanel.ToolStripPanelControlCollection.cs +++ b/src/System.Windows.Forms/src/System/Windows/Forms/Controls/ToolStrips/ToolStripPanel.ToolStripPanelControlCollection.cs @@ -34,14 +34,13 @@ internal override void AddInternal(Control? value) internal void Sort() { - if (_owner.Orientation == Orientation.Horizontal) + if (InnerList is not List list) { - InnerList.Sort(new YXComparer()); - } - else - { - InnerList.Sort(new XYComparer()); + Debug.Fail("InnerList is not a List?"); + return; } + + list.Sort(_owner.Orientation == Orientation.Horizontal ? new YXComparer() : new XYComparer()); } } } diff --git a/src/System.Windows.Forms/src/System/Windows/Forms/Layout/ArrangedElementCollection.cs b/src/System.Windows.Forms/src/System/Windows/Forms/Layout/ArrangedElementCollection.cs index 537eecb4c1f..980d862ffc0 100644 --- a/src/System.Windows.Forms/src/System/Windows/Forms/Layout/ArrangedElementCollection.cs +++ b/src/System.Windows.Forms/src/System/Windows/Forms/Layout/ArrangedElementCollection.cs @@ -7,30 +7,23 @@ namespace System.Windows.Forms.Layout; public class ArrangedElementCollection : IList { - internal static ArrangedElementCollection Empty = new(0); + internal static ArrangedElementCollection Empty { get; } = new(new List().AsReadOnly()); - internal ArrangedElementCollection() - : this(4) + internal ArrangedElementCollection() : this(4) { } - internal ArrangedElementCollection(List innerList) - { - InnerList = innerList; - } + internal ArrangedElementCollection(IList innerList) => InnerList = innerList; - private ArrangedElementCollection(int size) - { - InnerList = new List(size); - } + private ArrangedElementCollection(int size) => InnerList = new List(size); - private protected List InnerList { get; } + private protected IList InnerList { get; } internal virtual IArrangedElement this[int index] => InnerList[index]; public override bool Equals(object? obj) { - if (!(obj is ArrangedElementCollection other) || Count != other.Count) + if (obj is not ArrangedElementCollection other || Count != other.Count) { return false; } @@ -100,7 +93,12 @@ private protected void MoveElement(IArrangedElement element, int fromIndex, int InnerList[toIndex] = element; } - private static void Copy(ArrangedElementCollection sourceList, int sourceIndex, ArrangedElementCollection destinationList, int destinationIndex, int length) + private static void Copy( + ArrangedElementCollection sourceList, + int sourceIndex, + ArrangedElementCollection destinationList, + int destinationIndex, + int length) { if (sourceIndex < destinationIndex) {