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

Streamline Windows Forms application configuration and bootstrap #223

Merged
merged 1 commit into from
Jul 16, 2021

Conversation

RussKie
Copy link
Member

@RussKie RussKie commented May 28, 2021

This document describes the application configuration and bootstrap experience utilising Roslyn source generators that facilitates sharing of configurations with the VS Designer (so that it can continue to provide the true WYSIWYG experience) we wish to add to .NET 6.0.

Copy link
Member

@dsplaisted dsplaisted left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good from the .NET SDK side.

It looks like there isn't any work required in the .NET SDK targets, it will all be handled by the analyzers.

That is, whenever the designers surface process is started configuration information is read from a known location, and necessary configurations are applied (e.g. run the design surface in PerMonitorV2 mode, or set a form/usercontrol default font to "Arial, 14pt").<br/>
:warning: **out of scope**, tracked under https://github.com/dotnet/winforms-designer/issues/3192.

**NOTE:** The new functionality is opt-in, i.e. unless a developer makes a conscious decision to use the new configuration and the bootstrap mechanism existing applications will continue to work as-is, and the current developer experience will remain the same.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you actually mean opt-in, or do you mean "non-breaking"? I presume we'd want to update templates to start with this for example?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the plan is to update templates. For existing apps the behaviour is "opt-in" in the sense that developers will need to write the new method, and remove existing Application.* methods. If they don't do this - nothing happens, apps will continue to work as before.

accepted/2021/winforms/streamline-application-bootstrap.md Outdated Show resolved Hide resolved
@KlausLoeffelmann
Copy link
Member

We need to make sure, not to exclude the typical VB customer in that and make sure VB‘s application framework fits into this. Actually, I’d propose to have two templates for VB Apps and phase out the current principle of storing defaults in the application framework in favor of this concept. But still we should keep the Program.vb-less and application-events concept alive and also I am against the requirement for a VB dev to patch vbproj files directly.

@merriemcgaw
Copy link
Member

merriemcgaw commented May 28, 2021

We need to make sure, not to exclude the typical VB customer in that and make sure VB‘s application framework fits into this.

Do you have concerns that as proposed we are excluding them? I agree with the principle that we should keep VB customers in mind with the design and update the application framework as necessary to support it.

I am against the requirement for a VB dev to patch vbproj files directly.

I would have agreed with you with the old style project files. But with SDK style projects editing is so much more streamlined, it could/should very well be an option.

(edited for formatting)

@RussKie
Copy link
Member Author

RussKie commented May 29, 2021

It looks like there isn't any work required in the .NET SDK targets, it will all be handled by the analyzers.

I may need some help/guidance/reviews with analyzer inboxing, so we can provide a seamless exprience.

@RussKie
Copy link
Member Author

RussKie commented May 29, 2021

I am against the requirement for a VB dev to patch vbproj files directly.

Could please explain why not?
Even though some VB developers may be coming from non CS backgrounds and may not necessarily identify themselves as software engineers, I am sure, they are intelligent individuals, and would understand what a project file is and modify properties in it. May be it is time we should start breaking the stereotype about VB developers...

That said, I do agree with you that we should offer a GUI configuration in VS, and that's where the Project System comes into play. I've had a chat with @drewnoakes about it, and for C# it doesn't appear as a big job. Sadly for VB it maybe not the case - I was told (and I'm sure you know it all too well) that the AF makes it very hard to modernise in this area.

Actually, I’d propose to have two templates for VB Apps and phase out the current principle of storing defaults in the application framework in favor of this concept.

I'm happy to explore this. Will chat with you when you're back.

@KlausLoeffelmann
Copy link
Member

Sadly for VB it maybe not the case - I was told (and I'm sure you know it all too well) that the AF makes it very hard to modernise in this area.

And that’s exactly why I think we should try to get rid of the current UI experience for the AF in VB altogether and maybe have a somewhat identical experience (if not shareable) for C# and VB.

Don’t get me wrong, though! I don’t want to get rid of the AF. Just trying to make the UI part easier!

@eriawan
Copy link
Member

eriawan commented May 31, 2021

@RussKie

Apologize to barge in the middle of this PR review 🙏
Is this .NET design PR is supposed to be fully "open" as fully open, or it is meant for MS employee only?

Because in the streamline-application-bootstrap.md file, the link on this line 34 of https://github.com/dotnet/winforms-designer/issues/3192 is pointing to a private repo,
Since I'm not MS employee, I could not open this link at all,

@KlausLoeffelmann
Copy link
Member

@eriawan It's basically only a reference to the internal Winforms-Designer repo. Yes, the Designer repo is not open to the public, but we need to track the work, since there is Designer work which needs to be done.

For this discussion, it doesn't contain any relevant information except, that the planning/discussion for the designer work for this is out of scope (😄) and that the additional work, which would become necessary, if we did this, is tracked there.


Indeed, this sounds like a grand idea. However in reality Windows Forms app often have elaborate application bootstrap logic (e.g. [this](https://github.com/gitextensions/gitextensions/blob/master/GitExtensions/Program.cs), [this](https://github.com/luisolivos/Halconet/blob/4ed328a15ba69aab00e92076774977143790b5d3/HalcoNET%205.8.0/Ventas/Program.cs), or [this](https://github.com/aaraujo2019/PEQMIMERIA/blob/e11b6d57e3bbbe505834cbfeb717fe87066d6850/DBMETAL_SHARP/DBMETAL_SHARP/Program.cs)) that won't be possible in this scenario, and will require deeper investigations and feasibility studies.

In the long term we could consider supporting a host/builder model, similar to the suggestion in [dotnet/winforms#2861](https://github.com/dotnet/winforms/issues/2861) and implementation in [alex-oswald/WindowsFormsLifetime](https://github.com/alex-oswald/WindowsFormsLifetime). E.g.:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm very much in favor of standardizing the host/builder model across app types where appropriate. Is there any particular reason this should be a long term option rather than an immediate option?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This requires a thorough design and planning, and with the current workload the team doesn't have sufficient capacity for this to be delivered in .NET 6.0 timeframe.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's too late for .NET 6

@RussKie
Copy link
Member Author

RussKie commented Jun 10, 2021

I have restructured and in parts rewritten the proposal reflecting and incorporating the feedback sourced in a number of offline discussions.

@RussKie
Copy link
Member Author

RussKie commented Jun 30, 2021

The designer using the ApplicationFont property:
designer-fontstyle

@RussKie RussKie force-pushed the winforms_app_bootstrap branch 2 times, most recently from ccd3ad8 to 2ea2b0e Compare July 7, 2021 06:22
@RussKie
Copy link
Member Author

RussKie commented Jul 7, 2021

If there's no more feedback I'm planning to merge this in the next couple of days.

This document describes the application configuration and bootstrap
experience utilising Roslyn source generators that facilitates sharing
of configurations with the VS Designer (so that it can continue to
provide the true WYSIWYG experience) we wish to add to .NET 6.0.
@RussKie RussKie merged commit 9619011 into main Jul 16, 2021
@RussKie RussKie deleted the winforms_app_bootstrap branch July 16, 2021 01:28
@RussKie
Copy link
Member Author

RussKie commented Jul 19, 2021

In the top level scenario absent the ability to set [STAThread] right now, instead of requiring a developer to write the the following code by hand...

-   // Set STAThread
-   Thread.CurrentThread.SetApartmentState(ApartmentState.Unknown);
-   Thread.CurrentThread.SetApartmentState(ApartmentState.STA);

    ApplicationConfiguration.Initialize();
    Application.Run(new MainForm());

...we can emit it as part of the source generated Initialize() method. E.g.:

[CompilerGenerated]
internal static partial class ApplicationConfiguration
{
    public static void Initialize()
    {
+       // Set STAThread
+       Thread.CurrentThread.SetApartmentState(ApartmentState.Unknown);
+       Thread.CurrentThread.SetApartmentState(ApartmentState.STA);
+
        Application.EnableVisualStyles();
        Application.SetCompatibleTextRenderingDefault(false);
        Application.SetHighDpiMode(HighDpiMode.PerMonitorV2);
    }
}

Does anyone see any issues with this?

@eriawan
Copy link
Member

eriawan commented Jul 19, 2021

@RussKie

Nice proposal!

If the 2 calls of SetApartmentState() is included in a method in the top level, does the generated Initialize() method cover this too? Should there be additional analyzer for this?

For example:

stativ void SetSTA()
{
    Thread.CurrentThread.SetApartmentState(ApartmentState.Unknown);;
    Thread.CurrentThread.SetApartmentState(ApartmentState.STA);
}

SetSTA();
ApplicationConfiguration.Initialize();
Application.Run(new MainForm());

@ghost

This comment has been minimized.

@jkotas
Copy link
Member

jkotas commented Jul 19, 2021

Does anyone see any issues with this?

It will work, but it is not pretty and it is inefficient. The COM will be first initialized as MTA, then uninitialized, and then initialized again as STA. If it is a temporary solution that we will cleanup later, I think it would ok.

@AraHaan
Copy link
Member

AraHaan commented Jul 19, 2021

I feel like best course of action is to have COM initialized by the runtime as STA by default (and an backwards compat switch to have it use MTA for COM). This way both lines of code for Winforms applications would be needless (as COM would be initialized properly to STA from the beginning) without needing 1. STAThreadAttribute, and 2. setting the current thread appartment state to ApartmentState.Unknown and then to ApartmentState.STA which I would agree is not a very efficient hack than just having the runtime just default to STA thread mode by default for every application. I seen no issues before with adding the attribute in 1. to any type of applications (including console applications) so it should be a reasonable option to default it to that within the runtime itself.

And because Main will start being generated so we can use top level statements, the change would definitely need to be placed at either the runtime, or the compiler generated Main() functions.

@terrajobst
Copy link
Member

@jkotas

Does anyone see any issues with this?

It will work, but it is not pretty and it is inefficient. The COM will be first initialized as MTA, then uninitialized, and then initialized again as STA. If it is a temporary solution that we will cleanup later, I think it would ok.

Could we support the configuration as an assembly level attribute? This might eliminate the need for language syntax for putting attributes on Main and allows the generator to generate the attribute as well, removing it from the user code entirely.

@jkotas
Copy link
Member

jkotas commented Jul 19, 2021

I feel like best course of action is to have COM initialized by the runtime as STA by default

STA is wrong default for console and server applications. It would be major breaking change for those. STA is the right default for Windows UI frameworks only.

Could we support the configuration as an assembly level attribute?

There are number of solutions possible that can be used to solve the STAThreadAttribute problem. I agree that allowing the STAThreadAttribute at assembly level would work fine and look nice.

The argument for solving the problem using a language feature has been that there are other attributes that one may want to apply to Main method (e.g. debugger or MethodImpl attributes) and the language feature addresses them all.

@RussKie
Copy link
Member Author

RussKie commented Jul 19, 2021

@jkotas @terrajobst would this something we'd expect done in .NET6 timeframe, or should I amalgamate SetApartmentState into Initialize (#223 (comment))?

It will work, but it is not pretty and it is inefficient.

It is both, and a developer will have to have it one way or another until we have a better solution.

@terrajobst
Copy link
Member

@jaredpar would have to comment on the feasibility of language features in .NET 6.

@AraHaan
Copy link
Member

AraHaan commented Jul 20, 2021

I feel like best course of action is to have COM initialized by the runtime as STA by default

STA is wrong default for console and server applications. It would be major breaking change for those. STA is the right default for Windows UI frameworks only.

Could we support the configuration as an assembly level attribute?

There are number of solutions possible that can be used to solve the STAThreadAttribute problem. I agree that allowing the STAThreadAttribute at assembly level would work fine and look nice.

The argument for solving the problem using a language feature has been that there are other attributes that one may want to apply to Main method (e.g. debugger or MethodImpl attributes) and the language feature addresses them all.

I mean unless it is a console application that then also needs to pull in the UI frameworks for things like MessageBox.Show or TaskDialog's.

I also know of other console applications (QuickBMS for example) which utilizes the OpenFileDialog / SaveFileDialog's from the Windows APIs (which basically makes it an console-ui application hybrid.

As for assembly level attribute for STAThread that could work as well and have it source generated by the WindowsDesktop SDK. However it might require a breaking change to STAThread even after the cutoff time for .NET 6 to be able to do just that if we are all willing to break that attribute backwards compatibility ever so slightly just so we can benefit from this as a better solution before .NET 6 gets released outside of preview and rc publicly as an stable release.

@Shayan-To
Copy link

The argument for solving the problem using a language feature has been that there are other attributes that one may want to apply to Main method (e.g. debugger or MethodImpl attributes) and the language feature addresses them all.

@jkotas Is there a proposal for the language feature? Or a relevant issue? Haven't been able to find any.

@jkotas
Copy link
Member

jkotas commented Aug 13, 2021

I have created one: dotnet/csharplang#5045

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.