search

dotnet / winforms

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

In DemoConsole app, all tagPages are empty without controls displayed #11095

Closed Olina-Zhang closed 7 months ago

Olina-Zhang commented 7 months ago

.NET version

.Net 9.0: Main branch of Winforms repo

Did it work in .NET Framework?

Not tested/verified

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

Yes, it is a regression issue, last week for Winforms repo testing, we can see controls in tabPages of DemoConsole test app

Issue description

In DemoConsole test app of Winforms repo, when running it, all tabPages are empty without controls displayed as following:

https://github.com/dotnet/winforms/assets/26474449/874eb25f-bc0e-4926-8a60-14e92744111b

Steps to reproduce

Test sample: DemoConsole in Winforms repo

LeafShi1 commented 7 months ago

This issue caused by the PR #11081， PR changed null! to String.Empty at https://github.com/dotnet/winforms/blob/32bc644e90207096fc0b5a39077df4a80f0a36ff/src/System.Windows.Forms.Design/src/System/ComponentModel/Design/DesignerHost.cs#L922, but name=empty was not judged when create site https://github.com/dotnet/winforms/blob/32bc644e90207096fc0b5a39077df4a80f0a36ff/src/System.Windows.Forms.Design/src/System/ComponentModel/Design/DesignerHost.cs#L369-L386.

JeremyKuhne commented 7 months ago

Thanks @LeafShi1, if you want to change the condition on line 386 to be null or empty that would be great. :)

Tanya-Solyanik commented 7 months ago

I think that the correct fix is to change nullability in Container.CreateSite method, but we don't own it. Null is a valid input and it implies that container (designer host) should try to create a new name for the component. Empty string means that component does not have a name, and we should not attempt to create one. I don't remember what scenario this is. This issue could be fixed by reverting the change that introdcued it.

JeremyKuhne commented 7 months ago

@Tanya-Solyanik What is the bad in creating a name for string.Empty? Can you provide any context on why not having a name is important?

Tanya-Solyanik commented 7 months ago

@Tanya-Solyanik What is the bad in creating a name for string.Empty?

@JeremyKuhne - potentially we will be attempting to create a name multiple times in the case when the naming service is not available. Not all components are named, some inner ones are not. This is a protected virtual method, we don't know what assumption the overriders make when they call the base method, but it's reasonable to assume that anyone who implements DesignerHost or Container assumes the same behavior as in the InProc designer. We don't have sufficient test base to validate all design time scenarios in this repo, we will not see immediate failures, but this change has a potential to prevent migration.

Can you provide any context on why not having a name is important?

No, I don't remember the specific scenario

MelonWang1 commented 7 months ago

Verified this issue on winforms repo from main branch, it was fixed, now all tabPages show the controls.

https://github.com/dotnet/winforms/assets/94418985/dec0b933-3d11-4cc0-aaa7-a7f374cd7c4b