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

[API Proposal]: Allow ControlCollection.AddRange to use params keyword. #8938

Closed
ALiwoto opened this issue Apr 4, 2023 · 12 comments
Closed
Labels
api-approved (4) API was approved in API review, it can be implemented blocking Used by the API Review Board

Comments

@ALiwoto
Copy link

ALiwoto commented Apr 4, 2023

Background and motivation

Currently Control.ControlCollection.AddRange method is accepting an array of Control, this means each time that we want to add multiple controls at once, we have to make a new array. By using params keyword using this method will become easier.

API Proposal

  • Add params keyword to all collection AddRange methods that don't already have it.
namespace System.Windows.Forms;

public class Control
{
    public class ControlCollection
    {
        public virtual void AddRange(params Control[] controls);
    }
}

Likewise we want to update our other collections that have an AddRange:

// Note that parameter names are left as they currently are.

System.ComponentModel.Design.DesignerActionListCollection.AddRange(params DesignerActionList[] value)
System.Windows.Forms.AutoCompleteStringCollection.AddRange(params string[] value)
System.Windows.Forms.ComboBox.ObjectCollection.AddRange(params object[] items)
System.Windows.Forms.Design.Behavior.BehaviorServiceAdornerCollection.AddRange(params Adorner[] value)
System.Windows.Forms.Design.Behavior.GlyphCollection.AddRange(params Glyph[] value)
System.Windows.Forms.ImageList.ImageCollection.AddRange(params Image[] images)
System.Windows.Forms.ListBox.IntegerCollection.AddRange(params int[] items)
System.Windows.Forms.ListBox.ObjectCollection.AddRange(params object[] items)
System.Windows.Forms.ListView.ListViewItemCollection.AddRange(params ListViewItem[] items)
System.Windows.Forms.ListView.ColumnHeaderCollection.AddRange(params ColumnHeader[] values)
System.Windows.Forms.ListViewGroupCollection.AddRange(params ListViewGroup[] groups)
System.Windows.Forms.ListViewItem.ListViewSubItemCollection.AddRange(params ListViewSubItem[] items)
System.Windows.Forms.ListViewItem.ListViewSubItemCollection.AddRange(params string[] items)
System.Windows.Forms.TabControl.TabPageCollection.AddRange(params TabPage[] pages)
System.Windows.Forms.ToolStripItemCollection.AddRange(params ToolStripItem[] toolStripItems)
System.Windows.Forms.ToolStripPanel.ToolStripPanelRowCollection.AddRange(params ToolStripPanelRow[] value)
System.Windows.Forms.TreeNodeCollection.AddRange(params TreeNode[] nodes)

API Usage

this.Controls.AddRange(this.BtnControl1, this.BtnControl2);

Risks

No known risks.

Will this feature affect UI controls?

  • No, VS Designer doesn't need to support this feature.
  • The impact will be easier use of this method by users.
  • No, this feature doesn't need to be localized.
@ALiwoto ALiwoto 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 Apr 4, 2023
@JeremyKuhne
Copy link
Member

This is great, we should expand it to list the other collections where we have an AddRange(). @ALiwoto you can find the other ones by looking through the PublicAPI.shipped.txt file, would you be able to add the others? All you need is the API surface area. I've added one to the summary. If you can't, that's fine, I'll get to it when I have some time free.

@ALiwoto
Copy link
Author

ALiwoto commented Apr 4, 2023

@JeremyKuhne Sure, should I make another PR after finding them? Or I can just push to my current fork?

@JeremyKuhne
Copy link
Member

@ALiwoto listing them here is the priority. You can just expand the existing PR, but we won't be able to take it until we've got this approved by the .NET API review board. Please tag me when you think you've got everything listed here and I'll work on getting the official approval.

@elachlan
Copy link
Contributor

elachlan commented Apr 5, 2023

Awesome! Great idea, minimal api impact, and improves usability.

@ALiwoto
Copy link
Author

ALiwoto commented Apr 5, 2023

@JeremyKuhne
Here is the list I found:

24 in total.


I also found about System.Windows.Forms.NonNullCollection<T>.AddRange(IEnumerable<T> items):

/// <summary>
///  Adds the items in <paramref name="items"/> to the collection.
/// </summary>
public void AddRange(IEnumerable<T> items)

But the issue with it is that it takes IEnumerable<T> as argument, so we can't use params keyword for it (Perhaps we can overload it?)

@elachlan
Copy link
Contributor

elachlan commented Apr 5, 2023

NonNullCollection is Internal. So we can make a breaking change if it makes sense.

@JeremyKuhne
Copy link
Member

Here is the list I found:

Thanks @ALiwoto. I've double checked and removed the internal ones. I've updated the proposal with the complete list.

@JeremyKuhne JeremyKuhne added api-ready-for-review (2) API is ready for formal API review; applied by the issue owner blocking Used by the API Review Board and removed 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 Apr 5, 2023
@terrajobst
Copy link
Member

terrajobst commented Apr 11, 2023

Video

  • Looks good as proposed
namespace System.Windows.Forms;

public partial class Control
{
    public partial class ControlCollection
    {
        // Existing signature:
        // public virtual void AddRange(Control[] controls);
        public virtual void AddRange(params Control[] controls);
    }
}

@terrajobst terrajobst added api-approved (4) API was approved in API review, it can be implemented and removed api-ready-for-review (2) API is ready for formal API review; applied by the issue owner labels Apr 11, 2023
@elachlan
Copy link
Contributor

@ALiwoto did you want to do the PR to implement it?

@ALiwoto
Copy link
Author

ALiwoto commented Apr 12, 2023

@elachlan

did you want to do the PR to implement it?

Yes.

@JeremyKuhne can I commit the changes to my current PR? Or I have to revert the last commit from it first?

@JeremyKuhne
Copy link
Member

@ALiwoto you should probably change all of these in one PR. You can do it in your existing PR. Note, however, that you don't add to the shipped API file. There is an autofix for adding the changes to the unshipped file. Just build in VS and you can use the lightbulb to add them. You need to build both release and debug.

ALiwoto pushed a commit to ALiwoto/winforms that referenced this issue Apr 14, 2023
…ord.

Resolves dotnet#8938

Signed-off-by: Aliwoto <woto@kaizoku.cyou>
@ghost ghost added the 🚧 work in progress Work that is current in progress label Apr 14, 2023
ALiwoto pushed a commit to ALiwoto/winforms that referenced this issue Apr 29, 2023
…ord.

Resolves dotnet#8938

Signed-off-by: Aliwoto <woto@kaizoku.cyou>
ALiwoto pushed a commit to ALiwoto/winforms that referenced this issue May 14, 2023
…ord.

Resolves dotnet#8938

Signed-off-by: Aliwoto <woto@kaizoku.cyou>
@ghost ghost removed the 🚧 work in progress Work that is current in progress label Aug 9, 2023
@John-Qiao
Copy link
Member

Verified this issue in feature/9.0 branch, the params keyword is added to arguments of ControlCollection.AddRange method now, so closed it.
8938-testresult

@dotnet dotnet locked as resolved and limited conversation to collaborators Sep 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved (4) API was approved in API review, it can be implemented blocking Used by the API Review Board
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants