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

fix(media_control): remove subscription duplicate #53

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ordyakov
Copy link

@ordyakov ordyakov commented Sep 15, 2022

bindContainerEvents is called twice.

  • inside bindEvents
  • а few lines below (removed)

@kslimani
Copy link
Contributor

Be cautious, i think this line may be required, because in bindEvents, the CORE_ACTIVE_CONTAINER_CHANGED event may have been already dispatched (ie: before we listen). And therefore it is why (i think) it is called it after listening event.

Have you been able to reproduce a case where container events are bind twice on active container ?

@ordyakov
Copy link
Author

ordyakov commented Sep 15, 2022

  bindEvents() {
    this.stopListening()
    this.listenTo(this.core, Events.CORE_ACTIVE_CONTAINER_CHANGED, this.onActiveContainerChanged)
    this.listenTo(this.core, Events.CORE_MOUSE_MOVE, this.show)
    this.listenTo(this.core, Events.CORE_MOUSE_LEAVE, () => this.hide(this.options.hideMediaControlDelay))
    this.listenTo(this.core, Events.CORE_FULLSCREEN, this.show)
    this.listenTo(this.core, Events.CORE_OPTIONS_CHANGE, this.configure)
    this.listenTo(this.core, Events.CORE_RESIZE, this.playerResize)
    this.bindContainerEvents() // (2) FIRST TIME
  }
onActiveContainerChanged() {
    this.fullScreenOnVideoTagSupported = null
    this.bindEvents() // (1) INSIDE THIS CALL
    this.setInitialVolume()
    this.changeTogglePlay()
    this.bindContainerEvents() // (3) SECOND TIME
    this.settingsUpdate()
    this.container && this.container.trigger(Events.CONTAINER_PLAYBACKDVRSTATECHANGED, this.container.isDvrInUse())
    this.container && this.container.mediaControlDisabled && this.disable()
    this.trigger(Events.MEDIACONTROL_CONTAINERCHANGED)
  }

@ordyakov
Copy link
Author

Be cautious, i think this line may be required, because in bindEvents, the CORE_ACTIVE_CONTAINER_CHANGED event may have been already dispatched (ie: before we listen). And therefore it is why (i think) it is called it after listening event.

Have you been able to reproduce a case where container events are bind twice on active container ?

You can reproduce this behaviour here http://clappr.io/demo/ by adding breakpoint in any container listener. Every listener will be called twice.

Screenshot 2022-09-20 at 12 06 05

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.

None yet

2 participants