blazey [blazey]

Hello, and thanks for applying for a CVS account. I am adding the review tags, and some volunteers will review the code, and report what needs to be changed.

As per http://drupal.org/cvs-application/requirements, the motivation message should be expanded to contain more details about the features of the proposed module, and it should include also a comparison with the existing solutions.

Files available from third-party sites should not committed in CVS.

We actually review the code that will be committed in CVS; if files available from third-party site will not be committed, then those files should be removed from the archive uploaded here.

  include_once dirname(__FILE__) . '/cck_jplayer_widget.inc';

There is a function to call for such purposes; if the file is loaded in any cases, then it should be merged with the module file.

See http://drupal.org/coding-standards to understand how a module should be written. In particular, see how the code should be formatted, and what indentation character should be used.

Hi, I tested it on my dev site: http://dev.talentfactoryinc.net/news/news-1, great work!, we need this module, it works with html5 which means it works with iphone, hope to seeing more options. Great alternative for none flash friendly browsers.

1. Consider naming the module just "jplayer", as prefixing with cck could get confusing with other modules, and in the long term there shoudl only be one module that provides jplayer integration with Drupal.
2. You should include a basic README.txt
3. The Blue Monday files seem to be third-party? Are they GPLv2? Also, I am not sure how they are used?
4. Consider using the libraries module instead of using a specific folder to store the jplayer.
5. CSS files need a @file docblock and correct formatting.
6. Your tpl file needs to have $Id$ and a @file docblock. Also, you need to be using t() in the tpl file as well. You should prefix classes with your modules name, unless this is needed for jplayer specifically.

7. /*
    *  Description: hook_theme 
    */
   

   Note the format of the docblock and the hook implementation standards:

   /**
    * Implementation of hook_theme().
    */
   

8. function cck_jplayer_install() {
     content_notify('install', 'cck_jplayer');
   }
   function cck_jplayer_uninstall() {
     content_notify('uninstall', 'cck_jplayer');
   }
   function cck_jplayer_enable() {
     content_notify('enable', 'cck_jplayer');
   }
   function cck_jplayer_disable() {
     content_notify('disable', 'cck_jplayer');
   }
   

   These all need to be in an .install file.

9.     'cck_jplayer_widget' => array(
         'arguments' => array('element' => NULL),
         'file' => 'cck_jplayer_widget.inc',
       ),
   

   This refers to a file that does not exist.

10.       'label' => t('Audio player'),
    

    As there are other audio players, consider calling this something more specific.

11. It looks like you are checking file types by extensions. 1) Can you not use MIME type? 2) Is this necessary as filefield will not allow other extensions?
12. You need docblocks for all functions. See: http://drupal.org/coding-standards and http://drupal.org/project/coder
13. In JS, functions and files need docblocks as well.
14. In JS, look at using Drupal.behaviors so that the player can react to AJAX and JS events.

15.         $("#jquery_jplayer").jPlayer("setFile", myPlayList[playItem].mp3, myPlayList[playItem].ogg);
    

    This line suggests that .ogg is supported as well.

16. 
var files = settings.files;
var myPlayList = new Array();
for (i in files) {
myPlayList[i] = files[i];
}

All this seems a little redundant and wasteful.

Overall pretty good. Keep up the good work.

Remember to change status, when you upload new code.

I am still curious if the Blue Monday "theme" is used. I don't remember seeing it in the code anywhere? If it is not used, it should definitely not be included, and overall, the policy is not to allow third-party code. I always have trouble finding the article on third-party code.

See http://drupal.org/node/422996.

Well, I think you have a valid point, but I am still new to reviewing and I do not actually have power, so it would be nice if we could get someone who is more senior to review.

I think that it is generally difficult to find in the needed version means that, in example, the module is using an old version of a jQuery plugin, for which is not given anymore the link to download it. The fact you can provide a link to use to download the files mean it's not that hard to find the files; in any cases, it should be enough to add the download link in the project page, or in the README.txt provided with the module.

From what @kiamlaluno is suggesting, remove the third party code and put in instructions on how to obtain it.

I am changing status as per previous comment.

1. In jplayer.tpl.php, tabs need to be two spaces.
2. Functions in .install file need docblocks.
3. JS file needs to use two spaces for tabs, as well as overall Drupal JS coding standards.
4. You should be using Drupal behaviors instead of document.ready in JS. See this: http://mydrupalblog.lhmdesign.com/drupal-theming-jquery-basics-inc-drupa...
   

   $(document).ready(function () {
   

   Would be something like this:

   Drupal.behaviors.jplayer = function (context) {
     $('#jquery_jplayer:not(.jplayer-processed)', context).addClass('jplayer-processed').each(function () {
   

5. Also, looking at this above, using an IDs in your HTML limits all this to one player per page. This seems less than ideal. You should be setting an ID on the top level element in the template that uses the field name or something like that.

Looking better!

I found this, and I would suggest a couple of things myself as someone who is very interested in using this.

1) Create an admin section to turn various config options on/off. One reason for this is autoplay. I really don't want the file to play automatically, but others might.

2) Configurable skin options. I like 1PixelOut's simplicity.

3) Downloadable link for the file.

Just some tips from a potential user of the module.

1. You really need to rename classes and IDs to suport multiple players per page (nodes). It is really important as everyone assumes you can add multiple fields of the same type to the same content type. This should be fairly easy to make an ID from the field name and keep the classes you are already using.
2. For the future, any settings (like ones suggested @ccoppen) should be put in the widget settings, not in an admin page.

Unfortunately this has just been released:
http://drupal.org/project/jplayer

I think the new way to go is to try to become a co-maintainer of the module or put up another module for review. I am sorry.

Hi blazey, I am terribly, terribly sorry. I searched for existing modules first and didn't find any sign of one, so I went ahead and created the project. However I feel that it's taken into account most, if not all, of the concerns mentioned here (autoplay, unique IDs for multiple players, GPL concerns, etc.) In addition we also have support for multi-value CCK fields being displayed as a playlist, and views support for displaying an arbitrary collection of files as a playlist. It doesn't create a new widget for CCK, since adding such a widget is completely unnecessary. Instead it just creates formatters for existing FileFields similar to the way ImageCache or SWFtools work.

Regarding the new project, I am already responsible for plenty of other modules and I would be very happy to turn over ownership to blazey. I'd prefer to stay on as a co-maintainer (at least until we have a final release and Drupal 7 version), but since I totally slighted a new contributor accidentally, I'd be okay with turning over the project entirely if he'd like full responsibility for it. If his application is approved I'll transfer the project over to him.

So, it sounds like we should create a CVS account for blazey and quicksketch can make him a co-maintainer. Passing onto someone with power.

@zzolo: You can now approve CVS accounts (see #758046: Proposal to give to zzolo the role of CVS administrator).

@blazey you now have CVS access. You'll have to coordinate with @quicksketch to get co-maintainer access to the jplayer module. I have included some links in the message with your CVS approval; please read these to understand how Drupal CVS works.

I've added blazey as a maintainer and we'll co-ordinate any further details through e-mail. Thanks zzolo, kiamlaluno, and sun for the reviews.

how can enable support. ogg?

@kolebas this queue is specifically for CVS applications. Please see the jPlayer issue queue for support questions:
http://drupal.org/project/jplayer
http://drupal.org/project/issues/search/jplayer

I am giving credits to the users who participated in this issue.