Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix 11214 by adding a default font check before executing PerformAutoScale #11641

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

LeafShi1
Copy link
Member

@LeafShi1 LeafShi1 commented Jul 9, 2024

Fixes #11214

Proposed changes

-Add a check on the default font setting in the OnFontChanged function of ContainerControls.cs to ensure that PerformAutoScale is executed smoothly

Customer Impact

  • When the font for the entire application was adjusted, the controls on the form can be scaled appropriately

Risk

  • Minimal

Screenshots

Before

When I adjust the font for the entire application, the controls on the form don't scale appropriately
image

After

The controls on the form can be scaled appropriately after adjust the font for the entire application
image

Test methodology

  • Manually

Test environment(s)

  • .net 9.0.0-preview.7.24355.8
Microsoft Reviewers: Open in CodeFlow

@LeafShi1 LeafShi1 requested a review from a team as a code owner July 9, 2024 09:14
Copy link

codecov bot commented Jul 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.06831%. Comparing base (10f120d) to head (8af286b).
Report is 235 commits behind head on main.

Additional details and impacted files
@@                 Coverage Diff                 @@
##                main      #11641         +/-   ##
===================================================
+ Coverage   74.51776%   75.06831%   +0.55055%     
===================================================
  Files           3040        3059         +19     
  Lines         629560      632003       +2443     
  Branches       46839       46782         -57     
===================================================
+ Hits          469134      474434       +5300     
+ Misses        157058      154190       -2868     
- Partials        3368        3379         +11     
Flag Coverage Δ
Debug 75.06831% <100.00000%> (+0.55055%) ⬆️
integration 17.93212% <100.00000%> (+0.04159%) ⬆️
production 48.22199% <100.00000%> (+0.80597%) ⬆️
test 97.01631% <ø> (+0.06416%) ⬆️
unit 45.25976% <100.00000%> (+0.71726%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@LeafShi1 LeafShi1 added the waiting-review This item is waiting on review by one or more members of team label Jul 10, 2024
@Tanya-Solyanik Tanya-Solyanik changed the title Fix 11214 by adding font judgement before executing PerformAutoScale Fix 11214 by adding a default font check before executing PerformAutoScale Jul 10, 2024
@Tanya-Solyanik
Copy link
Member

I'm not sure that this fix addresses all possible cases. Fundamentally, we are missing information about what font/DPI the controls are scaled for right now. This fix assumes that the application was designed for the DefaultFont and that the current sizes are still set to the "initial" sizes. The former assumption is reasonable, the latter is not necessarily true. Maybe the application had been resized a couple of times and the current sizes are not appropriate for the default font anymore. Previous layout design relied on the AutoScaling ratios to maintain the current state as related to the original designed sizes. We would need to either revive that logic or to implement an extention on it. I suggest we revisit this bug in NET10

@LeafShi1 LeafShi1 marked this pull request as draft July 15, 2024 01:47
@dotnet-policy-service dotnet-policy-service bot added the draft draft PR label Jul 15, 2024
@Tanya-Solyanik
Copy link
Member

@LeafShi1 - let's revisit this change in NET10

@Tanya-Solyanik Tanya-Solyanik removed the waiting-review This item is waiting on review by one or more members of team label Jul 18, 2024
@lonitra lonitra changed the base branch from main to feature/10.0 July 23, 2024 00:55
@lonitra lonitra changed the base branch from feature/10.0 to main August 15, 2024 00:59
@LeafShi1
Copy link
Member Author

I'm not sure that this fix addresses all possible cases. Fundamentally, we are missing information about what font/DPI the controls are scaled for right now. This fix assumes that the application was designed for the DefaultFont and that the current sizes are still set to the "initial" sizes. The former assumption is reasonable, the latter is not necessarily true. Maybe the application had been resized a couple of times and the current sizes are not appropriate for the default font anymore. Previous layout design relied on the AutoScaling ratios to maintain the current state as related to the original designed sizes. We would need to either revive that logic or to implement an extention on it. I suggest we revisit this bug in NET10

@Tanya-Solyanik According to customer feedback, this fix can solve their problem.
This fix assumes that the font is changed before the handle is created. Once the handle is created, the scaling will be executed according to the original logic.

I tried to change the font several times, setting three different fonts in Form.cs and Form.Designer.cs, and the scaling results were normal.

But there is a question, should we scale the font before the handle is created?

@Tanya-Solyanik
Copy link
Member

But there is a question, should we scale the font before the handle is created?

We don't have this information at the Handle creation time. In some cases the requested size will be already scaled by the user app and in some it would be the default size. We have Scaling required properties for control sizes, but not for fonts.

@LeafShi1
Copy link
Member Author

But there is a question, should we scale the font before the handle is created?

We don't have this information at the Handle creation time. In some cases the requested size will be already scaled by the user app and in some it would be the default size. We have Scaling required properties for control sizes, but not for fonts.

Whether the form/control automatically scales should not be linked to the handle

I did the following experiment

  • When I set the font for the Form in winform.designer.cs, the form will scale according to the set font size. At this time, the handle of the form has not been created

The call stack it executes is as follows (Run OnFontChanged first and then run PerformAutoScale):

******OnFontChanged*****
   at System.Windows.Forms.ContainerControl.OnFontChanged(EventArgs e)
   at System.Windows.Forms.Form.OnFontChanged(EventArgs e)
   at System.Windows.Forms.Control.set_Font(Font value)
   at ScratchProject.Form1.InitializeComponent()
   at ScratchProject.Form1..ctor()
   at ScratchProject.Program.Main()
*******PerformAutoScale******
   at System.Windows.Forms.ContainerControl.PerformAutoScale(Boolean includedBounds, Boolean excludedBounds, Boolean causedByFontChanged)
   at System.Windows.Forms.ContainerControl.PerformNeededAutoScaleOnLayout()
   at System.Windows.Forms.ContainerControl.OnLayoutResuming(Boolean performLayout)
   at System.Windows.Forms.Control.ResumeLayout(Boolean performLayout)
   at ScratchProject.Form1.InitializeComponent()
   at ScratchProject.Form1..ctor()
   at ScratchProject.Program.Main()
  • When I set the font after InitializeComponent(), the page layout has been determined. At this time, the judgment (IsHandle created) is added, resulting in only the font being changed, and the Form is not scaled

The call stack it executes is as follows (Run PerformAutoScale first and then OnFontChanged):

******PerformAutoScale*******
   at System.Windows.Forms.ContainerControl.PerformAutoScale(Boolean includedBounds, Boolean excludedBounds, Boolean causedByFontChanged)
   at System.Windows.Forms.ContainerControl.PerformNeededAutoScaleOnLayout()
   at System.Windows.Forms.ContainerControl.OnLayoutResuming(Boolean performLayout)
   at System.Windows.Forms.Control.ResumeLayout(Boolean performLayout)
   at ScratchProject.Form1.InitializeComponent()
   at ScratchProject.Form1..ctor()
******OnFontChanged*****
   at System.Windows.Forms.ContainerControl.OnFontChanged(EventArgs e)
   at System.Windows.Forms.Form.OnFontChanged(EventArgs e)
   at System.Windows.Forms.Control.set_Font(Font value)
   at ScratchProject.Form1..ctor()
   at ScratchProject.Program.Main()
   at ScratchProject.Program.Main()

So whether the form automatically scales should not be linked to the handle.
It is indeed inappropriate to add the Font != DefaultFont judgment. As you said, the font may have been changed many times, so delete IsHandleCreated here should be the right solution

What do you think?

@Tanya-Solyanik
Copy link
Member

Whether the form/control automatically scales should not be linked to the handle

I'm concerned about scaling for the DPI. At the runtime, the form should be scaled according to the DPI of the screen it's created on from the DPI it was designed on. I.e. if the design time DPI scaling is 100%, but the runtime DPI scaling is 200%, form size should be doubled. If the Form uses AutoScaleMode.Dpi, then font should be rescaled. However, we don't always know what on what screen the control is displayed and we might have to scale font after handle is created, when we know the current screen DPI. And we don't know if the default font had been scaled. It is more efficient to scale the default font and have it applied to all controls in the application than to scale font on each container control.

Jeremy is planning to rework the default font handling. So fix for this issue will depend on the new infrastructure that he will put in. Could you please wait with this this change until that work is done?

@LeafShi1
Copy link
Member Author

Jeremy is planning to rework the default font handling. So fix for this issue will depend on the new infrastructure that he will put in. Could you please wait with this change until that work is done?

OK, I will wait for the new infrastructure

@LeafShi1 LeafShi1 closed this Sep 4, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Oct 6, 2024
@JeremyKuhne JeremyKuhne reopened this Jan 6, 2025
@JeremyKuhne
Copy link
Member

Redoing layout is not something we're going to get to for a while. We should look at a targeted fix for .NET 10 here, even if it is behind a compat switch.

@LeafShi1
Copy link
Member Author

LeafShi1 commented Jan 7, 2025

Redoing layout is not something we're going to get to for a while. We should look at a targeted fix for .NET 10 here, even if it is behind a compat switch.

Do you mean add an AppContext switch for it?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
draft draft PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Font autoscaling doesn't function in .NET Core as it does in .NET Framework 4.8.
3 participants