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.

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

Add version of ListViewItemCollection.AddRange() which accepts IList. #10924

Open
AraHaan opened this issue Feb 21, 2024 · 14 comments
Open

Add version of ListViewItemCollection.AddRange() which accepts IList. #10924

AraHaan opened this issue Feb 21, 2024 · 14 comments
Assignees
Labels
api-suggestion (1) Early API idea and discussion, it is NOT ready for implementation waiting-on-team This work item needs to be discussed with team or is waiting on team action in order to proceed
Milestone

Comments

@AraHaan
Copy link
Member

AraHaan commented Feb 21, 2024

Background and motivation

Currently when I call ToArray() on an List<ListViewItem> to then add the items in bulk in this way:

var sourceEntries = new List<ListViewItem>();
Array.ForEach(
    SettingsFile.SettingsJson.Sources,
    (x) => sourceEntries.Add(
        new ListViewItem(
            new string[]
            {
                x,
            },
            -1)));
this.ListView2.Items.AddRange(sourceEntries.ToArray());

This then generates a warning for the usage of ToArray() saying the inititialization can be simplified to:

this.ListView2.Items.AddRange([.. sourceEntries]);

Which compiles in the IDE, however when doing command line compiles via dotnet build -c Release this results in:

error CS0121: The call is ambiguous between the following methods or properties: 'ListView.ListViewItemCollection.AddRange(ListViewItem[])' and 'ListView.ListViewItemCollection.AddRange(ListView.ListViewItemCollection)'

As such a better option would be to add a version of AddRange that accepts List<ListViewItem> would be a great addition to the API and help fix this problem as well.

API Proposal

using System.Collections.Generic;

namespace System.Windows.Forms;

public partial class ListView
{
    /// <summary>
    ///  Represents the collection of items in a ListView or ListViewGroup
    /// </summary>
    [ListBindable(false)]
    public partial class ListViewItemCollection : IList
    {
        public void AddRange(List<ListViewItem> items);
    }
}

API Usage

var sourceEntries = new List<ListViewItem>();
Array.ForEach(
    SettingsFile.SettingsJson.Sources,
    (x) => sourceEntries.Add(
        new ListViewItem(
            new string[]
            {
                x,
            },
            -1)));
this.ListView2.Items.AddRange(sourceEntries);

Alternative Designs

Change the existing ListViewItemCollection overload of AddRange to also include all types that inherit from IList like List<T> for example.

Risks

Minimal, since it deals with adding ListViewItems from a List<ListViewItem> type of collection.

Will this feature affect UI controls?

I think this would minimally affect them.

@AraHaan AraHaan added api-suggestion (1) Early API idea and discussion, it is NOT ready for implementation untriaged The team needs to look at this issue in the next triage labels Feb 21, 2024
@AraHaan
Copy link
Member Author

AraHaan commented Feb 21, 2024

Edit: I think changing the overload to IList is actually for the best.

@AraHaan AraHaan changed the title Version of ListViewItemCollection.AddRange() which accepts List<ListViewItem>. Add version of ListViewItemCollection.AddRange() which accepts List<ListViewItem>. Feb 21, 2024
AraHaan added a commit to AraHaan/winforms that referenced this issue Feb 21, 2024
@AraHaan
Copy link
Member Author

AraHaan commented Feb 21, 2024

I made an experimental commit that should resolve this issue for .NET 9+: AraHaan@5a4d314

Things to note: While it won't be breaking runtime wise, it might force people to need to recompile their code if they use the version of the overload that accepts ListViewItemCollection and with that overload being changed to accept the interface instead which ListViewItemCollection itself inherits from. An alternative might be to create a stub version of it that accepts the type itself, downcasts to IList then calls it might work however.

@elachlan
Copy link
Contributor

Your test should use [WinFormsFact] not [WinFormsTheory]. I like IList over List.

@elachlan
Copy link
Contributor

@AraHaan can you please update the issue description with IList?

@AraHaan
Copy link
Member Author

AraHaan commented Feb 21, 2024

@AraHaan can you please update the issue description with IList?

Sure

@AraHaan AraHaan changed the title Add version of ListViewItemCollection.AddRange() which accepts List<ListViewItem>. Add version of ListViewItemCollection.AddRange() which accepts IList. Feb 21, 2024
@elachlan elachlan added the waiting-on-team This work item needs to be discussed with team or is waiting on team action in order to proceed label Feb 21, 2024
@AraHaan
Copy link
Member Author

AraHaan commented Feb 21, 2024

Your test should use [WinFormsFact] not [WinFormsTheory]. I like IList over List.

Changed it just now: AraHaan@7e4ac14

AraHaan added a commit to AraHaan/winforms that referenced this issue Feb 22, 2024
AraHaan added a commit to AraHaan/winforms that referenced this issue Feb 22, 2024
@merriemcgaw merriemcgaw removed the untriaged The team needs to look at this issue in the next triage label Feb 22, 2024
@merriemcgaw
Copy link
Member

@JeremyKuhne is actively looking at this and we agree it's a great idea. He's going to create a PR that addresses this space more broadly. Stay tuned, we will be taking a change here in .NET 9 🎉

@merriemcgaw merriemcgaw added this to the 9.0 Preview2 milestone Feb 22, 2024
@AraHaan
Copy link
Member Author

AraHaan commented Feb 22, 2024

@JeremyKuhne is actively looking at this and we agree it's a great idea. He's going to create a PR that addresses this space more broadly. Stay tuned, we will be taking a change here in .NET 9 🎉

So basically the changes I did will be in their PR or should I PR my changes first since they are already completed on this at least to save some of their time?

@JeremyKuhne
Copy link
Member

@AraHaan I already wrote the code, I should have it up tomorrow. It implements the IList<ListViewItem> interface and AddRange(IEnumerable<IListViewItem>) (which matches List<T>). I'll double check to make sure I don't see the ambiguity.

@JeremyKuhne
Copy link
Member

JeremyKuhne commented Feb 23, 2024

Ugh. This still has ambiguity after adding the IEnumerable overload. I think this may actually be an issue with collection expressions? @333fred this issue is another unexpected ambiguity. I haven't figured out how to make a simple repro in SharpLab yet. :/

public class ListView.ListViewItemCollection : IList
{
    public void AddRange(params ListViewItem[] items);
    public void AddRange(ListViewItemCollection items);
}
ListView listView = new();
List<ListViewItem> sourceEntries = [];

// This gives a suggestion for
listView.Items.AddRange(sourceEntries.ToArray()); 
// this, but this is ambiguous
// listView.Items.AddRange([.. sourceEntries]);

@lonitra lonitra modified the milestones: 9.0 Preview2, 9.0 Preview3 Feb 23, 2024
@JeremyKuhne
Copy link
Member

Have the standalone repro now:

using System;
using System.Collections;
using System.Collections.Generic;

public class ListViewItem
{
}

public class ListViewItemCollection : IList
{
    public object? this[int index] { get => default; set{}}
    public bool IsFixedSize => default;
    public bool IsReadOnly => default;
    public int Count => default;
    public bool IsSynchronized => default;
    public object SyncRoot => this;
    public int Add(object? value) => default;
    public void AddRange(params ListViewItem[] items) {}
    public void AddRange(ListViewItemCollection items) {}
    public void Clear() {}
    public bool Contains(object? value) => default;
    public void CopyTo(Array array, int index) {}
    public IEnumerator GetEnumerator() => default!;
    public int IndexOf(object? value) => default;
    public void Insert(int index, object? value) {}
    public void Remove(object? value) {}
    public void RemoveAt(int index) {}
}

public class Test
{
    public void M()
    {
        ListViewItemCollection listViewCollection = new();
        List<ListViewItem> sourceEntries = [];
        listViewCollection.AddRange(sourceEntries.ToArray());
        listViewCollection.AddRange([.. sourceEntries]);
    }
}

@JeremyKuhne
Copy link
Member

Ok, there is no great way to solve this completely transparently that doesn't make things worse. We could add an implicit conversion:

    public static implicit operator ListViewItemCollection(ListViewItem[] arr)
    {
        var col = new ListViewItemCollection();
        col.AddRange(arr);
        return col;
    }

But that adds additional overhead. If we add AddRange(IEnumerable) you don't have to ToArray() or [] from an existing collection and it will save allocations.

@JeremyKuhne
Copy link
Member

Linked in the C# proposal which would allow us to fix the collection expression ambiguity. Added an issue to track ambiguity problems: #10946

Also created a new API proposal to look at updating all of our collections: #10947

@LeafShi1 LeafShi1 removed this from the 9.0 Preview3 milestone Mar 19, 2024
@LeafShi1 LeafShi1 added this to the 9.0 Preview4 milestone Mar 19, 2024
@LeafShi1 LeafShi1 modified the milestones: 9.0 Preview4, 9.0 Preview5 Apr 23, 2024
@LeafShi1 LeafShi1 modified the milestones: 9.0 Preview5, 9.0 Preview6 May 22, 2024
@lonitra lonitra modified the milestones: 9.0 Preview6, 9.0 Preview7 Jun 20, 2024
@michael-hawker
Copy link

Would this just be solved by .NET itself supporting ranges by dotnet/runtime#18087?

@lonitra lonitra modified the milestones: 9.0 Preview7, 9.0 RC1 Jul 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion (1) Early API idea and discussion, it is NOT ready for implementation waiting-on-team This work item needs to be discussed with team or is waiting on team action in order to proceed
Projects
None yet
Development

No branches or pull requests

7 participants