dotnet / winforms

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

Breaking change in SuspendLayout + AutoScaleDimensions interplay vs .NET Framework #2986

Closed mvtec-bergdolll closed 4 years ago

mvtec-bergdolll commented 4 years ago

Problem description:

Porting our .NET Frameworks based library to .NET Core, we noticed a breaking change resulting in incorrectly scaled components. Reducing it down to a POC, showed the root of the problem seems to be the interplay between SuspendLayout and AutoScaleDimensions. The relevant code looks like this:

private void InitializeComponent()
{
    System.Drawing.Size ExpectedSize = new System.Drawing.Size(538, 558);

    this.SuspendLayout();
    this.components = new System.ComponentModel.Container();
    this.AutoScaleDimensions = new System.Drawing.SizeF(6F, 13F);
    this.AutoScaleMode = System.Windows.Forms.AutoScaleMode.Font;
    this.ClientSize = ExpectedSize;
    this.Text = "Form1";
    this.ResumeLayout(false);
    this.PerformLayout();

    System.Diagnostics.Debug.Assert(this.ClientSize == ExpectedSize);
}

Running the same code in .NET Frameworks 4.7.2 passes the assert, under .NET Core 3.1.102 the assert fails. This is a clear breaking change I couldn't find any documentation for. Is this intentional? Note, that taking out either SuspendLayout or AutoScaleDimensions makes the assert pass again.

Expected behavior:

Same behavior as .NET Framework or documented breaking change with porting advice.

Minimal repro:

Find the full .NET Core POC source here: https://github.com/mvtec-bergdolll/dotnet-core-layout-bug-poc

weltkante commented 4 years ago

Probably a side effect of changing the default font, which was an intentional breaking change documented e.g. here.

Your AutoScaleDimensions might not match the changed font so you get size adjustments. To verify you can explicitly set the Font in addition to AutoScaleDimensions and see if it behaves as it did before. (Use the Desktop Framework font because thats what the AutoScaleDimensions probably were coming from.)

mvtec-bergdolll commented 4 years ago

I'd find that surprising, given the assert passes when this.SuspendLayout(); is not called.

mvtec-bergdolll commented 4 years ago

Wow, you're right adding this.Font = new System.Drawing.Font("Microsoft Sans Serif", 8); fixes the issue, I still don't properly understand it though. Note I'm not well versed in windows forms, reading https://docs.microsoft.com/en-us/dotnet/api/system.windows.forms.control.suspendlayout?view=netframework-4.8 I fail to understand why the scaling behavior should be different after another PerformLayout().

weltkante commented 4 years ago

AutoScaleDimensions capture the font size as it was used in the WinForms Designer, if the font at runtime has a different size then a correction is applied. The documentation for this is on the AutoScaleDimensions property.

SuspendLayout suspends layout until you call PerformLayout so the whole block of designer instructions can be applied at once. If you are under layout suspension then this.ClientSize = ExpectedSize; will happen with the wrong AutoScaleDimension and will be corrected at the next possible time, which is after the call to PerformLayout. If you don't have layout suspension then the AutoScaleDimensions are corrected immediately and when setting the ClientSize there is nothing to correct anymore. (In other words, AutoScaleDimensions only works together with SuspendLayout.)

Basically you are introducing a bug if you remove SuspendLayout/PerformLayout from designer code, as the designer operates under the assumption that all statements are executed in sequence without layout occuring between individual statements.

That said AutoScaleMode=Font is an ancient technique and I don't think its recommended anymore, DPI scaling is working much better these days. Its just that most people never switched away from what had been the default for a decade.

If you have the chance you should be editing your Desktop Framework forms in the designer and switching to a different AutoScaleMode before porting to .NET Core. If thats not an option you may chose explicitly setting the Font to the old Desktop Framework Font to avoid porting issues.

(Note that these are my personal recommendations and I'm not part of the WinForms team or Microsoft, feel free to wait for an official response.)

mvtec-bergdolll commented 4 years ago

Thanks for the insightful answer! I'm wondering how we could help people avoid and or understand this imo unintuitive behavior. Maybe the SuspendLayout docs could mention that certain code like AutoScaleDimension assumes it is run during layout suspension, and add some cross references and mention that the behavior of 'batched' layout might not be the same as 'permanent' layout.

merriemcgaw commented 4 years ago

What if we make the default font in the Application Class as a write once property that you can only do when the Application is being created? Something like Application.SetDefaultFont(new System.Drawing.Font("Microsoft Sans Serif", 8)); This would allow devs to do the change you did above but only once across the entire application.

We don't want to go with a configuration option that would allow this to change without a recompile.

weltkante commented 4 years ago

@merriemcgaw yeah I think that had been suggested before and as far as I remember not been any major arguments against, its just not been implemented in time for 3.0 release and then nobody bothered anymore.

It certainly would help having to do less manual work when porting and allow adjustments to be made afterwards, as it is now you need to do any AutoScale adjustments in the Desktop Framework designer before porting or set the font in the form manually as a workaround if you want to do the designer adjustments after porting. Setting the font globally would allow you to easily delay the AutoScale adjustments to after porting and proceed immediately to see if there are other issues.

So in summary, I don't think its necessary to be able to set a global default font, but it would probably make porting work a bit easier because you have to do less manual work. So mostly convenience.

RussKie commented 4 years ago

It is hard to add anything else to what @weltkante already said (thanks mate 👍).

Unfortunately we can't change the AutoScaleMode defaults, else we may be breaking existing consumers (the number is quite significant). We can't make other changes (like changing AutoScaleMode, etc.) too sadly. Not at least until the Designer will have been shipped. Then we may be in a position to holistically re-evaluate the problem space.

I have raised a request for the new API to set application-wide font to help with migration of existing apps.