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.

Sign up for GitHub

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

Jump to bottom

Implement Windows Forms application configuration and bootstrap #5035

Merged
merged 4 commits into from
Jul 29, 2021
Merged

Implement Windows Forms application configuration and bootstrap #5035

merged 4 commits into from
Jul 29, 2021

Conversation

RussKie
Copy link
Member

@RussKie RussKie commented Jun 3, 2021 •
edited
Loading

Implements dotnet/designs#223
Resolves #4909
Pre-req for dotnet/windowsdesktop#1693
Pre-req for https://github.com/dotnet/winforms-designer/issues/3192

Proposed changes

  • Implement a C# source generator that generate a application bootstrap from configurations sources from MSBuild
  • Implements a general purpose analyzer for C# and VB to check for dpiAware configurations in app.manifest

Test methodology

  • Unit tests for source generators and analyzers
  • Manual integration tests via the WinformsControlsTest project
  • Manual packaging tests for Microsoft.Private.Winforms, our transport package
  • Manual packaging tests for the analyzer bundling in to the Microsoft.NET.Sdk.WindowsDesktop, see Package Windows Forms analyzers windowsdesktop#1693
  • Manual packaging tests for the analyzer props/targets references in the Microsoft.NET.Sdk.WindowsDesktop, see Ingest and import Windows Forms analyzer props wpf#4605
  • Manual "real world" simulations by updating the local version of .NET Windows Desktop SDK, and building a brand new Windows Forms app via dotnet new winforms

image

Microsoft Reviewers: Open in CodeFlow

@ghost ghost assigned RussKie Jun 3, 2021
@RussKie

This comment has been minimized.

Sign in to view
@RussKie RussKie requested review from DustinCampbell and a team June 3, 2021 07:19
@RussKie RussKie mentioned this pull request Jun 3, 2021
Merged
davidwengier
davidwengier reviewed Jun 3, 2021
View reviewed changes
...alyzers.CSharp/src/System/Windows/Forms/Generators/ApplicationConfigurationSyntaxReceiver.cs Outdated Show resolved Hide resolved
src/System.Windows.Forms.Analyzers/src/DiagnosticDescriptors.cs Outdated Show resolved Hide resolved
...ms.Analyzers.CSharp/src/System/Windows/Forms/Generators/ApplicationConfigurationGenerator.cs Outdated Show resolved Hide resolved
...stem.Windows.Forms.Analyzers.CSharp/src/System/Windows/Forms/Generators/ProjectFileReader.cs Outdated Show resolved Hide resolved
...stem.Windows.Forms.Analyzers.CSharp/src/System/Windows/Forms/Generators/ProjectFileReader.cs Outdated Show resolved Hide resolved
...stem.Windows.Forms.Analyzers.CSharp/src/System/Windows/Forms/Generators/ProjectFileReader.cs Outdated Show resolved Hide resolved
...stem.Windows.Forms.Analyzers.CSharp/src/System/Windows/Forms/Generators/ProjectFileReader.cs Outdated Show resolved Hide resolved
...stem.Windows.Forms.Analyzers.CSharp/src/System/Windows/Forms/Generators/ProjectFileReader.cs Outdated Show resolved Hide resolved
src/System.Windows.Forms.Analyzers/src/DiagnosticDescriptors.cs Outdated Show resolved Hide resolved
src/System.Windows.Forms.Analyzers.CSharp/tests/UnitTests/StringText.cs Outdated Show resolved Hide resolved
@RussKie RussKie mentioned this pull request Jun 3, 2021
Closed
RussKie
RussKie commented Jun 3, 2021
View reviewed changes
eng/Versions.props Outdated Show resolved Hide resolved
@RussKie RussKie force-pushed the project_configuration branch 5 times, most recently from 8fbf2ae to 4d2b7ea Compare June 7, 2021 05:44
@RussKie RussKie force-pushed the project_configuration branch 3 times, most recently from edc94bc to c79efb4 Compare June 15, 2021 04:19
@RussKie RussKie marked this pull request as ready for review June 15, 2021 04:26
@RussKie RussKie added the 📖 documentation: breaking A change in behavior that could be breaking for applications. Needs to be documented. label Jun 15, 2021
@RussKie RussKie force-pushed the project_configuration branch 2 times, most recently from bf16a7a to d003a72 Compare June 15, 2021 05:03
@RussKie RussKie marked this pull request as draft June 21, 2021 02:15
@RussKie RussKie added the Priority:1 Work that is critical for the release, but we could probably ship without label Jul 16, 2021
@RussKie RussKie added this to the 6.0 rc1 milestone Jul 16, 2021
@RussKie
Add ApplicationConfiguration.Initialize() source generator
31dcabd 
Add a C# source generator that emits application bootstrap as described
in https://github.com/dotnet/designs/blob/main/accepted/2021/winforms/streamline-application-bootstrap.md
for the following properties:
* ApplicationVisualStyles
* ApplicationDefaultFont
* ApplicationHighDpiMode
* ApplicationUseCompatibleTextRendering

For top level statements the bootstrap also emits code to set STA mode.

A build will fail if any of the properties contain invalid or otherwise
incorrect values.

FontConverter is not available in netstandard 2.0, to bridge the gap
FontConverter logic has been partially copied from the corresponding .NET
runtime implementation to facilitate the comparable behaviour.
The only missing gap is parsing of FontFamily (which requires GDI+ calls),
which means that if a supplied font descriptor contains an invalid font
name it will cause an app to crash. This however is a desired behaviour.
@RussKie RussKie force-pushed the project_configuration branch 2 times, most recently from 4a1a2ce to e064c54 Compare July 28, 2021 10:59
@RussKie
Add general purpose analyzers
4719079 
Add analyzers for C# and VB projects that check whether a user project
contains a manifest file, which includes dpi configurations.
If such manifest is detected the user is advised to use Application.SetHighDpiMode()
API instead.
@RussKie
Package analyzers
8848001
@RussKie
Local testing
329e453
@RussKie RussKie marked this pull request as ready for review July 28, 2021 12:27
@RussKie
Copy link
Member Author

RussKie commented Jul 28, 2021

I've done quite an extensive testing of building and packaging dotnet/winforms, dotnet/wpf and dotnet/windowsdesktop locally, then had the same flow simulated via the internal CI. I'm reasonably comfortable that this is working as intended.

I'm planning to merge this and a follow up PR in dotnet/windowsdesktop in coming days. There're no changes required to dotnet/wpf (that builds the Windows Desktop SDK).

@RussKie RussKie mentioned this pull request Jul 28, 2021
Merged
@RussKie RussKie merged commit b84073d into dotnet:main Jul 29, 2021
This was referenced Jul 29, 2021
Closed
Closed
@RussKie
Copy link
Member Author

RussKie commented Aug 2, 2021

@KlausLoeffelmann @KathleenDollard the analyzers also work for VB apps
image

If necessary you can update the wording here

winforms/src/System.Windows.Forms.Analyzers/src/Resources/SR.resx

Lines 67 to 69 in 6515f5f

<data name="WFAC010Message_VB" xml:space="preserve">
<value>Remove high DPI settings from {0} and configure via '{1}' property in Application Framework</value>
</data>

@AraHaan
Copy link
Member

AraHaan commented Aug 2, 2021

Sweet

@RussKie RussKie mentioned this pull request Aug 10, 2021
Closed
@RussKie RussKie removed the Priority:1 Work that is critical for the release, but we could probably ship without label Aug 20, 2021
RussKie added a commit to RussKie/winforms that referenced this pull request Aug 25, 2021
@RussKie
Update templates to use ApplicationConfiguration.Initialize()
5370724 
Relates to dotnet#5035
Relates to dotnet#5071
@RussKie RussKie mentioned this pull request Aug 25, 2021
Merged
@RussKie RussKie mentioned this pull request Aug 25, 2021
Merged
RussKie added a commit to RussKie/winforms that referenced this pull request Aug 26, 2021
@RussKie
Update templates to use ApplicationConfiguration.Initialize()
5a93314 
Relates to dotnet#5035
Relates to dotnet#5071
RussKie added a commit that referenced this pull request Aug 26, 2021
@RussKie
Update templates to use ApplicationConfiguration.Initialize() (#5529)
c47af80 
Relates to #5035
Relates to #5071
@Zheng-Li01 Zheng-Li01 mentioned this pull request Sep 7, 2021
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Jan 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@davidwengier davidwengier davidwengier left review comments

@jmarolf jmarolf jmarolf left review comments

@AraHaan AraHaan AraHaan left review comments

@vatsan-madhavan vatsan-madhavan vatsan-madhavan left review comments

@sharwell sharwell Awaiting requested review from sharwell

@ericstj ericstj Awaiting requested review from ericstj

@DustinCampbell DustinCampbell Awaiting requested review from DustinCampbell

Assignees

@RussKie RussKie

Labels
📖 documentation: breaking A change in behavior that could be breaking for applications. Needs to be documented.
Projects
None yet
Milestone
6.0 rc1
Development

Successfully merging this pull request may close these issues.

[Proposal] Streamline application configuration and bootstrap
5 participants
@RussKie @AraHaan @davidwengier @jmarolf @vatsan-madhavan