dotnet / winforms

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

TextFormatFlags.SkipAssertFlag provide bad ergonomics for consuming Winforms #7237

Closed kant2002 closed 11 months ago

kant2002 commented 2 years ago

.NET version

.NET 7

Did it work in .NET Framework?

No

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

Yes. I think that's working in .NET 5, maybe even in .NET 6

Issue description

When consume debug builds of WinForms with custom components, if using TextRenderer.DrawText on modified Graphics application spill asserts. I have to either pollute external components with knowledge about TextFormatFlags.SkipAssertFlag by adding | (TextFormatFlags)0x4000_0000 or build WinForms in release. I prefer to be able use WinForms in Debug mode for external apps.

Steps to reproduce

git clone https://github.com/kant2002/winforms-modernui
git switch kant/winforms

Inside Directory.Build.props set WinFormsRepoRoot to path to WinForms repo with debug build.

And then run MetroFramework.Demo project.

RussKie commented 2 years ago

Could you please explain what your expectations are here?

The asserts help us to ensure the expected device context state for the operations performed by the SDK: https://github.com/dotnet/winforms/blob/3a32c07bff581885526d941d910a6243b99aa63d/src/System.Windows.Forms/src/System/Windows/Forms/TextRenderer.cs#L19-L24

If we remove the flag we will degrade the SDK devex, and if we remove the asserts will re-introduce risks of regressions (e.g., https://github.com/dotnet/winforms/issues/4593).

If you choose to run a debug build you accept various risks, such as perf degradations (we had such complaints in the past too) or tripping on various asserts. And if your code running against a debug build trips on an assert it could be an indicator of a potential problem with your code.

kant2002 commented 2 years ago

I do understand that this is delicate topic, so I do not think that this Assert should be lifted. Maybe you found legitimate perf bug.

This is one of the places where issues appears

https://github.com/kant2002/winforms-modernui/blob/6bf025aa83d213707756a79c3d67154057d1362a/MetroFramework/Forms/MetroForm.cs#L231-L249

For me I cannot understand how this can be improved, to not trigger the performance consideration from WinForms.

RussKie commented 2 years ago

Yeah, I'm not sure what other options you have here except adding the SkipAssertFlag if you're running against a debug build.

kant2002 commented 2 years ago

Can I say, that Assert assume that best way to use API is to always pass additional flags TextFormatFlags.PreserveGraphicsClipping | TextFormatFlags.PreserveGraphicsTranslateTransform

RussKie commented 2 years ago

It depends on how the DC is used and the assumptons made by authors. But I guess the general guidance could be to use TextFormatFlags.PreserveGraphicsClipping | TextFormatFlags.PreserveGraphicsTranslateTransform if the was any clipping or transformations applied to the DC.

Adding @JeremyKuhne for more inputs.

JeremyKuhne commented 2 years ago

You'd only set the flags if you actually want them applied. If you don't, then the only option is to add the debug flag if you're using a debug build. As @RussKie points out, this assert is important for us to catch regressions.

elachlan commented 11 months ago

@kant2002 do the responses resolve your issue? Is it okay to close this?