microsoft / XamlBehaviors

This is the official home for UWP XAML Behaviors on GitHub.
MIT License
697 stars 112 forks source link

EventTriggerBehavior does not work in release mode on WinUI:NavView ItemInvoked event #142

Closed sibille closed 5 years ago

sibille commented 5 years ago

We found an issue using the EventTriggerBehavior on the ItemInvoked event of Navigationview from WinUI. It works fine in Debug Mode, but doesn't work when compiled in Release.

Using the EventTriggerBehavior on the Loaded event works fine in debug and release though.

Could you have a look to see what is causing this?

Repro project: BehaviorReleaseBug.zip

pedrolamas commented 5 years ago

So I've taken a look into this and I can confirm this issue, which as expected, is related with .NET Native compilation removing a bunch of metadata necessary for reflection (which the EventTriggerBehavior uses)

The Loaded event is not using reflection hence why you see it the behavior working correctly in Release mode with it...

The solution is to add directives to the .rd.xml file in your project to ensure that .NET Native keeps the necessary metadata for the ItemInvoked event to be handled.

However, I've yet to find the correct directives for this specific scenario... I'll keep trying and update once I find them.

sibille commented 5 years ago

Thanks Pedro!! Would it possible + does it make sense to configure this on the control itself to make it work out of the box?

jevansaks commented 5 years ago

Can the WinUI nuget package even include the .rd.xml so that it influences the consuming app's .NET Native behavior? I don't know how to do that.

Do you have to do this .rd.xml thing for system events too?

pedrolamas commented 5 years ago

~Unfortunately no, as there's no way to know what properties a developer is going to require and we can't just include all (that would defeat the purpose of the .NET Native optimizations)~

jevansaks commented 5 years ago

So is the recommendation that the app developer be aware of this and include the metadata manually? Why isn't this a problem for Windows platform types? (or is it?)

pedrolamas commented 5 years ago

Can the WinUI nuget package even include the .rd.xml so that it influences the consuming app's .NET Native behavior? I don't know how to do that.

Sorry, I don't think I replied correctly to this (that's what I get for doing it on the phone while in public transport)... If you are using the behavior directly for a single known event, then yes, you can change your rd.xml (you will find it inside the Properties node in your project)

image

So if you will always use this behavior for the NavView.ItemInvoked event, you should be able to add specific runtime directives to your rd.xml file that will ensure this event metadata is kept through .NET Native compilation.

If you don't know what event you will require, then it's up to the consumer project to ensure that it has the required runtime directives.

More information on Runtime Directives can be found here.

My personal experience is that this file is a complete nightmate to work on, as you will need to continuously build your app with .NET Native to test the directives and see if it fixed the problem, and that is a really time consuming task...

pedrolamas commented 5 years ago

After a lot of trials, I finally found the correct runtime directives that work for your sample code! 😀

On the sample project you sent, expand Properties (as shown on the picture on my previous comment) double click the Default.rd.xml file to open, and then add the following just under the "Add your application specific runtime directives here." comment:

<Type Name="Windows.Foundation.TypedEventHandler{Microsoft.UI.Xaml.Controls.NavigationView,Microsoft.UI.Xaml.Controls.NavigationViewItemInvokedEventArgs}" MarshalObject="Public" />

<Type Name="Microsoft.UI.Xaml.Controls.NavigationView">
  <Event Name="ItemInvoked" Dynamic="Required"/>
</Type>

The first one ensures that the event type metadata is maintained for marshaling/casting purposes, the second is to include metadata for the said event.

Bottom line, this isn't a bug or something we can do anything from our end; if you know what you will need, you can add it to your library rd.xml file, but if you don't, it's up to the consumer to specify in the final rd.xml file!

sibille commented 5 years ago

Thanks Pedro!!

Noemata commented 5 years ago

This should be re-opened, and directed to the .Net Native compiler group. This compiler bug will affect lots of code.

pedrolamas commented 5 years ago

This is not a bug, this is actually a well-documented compiler optimization! I sincerely doubt that this will get changed anytime soon (if ever)!

Noemata commented 5 years ago

I respectfully disagree. If UWP code works in a Debug Build and does not work in a Release Build, it's a .Net Native compiler bug, unless the Debug code is specifically intended to work only in Debug, full stop. Any other characterization is an excuse or a deflection or a delusional perspective. I put it in such harsh terms because end users suffering through .Net Compiler crashes are quite likely to have an even harsher view and rightfully so.

raviagarwalvm commented 4 years ago

Adding exception in the 1809 and 1903 version fixed the issue. But on 1803, I am not getting any exception but the ItemInvoked is not working. Am I doing anything obvious wrong ?

michael-hawker commented 4 years ago

FYI @MattWhilden