Refactor BindingCollection to replace ArrayList with List<Binding> #8364

Jericho · 2022-12-11T16:36:35Z

Related: #8140

Microsoft Reviewers: Open in CodeFlow

lonitra

Just a comment about enabling nullability since we seem to be doing those checks in the file anyways.

src/System.Windows.Forms/src/System/Windows/Forms/BindingsCollection.cs

hughbe · 2022-12-15T11:23:14Z

src/System.Windows.Forms/src/System/Windows/Forms/BindingsCollection.cs


        /// <summary>
        ///  Gets the bindings in the collection as an object.
        /// </summary>
-        protected override ArrayList List => _list ??= new ArrayList();
+        protected override ArrayList List => ArrayList.Adapter(_list);


is this breaking? Previously ArrayList and List would return the same instance, now a new instance is created each time

@hughbe, It doesn't creates new list but just wraps around it.

ghost · 2022-12-29T13:02:15Z

This submission has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 14 days.

It will be closed if no further activity occurs within 7 days of this comment.

elachlan · 2023-01-05T23:51:43Z

bumping this so it doesn't get closed.

ghost · 2023-01-20T01:02:17Z

This submission has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 14 days.

It will be closed if no further activity occurs within 7 days of this comment.

ghost · 2023-02-03T04:01:58Z

This submission has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 14 days.

It will be closed if no further activity occurs within 7 days of this comment.

elachlan · 2023-02-03T07:18:42Z

@Jericho if its okay with you, I can possibly take over this?

Jericho · 2023-02-03T16:06:55Z

@elachlan sure but all feedback was addressed and just waiting for merge.

elachlan · 2023-02-04T01:56:30Z

I'll leave it to the team to finish the review.

elachlan · 2023-02-16T03:27:19Z

@dreddy-work I think this might be waiting on the team. Could someone please review it and see what needs to be done?

dreddy-work · 2023-02-16T17:53:39Z

@dreddy-work I think this might be waiting on the team. Could someone please review it and see what needs to be done?

I looked into it again and i feel my comment above about initializing list still make sense in this scenario. If @Jericho / @elachlan push a change with respect to that, I will merge it. Rest all look good to me.

Jericho · 2023-02-17T18:07:02Z

@dreddy-work your feedback was addressed on Dec 13 2022 in commit 8a7327d

dreddy-work · 2023-02-17T18:09:17Z

@dreddy-work your feedback was addressed on Dec 13 2022 in commit 8a7327d

Thanks @Jericho, I saw comment was not resolved but didn't cross check the commit. Thank you.

dreddy-work · 2023-02-17T18:09:52Z

@Jericho , can you rebase this to get it merged?

…en instatiated

Jericho · 2023-02-17T18:33:24Z

Done: marked the conversation as resolved and rebased.

…API.

dreddy-work · 2023-02-17T19:04:15Z

src\System.Windows.Forms\src\System\Windows\Forms\BindingsCollection.cs(67,52): error RS0036: (NETCORE_ENGINEERING_TELEMETRY=Build) Symbol 'CollectionChanged' is missing nullability annotations in the declared API.

Jericho requested a review from a team as a code owner December 11, 2022 16:36

ghost assigned Jericho Dec 11, 2022

Jericho mentioned this pull request Dec 11, 2022

Convert ArrayList usage to List<T> where possible #8140

Open

lonitra reviewed Dec 12, 2022

View reviewed changes

src/System.Windows.Forms/src/System/Windows/Forms/BindingsCollection.cs Outdated Show resolved Hide resolved

lonitra added the 📭 waiting-author-feedback The team requires more information from the author label Dec 12, 2022

ghost removed the 📭 waiting-author-feedback The team requires more information from the author label Dec 12, 2022

dreddy-work reviewed Dec 12, 2022

View reviewed changes

src/System.Windows.Forms/src/System/Windows/Forms/BindingsCollection.cs Outdated Show resolved Hide resolved

dreddy-work added the 📭 waiting-author-feedback The team requires more information from the author label Dec 13, 2022

ghost removed the 📭 waiting-author-feedback The team requires more information from the author label Dec 13, 2022

dreddy-work added the 📭 waiting-author-feedback The team requires more information from the author label Dec 13, 2022

ghost removed the 📭 waiting-author-feedback The team requires more information from the author label Dec 13, 2022

dreddy-work added the 📭 waiting-author-feedback The team requires more information from the author label Dec 13, 2022

hughbe reviewed Dec 15, 2022

View reviewed changes

ghost added the 💤 no-recent-activity label Dec 29, 2022

ghost removed the 💤 no-recent-activity label Jan 5, 2023

ghost added the 💤 no-recent-activity label Jan 20, 2023

dreddy-work removed the 💤 no-recent-activity label Jan 20, 2023

ghost added the 💤 no-recent-activity label Feb 3, 2023

ghost removed the 💤 no-recent-activity label Feb 3, 2023

ghost removed the 📭 waiting-author-feedback The team requires more information from the author label Feb 3, 2023

dreddy-work previously approved these changes Feb 17, 2023

View reviewed changes

Jericho added 4 commits February 17, 2023 13:12

Refactor BindingCollection to replace ArrayList with List<Binding>

9167c49

Fix AddCore and indexer to handle situation where list has not yet be…

516b5e7

…en instatiated

Enable nullability as requested by @lonitra

c41d133

Ensure the list is never null

3c109ca

Jericho dismissed dreddy-work’s stale review via 3c109ca February 17, 2023 18:32

Jericho force-pushed the GH-8140_BindingCollection branch from 8a7327d to 3c109ca Compare February 17, 2023 18:32

Fix RS0036 Symbol is missing nullability annotations in the declared …

25232fe

…API.

dreddy-work approved these changes Feb 17, 2023

View reviewed changes

dreddy-work merged commit 5b971a5 into dotnet:main Feb 17, 2023

ghost added this to the 8.0 Preview2 milestone Feb 17, 2023

Jericho deleted the GH-8140_BindingCollection branch February 17, 2023 20:13

ghost locked as resolved and limited conversation to collaborators Mar 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor BindingCollection to replace ArrayList with List<Binding> #8364

Refactor BindingCollection to replace ArrayList with List<Binding> #8364

Jericho commented Dec 11, 2022 •

edited by ghost

Loading

lonitra left a comment

hughbe Dec 15, 2022

dreddy-work Feb 16, 2023

ghost commented Dec 29, 2022

elachlan commented Jan 5, 2023

ghost commented Jan 20, 2023

ghost commented Feb 3, 2023

elachlan commented Feb 3, 2023

Jericho commented Feb 3, 2023

elachlan commented Feb 4, 2023

elachlan commented Feb 16, 2023

dreddy-work commented Feb 16, 2023

Jericho commented Feb 17, 2023

dreddy-work commented Feb 17, 2023

dreddy-work commented Feb 17, 2023

Jericho commented Feb 17, 2023

dreddy-work commented Feb 17, 2023

Refactor BindingCollection to replace ArrayList with List<Binding> #8364

Refactor BindingCollection to replace ArrayList with List<Binding> #8364

Conversation

Jericho commented Dec 11, 2022 • edited by ghost Loading

Microsoft Reviewers: Open in CodeFlow

lonitra left a comment

Choose a reason for hiding this comment

hughbe Dec 15, 2022

Choose a reason for hiding this comment

dreddy-work Feb 16, 2023

Choose a reason for hiding this comment

ghost commented Dec 29, 2022

elachlan commented Jan 5, 2023

ghost commented Jan 20, 2023

ghost commented Feb 3, 2023

elachlan commented Feb 3, 2023

Jericho commented Feb 3, 2023

elachlan commented Feb 4, 2023

elachlan commented Feb 16, 2023

dreddy-work commented Feb 16, 2023

Jericho commented Feb 17, 2023

dreddy-work commented Feb 17, 2023

dreddy-work commented Feb 17, 2023

Jericho commented Feb 17, 2023

dreddy-work commented Feb 17, 2023

Jericho commented Dec 11, 2022 •

edited by ghost

Loading