Skip to content

fix: Restore missing TF for System.Windows.Forms.HighDpiMode#2932

Merged
RussKie merged 1 commit intodotnet:masterfrom
RussKie:fix_2925_Missing_HighDpiMode
Mar 3, 2020
Merged

fix: Restore missing TF for System.Windows.Forms.HighDpiMode#2932
RussKie merged 1 commit intodotnet:masterfrom
RussKie:fix_2925_Missing_HighDpiMode

Conversation

@RussKie
Copy link
Contributor

@RussKie RussKie commented Mar 2, 2020

Resolves #2925

System.Windows.Forms.HighDpiMode was accidentally placed under Internal namespace, which caused a missing type-forward decoration.
This resulted in users receiving the following error when attempting to roll forward from .NET Core 3.x to .NET 5:

System.MissingMethodException : Method not found: 'Boolean System.Windows.Forms.Application.SetHighDpiMode(System.Windows.Forms.HighDpiMode)'.

Proposed changes

  • Move the type into the correct location and add the missing TypeForward decoration.

Customer Impact

  • User can now roll forward from .NET Core 3.x and use Boolean System.Windows.Forms.Application.SetHighDpiMode(System.Windows.Forms.HighDpiMode)

Regression?

  • Yes

Risk

  • Minimal

Test methodology

  • Use the repro app or generate a new Windows Forms app targeting .NET Core 3.0 (or 3.1) and set it to rollforward:
    {
      "sdk": {
        "rollForward": "major"
      }
    }
  • Ensure no .NET Core 3.x runtime is installed. Best to run a VM with only .NET5 Preview1 installed
  • Run the app, observe the failure:
    image
  • Build the current commit with pack option:
    PS> .\build.ps1 -pack
  • Copy and replace System.Windows.Forms.dll and System.Windows.Forms.Primitives.dll
    • from .\artifacts\packages\Debug\NonShipping\Microsoft.Private.Winforms.5.0.0-dev.nupkg\lib\netcoreapp5.0 --> C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App\5.0.0-preview.2.20119.5
    • from .\artifacts\packages\Debug\NonShipping\Microsoft.Private.Winforms.5.0.0-dev.nupkg\ref\netcoreapp5.0 --> C:\Program Files\dotnet\packs\Microsoft.WindowsDesktop.App.Ref\5.0.0-preview.2.20119.5\ref\netcoreapp5.0
  • Re-run the app, observe the app start
    image
Microsoft Reviewers: Open in CodeFlow

Resolves dotnet#2925

`System.Windows.Forms.HighDpiMode` was accidentally placed under `Internal`
namespace, which caused a missing type-forward decoration.
This resulted in users receiving the following error when attempting to
roll forward from .NET Core 3.x to .NET 5:

```
System.MissingMethodException : Method not found:
'Boolean System.Windows.Forms.Application.SetHighDpiMode(System.Windows.Forms.HighDpiMode)'.
```

Move the type into the correct location and add the missing `TypeForward` decoration.
@RussKie RussKie requested a review from a team as a code owner March 2, 2020 05:50
@ghost ghost assigned RussKie Mar 2, 2020
@codecov
Copy link

codecov bot commented Mar 2, 2020

Codecov Report

Merging #2932 into master will increase coverage by 0.02815%.
The diff coverage is n/a.

@@                 Coverage Diff                 @@
##              master       #2932         +/-   ##
===================================================
+ Coverage   60.23314%   60.26129%   +0.02815%     
===================================================
  Files           1248        1248                 
  Lines         433396      433396                 
  Branches       38850       38850                 
===================================================
+ Hits          261048      261170        +122     
+ Misses        166968      166857        -111     
+ Partials        5380        5369         -11     
Flag Coverage Δ
#Debug 60.26129% <ø> (+0.02815%) ⬆️
#production 32.47618% <ø> (+0.04043%) ⬆️
#test 98.97398% <ø> (+0.01103%) ⬆️

@Shyam-Gupta
Copy link
Member

Shyam-Gupta commented Mar 2, 2020

@RussKie It seems at present HighDpiMode is in System.Windows.Forms namespace and is a public type:

Please let me know if I am missing something. I also noticed that this file is under Internals folder. Is that somehow forcing it to be an Internal type ? If it is an internal type then why does it not creating a problem for projects targeting .NET Core 5.0 ?

Are there other such types which need to be made public ?

@RussKie
Copy link
Contributor Author

RussKie commented Mar 2, 2020

Please let me know if I am missing something. I also noticed that this file is under Internals folder. Is that somehow forcing it to be an Internal type ? If it is an internal type then why does it not creating a problem for projects targeting .NET Core 5.0 ?

No, it is a simple convention with no enforcement - all public types to be places as per their namespace, internals to into Internal folder.
This makes it easier to see which types must be type-forwarded.

Are there other such types which need to be made public ?

I have double-checked all others, all good.
I have also thought of some sort of a test that could check the correctness, but the TFs are in System.Windows.Forms, and the types in System.Windows.Forms.Primitives... Without some horrible hard linking I couldn't come up with a test...
If you have any suggestions please let me know.

@Shyam-Gupta
Copy link
Member

Would it be better to move HighDpiMode type back to S.W.F ?

@Tanya-Solyanik
Copy link
Contributor

Is it possible to dump all public type names from System.Windows.Forms v3.1, then add a test that dumps out the same from the current version of this assembly and compare the lists? Then get
all TypeForwardAttributes from the current assembly, do attribute.Destination.FullName and confirm that list of full names is the same as the diff between v3.1 and 5.0? What am I missing?
This test would be useful regardless of how this bug is fixed, with type forwarding or by returning the enum to system.windows.forms

@RussKie
Copy link
Contributor Author

RussKie commented Mar 3, 2020

Would it be better to move HighDpiMode type back to S.W.F ?

We can't do that. It is used by types residing in System.Windows.Forms.Primitives.
All types in System.Windows.Forms.Primitives are there out of necessity.

Is it possible to dump all public type names from System.Windows.Forms v3.1, then add a test that dumps out the same from the current version of this assembly and compare the lists?

I think what you're proposing falls under the API Compat story (which we've been neglecting).
I'm not sure comparing like-for-like will work without additional efforts, the API surface is different between the two, and may further erode going into the future.

We can work on this separately, once we get over the hump. There is a significant value in automating the verification.

@Shyam-Gupta
Copy link
Member

We can't do that. It is used by types residing in System.Windows.Forms.Primitives.
All types in System.Windows.Forms.Primitives are there out of necessity.

Ok. TF seems fine then.

@RussKie
Copy link
Contributor Author

RussKie commented Mar 3, 2020

I run tests and verified the fix. Added test methodology section to the description.

@RussKie RussKie merged commit f3fabd7 into dotnet:master Mar 3, 2020
@RussKie RussKie deleted the fix_2925_Missing_HighDpiMode branch March 3, 2020 04:14
@ghost ghost added this to the 5.0 milestone Mar 3, 2020
M-Lipin pushed a commit to M-Lipin/winforms that referenced this pull request Mar 23, 2020
…t#2932)

Resolves dotnet#2925

`System.Windows.Forms.HighDpiMode` was accidentally placed under `Internal`
namespace, which caused a missing type-forward decoration.
This resulted in users receiving the following error when attempting to
roll forward from .NET Core 3.x to .NET 5:

```
System.MissingMethodException : Method not found:
'Boolean System.Windows.Forms.Application.SetHighDpiMode(System.Windows.Forms.HighDpiMode)'.
```

Move the type into the correct location and add the missing `TypeForward` decoration.
@ghost ghost locked as resolved and limited conversation to collaborators Feb 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

System.MissingMethodException at System.Windows.Forms.Application.SetHighDpiMode(System.Windows.Forms.HighDpiMode) in GreenShot unit tests

4 participants