dotnet / winforms

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

PropertyGrid randomly loses toolbar items #9223

Open RussKie opened 1 year ago

RussKie commented 1 year ago

.NET version

Microsoft.WindowsDesktop.App 6.0.16

Did it work in .NET Framework?

Yes

Did it work in any of the earlier releases of .NET Core or .NET 5+?

No idea

Issue description

I have an app with a PropertyGrid control, which is customised with few extra buttons. image

I observed that the custom buttons randomly disappear. Here's the trace of the issue:

    DarcUI.dll!DarcUI.ExtendedPropertyGrid.ToolStripButton2.OnParentChanged(System.Windows.Forms.ToolStrip oldParent = {System.Windows.Forms.PropertyGridToolStrip}, System.Windows.Forms.ToolStrip newParent = null) Line 29   C#
>   System.Windows.Forms.dll!System.Windows.Forms.ToolStripItem.SetOwner(System.Windows.Forms.ToolStrip newOwner) Line 3910 C#
    System.Windows.Forms.dll!System.Windows.Forms.ToolStripItemCollection.OnAfterRemove(System.Windows.Forms.ToolStripItem item = {DarcUI.ExtendedPropertyGrid.ToolStripButton2}) Line 443  C#
    System.Windows.Forms.dll!System.Windows.Forms.ToolStripItemCollection.Clear() Line 225  C#
    System.Windows.Forms.dll!System.Windows.Forms.PropertyGrid.SetupToolbar(bool fullRebuild) Line 4302 C#
    System.Windows.Forms.dll!System.Windows.Forms.PropertyGrid.OnSystemColorsChanged(System.EventArgs e = {System.EventArgs}) Line 4508 C#
    System.Windows.Forms.dll!System.Windows.Forms.Control.OnSystemColorsChanged(System.EventArgs e = {System.EventArgs}) Line 11230 C#
    System.Windows.Forms.dll!System.Windows.Forms.Control.OnSystemColorsChanged(System.EventArgs e = {System.EventArgs}) Line 11230 C#
    System.Windows.Forms.dll!System.Windows.Forms.Control.OnSystemColorsChanged(System.EventArgs e = {System.EventArgs}) Line 11230 C#
    System.Windows.Forms.dll!System.Windows.Forms.Control.OnSystemColorsChanged(System.EventArgs e = {System.EventArgs}) Line 11230 C#
    System.Windows.Forms.dll!System.Windows.Forms.Control.UserPreferenceChanged(object sender, Microsoft.Win32.UserPreferenceChangedEventArgs pref) Line 13440  C#

The issue appears to be here: https://github.com/dotnet/winforms/blob/89fcc580afb06911254f00cfc6650b0496c033ad/src/System.Windows.Forms/src/System/Windows/Forms/PropertyGrid.cs#L3940-L3944

The buttonList is rebuilt from only pre-defined elements, completely ignoring any additional elements the toolstrip contained.

Steps to reproduce

I can craft a sample, if necessary.

merriemcgaw commented 1 year ago

Thanks @RussKie! @JeremyKuhne will take a look.

RussKie commented 1 year ago

I don't know whether it worked in .NET Framework - there is no option for that. I suspect that it never did, so it probably isn't a regression.

elachlan commented 1 year ago

@RussKie can you give a quick explanation of how you are adding the items to the toolstrip? I was trying to repo.

It looks like its caused by a fullRebuild which can happen when

fullRebuild does not keep the existing buttons because they might not be needed anymore because the associated PropertyTab might have been removed. So it really depends how you are adding the toolstripitems.

RussKie commented 1 year ago

Here's the sample: https://github.com/RussKie/DarcUI/blob/930a9c8caac48a185e6bb277aeb2b7bc9fbe5741/src/DarcUI.WinForms/CustomControls/ExtendedPropertyGrid.cs#L51-L58 https://github.com/RussKie/DarcUI/blob/930a9c8caac48a185e6bb277aeb2b7bc9fbe5741/src/DarcUI.WinForms/CustomControls/ExtendedPropertyGrid.cs#L125-L137 image

I observed the issue to occur whenever I have my little app running and I launch VS, which triggers OnSystemColorsChanged event.

merriemcgaw commented 1 year ago

I'm going to remove the regression tagging because it doesn't really look like one. We'll investigate, but we're getting late enough into .NET 8 that I can't promise a fix would make it. Really, .NET 9 is more realistic if it turns out to be a runtime issue and not something you can address on the application side.

elachlan commented 1 year ago

@merriemcgaw I am fairly sure it would be an API change. So .NET9 makes the most sense.