Add preset sources, sharing, mute and validation

1. +++ b/jw_player.admin.inc
   @@ -25,6 +25,28 @@ function jw_player_settings_form($form) {
   +  $form['jw_player_hosting'] = array(
   +    '#type' => 'radios',
   +    '#title' => t('Hosting type'),
   +    '#options' => array(
   +      'self' => t('Self-Hosted'),
   +      'cloud' => t('Cloud-Hosted'),
   

   Isn't this implied by the license key vs cloud hosted option? What do we gain from an additional setting?

   Can't we just say that either the license key or the cloud url must be provided? Not sure what we should do when both are added.

2. +++ b/jw_player.admin.inc
   @@ -61,7 +102,82 @@ function jw_player_settings_form($form) {
   +  // Including a "preset source" allows for local Drupal presets that override
   +  // cloud-hosted player settings in JW Player 7.
   +  $form['jw_player_preset_source'] = array(
   +    '#type' => 'radios',
   +    '#title' => t('Preset source'),
   +    '#options' => array(
   +      'drupal' => t('Use Drupal-defined presets'),
   +      'jwplayer' => t('Use presets defined on JWPlayer.com'),
   +    ),
   +    '#default_value' => variable_get('jw_player_preset_source', 'drupal'),
   +    '#description' => t('When creating JW Player presets, select if the source should be defined on Drupal, or by using definitions set within your JWPlayer.com account.'),
   +    '#states' => array(
   +      'visible' => array(
   +        array(
   +          'select[name="jw_player_version"]' => array('value' => '7'),
   +          ':input[name="jw_player_hosting"]' => array('value' => 'cloud'),
   +        ),
   +      ),
   

   What if each preset has a checkbox "[x] Override cloud presets" or so? That's something that I've been thinking about. The other idea we've had is to have a per-preset cloud url (that's why the default one is called... default).

3. +++ b/plugins/export_ui/jw_player_ctools_export_ui.inc
   @@ -115,174 +115,330 @@ function jw_player_ctools_export_ui_form(&$form, &$form_state) {
   +  else {
   +    $form['settings']['player_library_url'] = array(
   +      '#type' => 'textfield',
   +      '#title' => t('Player Library URL'),
   +      '#description' => t('Enter the URL to the player created on JWPlayer.com that contains the settings for this preset.'),
   +      '#required' => TRUE,
   +    );
      }
   

   I see, that's basically what you did here.

4. +++ b/plugins/export_ui/jw_player_ctools_export_ui.inc
   @@ -115,174 +115,330 @@ function jw_player_ctools_export_ui_form(&$form, &$form_state) {
   +  $form['settings']['#attached']['js'] = array(
   +    drupal_get_path('module', 'jw_player') . '/js/jw_player.admin.js',
   

   why make it a library when you then add it with #attached js?

Right, that's pretty much what I had in mind. But for that, you don't need a global setting. That's something that you can and want to do per-preset.

Only suggestion on that would be to clarify in the description for the cloud based mode that providing a preset for the override is optional and it will use the global one by default. There is a default url, you're going to be using that for at least one of your presets I guess.

Some more things.

* The cloud URL validation fails on protocol-relative URLs (//content.jwplatform.com/libraries/...)
* is the skin really supposed to be the only thing shown when using cloud? why just that and not the other things?
* Based on what I wrote above, the library URL should be non-required for a preset.

And one more thing, sorry for the many comments.

As far as I see, the per-preset cloud URL isn't used yet? In 7.x, that's relatively easy to address, as you can add #attached js directly. But we do need to prevent that the global one isn't added.

In 8.x, it will be a bit more complicated, we'll have to register a library for each preset that uses a cloud-override and use that in place of the global one.

And because this wasn't enough fun yet. What happens if you mix multiple presets on the same page? One of the URL's will win then, so afaik, you can't have a big and some smaller players on the same page with cloud-based presets? Might be worth documenting the limitations somewhere.

I was successful in applying jw_player-preset_sharing_mute_validate-2713701-2.patch, but this was the resulting error after running update.php without the other patch further below because it failed to apply:

Warning: include_once(/.../sites/all/modules/jw_player/theme/jw_player.theme.inc): failed to open stream: No such file or directory in _theme_process_registry() (line 565 of /.../includes/theme.inc).
Warning: include_once(): Failed opening '/.../sites/all/modules/jw_player/theme/jw_player.theme.inc' for inclusion (include_path='.:/usr/lib/php:/usr/local/lib/php') in _theme_process_registry() (line 565 of /.../includes/theme.inc).

But, I just got this when trying to apply jw_player-refactor_theming-2713725-2.patch to jw_player-7.x-2.x-dev:

(Stripping trailing CRs from patch.)
patching file README.txt
(Stripping trailing CRs from patch.)
patching file jw_player.drupal.js
(Stripping trailing CRs from patch.)
patching file jw_player.api.php
(Stripping trailing CRs from patch.)
patching file jw_player.module
Hunk #1 FAILED at 64.
Hunk #2 FAILED at 89.
Hunk #3 FAILED at 116.
Hunk #4 FAILED at 238.
Hunk #5 FAILED at 283.
Hunk #6 FAILED at 298.
Hunk #7 FAILED at 482.
patch: **** malformed patch at line 761: function jw_player_preset_plugins($name = NULL) {

Just putting this here to make it easier to find via searches. Thanks for everyone's time and effort!

Note that file is created here:
/.../sites/all/modules/jw_player/jw_player.theme.inc

but the error is looking for it here:
/.../sites/all/modules/jw_player/theme/jw_player.theme.inc

Nice work. Applies fine here as well.

1. +++ b/plugins/export_ui/jw_player_ctools_export_ui.inc
   @@ -77,6 +77,26 @@ function jw_player_ctools_export_ui_form(&$form, &$form_state) {
   +  if (jw_player_use_legacy()) {
   +    $disabled = TRUE;
   +    $desc = t('When creating JW Player presets, select if the source should be defined on Drupal, or by using definitions set within your JWPlayer.com account. <strong>This option is only available for sites using JW Player 7 or above.</strong>');
   +  }
   +  else {
   +    $disabled = FALSE;
   +    $desc = t('When creating JW Player presets, select if the source should be defined on Drupal, or by using definitions set within your JWPlayer.com account.');
   +  }
   +  $form['settings']['preset_source'] = array(
   +    '#type' => 'radios',
   +    '#title' => t('Preset source'),
   +    '#disabled' => $disabled,
   +    '#options' => array(
   +      'drupal' => t('Use Drupal-defined presets'),
   +      'jwplayer' => t('Use presets defined on JWPlayer.com'),
   +    ),
   

   I've been wondering if it makes sense to allow overriding a local player with a preset.

   You might end up with a locally loaded jw player and another cloud player JS file.

   But I guess not a bigger problem than multiple cloud players. So, why not...

2. +++ b/plugins/export_ui/jw_player_ctools_export_ui.inc
   @@ -284,25 +421,150 @@ function jw_player_ctools_export_ui_form(&$form, &$form_state) {
   +      '!admin' => l('JW Player general settings', 'admin/config/media/jw_player/settings'),
   +    )),
   +    '#default_value' => !empty($settings['player_library_url']) ? $settings['player_library_url'] : variable_get('jw_player_cloud_player_default', FALSE),
   +    '#states' => array(
   +      'required' => array(
   +        ':input[name="settings[preset_source]"]' => array('value' => 'jwplayer'),
   +      ),
   

   Hm, still not convinced that it is better to have this required with the default. It might not behave as expected when you change the default because the unchanged presets would still use the old default then. But if you change the default settings, you have to revisit all your presets anyway I guess.

Decided to commit this like this. We can open a new issue if we want to discus the preset cloud URL behavior. I could also imagine an explicit third option "Use default cloud settings" when using the cloud player.

PS: I managed to trick the validation by accidentally copying the license key into the cloud URL, then switch to local and save. Was confused when the cloud-preset default value was the license key. But I don't think that's worth to care about.

PPS: Would be awesome If you're interesting in porting those patches to 8.x after they are in. Could be a good opportunity to learn about D8 and I can help when you get stuck.

Another thing that I forgot. We need to check if this needs an update function when we start to rely on thew new cloud/local variable and set it automatically, according to the existing settings.

Yes, new issue is fine.

Sorry, the first part was wrong/unclear. What I meant is overriding a local player with a *cloud* preset. But as I said, likely not weirder/more problematic than multiple cloud players.

Some improvements were also committed in #2713687: Improve formatter summaries.

And some more in #2725003: Remove remote Preset Source for JW Player 7 self-hosting.

Not finished yet, but most of the port is done, need to fix the formatter summary for the image field.

Added working version, just realized that autoplay is not working, I will open a followup for it.

Finally got a working version and it tests fixed.

Fixed some other things, now when no sharing site is selected all are selected by default. :D

Followup for fixing autostart created: #2767145: Fix autostart

+++ b/jw_player.module
@@ -193,3 +195,144 @@ function jw_player_get_key() {
+/**
+ * Helper function to display JW Player preset settings.
+ *
+ * @param string $preset_machine_name
+ *   Value set of the preset's machine name.
+ * @param string $format (optional)
+ *   Whether the preset settings are returned as an 'array' or formatted 'string'.
+ *
+ * @return array|string
+ *   The preset settings stored in an array or as an imploded string for rendering.
+ */
+function jw_player_preset_settings($preset_machine_name, $format = 'string') {
+  $summary = [];
+  $preset = Jw_player::load($preset_machine_name);

we have an entity in D8, so instead of passing in a preset machine name, we can make this a method on the entity.

Function moved. Also made some other small changes. :)

+++ b/src/Form/JwplayerSettingsForm.php
@@ -57,6 +75,23 @@ class JwplayerSettingsForm extends ConfigFormBase {
+            [
+              'select[name="jw_player_version"]' => ['value' => '5']
+            ]

lets drop support for selecting 5.x completely in a separate issue.

As discussed:

* Drop the stored hosting setting, it just makes things complicated (update function, ..) and it's not used yet. we can still introduce it if we really need it but I doubt it
* Some more tests for the legacy vs. non-legacy conditions in the preset form and selecting some more options and making sure it shows up in the UI.

As discussed changes made. Follow up opened: #2769853: Remove Jw Player 5 support.

Thanks, committed.