Update Form's Minimum and Maximum sizes with DPI_CHANGED event by dreddy-work · Pull Request #7467 · dotnet/winforms

dreddy-work · 2022-07-20T21:20:59Z

Description:

Currently, Form's MinimumSize and MaximumSize are not updated when a DPI_CHNAGED event received by the Form. This prevents scaling of Form to match with the Dpi when its minimum or maximum size properties are set. Users trying to work around this issue may see problems like #7251.

Creating this draft PR to assess the risk for .NET 7.0.

Change in behavior:

Application running in PermonitorV2 mode will see following behavioral change when application moved from one monitor to the other that have different Dpi/Scaling settings and Form has either Minimum and/or Maximum size property set.

Minimum and Maximum size of the Form will be changed and any runtime dependency on these might be impacted.
Raising MinimumSizeChnaged and/or MaximumSizeChnaged events that were not before.
Scaled Form size now have new constraint values for Minimum and Maximum sizes.

Before fix: ( moving from 100% to 300%)

After fix:

Microsoft Reviewers: Open in CodeFlow

fixes#7251
fixes#7535

RussKie

LGTM

… chnage.

dreddy-work · 2022-08-11T00:49:36Z

Tests succeed locally. Seems thread awareness is not being set on lab machines. Looking on how to confirm this theory. Review can continue knowing test failed with lab specific issue.

RussKie

Pass over the docs only. Will continue to the code later.

RussKie

The animation is great, but we'll need to redo it. I understand that it's showing two desktops, but it's too wide (a lot of our users still use HD monitors), and it contains information completely unrelated (i.e., the email).
The app could be tweaked to appear significantly closer to the monitor boundary, so that we can clip the view and dragging will require less distance.
It could be also great to add some test to the animation to indicate each monitor's settings.

dreddy-work · 2022-08-11T20:50:09Z

The animation is great, but we'll need to redo it. I understand that it's showing two desktops, but it's too wide (a lot of our users still use HD monitors), and it contains information completely unrelated (i.e., the email). The app could be tweaked to appear significantly closer to the monitor boundary, so that we can clip the view and dragging will require less distance. It could be also great to add some test to the animation to indicate each monitor's settings.

I tried but now screen separation isn't clear with latest one. I am sure people reading will understand and demo was just to show how it is picked up. Not planning on spending more time on this for now.

RussKie

Reviewed just the code-related changes, as this what we'll ship. We can iterate over the docs later.

LGTM with few minor changes.

The code is looking good

Tanya-Solyanik

Looks good.

Tanya-Solyanik

I added some more nits

Tanya-Solyanik · 2022-08-12T23:14:11Z

+ // Borrowed from https://github.com/dotnet/runtime/blob/main/src/libraries/Common/src/System/LocalAppContextSwitches.Common.cs
+ internal static partial class LocalAppContextSwitches
+ {
+ private const string ScaleTopLevelFormMinMaxSizeForDpiSwitchName = "System.Windows.Forms.ScaleTopLevelFormMinMaxSizeForDpi";


Make it internal for use in tests or access the private dynamically via the test helper.

Also going forward we might want to keep all these names in a special file, potentially in the common folder.

Agree on moving to a common file for these string const. Will make that change along with fixing skipped test. I enable auto-merge and it did merge when you approved.

Tanya-Solyanik · 2022-08-12T23:19:59Z

+ /// </summary>
+ /// <param name="xScaleFactor">The scale factor to be applied on width of the property being scaled.</param>
+ /// <param name="yScaleFactor">The scale factor to be applied on height of the property being scaled.</param>
+ /// <param name="updateContainerSize"><see langword="true"/> to resize of the container control along with properties being scaled; otherwise, <see langword="false"/>.</param>


nit: break this line into multiple lines

Tanya-Solyanik · 2022-08-12T23:23:43Z

+ form.Show();
+
+ // Explicitly opt-in to resize min and max sizes with Dpi changed event.
+ const string runtimeSwitchName = "System.Windows.Forms.ScaleTopLevelFormMinMaxSizeForDpi";


Use the const from the source code here

Tanya-Solyanik · 2022-08-12T23:27:20Z

+ Assert.NotEqual(form.MaximumSize, maxSize);
+
+ // Reset switch.
+ AppContext.SetSwitch(runtimeSwitchName, false);


If another tests that relies on this switch runs at the same time, you might have a problem. Perhaps these tests should be moved to the other test project, where they run sequentially.

RussKie · 2022-09-04T23:11:07Z

@dreddy-work a friendly reminder that this change must be documented, please raise a new "breaking change" issue at https://github.com/dotnet/docs.

RussKie · 2022-10-18T23:41:04Z

@dreddy-work has a breaking doc issue been raised?

dreddy-work · 2022-10-18T23:42:15Z

@dreddy-work has a breaking doc issue been raised?

Its linked above.

RussKie · 2022-10-18T23:44:00Z

Ah, right, thanks!

The doc: dotnet/docs#31020

dreddy-work added 2 commits July 1, 2022 11:19

Draft
6f722cf

Draft chnages for risk assesment.
1af2b94

ghost assigned dreddy-work Jul 20, 2022

ghost added the draft draft PR label Jul 21, 2022

RussKie reviewed Jul 21, 2022
View reviewed changes

dreddy-work added the on-hold label Jul 27, 2022

Introduce runtime-config settigns for opt-in or opt-out of a breaking…
4b07447
… chnage.

dreddy-work marked this pull request as ready for review August 10, 2022 21:23

dreddy-work requested a review from a team as a code owner August 10, 2022 21:23

dreddy-work removed draft draft PR on-hold labels Aug 10, 2022

dreddy-work added this to the 7.0 RC1 milestone Aug 10, 2022

Merge branch 'main' into dev/dreddy/fix7251
b296123

RussKie previously requested changes Aug 11, 2022
View reviewed changes

ghost added the waiting-author-feedback The team requires more information from the author label Aug 11, 2022

RussKie reviewed Aug 11, 2022
View reviewed changes

dreddy-work mentioned this pull request Aug 11, 2022
Form_DpiChanged_MinMaxSizeChanged_WithRuntimeSetting Fail in CI #7579
Open

ghost removed the waiting-author-feedback The team requires more information from the author label Aug 11, 2022

Address review feedback.
8117534

dreddy-work requested a review from RussKie August 11, 2022 20:14

RussKie reviewed Aug 12, 2022
View reviewed changes

Adress further feedback
96bfd93

dreddy-work requested a review from RussKie August 12, 2022 04:36

Tanya-Solyanik previously approved these changes Aug 12, 2022
View reviewed changes

Tanya-Solyanik reviewed Aug 12, 2022
View reviewed changes

Comment thread...ystem.Windows.Forms.Primitives/src/System/LocalAppContextSwitches/LocalAppContextSwitches.cs Outdated

Tanya-Solyanik reviewed Aug 12, 2022
View reviewed changes

Comment threadsrc/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/Dpi/FormTests.Dpi.cs Outdated

Fix test and address review comments.
d0746ba

dreddy-work dismissed Tanya-Solyanik’s stale review via d0746baAugust 12, 2022 18:46

Test skip that was accidentally removed.
22ce594

This was referenced Aug 12, 2022
Flaky System.Windows.Forms.Tests.Dpi.FormDpiTests.Form_DpiChanged_Bounds #7332
Closed
Flaky System.Windows.Forms.Tests.Dpi.FormDpiTests.Form_DpiChanged_Bounds #7365
Closed

dreddy-work enabled auto-merge (squash) August 12, 2022 20:25

Tanya-Solyanik approved these changes Aug 12, 2022
View reviewed changes

dreddy-work merged commit 01abac5 into mainAug 12, 2022

dreddy-work deleted the dev/dreddy/fix7251 branch August 12, 2022 23:21

Tanya-Solyanik reviewed Aug 12, 2022
View reviewed changes

RussKie added the 📖 documentation: breaking please open a breaking change issue https://github.com/dotnet/docs/issues/new?assignees=gewarren label Sep 4, 2022

dreddy-work mentioned this pull request Sep 6, 2022
[Breaking Change] Top level Forms scale their minsize and maxsize values according to the Dpi of the monitor dotnet/docs#31020
Closed
1 task

RussKie mentioned this pull request Sep 13, 2022
Update feature switch runtime configuration doc #7765
Merged

ghost locked as resolved and limited conversation to collaborators Oct 5, 2022

Uh oh!

Conversation

dreddy-work commented Jul 20, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Microsoft Reviewers: Open in CodeFlow

Uh oh!

RussKie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dreddy-work commented Aug 11, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RussKie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RussKie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dreddy-work commented Aug 11, 2022

Uh oh!

RussKie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Tanya-Solyanik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Tanya-Solyanik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Tanya-SolyanikAug 12, 2022

Choose a reason for hiding this comment

Uh oh!

Tanya-SolyanikAug 12, 2022

Choose a reason for hiding this comment

Uh oh!

dreddy-workAug 13, 2022

Choose a reason for hiding this comment

dreddy-work commented Jul 20, 2022•
edited
Loading

dreddy-work commented Aug 11, 2022•
edited
Loading