Right now the theme_ooyala_player() function hardcodes the call to player.js and the $_GET parameters that are passed to it. As a result in order to just add a single parameter to the call you have to override the whole function.

I propose passing the different $_GET parameters to the theme function as an array and then converting the array to the $_GET query string used with the player.js call.

I think that this will also help cleanup some of our theme_ functions which are starting to get really complicated function signatures, and will just continue to get more and more complex as we start making use of additional Ooyala Player API features.

CommentFileSizeAuthor
#7 999710.7-theme_function_refactor.patch13.44 KBdeviantintegral
#6 ooyala-999710-6-theme_function_refactor-eojthebrave.patch4.96 KBeojthebrave
#3 ooyala-999710-3-theme_function_refactor-eojthebrave.patch5.35 KBeojthebrave
#1 ooyala-999710-1-theme_function_refactor-eojthebrave.patch4.73 KBeojthebrave

Comments

eojthebrave’s picture

eojthebrave
he/him
Primary language English
Location Minneapolis, MN
commented
Status: Active » Needs review
StatusFileSize
new ooyala-999710-1-theme_function_refactor-eojthebrave.patch4.73 KB

And ... here's a first stab at this. I think these changes are a nice cleanup and will also make it easier to deal with this issue #962432: Implement the Player Authorization API in either a separate module or simply via the theme layer.

quicksketch’s picture

quicksketch
he/him
commented

This sounds like a good idea to me, you never know what parameters people are going to need. I'd like to see this code moved out of theme functions and into ooyala_player():

+  // Setup some default paramaters.
+  $params['embedCode'] = $embedcode;
+  $params['height'] = isset($params['height']) ? $params['height'] : variable_get('ooyala_video_height', 300);
+  $params['playerId'] = isset($player_id) ? $player_id : 'ooyala_player';
+  $params['width'] = isset($params['width']) ? $params['width'] : variable_get('ooyala_video_width', 400);

That'd make for less redundant code to override in the theme layer.

Also a nice way of writing something like this:

  $params['height'] = isset($params['height']) ? $params['height'] : variable_get('ooyala_video_height', 300);
  $params['width'] = isset($params['width']) ? $params['width'] : variable_get('ooyala_video_width', 400);

Could be switched to:

$params += array(
  'height' => variable_get('ooyala_video_height', 300),
  'width' => variable_get('ooyala_video_width', 300),
);
eojthebrave’s picture

eojthebrave
he/him
Primary language English
Location Minneapolis, MN
commented
StatusFileSize
new ooyala-999710-3-theme_function_refactor-eojthebrave.patch5.35 KB

Moved the $params stuff into the ooyala_player() function and cleaned up formatting for setting default values.

I also removed the $player_id argument from a number of functions since it was just being used to set $params['playerId'] anyway. You can still manually set it to whatever you want and this simplifies our function signatures even further.

With this change in place we'll probably want to recommend that people who want to manually display a video use the ooyala_player() function and not call theme_ooyala_player directly.

quicksketch’s picture

quicksketch
he/him
commented

This looks pretty good. What's the reason for passing in $video_data here when we're not using it? I suppose I'm also not clear on if $video_data is the value stored in CCK or if it's what's been retrieved from Ooyala directly.

eojthebrave’s picture

eojthebrave
he/him
Primary language English
Location Minneapolis, MN
commented

$video_data contains the length and status information pulled from Ooyala. It's not really necessary, and like you said we're not using it, but it may be information that someone may want to make use of in their theme. I'm cool with just removing it though as there are other ways to get at the data if necessary.

eojthebrave’s picture

eojthebrave
he/him
Primary language English
Location Minneapolis, MN
commented
StatusFileSize
new ooyala-999710-6-theme_function_refactor-eojthebrave.patch4.96 KB

Without $video_data.

deviantintegral’s picture

deviantintegral commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new 999710.7-theme_function_refactor.patch13.44 KB

The patch in #6 looks good to me. I rerolled it against master, and split it into 3 commits.

deviantintegral’s picture

deviantintegral commented
Status: Reviewed & tested by the community » Fixed

I've committed the commits from #7.

Status: Fixed » Closed (fixed)

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