-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Turn Visual and Binding diags on EnableDiagnostics #29836
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enables Visual and Binding diagnostics by switching the diagnostic flag from DebuggerHelper to a new runtime feature flag. Additionally, it removes the DebuggerHelper dependency and updates tests and build targets accordingly.
- Updated VisualDiagnostics.IsEnabled to use RuntimeFeature.EnableDiagnostics.
- Removed DebuggerHelper and exposed previously internal RuntimeFeature properties as public.
- Updated tests and build targets to use the new diagnostics flag.
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/Core/src/VisualDiagnostics/VisualDiagnostics.cs | Updated IsEnabled condition to rely on RuntimeFeature.EnableDiagnostics rather than debugger status. |
| src/Core/src/DebuggerHelper.cs | Removed obsolete DebuggerHelper functionality. |
| src/Core/src/RuntimeFeature.cs | Exposed several runtime feature properties as public and introduced the EnableDiagnostics property. |
| src/Controls/tests/... | Updated tests to correctly capture and restore the EnableDiagnostics flag state. |
| src/Controls/src/Core/Xaml/Diagnostics/BindingDiagnostics.cs | Added guard conditions based on RuntimeFeature.EnableDiagnostics. |
| src/Controls/src/Build.Tasks/nuget/buildTransitive/netstandard2.0/Microsoft.Maui.Controls.targets | Added a configuration option for EnableDiagnostics. |
Comments suppressed due to low confidence (1)
src/Core/src/RuntimeFeature.cs:31
- Changing these properties from internal to public could represent a breaking change for consumers; please confirm that this change is intentional and properly communicated.
public static bool IsIVisualAssemblyScanningEnabled =>
d672ef2 to
8c57338
Compare
jonathanpeppers
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a doc here, we've been listing each new feature switch:
...ontrols/src/Build.Tasks/nuget/buildTransitive/netstandard2.0/Microsoft.Maui.Controls.targets
Show resolved
Hide resolved
|
I locally copied the NuGet packages from your PR build: And the XAML Binding Failures window doesn't show binding failure anymore. The repro is:
|
|
If we made a new |
Yes, once I set |
let's go for added granularity |
3403960 to
013b614
Compare
PureWeen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Failing Unit Tests
013b614 to
549a0e9
Compare
should be fixed |
2b415f3 to
443b57e
Compare
|
failures looks unrelated |
more diagnostics to be plugged on this later - fixes #29809
rmarinho
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to me consistent some small changes
LGTM
443b57e to
b426224
Compare
| | EnableDiagnostics | Microsoft.Maui.RuntimeFeature.EnableDiagnostics | Enables diagnostic for the running app | | ||
| | EnableMauiDiagnostics | Microsoft.Maui.RuntimeFeature.EnableMauiDiagnostics | Enables MAUI specific diagnostics, like VisualDiagnostics and BindingDiagnostics. Defaults to EnableDiagnostics | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious why we're introducing a new feature switch just for the MAUI part?
Additionally - diagnostics in WASM relates to tracing, profiling and debugger. It's a runtime thing. Here it guards functionality which does logging through DI ILogger and fires events which can be subscribed to by anything in the app.
I understand that semantically these are similar, just want to make sure that we're aware of these differences and it's intentional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we don't need a second feature switch, but just a second MSBuild property?
$(EnableDiagnostics) adds the Mono diagnostic component in both iOS/Android workloads. But you might want to set $(EnableMauiDiagnostics) in a build without the Mono component but with the extra MAUI diagnostic information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we went back and forth with one, or 2, flags. I like having a second one for added granularity that defaults to the first one...
Description of Change
Turn Visual and Binding diags on EnableDiagnostics
more diagnostics to be plugged on this later
Issues Fixed