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

WUX/MUX prototype #1418

Closed
wants to merge 9 commits into from
Closed

Conversation

jkoritzinsky
Copy link
Member

Implement prototype WUX/MUX support for both consumption and authoring.

Tests are still TODO.

The remaining TODO-WuxMux items will provide a better UX or perf but are not required for basic functionality.

Copy link
Member

@Sergio0694 Sergio0694 left a comment

Choose a reason for hiding this comment

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

Just glanced it so far and left some random notes as I read through the code. Looking really good, and it's cool how the changes are pretty targeted and most code is the same 🚀

return UiXamlMode.MicrosoftUiXaml; // We default to MUX for back-compat with existing projects.
}

internal static UiXamlMode UiXamlModeSetting { get; } = GetUIXamlModeSetting();
Copy link
Member

Choose a reason for hiding this comment

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

Not needed for the initial prototype, but we might want to have the final version also include an ILLink.Substitutions.xml that can swap the body of the property getter with the hardcoded value based on the runtime switch configuration from MSBuild. I mean, it's simple enough but it's nice to have 🙂

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, that is a good idea! I'll see if I can figure it out (since it's not a bool I'll need to check the right way to do it)

@jkoritzinsky
Copy link
Member Author

I added some tests here to validate the experience.

@dongle-the-gadget
Copy link
Contributor

The cswinrt.exe generator had an issue that if it had to generate some classes in a particular namespace, if that namespace is present under strings\additions it would always pull those files in. This means that when we try to build a WUX only projection with Windows.UI.Colors, it would also pull in Windows.UI.Color, conflicting with the SDK-provided projection.

Is there a fix underway for this?

@jkoritzinsky
Copy link
Member Author

@dongle-the-gadget that's by design. The idea is that the WUX projection would only project the WUX namespaces and exclude any namespaces that are present in the SDK-provided projection and it would reference the SDK-provided projection of the rest of the OS-built-in Windows SDK.

@jkoritzinsky
Copy link
Member Author

jkoritzinsky commented Dec 19, 2023

Okay looks like I might need some help with getting the INPC test to run in GoogleTest (I need to start-up the WUX engine to get it to work). @Sergio0694 I might need your help on this nevermind, XAML Islands worked.

… INPC and types that need the XAML engine initialized on the thread.
@Sergio0694
Copy link
Member

Ahahah glad to hear you found a workaround! 😄

Also, just a note, as this will get merged after #1395, it will also eventually need to get the same tweaks so that in Release x64 mode the authoring test is published with NAOT and the manifest is skipped (so we can also verify that things work fine without it when there's no native host and the implementation .dll is the same). Just worth keeping in mind in case any of the things done here so far could potentially cause any issues with that (hopefully not, but figured I'd mention that just in case) 🙂

@jkoritzinsky
Copy link
Member Author

We'll still need a manifest here as it needs to specify a new-enough version of Windows to enable XAML Islands.

@jkoritzinsky
Copy link
Member Author

Marking this as ready for review as I have some tests now that validate activation and consumption scenarios (events end up validating both).

@jkoritzinsky jkoritzinsky marked this pull request as ready for review December 20, 2023 00:19
@Sergio0694
Copy link
Member

"We'll still need a manifest here as it needs to specify a new-enough version of Windows to enable XAML Islands."

Oh, I mean that's fine, but can we skip all the definitions of the activatabke classes though (perhaps with a different version of the manifest that's only used for the NAOT test)? Just to make sure that we're resting that too for consistency with the WASDK stuff.

Also... Not entirely sure why the CI isn't running 🤔

@jkoritzinsky
Copy link
Member Author

It's probably because I'm working from a fork instead of upstream. I'll push to a branch in microsoft/cswinrt and try again. I forgot about that aspect of this repo.

@jkoritzinsky jkoritzinsky mentioned this pull request Dec 20, 2023
@jkoritzinsky
Copy link
Member Author

Closing in favor of #1421 that will get CI

@jkoritzinsky jkoritzinsky deleted the wux-mux branch December 20, 2023 00:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants