net5.0 System.Windows.Forms.ListView no longer displays LargeIcon

[jcddcjjcd]

Sample code below works with a ListView in dotnet 4.8 and netcore3.1 but not in netcore5.0

The form for code below is simply a button and a ListView displaying in LargeIcon mode:

The checkboxes and text labels do work, just the image missing.

ownerDraw does work using the same bitmap from LargeImageList.

TestListView Core.zip

[jcddcjjcd]

Thanks for looking at it RussKie, Regards, John.

[RussKie]

I've tested the repro against the official releases: https://dotnet.microsoft.com/download/dotnet/5.0. The issue doesn't repro against 5.0.100-preview.8.20417.9, but repros against 5.0.100-rc.1.20452.10.

[RussKie]

It appears the regressions is caused by 03db3fb (#3601).

Reverting the change restores the functionality:

diff --git a/src/System.Windows.Forms/src/System/Windows/Forms/ListView.cs b/src/System.Windows.Forms/src/System/Windows/Forms/ListView.cs
index 1fc7c4a22..5de24a63e 100644
--- a/src/System.Windows.Forms/src/System/Windows/Forms/ListView.cs
+++ b/src/System.Windows.Forms/src/System/Windows/Forms/ListView.cs
@@ -1257,12 +1257,7 @@ namespace System.Windows.Forms
                     return;
                 }
 
-                var previousHandle = User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.NORMAL, value is null ? IntPtr.Zero : value.CreateUniqueHandle());
-                if (previousHandle != IntPtr.Zero)
-                {
-                    var result = ComCtl32.ImageList.Destroy(previousHandle);
-                    Debug.Assert(result.IsTrue());
-                }
+                User32.SendMessageW(this, (User32.WM)LVM.SETIMAGELIST, (IntPtr)LVSIL.NORMAL, value is null ? IntPtr.Zero : value.Handle);
 
                 if (AutoArrange && !listViewState1[LISTVIEWSTATE1_disposingImageLists])
                 {

@weltkante what do you think?

I added a repro in https://github.com/RussKie/winforms/tree/fix_4169

[RussKie]

@weltkante what I think is now happening - we have a total disconnect between a managed imagelist, to which a user assigns images, and an unmanaged imagelist that we assign via User32.SendMessageW(this, LVM.SETIMAGELIST, LVSIL.NORMAL, value.CreateUniqueHandle());, and which doesn't know about images set on the managed side.

[weltkante]

we have a total disconnect between a managed imagelist, to which a user assigns images, and an unmanaged imagelist [...] which doesn't know about images set on the managed side.

That was the point of the PR, because the sharing of image lists was totally broken (obviously I missed the usecase in this issue, sorry for that). Copying the ImageList seemed the sane way since it avoids you having to figure out the ownership rules of the native controls (which are not entirely documented, and WinForms was getting them wrong all the time).

I guess to support adding images after assigning the ImageList there are two options:

• Create a subscriber model where the managed side knows which controls are using an ImageList and can update them, i.e. the sharing being implemented on the managed side. This can be entirely internal API for now, just for tracking the sharing of ImageLists. Of course if you want to make it public so third party controls can opt in to these change notifications then a public API can be designed (now or later).
• Someone needs to invest the time and figure out the image list ownership rules of the native controls (i.e. in which cases the native control will delete a shared image list, and how to work around it, e.g. by clearing the image list handle before destroying the native control)

Both are significant work, so the second option has the advantage of being more optimized (no copying of ImageLists), however can only be done by someone who is able to look at the native controls code. If you do not have the resources for that I can offer to make a draft PR for the subscriber model. Let me know what you think (or if you have any other ideas how to implement it properly). Until then the workaround is to clear/reassign the ImageList in user code when you change the images after the initial assignment.

PS: I recommend strongly against reverting and going back to the previous implementation, it was totally broken and not restricted to multithreaded scenarios. While the multithreading initated the investigation, afterwards we noticed several other bugs also getting fixed through this PR.