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

Expose forwarded endpoints changed and selection changed from Endpoint. #1221

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

mstyura
Copy link
Contributor

@mstyura mstyura commented Apr 23, 2020

This PR deprecates #1113.

I'm using JVB 2.0 as a library to add custom logic on top of an existing functionality. Custom logic is injected in JVB as a custom bundle. So in implementing additional functionality I'm limited to public API of JVB.
Currently I very much need to know when endpoint's set of forwarded endpoints has changed, and need to know when endpoint is selected/unselected by other endpoints. There is no public way to know when this happen, because event notification is sent silently (to my custom logic) over message transport.

It would be really nice to have these events exposed from Endpoint, so I (and maybe others) is able to intercept an events and inject custom handler.

Thanks you very much for review and considering this functionality!

@mstyura mstyura requested a review from bbaldino April 23, 2020 17:51
@mstyura mstyura changed the title Expose forwarded events changed and selection changed from Endpoint. Expose forwarded endpoints changed and selection changed from Endpoint. Apr 23, 2020
Fixed wrong method used to remove handler.

Removed extra blank line.
@@ -1123,6 +1130,8 @@ public void sendLastNEndpointsChangeEvent(
forwardedEndpoints, endpointsEnteringLastN, conferenceEndpoints);

sendMessage(msg);

notifyForwardedEndpointsChanged(forwardedEndpoints, endpointsEnteringLastN, conferenceEndpoints);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, wondering if this feels a bit weird since this call already came from outside the endpoint. If anything, it seems like we shouldn't forward this here and instead have BitrateController be the source of truth (like in your other PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bbaldino ah, very good point! Then I'll bring back event from bitrate controller and move message sending and forwarding event into event hander of bitrate controller.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bbaldino I think I've fixed an issues you've raising during previous review. Could you please give another chance for this PR and have a look at it? Thanks a lot!

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.

2 participants