Skip to content

Add Page automatically, remove from None automatically #685

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

Closed
heaths opened this issue May 14, 2019 · 30 comments · Fixed by #1027
Closed

Add Page automatically, remove from None automatically #685

heaths opened this issue May 14, 2019 · 30 comments · Fixed by #1027
Assignees
Milestone

Comments

@heaths
Copy link

heaths commented May 14, 2019

When the SDK is "Microsoft.NET.Sdk.WindowsDesktop", it would be nice not to have to add <UseWPF>true</UseWPF>. Especially when migrating projects from old-style .csproj files, this is easy to forget and seems otherwise unnecessary since I already claimed what project type I'm going to build. And wouldn't this be true of both WPF and UWP projects anyway - to include *.xaml files as <Page> items?

Also, why must the IDE - or, when using VSCode, we (manually) - add <None Remove="..."/> when the targets could do this for any <Page> items. Could this also be done automatically in the targets for "Microsoft.NET.Sdk.WindowsDesktop"?

This could make WPF SDK-style .csproj files as slim as non-WPF SDK-style .csproj files.

@stevenbrix
Copy link
Contributor

My initial thought is that WinForms applications also reference the Microsoft.NET.Sdk.WindowsDesktop, so it's possible that is needed to help differentiate. @ryalanms is more familiar with the build then I am, and can probably help understand if this is is something we control, or if it's more suited for a repo like Microsoft/MSBuild

@vatsan-madhavan
Copy link
Member

/cc @nguerrera, @ericstj, @dsplaisted, @merriemcgaw

There are namespace overlaps between WPF & WinForms. Enabling both sets of references by default would likely cause some confusion (for both porting scenarios as well as new projects) rather than acting as a convenience.

Why would you need <None Remove="..."/> for Page items? Who or what are including them to None items in the first place that necessitates removal ? We have the ability to add something like this to the targets, as long as we are sure this is the right design to incorporate.

@heaths
Copy link
Author

heaths commented May 15, 2019

WinForm applications likely wouldn't have .xaml files, though. So maybe <UseWPF> is still required otherwise, but why not still add .xaml files to <Page> automatically?

As for adding <None>, Visual Studio does this whenever you add a new Page item. I supposed it's not specifically required, but then why is Visual Studio choosing to do it?

@weltkante
Copy link

weltkante commented May 15, 2019

WinForm applications likely wouldn't have .xaml files

I disagree, classic WinForms projects will default to compiling xaml as WPF <Page> if you add them, they never had any problem with that, you don't need a WPF project for adding WPF controls to your WinForms project.

In fact most of our projects are classic WinForms projects and still have WPF controls in them, works just fine. Whatever makes "WPF projects" special isn't needed in the classic project system. I don't see why that needs to change for .NET Core.

That said I don't mind flipping on a "UseWPF" switch if its required to get things working, more of an annoyance than a roadblock.

@dsplaisted
Copy link
Member

FYI @davkean

@heaths
Copy link
Author

heaths commented May 15, 2019

@weltkante but then doesn't that mean that the WindowsDesktop SDK should put all .xaml files into a <Page> unconditionally then? When used in a WinForms app, wouldn't they still have to be compiled into pages/controls?

@weltkante
Copy link

weltkante commented May 16, 2019

@heaths yes that's what happens in classic projects, you can add WPF Controls to WinForms apps without switching the project type or having to adjust build tags at all (only need to add references to the WPF framework assemblies if you don't have them, gets done automatically if you use the context menu to add a new WPF item from template instead of an existing from disk). I don't think its uncommon if you are gradually making use of WPF features in your WinForms application (instead of rewriting it from scratch in WPF only)

Actually, do you manually reference framework assembly in .NET Core or does <UseWPF> pull those in? I'm used to manage them manually and don't have a VS with me right now to verify how it works in .NET Core. If the tag is responsible for adding the references I'd see why you need it, you wouldn't want WPF references in a WinForms only app, and compiling XAML without references doesn't work (the generated code references APIs from the framework after all).

After considering that I still think I'd prefer a compile error when adding xaml to WinForms without having the right references, so I think they should always compiled as Page. If anything leave the automatic inclusion off for WinForms only apps (so it doesn't pick up xaml in the folder structure and I only get compile errors if I add it manually). Adding it as None for WinForms seems wrong to me because it is never useful.

@heaths
Copy link
Author

heaths commented May 16, 2019

No, not specifying <UseWPF>true</UseWPF> still builds, but I have to add the <Page> items myself in that case, hence the ask here: just adding .xaml files to <Page> automatically.

@grubioe grubioe added this to the 3.0 milestone May 24, 2019
@vatsan-madhavan
Copy link
Member

ok, I'm confused.

Is the Issue about <Page> or about <None>? or both?

As for adding , Visual Studio does this whenever you add a new Page item. I supposed it's not specifically required, but then why is Visual Studio choosing to do it?

What are the semantics of <None>? Exactly how did this break your workflow ?

When a project implies UseWpf != true, then WPF specific semantics (like Page globbing) aren't enabled - this is by design.

@wjk
Copy link
Contributor

wjk commented May 24, 2019

@vatsan-madhavan I think what @heaths is referring to is the glob for <None>. It includes all files not in bin or obj, or included in another rule; this includes XAML files. Although the WindowsDesktop SDK removes Page items from None, Visual Studio might also be adding a pointless <None Remove="something.xaml" /> line to the csproj when a XAML file is added through the New Item dialog. If so, then this is a project system bug, not a WPF bug.

@vatsan-madhavan
Copy link
Member

@wjk - thanks for puzzling that out - very helpful!

@vatsan-madhavan
Copy link
Member

vatsan-madhavan commented May 24, 2019

@heaths, we have talked about adding a suppressible warning when Microsoft.NET.Sdk.WindowsDesktop is in use, and neither UseWpf nor UseWinForms is set to true.

Don't have an issue tracking this yet, but I've come across several instances confusion - either missing UseWpf when using Microsoft.NET.Sdk.WindowsDesktop, or attempting to use UseWpf with Microsoft.NET.Sdk. In fact, the latter problem seems anecdotally more prevalent.

Maybe we need two changes -

  • A suppressible warning in Microsoft.NET.Sdk.WindowsDesktop indicating when neither UseWpf nor UseWinForms is set to true
  • A suppressible warning in Microsoft.NET.Sdk indicating when UseWpf or UseWinForms is being set to true despite the lack of support for those flags (i.e., not having imported Microsoft.NET.Sdk.WindowsDesktop targets).

@nguerrera, thoughts?

@nguerrera
Copy link

@dsplaisted @davkean are discussing related things. I'll defer to them on this.

@heaths
Copy link
Author

heaths commented May 28, 2019

I was away on vacation, but like the idea of at least warning when using the Microsoft.NET.Sdk.WindowsDesktop without either UseWPF or UseWinForms - neither of which I even knew about until I created a new project from scratch in VS and compared with my migrated projects. While documentation on these would be helpful, people generally don't read docs until there's a problem so a warning would at least provide some reason to dig further.

@miloush
Copy link
Contributor

miloush commented May 28, 2019

Just out of my curiosity - I have so many console applications referencing WindowsBase/PresentationCore/PresentationFramework assemblies that I have a special project template for those. What is the expected set-up in .NET Core for equivalent apps? Does UseWPF have any effect if there are no XAML files in the project?

Second, as @weltkante noted, in .NET Framework, you can add User Control (WPF) item to pretty much any type of project including WinForms, and I remember being told that was on purpose for control developers. Are both UseWPF and UseWinForms expected in this case?

@vatsan-madhavan
Copy link
Member

I have so many console applications referencing WindowsBase/PresentationCore/PresentationFramework assemblies

What’s the distinction between these and a WPF app ? Is it just that these are marked as subsystem=console?

@miloush
Copy link
Contributor

miloush commented May 29, 2019

Is it just that these are marked as subsystem=console?

Yeah, that sounds about right; and from compiler point of view, Main method would typically be specified and no App class generated. (at runtime, there would usually be no windows/dispatcher running)

This wasn't meant to be a trick question, it's just that I can't really find any information on what UseWPF actually does. This issue suggests that treating .xaml files as Page by default is one of its tasks.

@davidwengier
Copy link
Member

@vatsan-madhavan The project system will stop adding a <None Exclude="Page1.xaml" /> when pages are added to projects if the exclude from None is changed to use a filespec instead of @(Page). Are there any downsides to making that change?

@davkean
Copy link
Member

davkean commented Jun 5, 2019

@dsplaisted Is there a reason that Page items are not excluded from None by default like the other item types?

@vatsan-madhavan
Copy link
Member

@davidwengier There is a filespec for inclusion into Page - it is *.xaml. Or do you mean something else by filespec ?

@dsplaisted
Copy link
Member

@vatsan-madhavan The question is why is it using <None Remove="@(Page"/> instead of <None Remove="**/*.xaml" />? Visual Studio would work better with the second option, which is also what the base SDK does for other item types.

@vatsan-madhavan
Copy link
Member

We remove from None what we add into Page and ApplicationDefinition. Why isn't removing Page and ApplicationDefinition sufficient?

We don't remove **\*.xaml because that would be over-broad and would include $(DefaultItemExcludes);$(DefaultExcludesInProjectFolder) - files that we didn't include into Page or ApplicationDefinition.

@dsplaisted
Copy link
Member

The problem is that VS needs to know how to modify the project to include a Page item, or change the build action of an item, etc. It can understand basic globs, but not arbitrarily complex expressions for items (such <None Remove="@(Page)" />).

So today, if you add a new Page to a project, VS will add the following to your project:

  <ItemGroup>
    <None Remove="Page1.xaml" />
  </ItemGroup>

@davidwengier
Copy link
Member

Is being over-broad here really a problem? For example App.xaml or Application.xaml shouldn't be in None as they're already included in ApplicationDefinition so being over-broad in that case doesn't matter. Are there any .xaml files that you would expect should be in None?

@vatsan-madhavan
Copy link
Member

There is no clear benefit for WPF builds in modifying None and trying to tweak things imperfectly - we could just stop add/removing to None and leaving it all upto VS to decide how to deal with it. Would that be preferable overall (i.e., if we don't do things like <None Remove="@(Page)" /> etc. ) ?

@davkean
Copy link
Member

davkean commented Jun 5, 2019

We're asking the reverse; we want the same model we have with all other built-in items as per @dsplaisted: #685 (comment)

@davidwengier
Copy link
Member

@vatsan-madhavan Changing the glob to <None Remove="**/*.xaml" /> will fix the issue in VS, and make WPF match the existing default items globs that have been there since v1. In an ideal world we will also solve the VS problem directly, but unfortunately that's not feasible in the near term, so its better to change the globs and have a better experience for users.

@vatsan-madhavan
Copy link
Member

OK, we can add that, only when we handle Page items .

There will be situations (rare ones, I think) when we choose not to handle Page items at all - for e.g., when an app includes Microsoft.NET.Sdk.WindowsDesktop with *.xaml files, but targets netcoreapp2.2 - in which case we will skip over all Page related logic, including the None exclusion. (These kinds of situations will also result in a warning). This seem ok to you?

@davidwengier
Copy link
Member

Yes, that makes sense to me.

vatsan-madhavan added a commit that referenced this issue Jun 20, 2019
- #685: Remove **/*.xaml from None
- #327: Cannot multi-target netcoreap < 3.0 and use WindowsDesktop SDK unconditionally.
- #867: Show error when neither UseWpf nor UseWindowsForms is set to true
- #746: [Epic] Support WPF and WinForms specific FrameworkReferences a Profiles

Also cleans up the way in which we import Microsoft.WinFX.targets - UseLegacyPresentationBuildTasks has been broken for some time now and unusable.
@vatsan-madhavan vatsan-madhavan modified the milestones: 3.0, Preview Jun 20, 2019
vatsan-madhavan added a commit that referenced this issue Jun 21, 2019
- #685: Remove **/*.xaml from None
- #327: Cannot multi-target netcoreap < 3.0 and use WindowsDesktop SDK unconditionally.
- #867: Show error when neither UseWpf nor UseWindowsForms is set to true
- #746: [Epic] Support WPF and WinForms specific FrameworkReferences a Profiles

Also cleans up the way in which we import Microsoft.WinFX.targets - UseLegacyPresentationBuildTasks has been broken for some time now and unusable.
vatsan-madhavan added a commit that referenced this issue Jun 24, 2019
…eWinforms and multi-targeting experience (#1027)

* Fixes the following:

- #685: Remove **/*.xaml from None
- #327: Cannot multi-target netcoreap < 3.0 and use WindowsDesktop SDK unconditionally.
- #867: Show error when neither UseWpf nor UseWindowsForms is set to true
- #746: [Epic] Support WPF and WinForms specific FrameworkReferences a Profiles

Also cleans up the way in which we import Microsoft.WinFX.targets - UseLegacyPresentationBuildTasks has been broken for some time now and unusable.
@vatsan-madhavan
Copy link
Member

@davkean, @davidwengier - just merged this fix.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet