Skip to content

Update C# templates to introduce file scoped namespaces#4911

Merged
haonanttt merged 8 commits intomainfrom
user/haonantang/FileScopedNamespaces
Dec 17, 2024
Merged

Update C# templates to introduce file scoped namespaces#4911
haonanttt merged 8 commits intomainfrom
user/haonantang/FileScopedNamespaces

Conversation

@haonanttt
Copy link
Copy Markdown
Contributor

Purpose

Regarding to #3329, creating this PR to introduce file scoped namespaces.

What have changed

For all .cs files inside dev/VSIX/ItemTemplates and dev/VSIX/ProjectTemplates, the template code format have been changed to file scoped namespaces:

namespace MyNamespace;

public sealed partial class MainWindow : Window
{
    public MainWindow()
    {
        ...
    }
}

@haonanttt
Copy link
Copy Markdown
Contributor Author

1 more thing to be confirmed:
Should the .cs files inside dev/VSIX/Extension and dev/VSIX/Shared also be changed?

@michael-hawker
Copy link
Copy Markdown

1 more thing to be confirmed: Should the .cs files inside dev/VSIX/Extension and dev/VSIX/Shared also be changed?

I'm not as familiar with those, but it doesn't hurt to apply this change to every C# file across the entire repo.

File-scoped namespaces were added for .NET 6, which should be everything here.

Copy link
Copy Markdown

@michael-hawker michael-hawker left a comment

Choose a reason for hiding this comment

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

🦙❤️ Awesome, thanks @haonanttt for putting this together! 🎉🎉🎉 Couple of suggestions to improve partial class coverage for AOT and source generators.

I thought I remembered another small thing related to templates, but can't find it in my chat history. Nothing came to mind when going through the review. @niels9001 want to do a quick pass to see if I missed anything?

1. mark all classes 'partial'
2. align the MainWindow to be an empty Grid without the Button and handler
@haonanttt haonanttt marked this pull request as ready for review November 22, 2024 09:23
@niels9001
Copy link
Copy Markdown
Contributor

@haonanttt Love these changes 🚀! Since we are removing the Button click event, we should remove it from the Cpp templates as well?

With .NET9 WPF now comes with modern theming and has Mica enabled as default maybe this is something we should enable as well? Especially since this is something that is a default design pattern across Windows, and are just a 3 lines of XAML.

@codendone codendone requested a review from Scottj1s November 22, 2024 18:43
@michael-hawker
Copy link
Copy Markdown

@niels9001 good call out! This may have been what I was thinking about unconsciously. It was pretty jarring to see the difference with the new WPF template/theming enabled and the WinUI window just being a black box during our .NET Conf demo:

image

@haonanttt
Copy link
Copy Markdown
Contributor Author

@haonanttt please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@microsoft-github-policy-service agree company="Microsoft"

@haonanttt
Copy link
Copy Markdown
Contributor Author

Current changes:

  1. introduce file scoped namespaces for .cs files inside dev/VSIX
  2. Make sure all .cs files using partial classes
  3. Enable Mica for all window-level files

Need discussion:

  1. Shall we still keep the button and handler or make everything empty grid

2. Remove Background="{ThemeResource ApplicationPageBackgroundThemeBrush}" from Blank Page item template
Copy link
Copy Markdown

@michael-hawker michael-hawker left a comment

Choose a reason for hiding this comment

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

Looking great! Just did a quick pass to apply some C# style guidelines as suggestions which can be applied in batch. Mostly to remove this. and the m_ to just _ for window. (I linked to references in those first comments.) The only thing I couldn't make as a suggest was moving the _window member to the top of the class.

Thanks so much for going through this with us, this is going to be a great improvement to the templates to make it more streamlined for devs to get work done! 🦙❤️

2. Add empty line between functions
@haonanttt haonanttt self-assigned this Nov 28, 2024
@michael-hawker michael-hawker dismissed niels9001’s stale review December 4, 2024 07:21

Stale, feedback addressed

@haonanttt
Copy link
Copy Markdown
Contributor Author

Current status:

  • Comments from Michael and Niels have been resolved.

Next steps:

  • Will move on the PR after Scott's review

@michael-hawker
Copy link
Copy Markdown

michael-hawker commented Dec 6, 2024

FYI @Scottj1s, Niels and I are all good here as external approvers; we really appreciate the work @haonanttt has done with us here to modernize the templates and bring them up-to-date with all the latest .NET features/patterns. 🦙❤️

I believe we addressed @codendone's initial concern about the Button in the comment thread, but happy to chat about any concerns you have, feel free to ping me if you have any - otherwise hopefully we can just merge. Thanks!

@Scottj1s
Copy link
Copy Markdown
Member

Scottj1s commented Dec 6, 2024

@haonanttt I'll add my thanks too, as well as to @mrlacey, @dotMorten, who were involved in an earlier discussion on this, and to @wjk, who authored a similar PR a few years ago, later closed. I appreciate the need for discussion, but we clearly never got around to one, and the frustrations remain. My preference is to bias for action here and course correct if necessary.

A few things to consider:

  • There is much history here, which I myself don't recall. The C# templates were inspired by the C++, in turn cribbed from C++/WinRT. I have no problem with divergence from the C++/WinRT templates, but we should at least be consistent between C++ and C# templates here - removing the Button in both (see William's PR).
  • Ideally, we would also remove the unused C++ project template MyProperty (see William's PR) - however, there may be a technical limitation that requires at least one member in the class IDL.
  • Since we're removing the Button, we could consider adding a readme.txt, as we have for the C++ templates (e.g., here), to include a link to the Windows App SDK Samples, and/or add a comment in the Xaml/C# with that link.

@Scottj1s
Copy link
Copy Markdown
Member

Scottj1s commented Dec 6, 2024

... we should at least be consistent between C++ and C# templates here - removing the Button in both (see William's PR).

Ah, wasn't paying close attention - I see we've removed Button across the board

@michael-hawker
Copy link
Copy Markdown

@Scottj1s I like the idea of a readme, but I know for UWP VS would just pop open the 'getting started' guide when you started a new project to get these resources:

image

They do have this comment in the MainPage.xaml.cs:

// The Blank Page item template is documented at https://go.microsoft.com/fwlink/?LinkId=402352&clcid=0x409


For WinUI 3 by comparison, that 'getting started' guide does not appear - are we missing a flag or something - or was this just never ported over in VS?

image

And we already have this comment in the MainWindow.xaml.cs file:

// To learn more about WinUI, the WinUI project structure,
// and more about our project templates, see: http://aka.ms/winui-project-info.

But this landing page is a bit sparse, should we use a different aka.ms link here or improve that doc page? FYI @niels9001

@michael-hawker
Copy link
Copy Markdown

@Scottj1s could we merge these technical improvements into the templates now? And then take a follow-up with a task to talk about improving the getting started pages in VS across the board? It seems that may be unique to UWP, but a great feature. Like WPF doesn't do it either and just throws you into MainWindow.xaml:

image

Our other templates don't add readmes (as then folks tend to never go update those so we could get repos people push to GitHub that just have that file everywhere muddling search results). It really seems to be supplanting this feature that seems supported by VS somehow/somewhere for at least some project types, so it'd be great if we could work to improve this everywhere for everyone for all project types working across the VS ecosystem.

And in the interim, we should update the existing link's comment or improve the page it points to in the docs. @niels9001 is that a good aka.ms link which of those two comment options should we take?

@michael-hawker
Copy link
Copy Markdown

@haonanttt @Scottj1s now that we have this approved, what's left before we merge? How do these get integrated into releases from the repo here? Will it just be in the next latest drop on NuGet that pulls these in?

@haonanttt
Copy link
Copy Markdown
Contributor Author

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@michael-hawker
Copy link
Copy Markdown

Looks like some of the auxiliary project types target older .NET runtimes or something?

dev\VSIX\Extension\Cs\Dev17\VSPackage.cs(10,1): Error CS8370: Feature 'file-scoped namespace' is not available in C# 7.3. Please use language version 10.0 or greater.

dev\VSIX\Extension\Cs\Dev17\VSPackage.cs

If this is just for the package itself, we should be fine to just add <LangVersion> to the csproj here? Otherwise, we can put it back for this one, it matters less if it's not consumed by external folks and just a helper class (I'm not familiar with the specific of the VSIX config here beyond the C# templates).

dev\VSIX\Shared\WizardImplementation.cs

The same csproj above manages this file as well, though there's also an include of it in the C++ project as well?

<Compile Include="..\..\..\Shared\WizardImplementation.cs" Link="WizardImplementation.cs" />

Not as sure what this even means here. If these are just internal files to the VSIX, we don't have to update the scope here, or we can try adding <LangVersion> to both projects and see if that resolves it.

@haonanttt
Copy link
Copy Markdown
Contributor Author

Investigated and informed the file dev\VSIX\Extension\Cs\Dev17\VSPackage.cs and dev\VSIX\Shared\WizardImplementation.cs is used for project setup, will not show as some templates. So decide to revert the file scoped namespace for them to get less impact for those files.

@haonanttt
Copy link
Copy Markdown
Contributor Author

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@haonanttt haonanttt merged commit a0a435d into main Dec 17, 2024
@haonanttt haonanttt deleted the user/haonantang/FileScopedNamespaces branch December 17, 2024 05:12
@michael-hawker
Copy link
Copy Markdown

🦙❤️ Thanks @haonanttt! This is great! 🎉🎉🎉

codendone pushed a commit that referenced this pull request Jan 10, 2025
* Update C# templates to introduce file scoped namespaces

* introduce file scoped namespaces to dev/VSIX/Extension and dev/VSIX/Shared folders as well

* resolve comments:
1. mark all classes 'partial'
2. align the MainWindow to be an empty Grid without the Button and handler

* Enable Mica

* remove mica for non-window level

* 1. Remove 'Button' from C++ templates
2. Remove Background="{ThemeResource ApplicationPageBackgroundThemeBrush}" from Blank Page item template

* 1. Remove 'this.' and change 'm_' to '_'
2. Add empty line between functions

* revert file scoped namesapce for VSPackages.cs and WizardImplementation.cs

---------

Co-authored-by: Haonan Tang (from Dev Box) <haonantang@microsoft.com>
@HO-COOH
Copy link
Copy Markdown

HO-COOH commented Feb 26, 2025

Is the Int32 MyProperty; still in the idl file template? What's the reason for that to exist in BlankWindow and BlankPage template?

@michael-hawker
Copy link
Copy Markdown

@haonanttt I just downloaded VS 17.14 Preview 3, I see some of the template changes, but not all of them. The file-scoped namespaces for instance aren't there...? But I see them in main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants