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.

Sign up for GitHub

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

Jump to bottom

Feature event on receive #3411

Merged
merged 8 commits into from
Nov 17, 2020
Merged

Feature event on receive #3411

merged 8 commits into from
Nov 17, 2020

Conversation

FritzHeiden
Copy link
Contributor

@FritzHeiden FritzHeiden commented Sep 21, 2020 •
edited
Loading

resolves #3379

@dsilhavy
Copy link
Collaborator

dsilhavy commented Oct 5, 2020

@bbert Before I merge this: Does this align to your activities on refactoring the event bus?

@dsilhavy dsilhavy added this to the 3.2.0 milestone Oct 5, 2020
@bbert
Copy link
Contributor

bbert commented Oct 6, 2020

More generally, why not simply add 'isEventStart' in the event payload and then filter thoses specific SchemeIdUri events at application level?
I am not in favor of modifying MediaPlayer.on() API signature only for these SchemeIdUri events.

Regarding this PR proposal:

  • I would have kept 'scope' parameter as it is instead of moving it in 'options' parameter of Eventbus.on() method, and then having only 'priority' and 'mode' in options parameter.
  • For the 'options' parameter of Eventbus.trigger() function that should align with the 'filter' parameter I introduced in PR Refactor EventBus #3372

Other remark: do not push package-lock.json changes in the PR

@dsilhavy
Copy link
Collaborator

dsilhavy commented Oct 6, 2020

@bbert Thanks for the feedback:

  • We aligned the implementation with the DASH-IF spec: https://dashif.org/docs/EventTimedMetadataProcessing-v1.0.2.pdf Thats why the application explicitly registers for a dispatch mode. In addition, if we add an isEventStart attribute, events would be dispatched twice and existing implementations need to be adjusted.
  • What would be the reason to keep the scope out of the options?
  • I think we should merge the filter implementation of Refactor EventBus #3372 when this PR has been merged to dev

@bbert
Copy link
Contributor

bbert commented Oct 7, 2020 •
edited
Loading

  • "We aligned the implementation with the DASH-IF spec: https://dashif.org/docs/EventTimedMetadataProcessing-v1.0.2.pdf Thats why the application explicitly registers for a dispatch mode. In addition, if we add an isEventStart attribute, events would be dispatched twice and existing implementations need to be adjusted."
    Yes but this should not be a problem and this is already the case for some events. For example if you want to listen for specific metrics events, then you have to listen for all metrics events and then filter the events according to the metrics type you're interested in.

  • "What would be the reason to keep the scope out of the options?"
    scope is not optional. It is required especially to be able to unregister listener

  • "I think we should merge the filter implementation of Refactor EventBus #3372 when this PR has been merged to dev"
    Why not, or vice-versa, first merge Refactor EventBus #3372 and the rebase this PR to enrich filters parameter

@FritzHeiden FritzHeiden force-pushed the feature-event-on-receive branch from 7b02fcb to 39dc91e Compare November 9, 2020 09:24
dsilhavy
dsilhavy reviewed Nov 10, 2020
View reviewed changes
src/core/EventBus.js Outdated Show resolved Hide resolved
dsilhavy
dsilhavy reviewed Nov 10, 2020
View reviewed changes
src/core/EventBus.js Outdated Show resolved Hide resolved
FritzHeiden and others added 6 commits November 10, 2020 12:29
@FritzHeiden
Fixed various issues
bab80ff 
- fixed JSDoc
- changed on start and on receive event names to be more precise
- consolidated long .filter change to one to improve performance
- refactored code to use eventBus.on properly with priorities
- ignoring not set event mode
- skipping logic specific to event start triggers if event was just
  receive
@dsilhavy
Merge branch 'development' of https://github.com/Dash-Industry-Forum/…
086f4eb 
…dash.js into feature-event-on-receive

� Conflicts:
�	src/core/EventBus.js
@dsilhavy
Use explicit mode in EventController.js and adjust dispatching logic …
ae33892 
…in EventBus accordingly.
@dsilhavy
Add additional unit tests for the EventBus
ad07f72
@dsilhavy
Minor adjustment to the EMSG sample
d4237fc
@dsilhavy
Merge branch 'development' of https://github.com/Dash-Industry-Forum/…
f4df532 
…dash.js into feature-event-on-receive

� Conflicts:
�	src/core/EventBus.js
@dsilhavy dsilhavy merged commit a6006a6 into Dash-Industry-Forum:development Nov 17, 2020
@dsilhavy dsilhavy deleted the feature-event-on-receive branch June 26, 2021 07:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dsilhavy dsilhavy dsilhavy left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.2.0
Development

Successfully merging this pull request may close these issues.

event "on receive" dispatch mode
3 participants
@FritzHeiden @dsilhavy @bbert