dotnet / winforms

Windows Forms is a .NET UI framework for building Windows desktop applications.
MIT License
4.29k stars 957 forks source link

Improve `ListViewSubItem.Text` performance #11401

Closed elachlan closed 1 month ago

elachlan commented 1 month ago

Fixes #10963

Change ListViewSubItem.Text to pass index to ListViewItem.UpdateSubItems to avoid loop.

Microsoft Reviewers: Open in CodeFlow
elachlan commented 1 month ago

@Olina-Zhang can your team please test this?

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 74.26543%. Comparing base (0aa3a4d) to head (e2da404).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #11401 +/- ## =================================================== + Coverage 74.26256% 74.26543% +0.00287% =================================================== Files 3025 3025 Lines 626861 626861 Branches 46742 46742 =================================================== + Hits 465523 465541 +18 + Misses 157993 157977 -16 + Partials 3345 3343 -2 ``` | [Flag](https://app.codecov.io/gh/dotnet/winforms/pull/11401/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dotnet) | Coverage Δ | | |---|---|---| | [Debug](https://app.codecov.io/gh/dotnet/winforms/pull/11401/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dotnet) | `74.26543% <100.00000%> (+0.00287%)` | :arrow_up: | | [integration](https://app.codecov.io/gh/dotnet/winforms/pull/11401/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dotnet) | `17.99452% <12.50000%> (-0.00141%)` | :arrow_down: | | [production](https://app.codecov.io/gh/dotnet/winforms/pull/11401/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dotnet) | `46.99221% <100.00000%> (+0.00630%)` | :arrow_up: | | [test](https://app.codecov.io/gh/dotnet/winforms/pull/11401/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dotnet) | `96.98684% <ø> (ø)` | | | [unit](https://app.codecov.io/gh/dotnet/winforms/pull/11401/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dotnet) | `43.94371% <100.00000%> (-0.02247%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dotnet#carryforward-flags-in-the-pull-request-comment) to find out more.
Olina-Zhang commented 1 month ago

Hi @elachlan, tested before and after this PR change, I didn't see any behavior difference when updating one ListViewSubItem's Text property, noticed entire row is blinking once(The recorded video doesn't seem to show it), is it the issue? Not sure if I understand the client's question correctly.

https://github.com/dotnet/winforms/assets/26474449/72f65fe2-7e5f-4a5e-97bf-d40396ffa6e6

elachlan commented 1 month ago

Hi @Olina-Zhang, That is fantastic. There shouldn't be any behavior changes.

The issue will arise if there is a large number of sub items (1000) it will be very slow to update. After the change it should be very quick.

Olina-Zhang commented 1 month ago

That's because my test was a bit small, I'll try a bigger one.

Olina-Zhang commented 1 month ago

If ListView has a large number of sub items (>1000), updating the text of ListViewSubItem is also quite fast with before and after your PR change, looks like no difference.

https://github.com/dotnet/winforms/assets/26474449/e21f1014-a2e0-4b6f-be9d-c13b2d72a044

John-Qiao commented 1 month ago

Verified this issue on 9.0.100-preview.5.24273.4 with dlls built from winforms repo of main branch, the result is same as above which Olina did.

https://github.com/dotnet/winforms/assets/45864985/1586d2ba-6809-4396-9d24-542d1097ac79

John-Qiao commented 1 month ago

Verified this issue with 9.0.100-preview.5.24304.3 test pass build, it was fixed. Test result is same as above.