Fix ListView no longer displays images#4184
Conversation
weltkante
left a comment
There was a problem hiding this comment.
I don't think this fix is safe, ownership is transferred to the native control, the native image list can be deleted at arbitrary points in time.
There was a problem hiding this comment.
@RussKie Is this a clean rollback of the original workaround-PR now, or is there something to review? I'll look which issues need to be reopened (or followed up) for a rollback and link them.
I was apparently misremembering some designer issue being resolved by this PR, or I can't find it. The other issue was #3531 and is still open. It will need to be adressed, or you have to remove that usage pattern from all your tests, otherwise tests go back to flaky upon reverting. (I think they'll still be flaky anyways from other mishandled ownership scenarios, but it might be worth trying to see if avoiding that particular usage pattern can significantly reduce the amount of flakyness.)
Also test failures here may be related to assertions introduced in separate PRs, you'll have to remove assertions that verify no double-free happens on the ImageList.
|
Yes, there have been a few things done that reduced the amount of issues we have with the |
|
@weltkante I think I may have finally found a reasonable fix, thanks to your suggestions. We have a plumbing to alert the |
Codecov Report
@@ Coverage Diff @@
## master #4184 +/- ##
===================================================
- Coverage 97.97005% 97.96233% -0.00773%
===================================================
Files 497 497
Lines 258972 258972
Branches 4566 4566
===================================================
- Hits 253715 253695 -20
- Misses 4457 4473 +16
- Partials 800 804 +4
Flags with carried forward coverage won't be shown. Click here to find out more. |
This comment has been minimized.
This comment has been minimized.
|
Those failures are probably just #3898 again. could't reproduce those yet so not sure what they are. |
|
Yes, they feel very random. Subsequent builds succeeded. |
Changes in `ImageList` ownership model in dotnet#3601 means that very are now two instances of imagelists - one instance is tracked by Windows Forms (i.e. managed) side, and another one tracked by the underlying Win32 (unmanaged) side. This was done due to an observed ownership disconnect between the managed and unmanged code, that led to situations where the unmanaged code would effectively destroy and instance of an imagelist, which the managed code was oblivious to. However with the above change changes to images in an imagelist on the managed side, i.e. a user adding or replacing an image in the imagelist, would not be reflected in the imagelist on the unmanaged side, and thus would not be reflected in the UI (which is drawn by the Win32). The fix reuses the established infrastructure that notifies the managed imagelist implementation of changes to the images collection, and once a notification of a chance is received, the unmanaged imagelist is re-created, thus ensuring the UI has all the correct images to display. Fixes dotnet#4169
Revert "Explicit ImageList ownership management. (dotnet#3601)" This reverts commit 03db3fb. Revert "Fix `ListView` no longer displays images (dotnet#4184)" This reverts commit d0608e7. We have observed an instability of tests under stress (and reasonably high degree of concurrency) presumably caused by ImageList lifetime handling (notably dotnet#3358). The changes introduced in dotnet#3601 have helped with tests stability, however resulted in a number of regressions, such as dotnet#4169 and dotnet#4275. Restore the original implementation given it worked reasonably well for the past so many years.
Revert "Explicit ImageList ownership management. (#3601)" This reverts commit 03db3fb. Revert "Fix `ListView` no longer displays images (#4184)" This reverts commit d0608e7. We have observed an instability of tests under stress (and reasonably high degree of concurrency) presumably caused by ImageList lifetime handling (notably #3358). The changes introduced in #3601 have helped with tests stability, however resulted in a number of regressions, such as #4169 and #4275. Restore the original implementation given it worked reasonably well for the past so many years.
Revert "Explicit ImageList ownership management. (dotnet#3601)" This reverts commit 03db3fb. Revert "Fix `ListView` no longer displays images (dotnet#4184)" This reverts commit d0608e7. We have observed an instability of tests under stress (and reasonably high degree of concurrency) presumably caused by ImageList lifetime handling (notably dotnet#3358). The changes introduced in dotnet#3601 have helped with tests stability, however resulted in a number of regressions, such as dotnet#4169 and dotnet#4275. Restore the original implementation given it worked reasonably well for the past so many years. (cherry picked from commit b0ee5da)
Revert "Explicit ImageList ownership management. (#3601)" This reverts commit 03db3fb. Revert "Fix `ListView` no longer displays images (#4184)" This reverts commit d0608e7. We have observed an instability of tests under stress (and reasonably high degree of concurrency) presumably caused by ImageList lifetime handling (notably #3358). The changes introduced in #3601 have helped with tests stability, however resulted in a number of regressions, such as #4169 and #4275. Restore the original implementation given it worked reasonably well for the past so many years. (cherry picked from commit b0ee5da)
Fixes #4169
Proposed changes
Changes in
ImageListownership model in #3601 means that very are now two instances of imagelists - one instance is tracked by Windows Forms (i.e. managed) side, and another one tracked by the underlying Win32 (unmanaged) side. This was done due to an observed ownership disconnect between the managed and unmanaged code, that led to situations where the unmanaged code would effectively destroy and instance of an imagelist, which the managed code was oblivious to.However with the above change changes to images in an imagelist on the managed side, i.e. a user adding or replacing an image in the imagelist, would not be reflected in the imagelist on the unmanaged side, and thus would not be reflected in the UI (which is drawn by the Win32).
The fix reuses the established infrastructure that notifies the managed imagelist implementation of changes to the images collection, and once a notification of a chance is received, the unmanaged imagelist is re-created, thus ensuring the UI has all the correct images to display.
Customer Impact
ImageListthat gets its images dynamically modified will render the images correctly.Regression?
Risk