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.

Sign up for GitHub

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

Jump to bottom

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

Open
snnz opened this issue Mar 8, 2023 · 14 comments · May be fixed by #11300
Open

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

snnz opened this issue Mar 8, 2023 · 14 comments · May be fixed by #11300
Labels
📖 documentation: breaking A change in behavior that could be breaking for applications. Needs to be documented. 📚 documentation Documentation bug or improvement 🚧 work in progress Work that is current in progress help wanted Good issue for external contributors untriaged The team needs to look at this issue in the next triage
Milestone
Help wanted

Comments

@snnz
Copy link

snnz commented Mar 8, 2023 •
edited by Tanya-Solyanik

.NET version

6.0

Did it work in .NET Framework?

No

Did it work in any of the earlier releases of .NET Core or .NET 5+?

No

Issue description

When the tree nodes are appended to the non-empty TreeNodeCollection owned by the sorted TreeView with already created underlying window, the resulting order of items in the actual window does not match the one in the collection (which is correct).

The cause is how method TreeNode.PrevNode works: when called from within TreeNodeCollection.AddRange of non-empty collection, it always uses fixed index as the position of the previous node, assuming the normal backward insertion of the nodes in the unsorted tree. Yet in case of the sorted TreeView, insertions are made at various positions according to a sort order.

There obviously should be a check for the sorted state of the tree in TreeNode.PrevNode to avoid this.

Steps to reproduce

namespace TreeViewTest
{
  internal static class Program
  {
    [STAThread]
    static void Main()
    {
      ApplicationConfiguration.Initialize();

      var treeView = new TreeView();
      treeView.Nodes.Add(new TreeNode("2"));
      treeView.Sort();

      var form = new Form();
      form.Controls.Add(treeView);
      form.Load += (sender, e) => treeView.Nodes.AddRange(new TreeNode[] {
        new TreeNode("3"),
        new TreeNode("5"),
        new TreeNode("1"),
        new TreeNode("4")
      });
      Application.Run(form);
    }
  }
}

This results in:
Clipboard01
@snnz snnz added the untriaged The team needs to look at this issue in the next triage label Mar 8, 2023
@merriemcgaw
Copy link
Member

Thanks for filing this issue. I will hand this over to the community if someone has ideas for how to fix this and put it under a configuration switch. Given that this has been the behvavior since .NET Framework days, the team will not be able to prioritize it on our end.

@merriemcgaw merriemcgaw added the help wanted Good issue for external contributors label Mar 9, 2023
@ghost ghost added this to the Help wanted milestone Mar 9, 2023
@ghost
Copy link

ghost commented Mar 9, 2023

This issue is now marked as "help wanted", and we’re looking for a community volunteer to work on this issue. If we receive no interest in 180 days, we will close the issue. To learn more about how we handle feature requests, please see our documentation.

Happy Coding!

@merriemcgaw merriemcgaw removed the untriaged The team needs to look at this issue in the next triage label Mar 9, 2023
@snnz snnz mentioned this issue Mar 10, 2023
Closed
@ghost ghost added the 🚧 work in progress Work that is current in progress label Mar 10, 2023
@snnz

This comment was marked as outdated.

Sign in to view
@ghost ghost removed the 🚧 work in progress Work that is current in progress label Mar 10, 2023
@elachlan
Copy link
Contributor

elachlan commented Nov 3, 2023

See #8556 for examples of context switches.

elachlan added a commit to elachlan/winforms that referenced this issue Dec 18, 2023
@elachlan
Fixes dotnet#8768
831082a
@elachlan elachlan mentioned this issue Dec 18, 2023
Closed
@ghost ghost added the 🚧 work in progress Work that is current in progress label Dec 18, 2023
elachlan added a commit to elachlan/winforms that referenced this issue Jan 2, 2024
@elachlan
Fixes dotnet#8768
4dc936e
elachlan added a commit to elachlan/winforms that referenced this issue Feb 5, 2024
@elachlan
Fixes dotnet#8768
12bb875
@rossgurbutt rossgurbutt mentioned this issue Apr 19, 2024
Closed
@RutulPatel8 RutulPatel8 linked a pull request May 1, 2024 that will close this issue
Open
@RutulPatel8
Copy link
Contributor

RutulPatel8 commented May 1, 2024 •
edited

@lonitra
Here is my PR.
Can you please review it.

#11300

Proof of testing
image

@Tanya-Solyanik Tanya-Solyanik added the 💥 regression-release Regression from a public release label May 1, 2024
@RutulPatel8
Copy link
Contributor

@Tanya-Solyanik Can you please review my PR.
I can see the issue in XUnit Test but It is not related to the my code.

@Tanya-Solyanik
Copy link
Member

@RutulPatel8 - is this issue fixed by PR #11253?

@RutulPatel8
Copy link
Contributor

I'm not sure, but I can say one thing: There have been lots of changes made with these PRs, and I've made the same change by altering a single 'if' condition. Could you please review my PR?"

@Tanya-Solyanik Tanya-Solyanik removed the 💥 regression-release Regression from a public release label May 6, 2024
@Tanya-Solyanik
Copy link
Member

I had verified again, this is reproing in .NET Framework, my experiment was wrong because I re-sorted the TreeView after adding the unsorted range

@RutulPatel8
Copy link
Contributor

@Tanya-Solyanik It means still you have facing the same issue and my changes wasn't working with.net framework?

@Tanya-Solyanik
Copy link
Member

@RutulPatel8 - your change is good, except for the failing tests. All failing tests are related to treeviews and that they are not failing for other PRs.

@Tanya-Solyanik
Copy link
Member

Moving to triage to confirm that we don't want an opt-in switch for this change.

@Tanya-Solyanik Tanya-Solyanik added untriaged The team needs to look at this issue in the next triage 📚 documentation Documentation bug or improvement labels May 7, 2024
@merriemcgaw
Copy link
Member

This has been present since .NET Framework so we will need an opt-out switch for this behavior. @Tanya-Solyanik suggests TreeNodeCollectionAddRangeRespectsSortOrder

This will potentially be a breaking change, but the current behavior is not what we'd expected.

@merriemcgaw merriemcgaw added the 📖 documentation: breaking A change in behavior that could be breaking for applications. Needs to be documented. label May 15, 2024
@merriemcgaw
Copy link
Member

@RutulPatel8, are you planning on picking this one up, or should we have a team member wrap up the work? Thanks!

@Tanya-Solyanik Tanya-Solyanik added untriaged The team needs to look at this issue in the next triage and removed untriaged The team needs to look at this issue in the next triage labels May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
📖 documentation: breaking A change in behavior that could be breaking for applications. Needs to be documented. 📚 documentation Documentation bug or improvement 🚧 work in progress Work that is current in progress help wanted Good issue for external contributors untriaged The team needs to look at this issue in the next triage
Projects
None yet
Milestone
Help wanted
5 participants
@elachlan @Tanya-Solyanik @snnz @merriemcgaw @RutulPatel8