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

Export videojs.Flash.embed #1533

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@chikathreesix
Contributor

chikathreesix commented Sep 24, 2014

I'm developing this plugin for video.js and currently using swfobject to embed swf. However, video.js itself has embed method as Flash.embed, I'd like to export this method in order to avoid the redundant way.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Sep 24, 2014

Member

Very cool! Viki is great. With your solution what do you do in the case of mobile devices or desktops that don't support Flash?

I'm fine with exporting this. Does the function API work well enough for you or would you suggest any changes to it before we release it and can't change it?

Member

heff commented Sep 24, 2014

Very cool! Viki is great. With your solution what do you do in the case of mobile devices or desktops that don't support Flash?

I'm fine with exporting this. Does the function API work well enough for you or would you suggest any changes to it before we release it and can't change it?

@chikathreesix

This comment has been minimized.

Show comment
Hide comment
@chikathreesix

chikathreesix Sep 25, 2014

Contributor

Thank you!
In terms of mobile, because we have mobile apps, we are not supporting mobile web officially. But for PC, I think ideally it's better to show an error message that encourage user to install Flash Player when a user doesn't have it.
How are you handling with this situation in vjs.Flash?

Contributor

chikathreesix commented Sep 25, 2014

Thank you!
In terms of mobile, because we have mobile apps, we are not supporting mobile web officially. But for PC, I think ideally it's better to show an error message that encourage user to install Flash Player when a user doesn't have it.
How are you handling with this situation in vjs.Flash?

@chikathreesix

This comment has been minimized.

Show comment
Hide comment
@chikathreesix

chikathreesix Sep 25, 2014

Contributor

Finally I'd like to propose the change in the above commit. I added version parameter to vjs.Flash.embed so that it compares the Flash Player version and throws an error if the version is lower than the required version.
With this change, I will use videojs.Flash.embed like below

videojs.Youtube = videojs.MediaTechController.extend({
  init: function(player, options, ready) {
    /* some codes */

    try{
      videojs.Flash.embed('http://www.youtube.com/apiplayer?' +
        'version=3&enablejsapi=1&playerapiid=' + this.id_,
        this.el_, null, params, attributes, 9);
    }catch(e){
       this.player_.error({code: 4, message:e.message});
    }

   /* some codes */
  }

With this solution, I could show an error message that includes link to download Flash Player.

Contributor

chikathreesix commented Sep 25, 2014

Finally I'd like to propose the change in the above commit. I added version parameter to vjs.Flash.embed so that it compares the Flash Player version and throws an error if the version is lower than the required version.
With this change, I will use videojs.Flash.embed like below

videojs.Youtube = videojs.MediaTechController.extend({
  init: function(player, options, ready) {
    /* some codes */

    try{
      videojs.Flash.embed('http://www.youtube.com/apiplayer?' +
        'version=3&enablejsapi=1&playerapiid=' + this.id_,
        this.el_, null, params, attributes, 9);
    }catch(e){
       this.player_.error({code: 4, message:e.message});
    }

   /* some codes */
  }

With this solution, I could show an error message that includes link to download Flash Player.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Sep 29, 2014

Member

The version requirement should be handled the same way as in the Flash tech.
https://github.com/videojs/video.js/blob/master/src/js/media/flash.js#L227

In the Flash tech, if the version isn't support the tech will never be loaded. Then the player will either try a different tech or show an error that video can't be played.

With that I don't think the version addition is needed. We're going to do a release tomorrow and i'll pull in the first commits at least.

Member

heff commented Sep 29, 2014

The version requirement should be handled the same way as in the Flash tech.
https://github.com/videojs/video.js/blob/master/src/js/media/flash.js#L227

In the Flash tech, if the version isn't support the tech will never be loaded. Then the player will either try a different tech or show an error that video can't be played.

With that I don't think the version addition is needed. We're going to do a release tomorrow and i'll pull in the first commits at least.

@chikathreesix

This comment has been minimized.

Show comment
Hide comment
@chikathreesix

chikathreesix Sep 30, 2014

Contributor

I agree with your opinion. Indeed, Flash tech shouldn't have another version check since it checks the version in isSupported.
Could you export videojs.Flash.version instead so that I can implement this kind of error messaging in my tech?

Contributor

chikathreesix commented Sep 30, 2014

I agree with your opinion. Indeed, Flash tech shouldn't have another version check since it checks the version in isSupported.
Could you export videojs.Flash.version instead so that I can implement this kind of error messaging in my tech?

heff added a commit that referenced this pull request Sep 30, 2014

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Sep 30, 2014

Member

Ok, I merged in the first commit and also exported Flash.version. It's available in master and will go out with the next minor release. Thanks for the help!

Member

heff commented Sep 30, 2014

Ok, I merged in the first commit and also exported Flash.version. It's available in master and will go out with the next minor release. Thanks for the help!

@heff heff closed this Sep 30, 2014

@chikathreesix

This comment has been minimized.

Show comment
Hide comment
@chikathreesix

chikathreesix Oct 1, 2014

Contributor

Thank you so much for your help! 👍

Contributor

chikathreesix commented Oct 1, 2014

Thank you so much for your help! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment