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

Add globbing support for WinUI #3245

Open
stevenbrix opened this issue Jul 13, 2020 · 17 comments
Open

Add globbing support for WinUI #3245

stevenbrix opened this issue Jul 13, 2020 · 17 comments
Labels
Question General question, not a problem in source code or documentation (yet)
Milestone

Comments

@stevenbrix
Copy link
Contributor

The WinUI team needs support for globbing support so that Visual Studio will not edit the .csproj whenever you add a .xaml file to the project. See this bug as a reference: microsoft/microsoft-ui-xaml#2498

In following the same pattern as WPF, we can add a UseWinUI property that will pull in the Microsoft.NET.WindowsDesktop.Sdk and enable the globbing support.

If both UseWinUI and UseWpf are true, then the SDK can assume all xaml files are for WPF. The WinUI nuget will respect the use of UseWpf and not invoke the winui xaml compiler if that is the case. If only UseWinUI is true, then the SDK should not invoke PBT. Below is a table describing the different combinations and what should happen:

UseWpf UseWinUI Xaml Compiler invoked
True True PBT
True False PBT
False True WinUI Compiler
False False neither
@vatsan-madhavan
Copy link
Member

Should all globbing support live in the base SDK instead of living in the WindowsDesktop SDK? This may not work well if WPF's targets are deciding which markup compiler to invoke, but perhaps WPF's targets should not be deciding that anyway. IIRC WPF's targets are making local decisions for itself, based on UseWpf==true. IMHO other compilers should do so based on their own rules. Why do these rules have to be all coordinated in the same place anyway?

In .NET Framework/old-style projects, when there was no use of UseWpf, UseWinUI etc., *.xaml files were implicitly inferred to be WPF, UWP etc. These inferences required heuristic judgment. We did away with these heuristics and decided that the projects/developers had to explicitly state the intended XAML dialect being used by means these properties. This in turn disambiguates the XAML targets/compiler etc. Given this design, is there even a need for a central point-of-coordination based on a matrix of these property values? If both UseWpf and UseWinUI are set to true for the same XAML file, then both compilers would be invoked and things won't work out - but that's just a "developer beware" scenario that we can mitigate with warnings from the base SDK.

microsoft/microsoft-ui-xaml#2498 (comment)

We were originally planning on making an MSBuild SDK, but then didn't because we heard that the Microsoft.NET.Sdk.WindowsDesktop was going away. Is this still the plan?

BTW, does WinUI have its own SDK yet? If it does, you can set up globbing etc. in that SDK's props/targets. OTOH, if the plan is never to have an SDK (for e.g., perhaps because WinUI is going to ship as a first-class component in .NET 5, and taking a clue from WindowsDesktop SDK's merger with the base SDK, has now decided not to invent a separate SDK for itself), then a better place to make these changes would be base Microsoft.NET.SDK.

BTW, should Avalonia be rationalized here as well? AvaloniaUI/Avalonia#4102.

Related: microsoft/microsoft-ui-xaml#1438
/cc @grokys, @dsplaisted , @jkoritzinsky

@stevenbrix
Copy link
Contributor Author

We aren't planning on having an MSBuild SDK.

I don't know the timeframe, or if WinUI will be a first-class component of .NET. Definitely not in the .NET5 timeframe, but maybe for .NET6 when we can be an optional component. I think providing an error like you suggest for having UseWinUI and UseWpf both set in the same project is a good idea. If a WPF project wants to pick up WinUI and use Xaml Islands, it should just add a nuget reference to WinUI, and not set UseWinUI. If it wants to create custom controls, it then needs to have a separate WinUI Class Library project that has UseWinUI set and reference that from the WPF app.

As far as I'm concerned, Avalonia is welcome to piggy back on this as well.

@dsplaisted
Copy link
Member

I think the targets for the different UI frameworks will need to coordinate at some level to avoid stepping on each other's toes.

Do you think there will eventually be XAML island scenarios where you will want to have both WPF and WinUI xaml files in the same project? If so, then eventually I think you would want to be able to invoke both compilers, and determine which xaml files to pass to which compilers via a combination of convention and item metadata. For example, for a project that uses both, if the default is to interpret xaml files as WPF Pages, then you could have the following to select a file to be built with the WinUI compiler:

<ItemGroup>
    <Page Update="WinUIPage.xaml" XamlCompiler="WinUI" />
</ItemGroup>

It might have been a better idea if early on WinUI switched to a different file extension (ie .winui). That ship has probably already sailed though.

@stevenbrix
Copy link
Contributor Author

Do you think there will eventually be XAML island scenarios where you will want to have both WPF and WinUI xaml files in the same project?

@marb2000 and @MikeHillberg are better equipped to answer that.

@MikeHillberg
Copy link

It's a good scenario, but it's not in plan for WinUI3, which is the current focus.

@vatsan-madhavan
Copy link
Member

we can add a UseWinUI property that will pull in the Microsoft.NET.WindowsDesktop.Sdk and enable the globbing support.

This is what's really going on:

<ItemGroup Condition=" '$(_EnableWindowsDesktopGlobbing)' == 'true' ">
<ApplicationDefinition Include="App.xaml"
Condition="'$(EnableDefaultApplicationDefinition)' != 'false' And Exists('$(MSBuildProjectDirectory)/App.xaml') And '$(MSBuildProjectExtension)' == '.csproj'">
<Generator>MSBuild:Compile</Generator>
</ApplicationDefinition>
<ApplicationDefinition Include="Application.xaml"
Condition="'$(EnableDefaultApplicationDefinition)' != 'false' And Exists('$(MSBuildProjectDirectory)/Application.xaml') And '$(MSBuildProjectExtension)' == '.vbproj'">
<Generator>MSBuild:Compile</Generator>
</ApplicationDefinition>
<Page Include="**/*.xaml"
Exclude="$(DefaultItemExcludes);$(DefaultExcludesInProjectFolder);@(ApplicationDefinition)"
Condition="'$(EnableDefaultPageItems)' != 'false'" >
<Generator>MSBuild:Compile</Generator>
</Page>
<!--
See https://github.com/dotnet/wpf/issues/685
Visual Studio would prefer that we remove **/*.xaml instead of
being more precise.
<None Remove="@(Page)"
Condition="'$(EnableDefaultPageItems)' != 'false'" />
<None Remove="@(ApplicationDefinition)"
Condition="'$(EnableDefaultApplicationDefinition)' != 'false'" />
-->
<None Remove="**/*.xaml"
Condition="'$(EnableDefaultApplicationDefinition)' != 'false' And '$(EnableDefaultPageItems)' != 'false'" />
</ItemGroup>

<!--
In the WindowsDesktop .props, we globbed all .xaml files as Page items. If any of those files are included
as Resource, Content then remove them from the Page items.
-->
<Page Remove="@(Resource);@(Content)"
Condition="'$(EnableDefaultPageItems)' != 'false'" />
<!--
ApplicationDefinition must also be removed from Page. Otherwise, PresentationBuildTasks will include the
generated code for the ApplicationDefinition twice. We cannot do this in all cases in the props file as
ApplicationDefinition isn't available at that point in the evaluation when EnableDefaultApplicationDefinition
is false. We unconditionally remove it here as ApplicationDefinition is considered a special singleton Item
in WPF.
-->
<Page Remove="@(ApplicationDefinition)" />
<!--
The WindowsDesktop props file removes "**/*.xaml" from None if both EnableDefaultApplicationDefinition and
EnableDefaultPageItems are true. This is done so that we remove any implicitly globbed XAML files from None
in support of Visual Studio.
In the case where one or both of these properties are not true, the WindowsDesktop props file does not remove
any XAML files from None. Under those conditions, removing None from Page here will remove explicitly specified
pages causing compilation errors. We match the condition from the WindowsDesktop props file here so that we only
remove None from Page when we are guaranteed to not need those items.
-->
<Page Remove="@(None)"
Condition="'$(EnableDefaultApplicationDefinition)' != 'false' and '$(EnableDefaultPageItems)' != 'false'" />
</ItemGroup>
<!-- Generate error if there are duplicate page items. The task comes from the .NET SDK, and this target follows
the pattern in the CheckForDuplicateItems task, where the .NET SDK checks for duplicate items for the item
types it knows about. -->
<Target Name="CheckForDuplicatePageItems"
BeforeTargets="_CheckForInvalidConfigurationAndPlatform;CoreCompile"
DependsOnTargets="CheckForDuplicateItems"
Condition="('$(UseWPF)' == 'true') And ('$(_TargetFrameworkVersionValue)' != '$(_UndefinedTargetFrameworkVersion)') And ('$(_TargetFrameworkVersionValue)' >= '$(_WindowsDesktopSdkTargetFrameworkVersionFloor)')">
<CheckForDuplicateItems
Items="@(Page)"
ItemName="Page"
DefaultItemsEnabled="$(EnableDefaultItems)"
DefaultItemsOfThisTypeEnabled="$(EnableDefaultPageItems)"
PropertyNameToDisableDefaultItems="EnableDefaultPageItems"
MoreInformationLink="$(DefaultItemsMoreInformationLink)"
ContinueOnError="$(ContinueOnError)">
<Output TaskParameter="DeduplicatedItems" ItemName="DeduplicatedPageItems" />
</CheckForDuplicateItems>
<ItemGroup Condition="'$(DesignTimeBuild)' == 'true' And '@(DeduplicatedPageItems)' != ''">
<Page Remove="@(Page)" />
<Page Include="@(DeduplicatedPageItems)" />
</ItemGroup>
</Target>
<!--
This warning can be suppressed by setting $(MSBuildWarningsAsMessages) property, like this:
<PropertyGroup>
<MSBuildWarningsAsMessages>$(MSBuildWarningsAsMessages);NETSDK1106</MSBuildWarningsAsMessages>
</PropertyGroup>
-->
<Target Name="_WindowsDesktopFrameworkRequiresUseWpfOrUseWindowsForms"
BeforeTargets="_CheckForInvalidConfigurationAndPlatform"
Condition="('$(_TargetFrameworkVersionValue)' != '$(_UndefinedTargetFrameworkVersion)') And ('$(_TargetFrameworkVersionValue)' >= '$(_WindowsDesktopSdkTargetFrameworkVersionFloor)')">
<NetSdkWarning ResourceName="WindowsDesktopFrameworkRequiresUseWpfOrUseWindowsForms"
Condition="'$(UseWpf)' != 'true' And '$(UseWindowsForms)' != 'true'"/>
</Target>

Instead of enlightening WindowsDesktop SDK with more information about specific frameworks, why don't we consider making it less reliant on such knowledge in .NET 5 (including WPF) ? 😜 Bear with me - let me explain.

Given that one-project will always map to one XAML dialect (like WPF, WinUI etc.), we don't need to annotate XAML Item's with its dialect (like XamlCompiler=WinUI etc.). In fact, we can take away the $(UseWpf)==true check from CheckForDuplicateItems (_WindowsDesktopFrameworkRequiresUseWpfOrUseWindowsForms probably needs a fixup as well, although this might require some thinking).

With these changes, XAML files would always be globbed, and checked for duplicates etc. - and in NET.Sdk (you won't need NET.Sdk.WindowsDesktop). There would be no need to teach any pat of the framework anything special about WinUI etc. A WinUI project should just get the right XAML globbing inherited from WindowsDesktop's logic and it can just do its own compilation.

Do I have it right so far @dsplaisted ?

@stevenbrix
Copy link
Contributor Author

stevenbrix commented Jul 14, 2020

Our requirements are fairly simple, and I'm all for this being in the right place and done properly. If UseWinUI isn't needed for the globbing support then that's great and we probably don't need it then :)

Just brainstorming...one convention we could adopt for WinUI and WPF in the same project is for WinUI pages have the .winui.xaml convention. Then you could have something like this to gather the appropriate pages to pass to the correct compiler:

<ItemGroup>
  *<WinUIPages Include="@(Page->EndsWith('.winui.xaml'))"/>
  <WpfPages Include="@(Page)" Exclude="@(WinUIPages)"/>
</ItemGroup>

*ignore the fact that this isn't currently possible in MSBuild but it totally should be, you have to do something ugly like this:

<WinUIPages Include="@(Page)" Condition="$([System.String]::new('%(Identity)').EndsWith('.winui.xaml'))"/>

The VS Wizard would need to know the WinUI Item is being added to a WPF project so that it adds the appropriate file name, but this would allow the .csproj to stay clean and xaml files to still have the xaml extension

@jkoritzinsky
Copy link
Member

From the Avalonia perspective, I really like the XamlCompiler metadata if VS could use that to determine which XAML editor to load. That would solve one of the big issues Avalonia has when trying to work in VS.

@vatsan-madhavan
Copy link
Member

@stevenbrix if the same project is not going to have multiple XAML dialects (WinUI + WPF for e.g.), then where is the need to exclude WinUI XAML pages during globbing? A project can simply assume that all XAML pages belong to its own dialect (i.e, WPF project can assume everything is WPF, WinUI can assume that everything is WinUI etc.).

It looks like globbing for XAML needs to work as a first-class construct. Beyond that, it's not clear whether any dialect-specific logic is needed at all.

@jkoritzinsky Item-metadata hints to identify and load a different designer - iff one is registered - maybe interesting. /cc @diverdan92

@stevenbrix
Copy link
Contributor Author

stevenbrix commented Jul 14, 2020

@vatsan-madhavan and @dsplaisted so should this issue move to the https://github.com/dotnet/sdk repo then?

@dsplaisted
Copy link
Member

If we are going to eventually support multiple XAML dialects in the same project, then we should try to make design decisions today with that in mind. For example, if eventually WinUI XAML files would use a .winui.xaml extension, then ideally we would already require or encourage that today.

I can see the value of moving the XAML globs down into the .NET SDK, especially since WinUI doesn't currently have their own targets outside of a NuGet package. However, are the globs for WPF and WinUI actually the same? The WPF globs are not all that simple (see the snippets that @vatsan-madhavan linked).

If we do move the globs into the .NET SDK, note that they would still only be active if UseWPF or UseWinUI was set to true. We don't want to suddenly change how we handle projects that don't use either UI framework but happen to have a .xaml file in the project folder. This also means that WinUI projects should explicitly set UseWinUI to true in the project file. If it were just set in targets from the WinUI NuGet package, then you would have different globs depending on whether the NuGet package was restored, which isn't a great experience in VS.

@vatsan-madhavan
Copy link
Member

@dsplaisted if we move XAML globbing to .NET.SDK (which is an idea I like), then we should make it opt-in generically using EnableXamlGlobbing=true or something like that, and then UseWpf, UseWinUI etc. should turn it on. My larger point is that we should not design exclusively for WPF/WinUI - we should make allowances for other dialects like Avalonia or if MAUI wants to reuse/enable globbing etc.

@stevenbrix
Copy link
Contributor Author

I would also add that it shouldn't do anything with ApplicationDefinition - it should be bare minimal globbing of .xaml files so that VS is happy. I would imagine that most of WPFs targets stay the same, except instead of doing the globbing, they referenced the pre-globbed xaml items from the .NET SDK

@ryalanms
Copy link
Member

Can this issue be moved to https://github.com/dotnet/sdk? Thanks.

@stevenbrix
Copy link
Contributor Author

@ryalanms I was initially thinking this issue could just move, but we'll probably need a tracking issue to update the Windows SDK to w/e is done, so it makes sense to keep this around for that.

@dsplaisted while I agree with your general sentiment, I just think that if we're able to make the globbing in the .NET SDK simple and generic enough, then we won't be putting ourselves into a corner. We don't support WinUI files in a WPF project, so if we decide they should adopt the .winui.xaml extension, then we can do that once we decide to. WinUI files don't need this extension in a project that is purely WinUI - it would only be if you want to integrate. Another option is the special metadata that says which framework they are for. Either way, I think we'll be ok for when we want to make future improvements. Let me know if you feel differently, you have more experience in this area than I do so I could be missing something.

I've filed this issue in the sdk repo: dotnet/sdk#12520

@ryalanms ryalanms added the Question General question, not a problem in source code or documentation (yet) label Jul 16, 2020
@jtbrower
Copy link

jtbrower commented Aug 2, 2020

I have been globbing in WPF NetCore for so long now that I automatically started doing it in my Directory.Build.targets when I began writing WinUI code a few weeks ago without really knowing that it wasn't supported in WinUI. One sporadic issue that I see happen sometimes is that Visual Studio decides to add a "None Remove" to some of my files without re-adding them back in as a Page (I would rather it not touch them at all like you stated). Since it removes the file it goes missing from my solution and it causes quite the confusion when it happens.

@stevenbrix

I would also add that it shouldn't do anything with ApplicationDefinition - it should be bare minimal globbing of .xaml files so that VS is happy. I would imagine that most of WPFs targets stay the same, except instead of doing the globbing, they referenced the pre-globbed xaml items from the .NET SDK

I caused myself quite a headache today regarding ApplicationDefinition though. I renamed App.xaml, didn't assign a new ApplicationDefinition and was bouncing my head off of a null reference exception that is hit in the Microsoft.UI.Xaml.Markup.Compiler.interop.targets MarkupCompilePass2 target. There is no specific error message, just the "object reference not set to an instance of an object". This happened to me in the middle of a renaming refactor so I assumed the null ref was caused by some meta file, left-over binaries, manifest, something, anything..... until finally I gave up and reverted all of the changes to my code so I could start narrowing it down. Needless to say, I really punished myself just to rename that file for no good reason (My assembly name ended in App so rather than be forced to rename namespaces, the assembly and the folder containing the code, I figured it would be faster for my demo code to just rename App.xaml. Dear lord was I wrong).

I really do not miss the days of Visual Studio messing with my project files and look forward to when it stops touching the ones for WinUI. The power of globbing and SDK style projects are one of my top 3 favorite things that happened to NetCore.

@tibitoth
Copy link

tibitoth commented Oct 13, 2023

Is there any workaround for now if I want to use both UseWPF and UseWinUI in a project?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Question General question, not a problem in source code or documentation (yet)
Projects
None yet
Development

No branches or pull requests

8 participants