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

Feature Request - DevToolsClient add strongly typed Events #3666

Closed
1 task done
amaitland opened this issue Jul 2, 2021 · 5 comments
Closed
1 task done

Feature Request - DevToolsClient add strongly typed Events #3666

amaitland opened this issue Jul 2, 2021 · 5 comments

Comments

@amaitland
Copy link
Member

amaitland commented Jul 2, 2021

#3229 added support for calling DevTools methods in a strongly typed fashion.

  • Add support for strongly typed EventArgs and classes to represent the params.
@campersau
Copy link
Contributor

campersau commented Aug 26, 2021

This assumes that all event handlers for a specific eventName are of the same type.
https://github.com/CefNet/CefNet.DevTools.Protocol/blob/0a124720474a469b5cef03839418f5e1debaf2f0/CefNet.DevTools.Protocol/CrdtpSession.cs#L31

Currently we support multiple event handlers of different types for the same eventName. E.g.

devToolsClient.Network.RequestWillBeSent += (object sender, RequestWillBeSentEventArgs args) =>
{

};
devToolsClient.RegisterEventHandler("Network.requestWillBeSent", (object sender, DevToolsDomainEventArgsBase args) =>
{

});
devToolsClient.RegisterEventHandler("Network.requestWillBeSent", (object sender, MyRequestWillBeSentEventArgs args) =>
{

});

[DataContract]
private class MyRequestWillBeSentEventArgs : DevToolsDomainEventArgsBase
{

}

If we not only group event handlers by eventName but also by Type something like this might work. E.g. IDictionary<string, IDictionary<Type, IEventProxy>>
Or we declare that you can only add listeners for the same eventName of the same type as CefNet does.

(To be honest, I don't know how common it would be to use different types for the same eventName or even manually using RegisterEventHandler instead of the generated typed events.)

@ukandrewc
Copy link

Just a view: I came from WebView2, where there are no typed calls or events for DevTools. I have found the typed versions in CEFSharp to be very useful, so my vote would be for the same events typed as you have on the calls.

@amaitland
Copy link
Member Author

Or we declare that you can only add listeners for the same eventName of the same type as CefNet does.

That was my original intention, this would greatly simplify our code. Keeping support for multiple types is of course possible, just not clear it's required.

(To be honest, I don't know how common it would be to use different types for the same eventName or even manually using RegisterEventHandler instead of the generated typed events.)

It seems like an edge case that would rarely if ever be used. Long as you can manually subscribe with your own type or use the built in event handler then I think that will be sufficient.

We can then look at cleaning up the event handlers on Dispose with the simplified design.

I have found the typed versions in CEFSharp to be very useful, so my vote would be for the same events typed as you have on the calls.

@ukandrewc If you are using the strongly typed events provided currently e.g. devToolsClient.Network.RequestWillBeSent then there will be no change. If you are manually calling devToolsClient.RegisterEventHandler then there will be no need to dispose of the returned IRegistration instance as RegisterEventHandler will return void.

If you are using multiple types for the same event as detailed by @campersau then that will no longer be supported.

amaitland added a commit that referenced this issue Aug 28, 2021
Now it's only possible to register one System.Type per event.

- Remove RegisterEventHandler
- Add AddEventHandler and RemoveEventHandler
- Change constraint from DevToolsDomainEventArgsBase to EventArgs

#3666
@amaitland
Copy link
Member Author

PR for proposed changes is #3777

Save approx 2000 lines of generated code.

amaitland added a commit that referenced this issue Sep 3, 2021
* DevTools Client - Simplify Strongly Typed Event handling

Now it's only possible to register one System.Type per event.

- Remove RegisterEventHandler
- Add AddEventHandler and RemoveEventHandler
- Change constraint from DevToolsDomainEventArgsBase to EventArgs

#3666

* Changes were based on https://github.com/CefNet/CefNet.DevTools.Protocol

Add links to original source files

* Reset stream position

* AddEventHandler - Use GetOrAdd for Thread safety

* AddEventHandler - Fix double event handler registration for first call

* RemoveEventHandler - Remove IEventProxy when handler null
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants