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

[Experiment] Implement EventSetter #15373

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[Experiment] Implement EventSetter #15373

wants to merge 1 commit into from

Conversation

maxkatz6
Copy link
Member

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:

  1. Our ISetterInstance infra expects anything that has a value. EventSetter doesn't have that. Had to implement IValueEntry in a dumb way to make it work. Essentially, we need some ISetterInstance interface that is able to call Unsubscribe.
  2. RoutedEvent lookup is problematic. Correctly it should use RoutedEventRegistry and lookup by actual event name. On practice we can't do it compiler time and validate it. This PR implements a hack that look-ups by event definition name that must exist in target or base types. Alternatively, user can use x:Static which will work even for attached events.

Checklist

Fixed issues

Fixes #12627

@maxkatz6 maxkatz6 added api API addition area-xaml labels Apr 15, 2024
@maxkatz6 maxkatz6 requested a review from grokys April 15, 2024 05:00
@avaloniaui-bot
Copy link

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

/// </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)
Copy link
Member

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)

Copy link
Member Author

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.

Copy link
Member

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

Copy link
Member Author

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.

Copy link
Member Author

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.

@jmacato
Copy link
Member

jmacato commented Apr 25, 2024

@cla-avalonia recheck

@cla-avalonia
Copy link
Collaborator

cla-avalonia commented Apr 25, 2024

  • All contributors have signed the CLA.

@maxkatz6
Copy link
Member Author

@cla-avalonia agree

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.

[Feature Request]: Add EventSetter in Styles / ControlThemes
5 participants