Videos that are added to the page in the body via the WYSIWYG editor load once by JW Player, then as the page is cached, load the rest of the times via the HTML5 mode. This is due to the JW Player JavaScript being added in theme preprocess.

A solution would be to move the JW Player embed code into the template so that it (and it's custom values that relate to each video) are cached with the page.

CommentFileSizeAuthor
#13 jw_player_add_js-1312316-13.patch588 bytesjethro
#10 inline_player_code-1312316-10.patch5.46 KBgirishmuraly
#8 inline_player_code-1312316-8.patch4.22 KBgirishmuraly
#7 inline_player_code-1312316-7.patch3.45 KBgirishmuraly
#5 jw_player_inline_code-1312316-5.patch3.75 KBrickvug
#5 1312316-interdiff.patch2.33 KBrickvug
#4 JW_Player-place_player_code_inline-1312316-4.patch3.73 KBjarrodirwin
#1 JW_Player-place_player_code_inline-1312316-1.patch3.5 KBjarrodirwin
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jarrodirwin’s picture

jarrodirwin CreditAttribution: jarrodirwin commented
FileSize
JW_Player-place_player_code_inline-1312316-1.patch3.5 KB

I have created a rough patch that inserts the JW Player code into the template if a inline_js variable is set.

@todo
-Create UI to set/unset the inline variable.
-Only add jwplayer.js if the page contains a video

jarrodirwin’s picture

jarrodirwin CreditAttribution: jarrodirwin commented
Status: Active » Needs review

Changing to needs review

rickvug’s picture

rickvug CreditAttribution: rickvug commented
Status: Needs review » Needs work

@Jarrod Thanks for the patch! I think the approach is right but there could be a few changes:

  • This same problem manifests itself for blocks and anywhere else the text editor is used. For this reason the player JS can be added using hook_init so that all pages have it.
    • $variables['config'] holds all of the key-value pairs for the video. Loop through these values rather than hard-coding the keys to be added.
  • Would it be possible to use drupal_add_js() with the inline option instead? I haven't looked at the output myself to see if this would be feasable, but it certainly feels cleaner and more "drupaly".
  • No need for drupal_html_id() if you're just generating a hash anyhow.
  • Try to conform to Drupal coding standards. For example, "}else{" should be separate lines with a space between "else" and "{".
jarrodirwin’s picture

jarrodirwin CreditAttribution: jarrodirwin commented
Status: Needs work » Needs review
FileSize
JW_Player-place_player_code_inline-1312316-4.patch3.73 KB

Attached new patch.

-Now uses hook_init
-Variables are added to the js by looping through $variables['config']
-Removed drupal_html_id()
-Tidied up code to conform to drupal standards

As for using drupal_add_js() to insert the js, this doesn't look possible as the inline scope is only header or footer, both of which do not get cached and the same issue arises. You are meant to be able to add it to specific regions but looks like that was never implemented. See http://drupal.org/node/812388

It is a shame as that would be the cleaner way to add the inline js.

rickvug’s picture

rickvug CreditAttribution: rickvug commented
FileSize
1312316-interdiff.patch2.33 KB
jw_player_inline_code-1312316-5.patch3.75 KB

@Jarrod Thanks for the updated patch. Sorry about my delay in responding. Attached is a slightly modified version to fix a few (minor) white space and capitalisation errors. An "interdiff" is attached as well to make it easy to see the changes since last time. To finish this off I'd like to see a few things:

  • The creation of the inline JS should be done by a separate function to clean things up.
  • Looking forward to adding plugins support, the options array will now include additional arrays. The code will need to adapt to suit.
  • The UI to set the variable is required (as you know).
  • The adding of the inline JS can be simplified. No need for three opening php tags when one will do.

Aside from these clean up issues things are looking good. Depending on time I may end up taking on some of this work.

rickvug’s picture

rickvug CreditAttribution: rickvug commented
Status: Needs review » Needs work
girishmuraly’s picture

girishmuraly CreditAttribution: girishmuraly commented
Status: Needs work » Needs review
FileSize
inline_player_code-1312316-7.patch3.45 KB

Here's the patch applied to the latest commit

  • Inline JS code has been routed through json_encode()
  • Plugins support added in the above latest commit: #1338964: Support JW Player plugins
  • UI to set the variable created as a general settings form in admin/config/media/jw_player/settings
girishmuraly’s picture

girishmuraly CreditAttribution: girishmuraly commented
Status: Needs review » Needs work
FileSize
inline_player_code-1312316-8.patch4.22 KB

I had forgotten to attach theme/jw_player.tpl.php.

girishmuraly’s picture

girishmuraly CreditAttribution: girishmuraly commented

Just noticed a bug with patch #8:

If a preset plugin is being used and then later turned off at a later date, the preset would still have the settings of the disabled plugin and would continue to output their setttings to the player in the javascript.

girishmuraly’s picture

girishmuraly CreditAttribution: girishmuraly commented
Status: Needs work » Needs review
FileSize
inline_player_code-1312316-10.patch5.46 KB

Attached patch with better checking of loading preset plugin settings based on only the available (module enabled) plugins.

rickvug’s picture

rickvug CreditAttribution: rickvug commented
Status: Needs review » Fixed

Committed! Thanks Girish and Jarrod. I made a few small changes to the documentation but otherwise this is your guy's work.

The plugin issue that Girish raised in comment #9 has been split off into its own bug report: #1348518: Presets can reference plugins that are no longer available. I agree with the solution but I don't think it is appropriate to send along all variables.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

jethro’s picture

jethro CreditAttribution: jethro commented
Version: 7.x-1.x-dev » 7.x-2.x-dev
Status: Closed (fixed) » Active
FileSize
jw_player_add_js-1312316-13.patch588 bytes

It appears this issue has resurfaced in the 7.x-2.x branch of the module.
I it better to open a new issue or revive this ticket?

When a player is inserted through a wysiwyg player, like with the media module, the page is cached and subsequent page loads are missing
drupal_add_js(libraries_get_path('jwplayer') . '/jwplayer.js');
drupal_add_js(drupal_get_path('module', 'jw_player') . '/jw_player_seek.js');

The player is unable to load.

I don't see a better way than to add these in hook_init. Would it be preferable if this was a setting as not all sites may encounter this issue?

pbuyle’s picture

pbuyle CreditAttribution: pbuyle commented

Which cache is actually causing the issue?

Instead of using drupal_add_js(), could #attached be used somewhere in a cached renderable array?

pbuyle’s picture

pbuyle CreditAttribution: pbuyle commented
Issue summary: View changes

Page cache does include the JS added with drupal_add_js(). So the page being cached is not the issue. Could there be some other cache involved? How is the embed code inserted in the WYSIWYG created? Is the embedding done through a filter?

pbuyle’s picture

pbuyle CreditAttribution: pbuyle commented
Status: Active » Postponed (maintainer needs more info)

  • rickvug committed 1fb722f on 8.x-1.x 
    Issue #1312316 by jarrodirwin, girishmuraly: Add inline player loading....
ron_s’s picture

ron_s CreditAttribution: ron_s commented
Related issues: +#2713725: Refactor theming

Marking this as duplicate since the refactor theming patch removes all inline JavaScript and renders players using #attached and Drupal.behaviors.

Please review: https://www.drupal.org/node/2713725

ron_s’s picture

ron_s CreditAttribution: ron_s commented
Status: Postponed (maintainer needs more info) » Closed (duplicate)

Sorry, forgot to change the status.