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

Add support for adding/removing event handlers to/from events without any managed utils #23

Closed
wants to merge 4 commits into from

Conversation

lemonmojo
Copy link
Contributor

Like the title says, this PR allows you to add and remove event handlers to/from events without any managed helper classes. It builds upon the existing support for creating System.Delegate objects by specifying a block.

The implementation makes use of the reflection APIs to call AddEventHandler and RemoveEventHandler on the event you specify.

Here's an example:

Given you have an event declared in C# like this:

public event EventHandler<EventArgs> NewVersionReleased;

... you can now do the following in Dubrovnik to subscribe to the event:

[obj addEventHandler:[System_EventHandlerA1 universalDelegateWithConstructedType:[System_EventHandlerA1 constructCoreTypeWithParameters:@[System_EventArgs.class]] block:^System_Object *(NSArray *parameters) {
    System_Object* theObj = parameters[0];
    System_EventArgs* eventArgs = parameters[1];
    
    // ...
    
    return nil;
}] toEventNamed:@"NewVersionReleased"];

All event handlers registered with this mechanism are tracked and kept in memory until either removeEventHandler:fromEventNamed: is called or the object itself is deallocated.

There's also a new method for retrieving all currently registered native(!) event handlers for a specific event called eventHandlersForEventNamed:.

I guess with this in place it would be quite easy to extend the code generation utils to actually support generating bindings for events as well but I haven't (yet) looked at this.

What do you think?

@KosmicTask
Copy link
Contributor

This looks very interesting - the original event handler code was implemented long before we had extended the binding scope of the .NET base class libraries. I would love to get the code generator to output event binding : it would be very cool. The manual approach works but it's pretty laborious to set up. I need to leave myself an hour to go through this though. It would be good if you could add something to the unit test code for this as there is a lot going on in Dubrovnik and the tests are crucial for maintaining a degree of confidence in all this. It's great to have your input.

@lemonmojo
Copy link
Contributor Author

Sure, take your time to review!
I intentionally added only the bare minimum in this PR to keep it small and easy to review. There've been no changes, only additions so it should definitely not break existing code.

I can't claim to be 100% sure this works in all cases though and you're right, unit tests are definitely required if you were to merge this into master.

I think it would make more sense for you to review the code and evaluate if the approach I took makes sense from your perspective. If that's the case, we can work together on providing tests, changing the implementation if required and upgrading the samples to use the new event handling stuff.

And yes, getting the code generator to output event binding code would be tremendously helpful!
I think the crucial part here is that no code changes should be necessary on the managed side to ensure existing libraries and the system frameworks work just as well as user code.

@KosmicTask
Copy link
Contributor

I agree. I will get back to you on this as soon as I can.

@lemonmojo
Copy link
Contributor Author

I played with this a bit more and added custom bindings that hide most of the event handler plumbing and makes strongly typed event handlers possible. Would be really awesome if the generator could create something like this automatically.

Consider the following managed event:

public class TeamEventArgs : EventArgs
{
	public Team Team { get; set; }
}

public class Company
{
	public event EventHandler<TeamEventArgs> TeamAdded = delegate { };
}

I now have the following manual native bindings:

static NSString* EVENTNAME_TEAMADDED = @"TeamAdded";

typedef void(^DubLib_Company_TeamAddedBlock)(System_Object* sender, DubLib_TeamEventArgs* eventArgs);

- (System_EventHandlerA1*)teamAdded_addEventHandlerWithBlock:(DubLib_Company_TeamAddedBlock)block
{
    System_Type* eventHandlerType = [System_EventHandlerA1 constructCoreTypeWithParameters:@[DubLib_TeamEventArgs.class]];
    System_EventHandlerA1* eventHandler = [System_EventHandlerA1 universalDelegateWithConstructedType:eventHandlerType block:^System_Object *(NSArray *parameters) {
        System_Object* sender = parameters[0];
        DubLib_TeamEventArgs* eventArgs = parameters[1];
        
        block(sender, eventArgs);
        
        return nil;
    }];
    
    [self addEventHandler:eventHandler toEventNamed:EVENTNAME_TEAMADDED];
    
    return eventHandler;
}

- (void)teamAdded_removeEventHandler:(System_EventHandlerA1*)eventHandler
{
    [self removeEventHandler:eventHandler fromEventNamed:EVENTNAME_TEAMADDED];
}

- (NSArray<System_EventHandlerA1*>*)teamAdded_eventHandlers
{
    return (NSArray<System_EventHandlerA1*>*)[self eventHandlersForEventNamed:EVENTNAME_TEAMADDED];
}

Now, in Swift I can do something like this:

company?.teamAdded_addEventHandler { (sender, eventArgs) in
    if let team = eventArgs.team {
        NSLog("Team \"\(team.name!)\" added")
    }
}

Alternatively, you can just handle the event in a separate function:

company?.teamAdded_addEventHandler(teamAdded)

func teamAdded(sender: System_Object, eventArgs: DubLib_TeamEventArgs) {
    if let team = eventArgs.team {
        NSLog("Team \"\(team.name!)\" added")
    }
}

@KosmicTask
Copy link
Contributor

I should get time to look at this at the weekend. Do you have to update the pull request?

I use Dubrovnik exclusively from Obj-C. Did you have many issues using Swift? Have you configured module map files?

@Numpsy
Copy link
Contributor

Numpsy commented Jan 25, 2019

We've been using it from Swift (with a self-created module map and that works ok (might be nicer if Mono was modularized though) - actually - see #15 that I opened ages back and forgot about).

We've also been revisiting the Sequence thing I mentioned in #12 - works more nicely when the bindings for the collection classes don't have holes where their IEnumerable support should be :-)

@lemonmojo
Copy link
Contributor Author

lemonmojo commented Jan 25, 2019

No need to update the PR, I've been working off master + the changes from this PR the last couple days.

Regarding Swift: I'm using Swift only in the Cocoa App project, not in the framework libraries. Didn't have to deal with module maps. Instead I just made sure my prefix header is imported in the Swift header. This gives me access to everything I need.

No specific problems at the moment with this approach, although obviously the method names are not very Swifty but I can live with that. ;)

@KosmicTask
Copy link
Contributor

We've also been revisiting the Sequence thing I mentioned in #12 - works more nicely when the bindings for the collection classes don't have holes where their IEnumerable support should be :-)

Is that still an issue now that we have better mscorlib bindings?

@Numpsy
Copy link
Contributor

Numpsy commented Jan 25, 2019

I think the actual binding bug is fixed now.

@KosmicTask
Copy link
Contributor

I didn't merge this directly but I have tried to include the functionality. I moved some of the methods into a System_Object category rather than include them in System_Object directly.

This didn't look right

[eventInfo invokeMonoMethod:"System.Diagnostics.EventInfo.AddEventHandler(object,System.Delegate)" withNumArgs:2, self.monoObject, eventHandler];

So I modified it to
[eventInfo invokeMonoMethod:"System.Reflection.EventInfo.AddEventHandler(object,System.Delegate)" withNumArgs:2, self.monoObject, eventHandler];

I haven't been able to trial this out as yet but I should get opportunity soon.

@KosmicTask KosmicTask closed this Feb 6, 2019
Thesaurus pushed a commit that referenced this pull request Feb 6, 2019
…implicit tests for this new functionality.
@lemonmojo
Copy link
Contributor Author

Perfect! Thx!

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

Successfully merging this pull request may close these issues.

3 participants