Problem/Motivation

The module currently adds inline_js through the template, that it has to encode by hand. It also requires that the jw player is already loaded in the header.

Proposed resolution

This is going back to the optional feature in 7.x, this time, it's always on as #attached is working reliably in 8.x

This cleans up template preprocessing as we no longer have to do json encoding ourselv or printing inline JS. It also means that all the JS can be moved into the footer.

This also removes custom plugins. I think most of that can be handled with standard third party settings in 8.x, if not, we can always re-add if something is missing.

This is a rather big API change, as using #theme jw_player will no longer work. So custom integrations will break. Still seem important enough to do.

Remaining tasks

Improve and fix tests.

User interface changes

API changes

#theme jw_player is replaced with #type jw_player.

Data model changes

CommentFileSizeAuthor
#4 interdiff-2712615-2-4.txt4.25 KBjohnchque
#4 introduce_jw_player-2712615-4.patch16.18 KBjohnchque
#2 jw-player-type-js-2712615-2.patch12.83 KBberdir

Comments

Berdir created an issue. See original summary.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
StatusFileSize
new jw-player-type-js-2712615-2.patch12.83 KB

Status: Needs review » Needs work

The last submitted patch, 2: jw-player-type-js-2712615-2.patch, failed testing.

johnchque’s picture

johnchque
Primary language Spanish
Location Bolivia
commented
StatusFileSize
new introduce_jw_player-2712615-4.patch16.18 KB
new interdiff-2712615-2-4.txt4.25 KB

Changes made on the tests and some small comment fixing. Should be OK, let's see if testbot agrees.

johnchque’s picture

johnchque
Primary language Spanish
Location Bolivia
commented
Status: Needs work » Needs review

  • Berdir committed b0fee83 on 8.x-1.x authored by yongt9412 
    Issue #2712615 by yongt9412, Berdir: Introduce jw_player render element...
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Fixed

Tested this with our project, works well.

sgdev’s picture

sgdev commented

I don't think it's a good idea to use this line:

$element['#html_id'] = 'jwplayer-' . md5(rand());

Adding a random string on the end means #html_id will never be constant. This is an issue if attempting to use hooks. Also given that all of JW Player 7 items are controlled through CSS, the div id needs to be a static value for themers.

Please see how we handled this in the newest Refactor theming patch: https://www.drupal.org/node/2713725#comment-11144633

+  // Cannot completely guarantee uniqueness, but adding different settings should help.
+  $suffix = md5($settings['player_type'] . '_' . $item_id . '_' . $settings['jwplayer_preset']);
+  return drupal_html_id('jwplayer-' . $suffix);

Similar to what is currently done in 7.x-2.x, we are creating a string based on player type, item id, and preset name. This allows us to do the following in the API, where $id is a machine name version of the value created from the code above:

+/**
+ * Alter an individual player before it's rendered.
+ *
+ * Same as hook_jw_player_alter(), only including the $id in the function
+ * name instead of being passed in as an argument.
+ *
+ * @see hook_jw_player_alter()
+ */
+function hook_jw_player_PLAYER_ID_alter(&$player) {
+
+}
berdir’s picture

berdir
Primary language German
Location Switzerland
commented

The HTML ID shouldn't be used for theming anyway, happy to add more classes if needed. Alter hooks for fine, I have one myself where I'm adding a preview image, a bit complicated but you can easily access the ID there and adjust #attached with that key.

I did that altering in 8.x using an element_info_alter hook that adds a second pre_render callback, no special hook necessary. Although I guess it would be a bit easier if one would exist.

We had this before in exactly the same way, it's just a fallback if none is given.

It's very important for that ID to be unique, we've had cases where the same file is displayed multiple times

sgdev’s picture

sgdev commented

It is not possible to add classes, unless it is done in a wrapper. JW Player strips any classes added to the player and replaces it with their own.

I understand about the ID needing to be unique, that is important. However having it be random is not a good situation either. I've never heard of situations where the id of a media player div tag continually changes every time a page is loaded.

It is not possible to string together enough components to create an ID that is unique, but doesn't change every time the page renders?

sgdev’s picture

sgdev commented

Also to be clear, HTML ID isn't being used for theming. In the refactor theming patch, we're using a variation of HTML ID to create a unique machine name. We need to create something unique for theming -- I'm not sure what else might be used, what would you recommend?

Status: Fixed » Closed (fixed)

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