Refactor theming

Attached is a patch for review. The approach we've used is similar to the structure of the Leaflet module. We're open to modifying the approach if you believe some changes are necessary to more closely match 8.x.

Also important to note, you may get a warning when applying this patch, because of the changes we're attempting to make in multiple patches. However based on our review, this patch is only a warning and should be applied correctly.

To ensure all functionality works as expected, we'd highly recommend adding a few other patches we have recently created to this one:

• Fix legacy skins and allow JW Player 7 skins to be set: https://www.drupal.org/node/2713675
• Add managed selected preset option: https://www.drupal.org/node/2713679
• Improve formatter summaries: https://www.drupal.org/node/2713687
• Add preset sources, sharing, mute and validation: https://www.drupal.org/node/2713701

Adding a link to this thread for 8.x (https://www.drupal.org/node/2645242), since this patch includes an option for the link_field.

We'll want to make sure this patch includes any necessary link validation checking that we might have missed.

Worthwhile to mention that this patch and the preset sources patch (https://www.drupal.org/node/2713701) introduce two new JavaScript files that have been put into a new "js" directory.

I would have liked to also move the jw_player_seek.js file into this directory to be consistent, but decided to leave it out for now. If we agree to start using the js directory, we can create another patch after the commits are done to move jw_player_seek.js.

Your structure is a bit different to what I did in the 8.x branch, I have a players list where the key is the html_id.

I'm assuming I should be looking at this patch? https://www.drupal.org/files/issues/jw-player-type-js-2712615-2.patch

Yes, we're just creating a simple $settings array, since each $build is it's own player:

+++ b/jw_player.module
@@ -693,10 +593,230 @@ function jw_player_library() {
+  $settings = array(
+    'html_id' => $html_id,
+    'options' => $options,
+  );

As I mentioned in #2, we liked the approach that the Leaflet module took to structuring this code, and leveraged some concepts (http://cgit.drupalcode.org/leaflet/tree/leaflet.module?id=e505e9c#n90). There are a lot of similarities in the basic principles of both modules, including the idea of creating a single function that can be called to build the end result: http://cgit.drupalcode.org/leaflet/tree/README.txt?id=e505e9c#n25 Their "features" is basically our "options".

If users want to create a JW Player in code and render it to the page, they should be able to do so. It doesn't need to be tied to any particular field or file to accomplish this. In fact, it would allow developers to create wide-ranging solutions, including pulling files from different fields or feeds into one playlist. I would think that keeping the html_id out of the associative array would help in creating jw_player_build_player elements as easily as possible, but I'm certainly open to discussing further.

I was thinking about a similar hook in the 8.x issue too. But with a #type, people could also just alter that and add another pre_render callback. A bit more complicated.

Yes, maybe look a bit at those Leaflet functions. There are probably even some improvements that can be made to the patch I posted, but I'm not sure we're ready to have JW Player make that big of a change. If you think we can take it further and support a full build function, I'm open to suggestions.

Probably needs a bit more documentation about what $build exactly is (is it one player now or multiple) ?

Yes, more documentation is always good. JW Player has different associative array styles depending on if it is a single player, playlist, or playlist with fallback sources. If the array is structured in the right way and passed through $build, JW Player will render it. The information they have in Leaflet is a good starting point for what could be done:
http://cgit.drupalcode.org/leaflet/tree/README.txt?id=e505e9c#n25
http://cgit.drupalcode.org/leaflet/tree/leaflet.api.php?id=e505e9c

Yeah, so this is a BC problem.
...
We still have the template. What if we support both? looks like the build function actually just uses an HTML tag right now?
...
Either way, the preprocess function could detect if it gets the build settings or the old stuff, and if everyting is prepared already, it could just bail out.
...
Problem is that it's too late to use #attached I think at that point in 7.x

What about trying to conditionalize the code? Add an option on the general settings page such as...

[x] Legacy theming: uses traditional templating and may have JavaScript aggregation/compression issues.
[ ] Improved theming: hook available to change player options, function to build players in code, and best approach for caching and aggregation.
Note: if changing from Legacy to Improved theming and templates were previously used, updates may be necessary.

Haven't entirely thought through if this could work, but wanted to mention as an idea.

Making a note that we've identified a minor bug. The following code:

@@ -109,6 +116,20 @@ function jw_player_field_formatter_settings_form($field, $instance, $view_mode,
   // If there are presets prompt the user to select one or create another.
   // If no, prompt to create a preset.
   if (!empty($presets)) {
+    if (!jw_player_use_legacy()) {

Should instead be this:

@@ -109,6 +116,20 @@ function jw_player_field_formatter_settings_form($field, $instance, $view_mode,
   // If there are presets prompt the user to select one or create another.
   // If no, prompt to create a preset.
   if (!empty($presets)) {
+    if (!jw_player_use_legacy() && $display['type'] == 'jw_player') {

Waiting for further feedback before creating any new patches.

We've tested out the idea in #6 of legacy vs. improved theming, and I think this is going to work perfectly. We keep all the same code as exists today in 7.x-2.x, and wrap it with a jw_player_use_legacy_theming function.

Once we get the other patches committed, we'll create a new patch for the theming. Thanks.

Here is an image of how the approach looks on the settings page.

@deanflory, sorry we've been making some adjustments in the past few days. Also the #2 patch on this thread no longer matches the patch on Issue #2713701.

Waiting for some feedback from @Berdir, but we already have a new patch ready for this thread once we can get the other patch committed. Hopefully we'll have it ready soon. Thanks!

(I should also mention it sounds like jw_player.theme.inc was probably put at the root of your site in a /theme folder. Depending on how you apply patches, that can happen when adding new files. Check to see if you have a file located there.)

Ok, this is really starting to look good. We've got a defined array structure for a theming API, and able to create custom players on demand directly in code.

Here is a playlist I just created for a sidebar block. Very straightforward, and have written up all the documentation on how to do this in the README file.

function _get_video_block() {
  $playlist_example = array(
    'my_custom_playlist' => array(
      'settings' => array(
        'preset' => 'my_special_preset',
        'image_style' => '480x480',
      ),
      'options' => array(
        'playlist' => array(
          0 => array(
            'file' => 'https://www.youtube.com/watch?v=suWsd372pQE',
            'title' => 'Sample Video 1',
            'image' => 'http://mysite.com/sites/default/files/lotus-flower-small.jpg',
          ),
          1 => array(
            'file' => 'https://www.youtube.com/watch?v=iNJdPyoqt8U',
            'title' => 'Sample Video 2',
          ),
          2 => array(
            'file' => 'https://www.youtube.com/watch?v=xDMP3i36naA',
            'title' => 'Sample Video 3',
            'image' => 'http://mysite.com/sites/default/files/daisy-flower.jpg',
          ),
        ),
        'skin' => 'bekle',
        'width' => '100%',
        'aspectratio' => '4:3',
        'stretching' => 'uniform',
        'controls' => true,
        'sharing' => array(
          'sites' => array(
            0 => 'email',
            1 => 'facebook',
            2 => 'twitter',
          )
        ),
      )
    ),
  );
  return jw_player_build_player($playlist_example);
}

All the functionality is outside of the existing theming, so it's 100% backwards compatible. Look forward to getting this out there for review!

Also let me know if you think it's easier for developers if the id passed in is part of the data array, or outside of it. For example, could have:

jw_player_build_player($player_id, $player_data);

Where $player_id is pulled out of the array, or:

jw_player_build_player($player_data);

As it is structured in comment #13. I'm fine with either, but not sure if some people might find it easier with an array that is one level smaller.

One other comment worth mentioning... because the API matches directly to JW Player's configuration API, it's possible to add functionality that doesn't exist in the module UI.

For example, I was able to add a logo to my player just by adding the following to the API 'options' array:

        'logo' => array(
          'file' => 'http://mysite.com/sites/default/files/logo.png',
          'hide' => true,
          'position' => 'top-left',
        ),

Even though this is not available on the site's front-end, developers now have the ability to tailor the player to meet their specific requirements.

If someone needs advertising, captions, related videos, or any other feature not included in the module, it can be added to the array in code.

The attached patch uses #type = 'jw_player' and a #pre_render callback. I think this should meet all of the possible needs.

Please see README.txt and jw_player.api.php for full information on API usage. Thanks.

Worth mentioning that we've moved all the "build" functions into an include file called jw_player.build.inc, and placed it in an "includes" directory.

Once the rest of this functionality is approved and committed, we plan to reorganize functions into the following includes:

includes/jw_player.admin.inc (moved from module folder)
includes/jw_player.build.inc (included in this patch)
includes/jw_player.formatters.inc (all formatter functions)
includes/jw_player.legacy.inc (functions related to legacy theming)
includes/jw_player.library.inc (all library functions)

This should cleanup jw_player.module quite a bit, and have it focused on main functions and the #pre_render process.

Fixed the white spaces, and a few other minor items.

A couple of minor adjustments.

For sites that have not saved with the new preset options, there will be an error when attempting to view the field formatters. Values such as $preset_settings['mute'] will not be defined, therefore we need some isset conditionals.

Also realized while doing some testing that the $preset_settings['skin'] value needs to be modified to an array. Otherwise those hooking the player won't be able to change settings like skin background color without initially converting the value to an array on their own.

Just ran into a major issue. #pre_render causes File entity types to throw errors and content does not load. We did not see this problem with the original approach.

Feedback?

We found the issue. #file is used by File entity types, and if we attempt to use #file in the JW Player element, it's going to cause serious issues. Changing this to another name fixes the issue.

Let me know if you prefer something like #files or #jwplayer_files. We'll create a new patch with #files for now.

Here is the update. Changes all references in the hook_element_info approach from #file to #files to ensure no conflicts with File Entities.

First, pretty long review of the new approach below. Make sure you read everything before starting to do something or responding, I changed my opinion sometimes while learning more about the patch. We might also want discuss this in IRC or so before you continue to work.

I think I follow your thought process fairly clearly. There are some items we'll need to discuss further. The few patches I've recently posted I believe are:

1) tasks outside the scope of the core refactor theming issues
2) not particularly relevant to comments you've made
3) items that I'm hoping we can get committed relatively easily so we are able to focus on issues that need actual attention

Responding to your comments, let me start by mentioning the ones where I think we can agree, with exceptions/comments in parentheses.

Agree:
#1 (See "Use of #player_type" below for further discussion.)
#2
#4
#6
#7 (Error, needs to be in hook_install)
#10 (#type should have been previously removed)
#13 (Already fixed in https://www.drupal.org/node/2719995)
#15 (Just trying to match the existing jw_player_use_legacy function. We've changed it.)
#19 (Partially addressed in https://www.drupal.org/node/2719977. The rest of your comments, see below.)
#20 (Depends on other things, but essentially agree.)
#21 (Already resolved in https://www.drupal.org/node/2719851)

---------------------

In this section, let me mention the other issues (in descending order of importance):

Use of #player_type, default to legacy for existing sites

Related to #1, #5, #9, #12, #14. Our recommendation is to reduce from three field formatters to one. I really don't see a strong reason for three at this point. With JW Player 7, everything is contained in one array, and some minor adjustments allows us to switch from player to playlist to sources (see attached picture). It also makes the API much simpler, since a value for '#player_type' can be included in the renderable array.

Other than backward compatibility, is there a reason we need three formatters? This is a strong reason why I want old format to stay as old format for existing sites. It says in the patch: Note: if changing from Legacy to Improved, updates to templates, presets, and field configuration may be necessary.

Why should we force existing sites to use the new format, if they are happy with how it works? I prefer to put control in the hands of the developer to decide.

Per-preset cloud players

Related to #19, yes this code is missing and needs to be addressed. I think it would be best for you to see what we have in the next version of the patch, and then we can discuss. This also relates to your comment #16, which has been changed.

You said, "Instead of a single jw_player library, we will have to register one for each preset." This is exactly what we have. We are building $libraries['jw_player_' . $preset_name] for each saved preset, storing it in cache, and then attaching in pre_render_element as required. It gets a bit more tricky when someone is including a #player_library_url via API (this is missing from hook_element_info), but I think we have a way of handling it.

Also you asked, "I'm not sure what should happen exactly when you have the license key globally and select a cloud preset?" This is not a possible situation. License key is only present during self-hosting. Cloud preset is only an option in cloud-hosting.

html_id format

Related to #3, #11, #17. First, I think we can make it less complex. The hash got introduced for extra long names. What I prefer is something like "player_34_640x480" and then hash when greater than 32 chars, or drop the hash entirely.

Regarding predictability, I don't understand how this is any more difficult than asking a developer to decipher machine names for blocks and views. Looking at id in Firebug or Chrome provides the answer. If we really want to be helpful, why not create an admin page that lists all rendered players and their matching ids?

As for hook implementations being cached, I have no problem with removing hook_jw_player_PLAYER_ID_alter. However we strongly need hook_jw_player_alter, with id passed in as a variable.

I'm curious, why does 8.x request users to explicitly set html_id? I understand about your previous problems of players not rendering, but this seems unnecessary (other than if a developer is building a player via API). The code should be smart enough to handle this in 95% of the cases. I don't like asking users for extra information when it's really not needed. The process should be simple.

Registering different template

Related to #9, #20, #22. Yes, JW Player (at least JW7) removes all classes. If we add a wrapper, aren't we changing the template design anyway? The goal was to create something that improves theming, but does not change the existing template. That is what we've done. I did not have the expectation that we're trying to use the existing template for the new approach.

I am confused about this comment in #20: "if we do my suggestion above, then this would change to some check to check if this is coming from the #type or not." My understanding is the pre-render works on the element, not the variables. There is no #type available in the template preprocess variables as far as we can see. Is there something we are missing?

We added early returns in the existing template preprocess and hook functions, but as you mentioned they really don't do anything right now since we're using different templates.

Also I'm not sure what you mean about "printing video tags" in #22. JW Player renders to a div. I've never seen it done another way.

API support for old format

In #14 you said, "I think I would still prefer an approach where the primary and practically only difference between the legacy/improved setting is this. And that we support both in the API." I'm sorry, this is not something we have time to do as part of this effort. The goal of the refactoring was to create a new approach to theming, while ensuring anyone who uses the current method has no lost functionality. We have done that.

Specifying JavaScript settings

In #18 you said, "i find this a bit a weird way to specify the settings (without an explicit, unique key)". Actually I've never seen a module that implements settings in a way other than this when needing to support settings for multiple elements. Just a few modules that do it this way: Admin Menu, AdvAgg, jQuery Update, Leaflet, Hierarchical Select, Quicktabs, Views, Webform. Using the method I have here, settings for each player are loaded into a single global 'jw_player' object.

Moving code to include files

In #8 you said, "I personally don't like moving code to include files when they can't be autoloaded." I understand, but I like it because it helps with development and code manageability. Not worth discussing, we have many other things we need to get done.

Here is a new version of the patch for review.

There is a picture that is mentioned in comment #27, but forgot to attach as a reference.

Included is a minor update to the last patch. We did some additional testing and realized the proper libraries were not being attached when switching between Drupal presets and JW Player libraries. This version should fix the issue.

After reviewing the following 7.x-1.x issue (https://www.drupal.org/node/1555960), realized that we are missing one key use case.

If a site builder selects "player" as the player type and sets the field to have multiple files, the current patch only supports the first file being rendered in a JW Player. What should be done is to loop through the files and create a new player for each individual item. This is different from a playlist, which displays multiple files in a single player.

To accomplish such a feature, it is necessary to group the JavaScript settings differently, and also change the way the players are built in JS. See attached, and also the modifications we have made to the patch in Issue #2719977. Thanks.

Just so you're aware, we already have additional bug fixes we have made to patch #31. However I'm going to wait to post another version until we can reach consensus on the feedback outlined in comment #27. Thanks.

@Berdir, I understand 8.x has been a priority in recent months. Is there anything we can do to move 7.x-2.x toward getting the newer patches committed?

The current version of 7.x-2.x-dev is fairly useless. Probably the only version that really works is the copy we have in our development code. I'd really like to be able to allow others to benefit from the improvements we've added.

Let me know what we can do to help. Thanks.

@aitala, as I said in my previous comment, the current -dev version is completely useless relative to these patches. It won't work without getting some of them committed first.

We invested a lot of time last year creating fixes for 7.x-2.x, and asked the module owners both publicly and privately for their help. Unless this is made a priority, there's not much we can do. Our team is not going to continue to put time toward something that is a dead end.

@aitala, I think we're just going to confuse people if there are two repositories for JW Player, and I really see no need for two separate versions of the module.

When we started working on the 7.x updates, it was with the understanding that our work would be useful in building the 8.x version. As we added more features, the same concepts could be used to incrementally improve 8.x too. Beyond the patches we've posted, there are at least five other really great ideas we have to improve the module.

However, that's not what happened. Once 8.x-1.0-beta4 was launched last August, there was no further communication from the module owners.

At this point I only see three possible options:

1) Module owners re-engage with us to get the new 7.x functionality committed.
2) Module owners give us commit authority so we can post new 7.x code.
3) MD Systems decides they no longer want to maintain the project and relinquish ownership.