Do not use fixed index in the TreeNode.PrevNode if the TreeView is sorted

[elachlan]

Fixes #8768

This is a re-roll of the PR in #8774, but with the switch added.

Microsoft Reviewers: Open in CodeFlow

[elachlan]

CC: @snnz

[elachlan]

@lonitra I added the xml comment and renamed the switch to be more specific to its behavior.

[snnz]

But this re-roll, it is totally incorrect. The purpose is to make sure that fixed index is NOT used if the tree is sorted, not vice versa. See the original PR: there should be !TreeView.Sorted in the condition.

[lonitra]

@lonitra I added the xml comment and renamed the switch to be more specific to its behavior.

Thank you! The change looks good. Could you add tests to capture the behavior of the switch when it is on?

[ghost]

This submission has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 14 days.

It will be closed if no further activity occurs within 7 days of this comment.

[elachlan]

Sorry! I'll try and get this sorted soon.

[elachlan]

@lonitra I am unsure how we can unit test this. As the behavior of affects the underlying win32 tree view control.

Edit: I think I found a solution. I will push through in a bit.

[elachlan]

I did some manual testing and was getting different results than what was specified in the #8768.

I then looked into the differences, we changed the behavior of the insert in #9078 by @gpetrou

Did we want to change the behavior back, or keep it as is? It used to add in "almost" reverse order, but now it adds it in the order provided (almost).

Before:

After:

Method of adding:

  treeView1.Nodes.AddRange([
  new TreeNode("3"),
  new TreeNode("5"),
  new TreeNode("1"),
  new TreeNode("4")
]);

[elachlan]

Test is showing some weird behavior and I am not sure why. It doesn't line up with what I observed when manually testing.

Expected: 2,5,3,1,4
Actual: 2,3,4,5,1

[lonitra]

Did we want to change the behavior back, or keep it as is? It used to add in "almost" reverse order, but now it adds it in the order provided (almost).

Had you investigated what exactly caused the behavior change? From glancing at the PR it doesn't seem like the change was intentional.

Test is showing some weird behavior and I am not sure why. It doesn't line up with what I observed when manually testing.

Hm.. That is concerning. It seems like the state between our control and the underlying one is mismatched and the results you are getting are actually being sorted when it comes to the underlying control.. This looks a bit suspicious to me

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

Lines 315 to 318 in 226a5b5

	if (tv is not null && tv.Sorted)
	{
	return _owner.AddSorted(tv, node);
	}

this is where it looks like we eventually get to from AddRange. Could you maybe debug through this and see if somewhere along the line we are adding the tree node to a unexpected index for the underlying control? -- I cannot look into this too deepy right now, apologies!

[elachlan]

Sorry I haven't sorted this yet. Hopefully soon.

[Tanya-Solyanik]

The idea of this fix is that we want to skip the fixed index perf optimization when we are sorting the treeview.

[elachlan]

Yes, based on this comment from the issue reporter.
#10493 (comment)

[Tanya-Solyanik]

Then we don't have to set FIxedIndex to the currently displayed number of child nodes, I prefer fix in #11300

[elachlan]

No problem. Ill close this then. The tests still seem weird. So worth investigating.