fix issue 11243 TreeNodeCollection.AddRange has been broken in .NET 8 and no longer results in TreeNodes being drawn at the correct location

[Epica3055]

Fixes #11243

Screenshots

Before

After

Test methodology

• manually

Test environment(s)

9.0.0-preview.4.24219.3

[Tanya-Solyanik]

What this code added in NET8?

[Epica3055]

Actually no.

According to the comments I saw in the code, I think it was introduced to optimize AddRange performance.

But somehow after PR 9078, the logic about FixedIndex is not stable any more, which behaves like #11243.

So I tried to debug, and I found that the logic about FixedIndex didn't make scene to me about how to optimize performance and the logic is a little hard for me to understand. Then I tried to remove FixedIndex and I found that the issue is fixed.

I also made some tests like, using AddRange to add an array whose length is 1000. And I think it is not slow.

[Tanya-Solyanik]

found that the logic about FixedIndex didn't make scene to me

Interesting, maybe the original logic was broken by the refactoring changes? Could you please review this logic in the NET Framework sources or on the referencesource website?

I'm concerned about removing performance optimization, some application might be relying on it.

I remember that PR that introduced the standard comparer had regressed something else in the TreeView, could you please review the file history and bugs related to the treeview? I wonder if it makes sence to revert the refactoring change?

[Epica3055]

Yes, I checked NET Framework code. Before PR 9078 they are basically identical regarding AddRange logic and the code

The difference between before and after PR 9078 is here:

this is main branch

winforms/src/System.Windows.Forms/src/System/Windows/Forms/Controls/TreeView/TreeNodeCollection.cs

Lines 207 to 234 in b75e074

	public virtual void AddRange(params TreeNode[] nodes)
	{
	ArgumentNullException.ThrowIfNull(nodes);
	if (nodes.Length == 0)
	{
	return;
	}
	TreeView? tv = _owner.TreeView;
	if (tv is not null && nodes.Length > TreeNode.MAX_TREENODES_OPS)
	{
	tv.BeginUpdate();
	}
	_owner.Nodes.FixedIndex = _owner._childNodes.Count;
	_owner._childNodes.EnsureCapacity(nodes.Length);
	for (int i = 0; i < nodes.Length; i++)
	{
	AddInternal(nodes[i], i);
	}
	_owner.Nodes.FixedIndex = -1;
	if (tv is not null && nodes.Length > TreeNode.MAX_TREENODES_OPS)
	{
	tv.EndUpdate();
	}
	}

See line 224-227

winforms/src/System.Windows.Forms/src/System/Windows/Forms/TreeNodeCollection.cs

Lines 263 to 290 in 9ec7d07

	public virtual void AddRange(TreeNode[] nodes)
	{
	ArgumentNullException.ThrowIfNull(nodes);
	if (nodes.Length == 0)
	{
	return;
	}
	TreeView tv = owner.TreeView;
	if (tv is not null && nodes.Length > TreeNode.MAX_TREENODES_OPS)
	{
	tv.BeginUpdate();
	}
	owner.Nodes.FixedIndex = owner.childCount;
	owner.EnsureCapacity(nodes.Length);
	for (int i = nodes.Length - 1; i >= 0; i--)
	{
	AddInternal(nodes[i], i);
	}
	owner.Nodes.FixedIndex = -1;
	if (tv is not null && nodes.Length > TreeNode.MAX_TREENODES_OPS)
	{
	tv.EndUpdate();
	}
	}

this is 7.0 branch
See line 280-284

---

So now we use List<TreeNode> to store subTreeNodes instead of TreeNode[], so it's impossible to do reverse iteration.

But if we don't do reverse iteration the logic here will break down, which I think is the cause of this issue.

winforms/src/System.Windows.Forms/src/System/Windows/Forms/Controls/TreeView/TreeNode.cs

Lines 804 to 831 in b75e074

	public TreeNode? PrevNode
	{
	get
	{
	if (_parent is null)
	{
	return null;
	}
	// fixedIndex is used for perf. optimization in case of adding big ranges of nodes
	int currentInd = _index;
	int fixedInd = _parent.Nodes.FixedIndex;
	if (fixedInd > 0)
	{
	currentInd = fixedInd;
	}
	if (currentInd > 0 && currentInd <= _parent.Nodes.Count)
	{
	return _parent.Nodes[currentInd - 1];
	}
	else
	{
	return null;
	}
	}
	}

I wonder if it makes sence to revert the refactoring change?

Yes, that's the solution that I came up with in the first place see PR 11253

[Tanya-Solyanik]

I see, we can't insert an item into a generic list at an index past the actual list size, even when within the capacity, like we were inserting into an empty portion of the array.
We can't revert the "FixedIndex" optimization because we don't know the specific scenario it solves. It might require a ListView with some specific properties and your test might not be sufficient.
I prefer reverting the change that replaced array of child nodes with the generic list. Also add an explanation comment as to why the List is not suitable in this case above the _children fiend declaration.