-
Notifications
You must be signed in to change notification settings - Fork 955
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
8768 :: Resolve Addrange issue after sorting. #11423
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #11423 +/- ##
===================================================
+ Coverage 74.29395% 74.39955% +0.10560%
===================================================
Files 3026 3032 +6
Lines 627152 628196 +1044
Branches 46758 46813 +55
===================================================
+ Hits 465936 467375 +1439
+ Misses 157863 157468 -395
Partials 3353 3353
Flags with carried forward coverage won't be shown. Click here to find out more. |
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.
Hi @RutulPatel8 - I apologize that we had accidentally destroyed your branch when reviewing and trying to rebase it on the latest. This PR restores your changes with minor modifications, see the commit history, we applied the fix to the failing test, used fluent assertions in the new test and used [WinFormsTheory] instead of the [WinFormFact] on the test. You can continue working in this branch by pulling it down using GH Client gh pr checkout 11423
, GitHub Desktop, or VS-Code. Or you can cherry-pick your commit c6dd018 and create a new PR.
Because this change could be breaking for someone who is not sorting nodes intentionally, it requires an opt-out switch. If you are not planning to pick it up, a team member can add this switch and finish this work. Please let us know if you have the bandwidth to wrap it up in 2 weeks. This will give us time to get this change into the next preview.
src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/TreeNodeCollectionTests.cs
Outdated
Show resolved
Hide resolved
src/System.Windows.Forms/src/System/Windows/Forms/Controls/TreeView/TreeNodeCollection.cs
Outdated
Show resolved
Hide resolved
src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/TreeNodeCollectionTests.cs
Outdated
Show resolved
Hide resolved
src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/TreeNodeCollectionTests.cs
Outdated
Show resolved
Hide resolved
@@ -24,6 +24,7 @@ internal static partial class LocalAppContextSwitches | |||
internal const string DataGridViewUIAStartRowCountAtZeroSwitchName = "System.Windows.Forms.DataGridViewUIAStartRowCountAtZero"; | |||
internal const string NoClientNotificationsSwitchName = "Switch.System.Windows.Forms.AccessibleObject.NoClientNotifications"; | |||
internal const string EnableMsoComponentManagerSwitchName = "Switch.System.Windows.Forms.EnableMsoComponentManager"; | |||
internal const string TreeNodePrevNodeDoNotUseFixedIndexWithSortedTreeViewSwitchName = "System.Windows.Forms.TreeNodeDoNotUseFixedIndexWithSortedTreeView"; |
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 use TreeNodeCollectionAddRangeRespectsSortOrder
as a switch name, and make it an opt-out switch, i.e. default value should be true.
src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/TreeNodeCollectionTests.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.
Added a minor comment, otherwise looks good
@@ -202,6 +209,16 @@ public static bool EnableMsoComponentManager | |||
get => GetCachedSwitchValue(EnableMsoComponentManagerSwitchName, ref s_enableMsoComponentManager); | |||
} | |||
|
|||
/// <summary> | |||
/// Indicates whether TreeNode.PrevNode uses a fixed index to return its value. |
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.
Suggestion:
When set to (default), API will insert nodes in the sorted order. To get behavior compatible with the previous versions of .NET and .NET Frameworek, set this switch to .
Hi @Tanya-Solyanik , Let me know so I can generate PR with all existing and remaining changes. |
@RutulPatel8 - thank you! We are done with the new appcontext switch and tests. This fix will ship in the next preview, assuming everything goes well, please test it on your side. But we have many other issues we are looking for help with. |
Fixes #8768
Proposed changes
if tree is already sorted then we don't want to set fixed Index value. due to these reason It is misbehave.
Customer Impact
Regression?
Risk
Before
When the user adds a value to the treenode list and sorts it using treeview.sort(), it works fine. Subsequently, when the user adds individual trees through the add event, it functions as expected and sorts the entire tree automatically. However, if the user adds multiple values through AddRange, it does not work as expected.
After
I've fixed the sorting issue we discussed earlier, and now everything is working smoothly. When users add values individually or use treeview.sort(), the sorting works fine. Even when they add multiple values at once using AddRange, the sorting behaves as expected.
Test methodology
Test environment(s)
Microsoft Reviewers: Open in CodeFlow