Skip to content

Fix Clear and Remove after Sort in TreeView#10377

Merged
JeremyKuhne merged 5 commits intodotnet:mainfrom
gpetrou:TreeNodeSort
Nov 29, 2023
Merged

Fix Clear and Remove after Sort in TreeView#10377
JeremyKuhne merged 5 commits intodotnet:mainfrom
gpetrou:TreeNodeSort

Conversation

@gpetrou
Copy link
Copy Markdown
Contributor

@gpetrou gpetrou commented Nov 25, 2023

Fixes #10376

Proposed changes

Microsoft Reviewers: Open in CodeFlow

@gpetrou gpetrou requested a review from a team as a code owner November 25, 2023 08:27
@ghost ghost assigned gpetrou Nov 25, 2023
@gpetrou gpetrou force-pushed the TreeNodeSort branch 4 times, most recently from d5dcd7c to 23499f7 Compare November 26, 2023 08:16
@JeremyKuhne JeremyKuhne self-assigned this Nov 27, 2023
Copy link
Copy Markdown
Member

@JeremyKuhne JeremyKuhne left a comment

Choose a reason for hiding this comment

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

Thanks, @gpetrou. I've added comments.

I'm thinking about whether or not we should put back the original logic. There is some risk that it will change behavior with nodes that are equivalent.

In general, one has to be extremely careful with messing with sort algorithms or hashes.

Comment thread src/System.Windows.Forms/src/System/Windows/Forms/TreeView/TreeNode.cs Outdated
Comment thread src/System.Windows.Forms/src/System/Windows/Forms/TreeView/TreeNode.cs Outdated
@ghost ghost added the waiting-author-feedback The team requires more information from the author label Nov 27, 2023
@JeremyKuhne
Copy link
Copy Markdown
Member

@gpetrou It would be good to make sure we have tests that exercise the TreeView nodes and validate their indices after manipulation. Recursive sorting should be captured as well.

Whatever we do here will be in the February update (the next update is already locked) so we've got some time to make sure we've got good test coverage.

@ghost ghost removed the waiting-author-feedback The team requires more information from the author label Nov 27, 2023
Copy link
Copy Markdown
Member

@JeremyKuhne JeremyKuhne left a comment

Choose a reason for hiding this comment

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

Added another comment.

Comment thread src/System.Windows.Forms/src/System/Windows/Forms/TreeView/TreeNode.cs Outdated
@JeremyKuhne
Copy link
Copy Markdown
Member

@gpetrou we discussed this further as a team and we want to put back the original sorting logic to avoid any further regressions around visibility of index changes during sorting and sorting behavior with equivalent nodes. Are you ok with doing this?

Copy link
Copy Markdown
Member

@JeremyKuhne JeremyKuhne left a comment

Choose a reason for hiding this comment

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

One more nit. Are you up for adding index validation tests?

Comment thread src/System.Windows.Forms/src/System/Windows/Forms/TreeView/TreeNode.cs Outdated
@ghost ghost added the waiting-author-feedback The team requires more information from the author label Nov 28, 2023
@ghost ghost removed the waiting-author-feedback The team requires more information from the author label Nov 29, 2023
Copy link
Copy Markdown
Member

@lonitra lonitra left a comment

Choose a reason for hiding this comment

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

🚢

@JeremyKuhne JeremyKuhne merged commit 172405c into dotnet:main Nov 29, 2023
@ghost ghost added this to the 9.0 Preview1 milestone Nov 29, 2023
@lonitra
Copy link
Copy Markdown
Member

lonitra commented Nov 29, 2023

/backport to release/8.0

@github-actions
Copy link
Copy Markdown
Contributor

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

@github-actions
Copy link
Copy Markdown
Contributor

@lonitra 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 Clear and Remove after Sort in TreeView
Using index info to reconstruct a base tree...
A	src/System.Windows.Forms/src/System/Windows/Forms/Controls/TreeView/TreeNode.cs
M	src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/TreeViewTests.cs
Falling back to patching base and 3-way merge...
Auto-merging src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/TreeViewTests.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 Clear and Remove after Sort in TreeView
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!

@gpetrou gpetrou deleted the TreeNodeSort branch November 30, 2023 05:35
@github-actions github-actions Bot locked and limited conversation to collaborators Dec 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

When operating on part of a sorted TreeView tree: Clear() hangs in a loop; Remove() removes wrong nodes.

3 participants