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.

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

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

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,15 @@ internal static partial class LocalAppContextSwitches
internal const string TrackBarModernRenderingSwitchName = "System.Windows.Forms.TrackBarModernRendering";
private const string DoNotCatchUnhandledExceptionsSwitchName = "System.Windows.Forms.DoNotCatchUnhandledExceptions";
internal const string DataGridViewUIAStartRowCountAtZeroSwitchName = "System.Windows.Forms.DataGridViewUIAStartRowCountAtZero";
internal const string TreeNodePrevNodeDoNotUseFixedIndexWithSortedTreeViewSwitchName = "System.Windows.Forms.TreeNodeDoNotUseFixedIndexWithSortedTreeView";

private static int s_scaleTopLevelFormMinMaxSizeForDpi;
private static int s_anchorLayoutV2;
private static int s_servicePointManagerCheckCrl;
private static int s_trackBarModernRendering;
private static int s_doNotCatchUnhandledExceptions;
private static int s_dataGridViewUIAStartRowCountAtZero;
private static int s_treeNodePrevNodeDoNotUseFixedIndexWithSortedTreeView;

private static FrameworkName? s_targetFrameworkName;

Expand Down Expand Up @@ -160,5 +162,15 @@ public static bool DataGridViewUIAStartRowCountAtZero
get => GetCachedSwitchValue(DataGridViewUIAStartRowCountAtZeroSwitchName, ref s_dataGridViewUIAStartRowCountAtZero);
}

/// <summary>
/// Indicates whether TreeNode.PrevNode uses a fixed index to return its value.
/// Thus mirroring the behaviour of the TreeNodeCollection.
/// </summary>
public static bool TreeNodePrevNodeDoNotUseFixedIndexWithSortedTreeView
{
[MethodImpl(MethodImplOptions.AggressiveInlining)]
get => GetCachedSwitchValue(TreeNodePrevNodeDoNotUseFixedIndexWithSortedTreeViewSwitchName, ref s_treeNodePrevNodeDoNotUseFixedIndexWithSortedTreeView);
}

internal static void SetDataGridViewUIAStartRowCountAtZero(bool value) => s_dataGridViewUIAStartRowCountAtZero = value ? 1 : 0;
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using System.Runtime.InteropServices;
using System.Runtime.Serialization;
using System.Text;
using System.Windows.Forms.Primitives;

namespace System.Windows.Forms;

Expand Down Expand Up @@ -812,17 +813,20 @@ public TreeNode? PrevNode
}

// fixedIndex is used for perf. optimization in case of adding big ranges of nodes
int currentInd = _index;
int fixedInd = _parent.Nodes.FixedIndex;
int currentIndex = _index;
int fixedIndex = _parent.Nodes.FixedIndex;

if (fixedInd > 0)
if (fixedIndex > 0)
{
currentInd = fixedInd;
if (!LocalAppContextSwitches.TreeNodePrevNodeDoNotUseFixedIndexWithSortedTreeView || TreeView is null || !TreeView.Sorted)
{
currentIndex = fixedIndex;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}

if (currentInd > 0 && currentInd <= _parent.Nodes.Count)
if (currentIndex > 0 && currentIndex <= _parent.Nodes.Count)
{
return _parent.Nodes[currentInd - 1];
return _parent.Nodes[currentIndex - 1];
}
else
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System.Collections;
using System.ComponentModel;
using System.Drawing;
using System.Windows.Forms.Primitives;
using System.Windows.Forms.TestUtilities;
using Moq;

Expand Down Expand Up @@ -7296,6 +7297,92 @@ public void Verify_NodeValue_AfterSortAndRemove()
Assert.Equal(treeNode2, parent.Nodes[0]);
}

[WinFormsTheory]
[BoolData]
public unsafe void Verify_Order_AfterSort_ContextSwitch(bool switchValue)
{
dynamic testAccessor = typeof(LocalAppContextSwitches).TestAccessor().Dynamic;

try
{
// Set the context switch
AppContext.SetSwitch(LocalAppContextSwitches.TreeNodePrevNodeDoNotUseFixedIndexWithSortedTreeViewSwitchName, switchValue);
Assert.Equal(switchValue, LocalAppContextSwitches.TreeNodePrevNodeDoNotUseFixedIndexWithSortedTreeView);

using TreeView treeView = new();

TreeNode Node1 = new("1");
TreeNode Node2 = new("2");
TreeNode Node3 = new("3");
TreeNode Node4 = new("4");
TreeNode Node5 = new("5");

treeView.Nodes.Add(Node2);

// Set the sort
treeView.Sort();
treeView.CreateControl();

// Nodes
treeView.Nodes.AddRange(
[
Node5,
Node4,
Node3,
Node1
]);

// Assert standard ordering
Assert.Null(Node1.PrevNode);
Assert.Equal(Node1, Node2.PrevNode);
Assert.Equal(Node2, Node3.PrevNode);
Assert.Equal(Node3, Node4.PrevNode);
Assert.Equal(Node4, Node5.PrevNode);

// Expected orders based on switchValue
List<string> expectedOrder = switchValue ? ["1", "2", "3", "4", "5"] : ["2", "5", "3", "1", "4"];

// Get root node
IntPtr rootItem = PInvoke.SendMessage(treeView, PInvoke.TVM_GETNEXTITEM, PInvoke.TVGN_ROOT, IntPtr.Zero);

// Iterate through items
List<string> actualOrder = [];
IntPtr currentItem = rootItem;
char* textBuffer = stackalloc char[256];
int index = 0; // Initialize index
while (currentItem != IntPtr.Zero && index < expectedOrder.Count)
{
TVITEMW item = new()
{
hItem = (HTREEITEM)currentItem,
mask = TVITEM_MASK.TVIF_TEXT,
pszText = textBuffer,
cchTextMax = 256
};

// Send the TVM_GETITEM message to fill the item structure
PInvoke.SendMessage(treeView, PInvoke.TVM_GETITEMW, 0, ref item);

// Convert the buffer to a string and check if equal
actualOrder.Add(new string((char*)item.pszText));

// Increment the index for the next item
index++;

// Move to the next item. This example only moves to the next sibling.
// To traverse children, you would need to first attempt to get a child with TVGN_CHILD, then siblings.
currentItem = PInvoke.SendMessage(treeView, PInvoke.TVM_GETNEXTITEM, PInvoke.TVGN_NEXT, currentItem);
}

// Is actual order the same as expected?
Assert.Equal(expectedOrder, actualOrder);
}
finally
{
testAccessor.s_treeNodePrevNodeDoNotUseFixedIndexWithSortedTreeView = 0;
}
}

private class SubTreeView : TreeView
{
public new bool CanEnableIme => base.CanEnableIme;
Expand Down
Loading