-
-
Notifications
You must be signed in to change notification settings - Fork 20
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
fix: add pattern matching for ios support #187
Conversation
@@ -2264,19 +2264,31 @@ protected List<string> GetActiveEventHandlers() | |||
|
|||
foreach (PropertyInfo callbackInfo in callbacks) | |||
{ | |||
dynamic? callback = callbackInfo.GetValue(this); | |||
object? callback = callbackInfo.GetValue(this); |
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.
@nwestfall this is great! I have a few suggestions on how we can make it more streamlined.
- On line 2260, we could actually pull the
EventCallback
s andFunc
s separately as propertyInfos, instead of together. This would eliminate the need to check if it's a Func later. - There is an interface,
IEventCallback
that all theEventCallback
s andEventCallback<T>
s inherit from. I think we could just cast to that instead of each return type. This would make it more extensible, as with the current version, if we add new EventCallbacks with different types, we have to remember to update this method.
I think I'd like to see a refactor using one or both of these ideas before we merge. Let me know if you would like me to implement that myself, or you are welcome to update this branch and PR.
Thanks again for finding this and working on it!
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.
@TimPurdum I'll look at your first point. For the second point, I actually tried this but it's an internal interface.
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.
Ah, I missed that. Bummer!
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.
If we can't find a way around enumerating the callback T
s, I'm ok with keeping it.
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.
The only way I could think of doing it (and maybe this is longer term), is if all the GeoBlazor Events (ClickEvent
, BlurEvent
), implemented an interface. Would have to test, but maybe then you could do EventCallback<IGeoBlazorEvent>
to catch all of them.
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.
Doesn't look like you can :(, so I don't have any other ideas at this time.
Fixes #186
Instead of using
dynamic
inGetActiveEventHandlers
, we can detect if it's a callback using pattern matching. This is allowed in iOS.https://learn.microsoft.com/en-us/xamarin/ios/internals/limitations#systemreflectionemit
I tested using the Server and WASM projects, all events still show.