JW Player 7 includes the option to set 9 different skins, with no extra cost. We've included this functionality in a new patch.

Also, there seems to be a bug in the existing legacy skins. It seems as though the "beelden" and "stormtrooper" skins were combined into one. This is what we think it should be instead:

     $skin_options = array(
       'bekle' => 'Bekle*',
       'modieus' => 'Modieus*',
       'glow' => 'Glow*',
       'five' => 'Five*',
-      'beelden' => 'Stormtrooper*',
+      'beelden' => 'Beelden*',
+      'stormtrooper' => 'Stormtrooper*',
       'vapor' => 'Vapor*',
       'roundster' => 'Roundster*'
     );

A patch will be posted for review in a few minutes.

CommentFileSizeAuthor
#13 fix_legacy_skins_and-2713675-13.patch5.33 KBjohnchque
#13 interdiff-2713675-11-13.txt2.89 KBjohnchque
#11 interdiff-2713675-9-11.txt1.49 KBjohnchque
#11 fix_legacy_skins_and-2713675-11.patch3.99 KBjohnchque
#9 fix_legacy_skins_and-2713675-9.patch2.49 KBjohnchque
#6 jw_player-skins_jw7_fix_legacy-2713675-6.patch2.9 KBsgdev
#2 jw_player-skins_jw7_fix_legacy-2713675-2.patch2.64 KBsgdev

Comments

ron_s created an issue. See original summary.

sgdev’s picture

sgdev commented
StatusFileSize
new jw_player-skins_jw7_fix_legacy-2713675-2.patch2.64 KB

Attached is a patch for review. Thanks.

sgdev’s picture

sgdev commented
Status: Active » Needs review

Sorry, forgot to change the status.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work 
+++ b/plugins/export_ui/jw_player_ctools_export_ui.inc
@@ -91,30 +91,50 @@ function jw_player_ctools_export_ui_form(&$form, &$form_state) {
+    '#description' => t('Select a player skin.@premium', array(
+      '@premium' => jw_player_use_legacy() ? ' Some skins (*) require a premium license.' : '',
+    )),

This doesn't work. The second part is then not translatable. And two t() calls are problematic, for example for right to left languages.

The safest way is to have two complete t() strings, one without the premium part and one with. Then use one or the other: legacy ? t('bla with premium') : t('short version').

sgdev’s picture

sgdev commented

Sorry about that, we can fix this right away and get you a new patch. Thanks.

sgdev’s picture

sgdev commented
Status: Needs work » Needs review
StatusFileSize
new jw_player-skins_jw7_fix_legacy-2713675-6.patch2.9 KB

Ok, this is definitely a better approach. Should have done it this way originally.

Let me know if the CRLF issues are resolved with this patch too. Thanks.

  • Berdir committed 5d422e5 on 7.x-2.x authored by ron_s 
    Issue #2713675 by ron_s: Fix legacy skins and allow JW Player 7 skins to...
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Version: 7.x-2.x-dev » 8.x-1.x-dev
Status: Needs review » Patch (to be ported)

Thanks, looks good now. Definitely something to improve in 8.x as well. Not sure about the current situation there.

johnchque’s picture

johnchque
Primary language Spanish
Location Bolivia
commented
Status: Patch (to be ported) » Needs review
StatusFileSize
new fix_legacy_skins_and-2713675-9.patch2.49 KB

We were not listing any predefined skin at all in the d8 version. :)

Status: Needs review » Needs work

The last submitted patch, 9: fix_legacy_skins_and-2713675-9.patch, failed testing.

johnchque’s picture

johnchque
Primary language Spanish
Location Bolivia
commented
Status: Needs work » Needs review
StatusFileSize
new fix_legacy_skins_and-2713675-11.patch3.99 KB
new interdiff-2713675-9-11.txt1.49 KB

Tests fixed.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work 
+++ b/src/Tests/JwPlayer7ConfigurationTest.php
@@ -87,7 +87,7 @@ class JwPlayer7ConfigurationTest extends WebTestBase {
-    $this->assertNoField('settings[skin]');
+    $this->assertFieldByName('settings[skin]');
     $this->assertFieldByName('settings[advertising][client]', 'vast');
     $this->assertFieldByName('settings[advertising][tag]', 'www.example.com/vast');
     $this->assertFieldByName('settings[controlbar]', 'bottom');
diff --git a/src/Tests/JwPlayerConfigurationTest.php b/src/Tests/JwPlayerConfigurationTest.php

diff --git a/src/Tests/JwPlayerConfigurationTest.php b/src/Tests/JwPlayerConfigurationTest.php
index d1a9ada..4ed8e3f 100644

index d1a9ada..4ed8e3f 100644
--- a/src/Tests/JwPlayerConfigurationTest.php

--- a/src/Tests/JwPlayerConfigurationTest.php
+++ b/src/Tests/JwPlayerConfigurationTest.php

+++ b/src/Tests/JwPlayerConfigurationTest.php
+++ b/src/Tests/JwPlayerConfigurationTest.php
@@ -86,6 +86,7 @@ class JwPlayerConfigurationTest extends WebTestBase {

@@ -86,6 +86,7 @@ class JwPlayerConfigurationTest extends WebTestBase {
     $this->assertFieldByName('label', 'Test preset');
     $this->assertFieldByName('description', 'Test preset description');
     $this->assertFieldByName('settings[mode]', 'html5');
+    $this->assertFieldByName('settings[skin]');
     $this->assertFieldByName('settings[advertising][client]', 'vast');

Lets make sure we can actually select a skin then (different one for 6 and 7) and that it shows up in the configuration json.

johnchque’s picture

johnchque
Primary language Spanish
Location Bolivia
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2713675-11-13.txt2.89 KB
new fix_legacy_skins_and-2713675-13.patch5.33 KB

By now we can just assert skins like that. Will improve the tests in #2713701: Add preset sources, sharing, mute and validation

  • Berdir committed ef82e44 on 8.x-1.x authored by yongt9412 
    Issue #2713675 by yongt9412, ron_s: Fix legacy skins and allow JW Player...
berdir’s picture

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

Thanks, committed.

Status: Fixed » Closed (fixed)

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