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

Refactor ArrangedElementCollection to use List<IArrangedElement> instead of ArrayList #8207

Merged
merged 10 commits into from
Nov 29, 2022
Merged

Refactor ArrangedElementCollection to use List<IArrangedElement> instead of ArrayList #8207

merged 10 commits into from
Nov 29, 2022

Conversation

Jericho
Copy link
Contributor

@Jericho Jericho commented Nov 16, 2022 •
edited by ghost
Loading

Related: #8140

Microsoft Reviewers: Open in CodeFlow

@Jericho Jericho requested a review from a team as a code owner November 16, 2022 17:23
@ghost ghost assigned Jericho Nov 16, 2022
This was referenced Nov 16, 2022
Open
Closed
@elachlan
Copy link
Contributor 

src\System.Windows.Forms\src\System\Windows\Forms\Control.ControlCollection.cs(193,43): error CS8604: (NETCORE_ENGINEERING_TELEMETRY=Build) Possible null reference argument for parameter 'item' in 'bool List<IArrangedElement>.Contains(IArrangedElement item)'.

@Jericho
Copy link
Contributor Author

Jericho commented Nov 16, 2022

@elachlan Thanks for pointing this out. Fixed.

dreddy-work
dreddy-work previously requested changes Nov 17, 2022
View reviewed changes
Copy link
Member

@dreddy-work dreddy-work left a comment

Choose a reason for hiding this comment

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

Please add description with details for each change types to help review better.

@ghost ghost added 📭 waiting-author-feedback The team requires more information from the author and removed 📭 waiting-author-feedback The team requires more information from the author labels Nov 17, 2022
@Jericho Jericho force-pushed the GH-8140_ArrangedElementCollection branch from 421b548 to ed827e8 Compare November 22, 2022 14:18
@Jericho Jericho force-pushed the GH-8140_ArrangedElementCollection branch from ed827e8 to 342c6ce Compare November 23, 2022 15:29
@elachlan
Copy link
Contributor

@dreddy-work this looks like flaky tests, can you please refresh the pipeline?

JeremyKuhne
JeremyKuhne requested changes Nov 28, 2022
View reviewed changes
Copy link
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 for doing this! I've got a couple of comments to address, but otherwise looks good!

src/System.Windows.Forms/src/System/Windows/Forms/Layout/ArrangedElementCollection.cs Outdated Show resolved Hide resolved
src/System.Windows.Forms/src/System/Windows/Forms/Layout/ArrangedElementCollection.cs Outdated
int IList.Add(object? value) => InnerList.Add(value);
int IList.Add(object? value)
{
if (value is IArrangedElement element)
Copy link
Member

Choose a reason for hiding this comment

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

Typing this collection is a breaking change so it is probably better to throw an argument exception here and change the documentation to make it clear that the collection will only take IArrangedElement now. -1 is not something you would ever get from ArrayList.Add.

Copy link
Member

Choose a reason for hiding this comment

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

One option here is to cast the inner list to IList so you get the same exact throw it would do (ArgumentException). You'll also get the count out that way.

Copy link
Contributor Author

@Jericho Jericho Nov 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I like your idea to cast to IList. I'll make the change.

FYI: as part of #8140, we modified several Add, Contains, IndexOf, etc. methods that could probably have benefited from your suggestion. Here are just a few examples: here, here, here and here.

Copy link
Member

Choose a reason for hiding this comment

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

@Jericho would you mind updating those?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely. I'll go back to all PRS where we refactored classes to use List instead if Array List.

@ghost ghost added the 📭 waiting-author-feedback The team requires more information from the author label Nov 28, 2022
@JeremyKuhne JeremyKuhne added the 📖 documentation: breaking A change in behavior that could be breaking for applications. Needs to be documented. label Nov 28, 2022
@ghost ghost removed the 📭 waiting-author-feedback The team requires more information from the author label Nov 29, 2022
@Jericho Jericho force-pushed the GH-8140_ArrangedElementCollection branch from 0421b3c to 5e5fda2 Compare November 29, 2022 16:06
JeremyKuhne
JeremyKuhne reviewed Nov 29, 2022
View reviewed changes
Copy link
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.

:shipit:

@JeremyKuhne JeremyKuhne merged commit 4485ce5 into dotnet:main Nov 29, 2022
@ghost ghost added this to the 8.0 Preview1 milestone Nov 29, 2022
@Jericho Jericho deleted the GH-8140_ArrangedElementCollection branch December 2, 2022 14:50
@Jericho Jericho mentioned this pull request Dec 2, 2022
Merged
@gpetrou gpetrou mentioned this pull request Dec 22, 2022
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Jan 1, 2023
@dreddy-work
Copy link
Member

@elachlan , i see change to type IArrangedElement is internal only. Is this impacting any public APIs?

@JeremyKuhne JeremyKuhne removed the 📖 documentation: breaking A change in behavior that could be breaking for applications. Needs to be documented. label Sep 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dreddy-work dreddy-work dreddy-work left review comments

@elachlan elachlan elachlan left review comments

@JeremyKuhne JeremyKuhne JeremyKuhne approved these changes

Assignees

@Jericho Jericho

Labels
None yet
Projects
None yet
Milestone
8.0 Preview1
Development

Successfully merging this pull request may close these issues.
4 participants
@Jericho @elachlan @dreddy-work @JeremyKuhne