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

Set Control.Defaultfont to SystemFonts.MessageBoxFont #656

Merged
merged 1 commit into from
Jun 3, 2019

Conversation

wjk
Copy link
Contributor

@wjk wjk commented Mar 30, 2019

This change ensures Segoe UI is used by default on Windows Vista and later, while still not breaking compatibility with Windows XP or systems compatible with it. Fixes #524.

Note that while I have not yet been able to test the code in System.Design.Editors as requested in #524 (comment) due to #632 not yet being merged, I have tested with the WinformsControlTest project, and all of its forms/controls (including the ThreadExceptionDialog) are sized properly to avoid clipping text, with no changes needed. While I do not think any changes will be needed to the designer code either, I am willing to hold on this PR until that can be tested. Thanks!

@wjk wjk requested a review from a team as a code owner March 30, 2019 22:21
@codecov
Copy link

codecov bot commented Mar 30, 2019

Codecov Report

Merging #656 into master will decrease coverage by 0.02931%.
The diff coverage is 100%.

@@                Coverage Diff                 @@
##              master       #656         +/-   ##
==================================================
- Coverage   34.00101%   33.9717%   -0.02931%     
==================================================
  Files           1103       1103                 
  Lines         320714     320714                 
  Branches       38944      38944                 
==================================================
- Hits          109046     108952         -94     
- Misses        207193     207295        +102     
+ Partials        4475       4467          -8
Flag Coverage Δ
#Debug 33.9717% <100%> (-0.02932%) ⬇️
#production 22.04788% <100%> (-0.03473%) ⬇️
#test 98.56731% <100%> (ø) ⬆️

@zsd4yr
Copy link
Contributor

zsd4yr commented Apr 1, 2019

Thanks @wjk

Due to the testing gap, let's hold our horses until #632 gets in

@merriemcgaw
Copy link
Member

@dreddy-work can you review this and see if this approach will work for WinForms?

@zsd4yr
Copy link
Contributor

zsd4yr commented Apr 15, 2019

@dreddy-work

@zsd4yr zsd4yr requested a review from dreddy-work April 15, 2019 17:31
@dreddy-work
Copy link
Member

@wjk, In your testing, did you check caption and menbar fonts as well?

DefaultFont was using 'GetStockObject' with 'DEFAULT_GUI_FONT' and its been suggested to not use that as it may not work for all windows themes. This makes me think if we should make the change in 'DefaultFont' directly.

@wjk
Copy link
Contributor Author

wjk commented Apr 15, 2019

DefaultFont is part of System.Drawing, which is in corefx, not here. That's one reason why I made the change the way I did.

I have not tested caption and menubar fonts, but those should be Segoe UI already. I know that MenuStrips default to Segoe UI, even in a form that uses MS Sans Serif, because it sets its font to the menubar font.

@dreddy-work
Copy link
Member

@wjk i have seen your comment on other issue. We can request a PR on corefx but please wait until we hear from Windows team on this. We will update this thread soon to close on this.

@zsd4yr
Copy link
Contributor

zsd4yr commented May 10, 2019

@dreddy-work any word from Windows?

@zsd4yr
Copy link
Contributor

zsd4yr commented May 15, 2019

pinging @dreddy-work

@dreddy-work
Copy link
Member

dreddy-work commented May 15, 2019

We have not received any update from windows team on this. Change here is simple. Concern is, will there be any scenario where the text will be truncated on some controls because of Font/FontSize change? should this be controlled with opt-in or opt-out somehow? or, we should be okay to break those scenarios? I am in favor of taking this as is. @Tanya-Solyanik as FYI.

@merriemcgaw
Copy link
Member

@wjk We think this looks good to go. Can you resolve conflicts so I can merge?

@RussKie
Copy link
Member

RussKie commented May 30, 2019

Please squash and rebase on top of the latest master.
Thank you

@wjk wjk force-pushed the SegoeUI branch 3 times, most recently from 4afaef7 to 7abeb7f Compare May 30, 2019 18:18
@wjk
Copy link
Contributor Author

wjk commented May 30, 2019

@RussKie @merriemcgaw What should be done about that test failure? The issue is that the font set by default is now identical to the font being set in the test in at least one (but quite possibly all) of the test cases. I could change this test to use e.g. Consolas or hard-coded MS Sans Serif (or some other font installed as part of Windows) instead. Does that sound good?

@RussKie
Copy link
Member

RussKie commented May 31, 2019

How about this?

diff --git a/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/ComboBoxTests.cs b/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/ComboBoxTests.cs
index 74cea883..254dc4f1 100644
--- a/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/ComboBoxTests.cs
+++ b/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/ComboBoxTests.cs
@@ -377,3 +377,3 @@ namespace System.Windows.Forms.Tests
             // Set different.
-            Font font1 = SystemFonts.MenuFont;
+            Font font1 = new Font("Arial", 8.25f);
             control.Font = font1;

@wjk
Copy link
Contributor Author

wjk commented May 31, 2019

@RussKie I've pushed your suggested change to the PR branch. Unfortunately, when locally it doesn't seem to have any effect. I would appreciate some pointers. (I'll squash the branch again once the tests are passing.) Thanks!

@RussKie
Copy link
Member

RussKie commented Jun 1, 2019

For your branch with the change I suggested all tests pass, if I run them locally from the cli (.\build -test) or run the tests in a vscode.
VS gives all sorts of red herrings, but not for the tests that fail on the build server.
If I rebase your changes on top of the latest master (65a813c) the tests are succeeding as well...

Looking at the build server results, the failure is caused by unrelated tests.
@AdamYoblick any thoughts?

@RussKie RussKie closed this Jun 3, 2019
@RussKie RussKie reopened this Jun 3, 2019
@RussKie
Copy link
Member

RussKie commented Jun 3, 2019

You have missed one more instance where the font needed changing.
Please squash-rebase and we'll get it merged.
Thanks

This change ensures Segoe UI is used by default on Windows Vista
and later, while still not breaking compatibility with Windows XP
or systems compatible with it.
@AdamYoblick
Copy link
Member

Yep looks like the build passed. Must have been some flaky tests. As more tests get added, this seems to be happening more often. Probably a indication that some of our code is non-deterministic.

@RussKie RussKie merged commit 0508793 into dotnet:master Jun 3, 2019
@RussKie
Copy link
Member

RussKie commented Jun 3, 2019

Thank you

@199621616
Copy link

我的迁移过程,遇到了这个痛苦的抉择。是迁移到.NetCore,还是停止迁移?
#2503
可以自定义默认字体嘛?

@RussKie
Copy link
Member

RussKie commented Dec 16, 2019

@199621616 I have already asked you to write messages in English. Further non-English messages will be deleted.

RussKie added a commit to RussKie/winforms that referenced this pull request Sep 18, 2020
Whenever ambient font changed, this change would stomp over
user-defined DataGridView font styles, such as
* DefaultCellStyle
* ColumnHeadersDefaultCellStyle
* RowHeadersDefaultCellStyle

Whilst the behaviour goes all the way back to .NET Framework, it becomes
a significant migration issue for projects that want to keep the original
default font (that was changed in dotnet#656).

Prevent changes to either of the above cellstyles, if those configured by
a user.

Resolves dotnet#3033
RussKie added a commit that referenced this pull request Sep 21, 2020
Whenever ambient font changed, this change would stomp over
user-defined DataGridView font styles, such as
* DefaultCellStyle
* ColumnHeadersDefaultCellStyle
* RowHeadersDefaultCellStyle

Whilst the behaviour goes all the way back to .NET Framework, it becomes
a significant migration issue for projects that want to keep the original
default font (that was changed in #656).

Prevent changes to either of the above cellstyles, if those configured by
a user.

Resolves #3033
RussKie added a commit to RussKie/winforms that referenced this pull request May 18, 2021
The default font has been updated in .NET Core 3.0 (dotnet#656) and documented.
However for some users who built their apps in pixel-perfect manner this
change has proved to be a significant hurdle in migrating their apps to
.NET.

Allow setting application-wide default font in a similar manner we set
high dpi or visual styles:

    Application.SetDefaultFont(new Font(new FontFamily("Calibri"), 11f));

* The application-wide default font can only be set before the first
window is created by an application.
* The font will be scaled by the system text scale factor, whenever it is
getting changed.

Resolves dotnet#3001
RussKie added a commit that referenced this pull request May 18, 2021
The default font has been updated in .NET Core 3.0 (#656) and documented.
However for some users who built their apps in pixel-perfect manner this
change has proved to be a significant hurdle in migrating their apps to
.NET.

Allow setting application-wide default font in a similar manner we set
high dpi or visual styles:

    Application.SetDefaultFont(new Font(new FontFamily("Calibri"), 11f));

* The application-wide default font can only be set before the first
window is created by an application.
* The font will be scaled by the system text scale factor, whenever it is
getting changed.

Resolves #3001
RussKie added a commit that referenced this pull request May 18, 2021
The default font has been updated in .NET Core 3.0 (#656) and documented.
However for some users who built their apps in pixel-perfect manner this
change has proved to be a significant hurdle in migrating their apps to
.NET.

Allow setting application-wide default font in a similar manner we set
high dpi or visual styles:

    Application.SetDefaultFont(new Font(new FontFamily("Calibri"), 11f));

* The application-wide default font can only be set before the first
window is created by an application.
* The font will be scaled by the system text scale factor, whenever it is
getting changed.

Resolves #3001
@ghost ghost locked as resolved and limited conversation to collaborators Feb 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
📖 documentation: breaking A change in behavior that could be breaking for applications. Needs to be documented.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No more Microsoft Sans Serif, return actual system font from SystemFonts.DefaultFont?
9 participants