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

feat: Every RoutedEvent should be usable as Attached Event #15274

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

workgroupengineering
Copy link
Contributor

@workgroupengineering workgroupengineering commented Apr 8, 2024

What does the pull request do?

Every RoutedEvent should be usable as Attached Event. If the event name is prefixed with Preview, the handler is registered as Tunnel.

What is the current behavior?

Only RoutedEvent which have Add[EventName]Handler and Remove[EventName]Handler method can be used as Attached Event in Xaml. Tunnel only registrer by code.

What is the updated/expected behavior with this PR?

How was the solution implemented (if it's not obvious)?

Added XamlAstTransformer which checks if there is a static field named [PropertyName]Event whose type is compatible with the RoutedEvent type and generates the code to wire the RoutedEvent to handler.

Checklist

Breaking changes

Obsoletions / Deprecations

Fixed issues

Fixes #9507
Closes #13233

@workgroupengineering workgroupengineering marked this pull request as ready for review April 9, 2024 16:11
}
else
{
//TODO: Throw Exception?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure whether to throw the exception or not

Copy link
Member

Choose a reason for hiding this comment

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

If otherwise we have a runtime error: yes, exception.
If otherwise we have an unintended behavior, but app works: a new XAML warning.

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0047088-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0047144-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0047308-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

instance.Setters.Add(new XamlDirectCallAddHandler(eventField,
targetRef.Type,
xkt.Interactivity.AddHandlerT.MakeGenericMethod(new[] { agrument }),
context.Configuration.TypeSystem.FindType("System.EventHandler`1").MakeGenericType(agrument)
Copy link
Member

Choose a reason for hiding this comment

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

EventHandler should be moved to well known types.

Comment on lines +26 to +27
var eventName = $"{prop.Name}Event";
if (declaringRef.Type.GetAllFields().FirstOrDefault(f => f.IsStatic && f.Name == eventName) is { } eventField)
Copy link
Member

Choose a reason for hiding this comment

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

It has the same problem, as in my EventSetter PR, since event name might not necessary match field name+"event".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should give ourselves a metric. In the transaction period, an analyzer could be created that signals the lack of convection as happens for avalonia properties.

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0047812-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

[Fact]
public void Attached_Event_Tunnel_Routed_Event_Handler()
{
var xaml = @"<Panel xmlns='https://github.com/avaloniaui' PreviewKeyDown='OnKeyDown'><TextBox Name='target'/></Panel>";
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this PR should include "Preview" tunnel handling as a new feature.

Copy link
Member

Choose a reason for hiding this comment

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

See #13628

Copy link
Contributor Author

@workgroupengineering workgroupengineering left a comment

Choose a reason for hiding this comment

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

I have removed the "Preview" feature. Although I'm not convinced.

else
{
context.ReportDiagnostic(new XamlX.XamlDiagnostic(
AvaloniaXamlDiagnosticCodes.InvalidXAML,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@maxkatz6 is this the correct way to handle the error? Do I get a new error code?

Copy link
Member

Choose a reason for hiding this comment

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

It's a valid XAML though, so error code is not correct.

If you want a generic one, then TransformError. (equivalent of throwing XamlTransformException).
But if you want to make it possible to reconfigure this specific error/warning to be ignored (like, with editorconfig), then it needs to have a unique error code.

Copy link
Member

Choose a reason for hiding this comment

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

But for this specific error, I believe, TransformError is more than enough.

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0047826-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Housekeeping][DevAnalyzers] Add Events Analyzer Every RoutedEvent should be usable as Attached Event
3 participants