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 convenience / perf methods to ListViewItem/ControlCollection #10944

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JeremyKuhne
Copy link
Member

@JeremyKuhne JeremyKuhne commented Feb 23, 2024

ListViewItemCollection doesn't have an AddRange that takes an IEnumerable<ListViewItem>, which necessitates unnecessary array allocations when adding from a collection.

It also doesn't implement a generic IList<T> which makes typed usage and LINQ usage difficult. This adds that.

Also add IEnumerable<Control> to ControlCollection to address the LINQ scenario. Control has IList, but indexed setting throws. I don't want to add IList<Control> until we've had time to evaluate the implications of implicitly doing the public steps with the collection that replicate what those setters should be doing.

Also tweak ArrangedElementCollection so the static empty collection can't be written to.

This doesn't fix the ambiguity of AddRange([..]) with ListViewItemCollection, but it does avoid the need for it in some cases.

Microsoft Reviewers: Open in CodeFlow

lonitra
lonitra previously approved these changes Feb 23, 2024
Copy link
Member

@lonitra lonitra left a comment

Choose a reason for hiding this comment

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

🚀

Copy link

codecov bot commented Feb 23, 2024

Codecov Report

Attention: Patch coverage is 54.54545% with 30 lines in your changes missing coverage. Please review.

Project coverage is 75.09169%. Comparing base (1e92b29) to head (2bad759).
Report is 251 commits behind head on feature/10.0.

Additional details and impacted files
@@                  Coverage Diff                   @@
##           feature/10.0      #10944         +/-   ##
======================================================
+ Coverage      74.22517%   75.09169%   +0.86651%     
======================================================
  Files              3020        3119         +99     
  Lines            626328      656865      +30537     
  Branches          46693       51738       +5045     
======================================================
+ Hits             464893      493251      +28358     
- Misses           158039      160207       +2168     
- Partials           3396        3407         +11     
Flag Coverage Δ
Debug 75.09169% <54.54545%> (+0.86651%) ⬆️
integration 19.27194% <23.07692%> (+1.00504%) ⬆️
production 48.95102% <53.84615%> (+2.07397%) ⬆️
test 97.11269% <100.00000%> (+0.07563%) ⬆️
unit 45.94874% <53.84615%> (+2.11686%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

…numerable<ListViewItem>`, which necessitates unnecessary array allocations when adding from a collection.

It also doesn't implement a generic `IList<T>` which makes typed usage and LINQ usage difficult. This adds that.

Also add `IEnumerable<Control>` to `ControlCollection` to address the LINQ scenario. Control has `IList`, but indexed setting throws. I don't want to add `IList<Control>` until we've had time to evaluate the implications of implicitly doing the public steps with the collection that replicate what those setters should be doing.

Also tweak `ArrangedElementCollection` so the static empty collection can't be written to.

This doesn't fix the ambiguity of `AddRange([..])` with `ListViewItemCollection`, but it does avoid the need for it in some cases.
@JeremyKuhne JeremyKuhne force-pushed the listviewitemcollection branch from a3ca32b to 2bad759 Compare May 3, 2024 17:13
@JeremyKuhne JeremyKuhne enabled auto-merge (squash) May 3, 2024 17:34
@JeremyKuhne JeremyKuhne disabled auto-merge May 3, 2024 17:51
lonitra
lonitra previously approved these changes May 3, 2024
@elachlan
Copy link
Contributor

@JeremyKuhne was there anything blocking merging this?

@JeremyKuhne
Copy link
Member Author

@JeremyKuhne was there anything blocking merging this?

Ran out of bandwidth to figure out the best way to implement improvements here that are consistent across our internal collections. I hope to come up with a proposal that API review will be happy with...

@lonitra lonitra changed the base branch from main to feature/10.0 July 23, 2024 00:58
@lonitra lonitra changed the base branch from feature/10.0 to main August 15, 2024 01:00
@lonitra lonitra dismissed their stale review August 15, 2024 01:00

The base branch was changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants