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
[Experiment] Implement EventSetter #15373
base: master
Are you sure you want to change the base?
Conversation
You can test this PR using the following package version. |
/// </summary> | ||
/// <param name="eventName">The particular routed event that the <see cref="EventSetter"/> responds to.</param> | ||
/// <param name="handler">The handler to assign in this setter.</param> | ||
public EventSetter(RoutedEvent eventName, Delegate handler) |
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.
I'd prefer something typed here rather than a plain Delegate. It kinda affects perf and had problems with NAOT previously (probably reflectionless one, don't remember)
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.
Shouldn't matter here.
Compiler still generates a typed EventHandler<RoutedEventArgs>
instance. Which should give enough information to NAOT compiler.
It's then downcasted to Delegate, but our routed events infra uses Delegate internally anyway.
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 were problems with invoking the delegate, IIRC
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.
Well, I sure can make EventSetter a generic type. And wire it up with some compiler magic.
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.
But I probably won't spend much time on it, unless @grokys approves these ISetterInstance hacks.
@cla-avalonia recheck |
|
@cla-avalonia agree |
What does the pull request do?
I was wondering how hard it would be to implement something like this.
After a bit I found two main problems:
Checklist
Fixed issues
Fixes #12627