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

Set source properly on YouTube #1829

Merged
merged 15 commits into from Sep 1, 2016
Merged

Set source properly on YouTube #1829

merged 15 commits into from Sep 1, 2016

Conversation

rafa8626
Copy link
Contributor

@rafa8626 rafa8626 commented Aug 31, 2016

This PR adds a new workflow to set a source properly on YouTube based on comments from #1759. Also solves issues #1410, #1435 and #1704

@rafa8626
Copy link
Contributor Author

OK @dazweeja give it a try now; I added an autoplay functionality, so if the video tag has autoplay attribute, it will. And when you load a new source, it will stop and load the new one until you click Play button

@dazweeja
Copy link

dazweeja commented Aug 31, 2016

So much cleaner than my convoluted event method! I looked at cueVideoById but assumed it wasn't the method I needed when it clearly was.

Your patch achieves the stated functionality. I don't mean to be a bother but does this match the HTML5 functionality? I think with with the HTML5 player if you set source and have the autoplay attribute set, it will autoplay the new video too? For the YouTube case this would mean:

if (this.getAttribute('autoplay') !== null) {
  this.pluginApi.loadVideoById(videoId);
}
else {
  this.pluginApi.cueVideoById(videoId);
}

This needs a small tweak - which I think is a good idea regardless - to getAttribute in mejs.PluginMediaElement.prototype so that it returns null. This works better when testing for undefined attributes in a few other places in the code:

getAttribute: function(name){
  if (this.hasAttribute(name)) {
    return this.attributes[name];
  }
  return null;
},

I'm not even sure if the YouTube autoplay of subsequent videos is a good idea but I just mention it as a consistency thing.

@rafa8626
Copy link
Contributor Author

@dazweeja Thank you so much for your comments. Yes, if autoplay attribute is set the proper way is to play the new video. I'll add it so you can give it a try. About your recommendations for the attribute, can you indicate which places in the code will benefit from adding this tweak?

@rafa8626
Copy link
Contributor Author

rafa8626 commented Aug 31, 2016

@dazweeja Nevermind I found all the places that benefit from this tweak. I added the changes you mentioned before as it will mimic the HTML5 behavior when autoplay attribute is set. Let me know if it works for you

@rafa8626
Copy link
Contributor Author

@dazweeja BTW I merged PR #1802 into this one since it is all related to YouTube at this point, and it was only a minor addition

@dazweeja
Copy link

dazweeja commented Sep 1, 2016

Tested and it all works fine in my use case. I was actually modifying the setSrc function at line 184 of src/js/me-mediaelements.js when developing but putting it in the earlier setSrc function in mep-player.js works just as well.

Thanks for doing all this work. It will be good to be able to use an unpatched release again!

@rafa8626 rafa8626 merged commit 3c809e9 into mediaelement:master Sep 1, 2016
@rafa8626 rafa8626 deleted the youtube-set-src branch September 1, 2016 10:56
@dwatts3624
Copy link

@Ron666 I must have completely missed something but I've been going in circles for an hour now and figured it might be time to stop and see if I'm the only one having this issue and am just doing something wrong.

After building fresh (grunt html5only) from the current master just to be sure I'm using the right code I get:
Uncaught TypeError: b.media.setSrc is not a function

Building was more just a matter of due diligence. I get the same error just cloning master and using mediaelement-and-player.min.js.

The player is successfully loading. I can access all events, etc.

Partial code below:

var player = new MediaElementPlayer(domId, config);
player.setSrc(ytVideoUrl);

The strange thing is that I don't even see setSrc anywhere in the inspector before or after success.

I've also tried changing it in the success() callback.

Any advice?

@dwatts3624
Copy link

Okay. So right after posting I get it to work! Go figure!

For brevity I shortened the code:

var player = new MediaElementPlayer(domId, {
  success: mediaElementSuccess
});
function mediaElementSuccess(mediaElement, domObject) {
  player.setSrc(ytVideoUrl);
}

Is this the intended behavior? It works fine but the documentation seems to suggest that my first approach should work.

@johndyer
Copy link
Collaborator

johndyer commented Dec 22, 2016 via email

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

4 participants