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.

[merriemcgaw]

Unfortunately, we're in a situation right now where migration could well be blocked. If users of the ListView need to use the LargeIcon feature they have no way to do so - with no viable workaround and no real alternative. We also don't know if there are other ImageList scenarios that may also be broken. Our hunch is that there may well be. It's too bad we can't get it into the GA release, but we'll be getting something in for the first servicing instance.

The best course of action right now is to get our users into a known state - even if this state is broken, it's the state they've been in for the past 20 years. So we should revert the change that causes the problem and get a feature request filed in the repo ASAP to take a deeper look at scenarios around ImageLists. We'll be syncing internally with the comctl32.dll team to understand what they're doing and how we might be able to predictably work with their ownership model (if that's even possible). I'd rather take the time to do the feature right than to rush something through that has the potential to create bigger problems than it solves. As part of implementing the 6.0 feature we should certainly leverage the work done in #3601.

[weltkante]

with no viable workaround

I stated a workaround above, the image list property can be cleared and reassigned. However I aknowledge your choice of keeping the old bugs over trading them for new bugs.

even if this state is broken, it's the state they've been in for the past 20 years

It has been causing significant pain in your own test infrastructure though, so I imagine the cost of reverting is higher than you expect, but its your decision to make. You may also need to revert more than just that single PR, since later PRs added stricter error checking, which will now fail when you reintroduce the ownership handling bugs in the WinForms implementation.

Reverting probably needs to repriorize #3531 because it will be possible to trigger that again from applications, and also will make the unit tests flaky again (#3358), so you might not want to take your time and still look for an intermediate solution.

[RussKie]

If we'd discovered the issue while working on #3601, we wouldn't have taken it in the current shape. Unfortunately we're out of time to make additional explorations for .NET 5.0.
I'm liaising with the comctl team, and hopefully we may be able to better understand what's going on.

I stated a workaround above, the image list property can be cleared and reassigned.

I suppose we can explore reassigning the native imagelist every time the managed one changes. Again, it isn't something we would do for .NET 5.0.

[RussKie]

@jcddcjjcd as a workaround until we implement a fix you need unbind and rebind the imagelist after it changed, e.g. the following makes your sample work again:

            mfref.SafeInvoke(delegate
            {
                lvwImages.EndUpdate();
                lvwImages.ResumeLayout();

                TimeSpan ts = DateTime.Now.Subtract(StartTime);
                string elapsedTime = String.Format("{0:00}h {1:00}m {2:00}s.{3:000}ms",
                        ts.Hours, ts.Minutes, ts.Seconds,
                        ts.Milliseconds);
                //Debug.WriteLine(elapsedTime, "RunTime");

+               lvwImages.LargeImageList = null;
+               lvwImages.LargeImageList = largeThumbnailList;

                lvwImages.Update();

                Cursor = Cursors.Default;
            }, false);

[jcddcjjcd]

RussKie.
I applied your suggestion to my full app and it work nicely. Easier than OwnerDraw.
Thanks.

[harborsiem]

The bug occours also the SmallImageList.
@RussKie
Your suggestion helps also for the SmallImageList.

[RussKie]

The issue is with the ImageList, so all imagelists are affected.

[jcddcjjcd]

RussKie.
I have discovered another issue with ListView that is probably related and may well be solved by your current PR.
In the code I sent you in DisplayThumnails() the line:

lvwImages.Items.AddRange(listOfItems.ToArray());

is intermittently crashing with large selections of 1500 and larger mainly and I think it is related to current bug in ListView.

Regards, John.

[weltkante]

I don't think this is related or will be fixed by the PR, the bug was caused by not updating the image list after initial creation, "not updating" something shouldn't be crashing.

You may just be exhausting some memory limits by adding a huge amount of images? For fixing the ImageList ownership issues the current implementation copies ImageLists which does increase the memory usage in .NET Core (which I noted in the PR #3601), so code that runs close to the limits on Desktop Framework may start to fail on .NET Core due to running out of memory.

That may not be solvable unless someone takes the time to figure out how the native controls work and can implement an optimized version of the ImageList handling. In either case its probably a separate issue, if you have a repro that works on Desktop and fails on Core it would probably be appreciated if you open an issue to keep track of the problem.

[docfresh]

Similar (same?) issue reported on Feedback hub here , but affecting Listview in Details mode

[Zheng-Li01]

Verified the issue with .NET 6.0.100-alpha.1.20573.25 and .NET 5.0.200-preview.20573.23, the image can be load successfully for now. see below GIF.

TestListView.Core.zip

[Zheng-Li01]

Verified this issue in .Net SDK 5.0.101 build, this issue is fixed.