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

Refactor EventBus #3372

Merged

Conversation

bbert
Copy link
Contributor

@bbert bbert commented Aug 19, 2020

The goal of this PR is to refactor the EventBus, mainly to add the possibility to listen for events for a specific stream id (period) and a adaptation/media type.

Bertrand Berthelot added 2 commits August 19, 2020 14:51
- add possibility to trigger events for specific stream id and media type, only listenesr that have been created for the given stream id and media type are called
- add getStreamId() and getType() for classes, used by EventBus to filter listeners
- remove all tests in listener methods to check for correct stream id and media type
@bbert bbert linked an issue Aug 19, 2020 that may be closed by this pull request
@bbert bbert added this to the 3.1.4 milestone Aug 19, 2020
bbert pushed a commit to Orange-OpenSource/dash.js that referenced this pull request Aug 19, 2020
@@ -75,17 +82,33 @@ function EventBus() {
handlers[type][idx] = null;
}

function trigger(type, payload) {
function trigger(type, payload = {}, streamId = undefined, mediaType = undefined) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We are currently working on #3379 and would like to introduce an additional parameter in the trigger function as well. In order to align this and avoid a large constructor I suggest we make the third parameter and object {streamId,mediaType,additonalParameter}. That way we can enhance it even further later

Copy link
Contributor Author

@bbert bbert Sep 15, 2020

Choose a reason for hiding this comment

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

Why not enriching payload parameter with all these additional parameters? for example:

eventBus.trigger(Events.INIT_FRAGMENT_NEEDED, {
    streamId: streamInfo.id,
    mediaType: type,
    representationId: currentRepresentationInfo.id
});

Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer having a third object in the function for all internal attributes which are not supposed to be dispatched to the client/app

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK I see
we may get heavier source code, but nevermind
So this could be a 3rd function parameter called 'filters'? Or this may contains other parameters that could have other utility than even filtering?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes basically everything which we can use for filtering and decision making when triggering the event. We will adjust the PR/request accordingly: #3379

@bbert bbert mentioned this pull request Oct 6, 2020
bbert pushed a commit that referenced this pull request Nov 3, 2020
This reverts commit e6ddd34.
Bertrand Berthelot added 2 commits November 4, 2020 08:47
@dsilhavy dsilhavy merged commit efa5e90 into Dash-Industry-Forum:development Nov 4, 2020
@bbert bbert mentioned this pull request Nov 12, 2020
@bbert bbert deleted the refactor-eventbus branch November 12, 2020 16:42
@bbert bbert mentioned this pull request Nov 17, 2020
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.

Include metadata in the EventBus
2 participants