fix issue 11243 TreeNodeCollection.AddRange has been broken in .NET 8 and no longer results in TreeNodes being drawn at the correct location by Epica3055 · Pull Request #11253 · dotnet/winforms

Epica3055 · 2024-04-23T10:17:51Z

Fixes #11243

Screenshots

Before

After

Test methodology

manually

Test environment(s)

9.0.0-preview.4.24219.3

… and no longer results in TreeNodes being drawn at the correct location

elachlan · 2024-04-23T21:44:51Z

    internal int _index;                  // our index into our parents child array
-    internal List<TreeNode> _childNodes = [];
+    internal int _childCount;
+    internal TreeNode[] _children;


error CS8618: Non-nullable field '_children' must contain a non-null value when exiting constructor. Consider declaring the field as nullable

Tanya-Solyanik · 2024-04-25T17:22:59Z

@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.

Tanya-Solyanik · 2024-04-26T18:38:23Z

                    }

-                    if (sorter.Compare(_childNodes[j] /*previous*/, _childNodes[min] /*current*/) <= 0)
+                    if (sorter.Compare(_children[j] /*previous*/, _children[min] /*current*/) <= 0)


Suggested change

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)

actually they are not.

codecov · 2024-04-29T02:37:29Z

Codecov Report

Attention: Patch coverage is 82.84024% with 29 lines in your changes are missing coverage. Please review.

Project coverage is 74.24623%. Comparing base (be78cbb) to head (f0ccdfa).
Report is 44 commits behind head on main.

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

Flag	Coverage Δ
Debug	`74.24623% <82.84024%> (-0.07802%)`	⬇️
integration	`18.27645% <27.55906%> (-0.04969%)`	⬇️
production	`46.92204% <77.16535%> (-0.03560%)`	⬇️
test	`97.03764% <100.00000%> (-0.00375%)`	⬇️
unit	`43.87918% <77.16535%> (-0.02061%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Tanya-Solyanik · 2024-04-29T22:18:01Z

Please add a dedicated test for this scenario - call AddRange twice

gpetrou · 2024-05-01T04:37:14Z

Please check #11243 (comment). Also, even if there is still a problem, can't we still use a List instead of an array?

Tanya-Solyanik · 2024-05-02T16:36:42Z

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.

Tanya-Solyanik

Added some minor comments, and please address the previous set of comments

Tanya-Solyanik · 2024-05-07T17:17:44Z

+
+        form.Load += (s, e) =>
+        {
+            treeView.Nodes[0].Nodes.AddRange(treeNode1, treeNode2, treeNode3);


use rootNode local variable instead of the item accessor.

Tanya-Solyanik · 2024-05-07T17:43:14Z

+        };
+        form.Show();
+
+        TreeNode childNode1 = treeView.Nodes[0].Nodes[0];


Verify that treeView.Nodes[0] is still the rootNode and then use rootNode local variable instead of the collection item accessor

Tanya-Solyanik · 2024-05-07T17:46:03Z

+
+        form.Load += (s, e) =>
+        {
+            treeView.Nodes[0].Nodes.AddRange(treeNode1, treeNode2, treeNode3);


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

Tanya-Solyanik

Thank you!

Epica3055 · 2024-05-11T01:45:15Z

/backport to release/8.0

github-actions · 2024-05-11T01:45:26Z

Started backporting to release/8.0: https://github.com/dotnet/winforms/actions/runs/9040166498

github-actions · 2024-05-11T01:45:49Z

@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!

fix issue 11243 TreeNodeCollection.AddRange has been broken in .NET 8…

10e616f

… and no longer results in TreeNodes being drawn at the correct location

Epica3055 requested a review from a team as a code owner April 23, 2024 10:17

dotnet-policy-service Bot assigned Epica3055 Apr 23, 2024

Epica3055 marked this pull request as draft April 23, 2024 10:18

dotnet-policy-service Bot added the draft draft PR label Apr 23, 2024

elachlan reviewed Apr 23, 2024

View reviewed changes

Epica3055 mentioned this pull request Apr 26, 2024

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

Closed

Tanya-Solyanik reviewed Apr 26, 2024

View reviewed changes

Comment thread src/System.Windows.Forms/src/System/Windows/Forms/Controls/TreeView/TreeNode.cs Outdated

Tanya-Solyanik reviewed Apr 26, 2024

View reviewed changes

Comment thread src/System.Windows.Forms/src/System/Windows/Forms/Controls/TreeView/TreeNode.cs Outdated

Tanya-Solyanik reviewed Apr 26, 2024

View reviewed changes

fix comments

c16015c

Epica3055 marked this pull request as ready for review April 28, 2024 08:05

fix error CS8618: Non-nullable field '_children'

c03c48b

Epica3055 force-pushed the Issue_11243_2 branch from 880db6f to c03c48b Compare April 28, 2024 08:50

Epica3055 removed the draft draft PR label Apr 28, 2024

Epica3055 added 4 commits April 28, 2024 17:07

fix error

7af2460

fix variable names

9c079a1

fix tests

f517f73

fix tests

79ccf7f

Tanya-Solyanik added the waiting-author-feedback The team requires more information from the author label Apr 29, 2024

Tanya-Solyanik mentioned this pull request May 1, 2024

8768 :: Resolve Addrange issue after sorting. #11300

Closed

Tanya-Solyanik mentioned this pull request May 6, 2024

TreeNodeCollection.AddRange works incorrectly for non-empty collection of the sorted TreeView #8768

Closed

dotnet-policy-service Bot removed the waiting-author-feedback The team requires more information from the author label May 7, 2024

add unit test

7970656

Epica3055 force-pushed the Issue_11243_2 branch from 2bc2484 to 7970656 Compare May 7, 2024 02:39

Tanya-Solyanik reviewed May 7, 2024

View reviewed changes

Tanya-Solyanik added the waiting-author-feedback The team requires more information from the author label May 7, 2024

use createControl remove Form in unit test

ec750e5

dotnet-policy-service Bot removed the waiting-author-feedback The team requires more information from the author label May 8, 2024

fix comment

f0ccdfa

Tanya-Solyanik approved these changes May 8, 2024

View reviewed changes

Tanya-Solyanik added the ready-to-merge PRs that are ready to merge but worth notifying the internal team. label May 8, 2024

Tanya-Solyanik merged commit b76827c into dotnet:main May 9, 2024

dotnet-policy-service Bot added this to the 9.0 Preview5 milestone May 9, 2024

dotnet-policy-service Bot removed the ready-to-merge PRs that are ready to merge but worth notifying the internal team. label May 9, 2024

github-actions Bot locked and limited conversation to collaborators Jun 11, 2024

Epica3055 deleted the Issue_11243_2 branch January 8, 2025 08:31

	if (sorter.Compare(_children[j] /previous/, _children[min] /current/) <= 0)
	if (sorter.Compare(previous: _children[j], current: _children[min]) <= 0)

Uh oh!

Conversation

Epica3055 commented Apr 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Screenshots

Before

After

Test methodology

Test environment(s)

Uh oh!

elachlan Apr 23, 2024

Choose a reason for hiding this comment

Uh oh!

Tanya-Solyanik commented Apr 25, 2024

Uh oh!

Uh oh!

Uh oh!

Tanya-Solyanik Apr 26, 2024

Choose a reason for hiding this comment

Uh oh!

Epica3055 May 8, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Apr 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Tanya-Solyanik commented Apr 29, 2024

Uh oh!

gpetrou commented May 1, 2024

Uh oh!

Tanya-Solyanik commented May 2, 2024

Uh oh!

Tanya-Solyanik left a comment

Choose a reason for hiding this comment

Uh oh!

Tanya-Solyanik May 7, 2024

Choose a reason for hiding this comment

Uh oh!

Tanya-Solyanik May 7, 2024

Choose a reason for hiding this comment

Uh oh!

Tanya-Solyanik May 7, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Tanya-Solyanik left a comment

Choose a reason for hiding this comment

Uh oh!

Epica3055 commented May 11, 2024

Uh oh!

github-actions Bot commented May 11, 2024

Uh oh!

github-actions Bot commented May 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Epica3055 commented Apr 23, 2024 •

edited

Loading

codecov Bot commented Apr 29, 2024 •

edited

Loading