-
Notifications
You must be signed in to change notification settings - Fork 953
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
fix issue 11243 TreeNodeCollection.AddRange has been broken in .NET 8 and no longer results in TreeNodes being drawn at the correct location #11253
Conversation
… and no longer results in TreeNodes being drawn at the correct location
@@ -88,7 +88,8 @@ internal TreeNodeImageIndexer StateImageIndexer | |||
} | |||
|
|||
internal int _index; // our index into our parents child array | |||
internal List<TreeNode> _childNodes = []; | |||
internal int _childCount; | |||
internal TreeNode[] _children; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error CS8618: Non-nullable field '_children' must contain a non-null value when exiting constructor. Consider declaring the field as nullable
@Epica3055 , is this a straight revert of the #9078 PR? It would be good to understand what specific change introduced the regression. Once we know it, it will be easier to decide on the direction of the fix. |
src/System.Windows.Forms/src/System/Windows/Forms/Controls/TreeView/TreeNode.cs
Outdated
Show resolved
Hide resolved
src/System.Windows.Forms/src/System/Windows/Forms/Controls/TreeView/TreeNode.cs
Outdated
Show resolved
Hide resolved
@@ -1250,20 +1250,20 @@ private void SortChildren(TreeView? parentTreeView) | |||
continue; | |||
} | |||
|
|||
if (sorter.Compare(_childNodes[j] /*previous*/, _childNodes[min] /*current*/) <= 0) | |||
if (sorter.Compare(_children[j] /*previous*/, _children[min] /*current*/) <= 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (sorter.Compare(_children[j] /*previous*/, _children[min] /*current*/) <= 0) | |
if (sorter.Compare(previous: _children[j], current: _children[min]) <= 0) |
(if these are indeed the parameter names)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually they are not.
src/System.Windows.Forms/src/System/Windows/Forms/Controls/TreeView/TreeNode.cs
Outdated
Show resolved
Hide resolved
src/System.Windows.Forms/src/System/Windows/Forms/Controls/TreeView/TreeNode.cs
Show resolved
Hide resolved
src/System.Windows.Forms/src/System/Windows/Forms/Controls/TreeView/TreeNodeCollection.cs
Outdated
Show resolved
Hide resolved
880db6f
to
c03c48b
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11253 +/- ##
===================================================
- Coverage 74.32424% 74.24623% -0.07802%
===================================================
Files 3011 3023 +12
Lines 625742 626382 +640
Branches 46547 46711 +164
===================================================
- Hits 465078 465065 -13
- Misses 157265 157917 +652
- Partials 3399 3400 +1
Flags with carried forward coverage won't be shown. Click here to find out more. |
Please add a dedicated test for this scenario - call AddRange twice |
Please check #11243 (comment). Also, even if there is still a problem, can't we still use a List instead of an array? |
Yes, this is a stable repro, when you call AddRange after the native tree view has been populated. This code references a perf optimization for cases when we add large ranges to the treeview, and it seems that the workaround is to insert after the "fixed" node - the last node as previously populated treeview, as opposed to appending after the actual last node. To preserve this workaround as is, we would be inserting into the managed collection of child nodes at the "fixed index" as well, instead of appending, this seems complicated and an array is better suited for such use. To re-do this optimization, we need the specific scenario, Epica has experimented with large ranges and didn't see any difference with or without this perf optimization, I guess that scenario requires image lists, or other complications. But with no understanding of what this code is optimizing, it's risky to remove or modify it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some minor comments, and please address the previous set of comments
|
||
form.Load += (s, e) => | ||
{ | ||
treeView.Nodes[0].Nodes.AddRange(treeNode1, treeNode2, treeNode3); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use rootNode
local variable instead of the item accessor.
}; | ||
form.Show(); | ||
|
||
TreeNode childNode1 = treeView.Nodes[0].Nodes[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verify that treeView.Nodes[0]
is still the rootNode
and then use rootNode
local variable instead of the collection item accessor
|
||
form.Load += (s, e) => | ||
{ | ||
treeView.Nodes[0].Nodes.AddRange(treeNode1, treeNode2, treeNode3); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add 4 nodes to force collection resize, as far as I remember initial size is 4, and we are adding exactly 4 nodes conting child
src/System.Windows.Forms/src/System/Windows/Forms/Controls/TreeView/TreeNode.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
/backport to release/8.0 |
Started backporting to release/8.0: https://github.com/dotnet/winforms/actions/runs/9040166498 |
@Epica3055 backporting to release/8.0 failed, the patch most likely resulted in conflicts: $ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: fix issue 11243 TreeNodeCollection.AddRange has been broken in .NET 8 and no longer results in TreeNodes being drawn at the correct location
Using index info to reconstruct a base tree...
A src/System.Windows.Forms/src/System/Windows/Forms/Controls/TreeView/TreeNode.cs
A src/System.Windows.Forms/src/System/Windows/Forms/Controls/TreeView/TreeNodeCollection.cs
Falling back to patching base and 3-way merge...
Auto-merging src/System.Windows.Forms/src/System/Windows/Forms/TreeNodeCollection.cs
CONFLICT (content): Merge conflict in src/System.Windows.Forms/src/System/Windows/Forms/TreeNodeCollection.cs
Auto-merging src/System.Windows.Forms/src/System/Windows/Forms/TreeNode.cs
CONFLICT (content): Merge conflict in src/System.Windows.Forms/src/System/Windows/Forms/TreeNode.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 fix issue 11243 TreeNodeCollection.AddRange has been broken in .NET 8 and no longer results in TreeNodes being drawn at the correct location
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128 Please backport manually! |
Fixes #11243
Screenshots
Before
After
Test methodology
Test environment(s)
9.0.0-preview.4.24219.3