Move Xaml.VisualDiagnostics to Maui.Core #4333
Conversation
# Conflicts: # src/Controls/src/Core/Xaml/Diagnostics/BindingErrorEventArgs.cs # src/Controls/src/Core/Xaml/Diagnostics/XamlSourceInfo.cs # src/Core/src/VisualDiagnostics/VisualDiagnostics.cs # src/Core/src/VisualDiagnostics/VisualTreeChangeEventArgs.cs
|
can we wrap all that code in some kind of #if ENABLE_TOOLING or some or some [Conditional] ? The goal is still to be able to provide 2 versions of Maui, one used when you build for debug (and has tooling enabled) and the other one having all those calls stripped, for perf, when you build for distribution |
|
@StephaneDelcroix I think we should talk about that, but, IMO, I think it's outside the scope of this PR. Thinking about multiple DLLs and packages for tooling would make this much more complex and open up a can of worms that would probably be better set for a fully thought out plan. This PR is more limited in scope. There are implications moving VisualDiagnostics to Core (Mostly being that more people who use Core only can access it, opening them up to Hot Reload tooling) but I wanted to make sure everything in Controls still functioned the same as it does today. |
|
I agree, and added a tracking issue for the work to have this stuff removed in release builds: #4409 We can schedule that work post-RC release. I think if everyone's happy with this PR and we can get it green we should merge it ASAP. |
|
@drasticactions does this require a change from UI Tools to consume? Should we add a TypeForwardedTo attribute for these moving types? |
There was a problem hiding this comment.
Looks mostly OK, but this should all be in a separate namespace Microsoft.Maui.VisualDiagnostics as this is a tooling feature and not meant to be used by apps at all. And, as Stephane says, this might only be available in a debug build. The API might exist, but for release we might decide to have no-ops and stuff.
| namespace Microsoft.Maui.Controls.Xaml.Diagnostics | ||
| namespace Microsoft.Maui | ||
| { | ||
| static class DebuggerHelper | ||
| public static class DebuggerHelper |
There was a problem hiding this comment.
Can we keep this internal? At the very least, this must be in a separate namespace that no-one uses it. This is purely for our testing.
There was a problem hiding this comment.
I'm happy to leave it internal, I just couldn't figure out how to make it work, since I think that would mean giving Internal access to Maui.Controls for Maui.Core, and I'm not sure if that's wanted?
If there's a way to do it and keep it working, I'm 100% happy to do it.
There was a problem hiding this comment.
I think it is fine to make it internal for now - especially since it is just for testing? The Controls library itself doesn't actually use this. Or it shouldn't.
There was a problem hiding this comment.
Ah, I'm dumb. I didn't notice that Internals were already exposed for Maui.Controls. I needed to add Controls.Xaml to InternalsVisibleTo and now it all works.
I'm cool with moving the types, but the way it's set up right now would result in |
|
I suppose the namespace is fine to be in the root. |
|
it's better to keep the root namespace for publicly accessible types for end-users. Tooling, or Design would be an appropriate name for the ns too |
drasticactions
changed the base branch from
main
to
release/6.0.2xx-preview13
The current VisualDiagnostics stack is used for XAML Hot Reload, for holding the "XamlSourceInfo" and firing Visual Tree changes. While this works for the general MAUI use case, it won't work for anyone who uses MAUI.Core and not MAUI.Controls.
As these APIs are not XAML specific and can be used in a more general case, we can move these static classes down to Maui.Core, which should enable anyone to fire Visual Tree changes and register source info, which should let more MAUI implementations to get XET support.