Inplace Gallery

Please do not push into the master branch, follow the instructions on naming conventions here:
http://drupal.org/node/1015226, git branching: http://drupal.org/node/1066342

Your code looks good. Coder gives me just one issue:

inplace_gallery.module

Line 8: There should be no trailing spaces
      'title' => t('Administer Inplace Gallery'), 

What I really don't like is the way you are processing the CSS in line 400 to 508.
I never did something like this but I'm shure there are better ways like: reading a css with placeholders; replacing the placeholders with the nid; adding it inline

regards

Yes, it is more clear if the master branch is empty. You can leave a README.txt there like this one http://drupalcode.org/project/rules.git/blob/refs/heads/master:/README.txt

• you should not include third party libraries with your module. This is only allowed under very rare circumstances, see http://drupal.org/node/422996
• the info file should not contain a "project" entry, see http://drupal.org/node/542202
• hook_schema(): "The primary identifier for a node.?" but the DB field is called name? Should it be a node id, but why the varchar then? Also it would be nice if you could provide useful descriptions to your DB fields, they don't have to be long.
• module file: @file doc block missing, see http://drupal.org/node/1354#files
• inplace_gallery_preset_load(): what is this function for? Does the same as inplace_gallery_preset().
• do not use $entity->nid, this will only work for nodes. Use entity_extract_ids() instead.
• info file: the term CCK does not exist in Drupal 7
• info file: you should depend on the field module, any other core modules that are reuired for your module?

• The Libraries API module is a recommended method for adding 3rd party dependencies without directly including the code on Drupal.org. It is not ideal to let people download the libraries to your module's folder because it makes updating the module harder.
• inplace_gallery_node_delete(): I think you want to use hook_entity_delete() instead. Also the "changed" property is not available on every entity type, so it would need some further adjustments to make this work cleanly with all entities. W
• inplace_gallery_node_delete(): why can't you use file_unmanaged_delete_recursive()?

No need to upload your module here in the queue, pushing to the git repository is enough.

I think this RTBC. Now while we are waiting for the access grants to create full projects would you be so kind to do a review of the other applications pending? Just pick one from this list: http://drupal.org/project/issues/projectapplications?status=8

Please take a moment to make your project page follow tips for a great project page.

The image title display is vulnerable to cross site scripting. To see this vulnerable enable the alt and title fields. Enter <script>alert('title');</script> as the title for an image and then save the node.

There is a text filtering cheat sheet which might be helpful. In this specific case I suggest using the l() function which provides automatic text sanitizing for the arguments in addition to building links.

The handbook on writing secure code has more information on how to write secure code in Drupal. Please read that page and all subpages of it.

don't RTBC your own issues, needs review first.

Review of the 7.x-1.x branch:

• Lines in README.txt should not exceed 80 characters, see the guidelines for in-project documentation.
• Comments: there should be a space after "//".
  

  inplace_gallery.module:420:  //Write CSS
  js/inplace_gallery_load.js:114:  //Assign handlers to the simple direction handlers.
  js/inplace_gallery_load.js:125:  //Swipe handlers.
  js/inplace_gallery_load.js:126:  //The only arg passed is the original touch event object
  

• ./inplace_gallery.module: comment lines should break at 80 characters, see http://drupal.org/node/1354#general
  

      // The last pics width has to fill up the full width if display is set to adjust
        // Scale first big image and set height for the following so every picture in one node has the same height
  

• ./inplace_gallery.admin.inc: comment lines should break at 80 characters, see http://drupal.org/node/1354#general
  

    // Flush all Caches, so the Formatter shows up on the Content Type->Manage Display Page
    // Flush all Caches, so the Formatter doesn't show up on the Content Type->Manage Display Page
  

• ./inplace_gallery.admin.inc: all functions should have doxygen doc blocks, see http://drupal.org/node/1354#functions
  

  
  function inplace_gallery_preset_form_validate($form, &$form_state) {
  --
  
  function inplace_gallery_preset_form_submit($form, &$form_state) {
  --
  
  function inplace_gallery_preset_delete_form($node, &$form_state) {
  --
  
  function inplace_gallery_preset_delete_form_submit($form, &$form_state) {
  

• Assignments should have a space before and after the operator, see http://drupal.org/node/318#operators
  

  ./js/inplace_gallery.admin.js:24:                for (i=0; i < number_pix; i++) {
  ./js/inplace_gallery.admin.js:62:                  for (i=0; i < number_pix; i++) {
  ./inplace_gallery.admin.inc:283:              for ($i=0; $i < $number_pix; $i++) {
  

This automated report was generated with PAReview.sh, your friendly project application review script. Please report any bugs to klausi.

manual review:

• jcarousel_skin_presets.css: indentation errors, see http://drupal.org/node/302199
• "'data' => theme_image($image_vars),": do not use theme_image() directly, but use theme('image', ...) so that themes can override it.
• "'alt' => check_plain($image['title']),": do not use check_plain() here, as theme_image() alreay takes care of attribute sanitization. Same for the title attribute. And in other places.

1. Line 128: in most cases, replace the string function with the drupal_ equivalent string functions

      $touch_version = substr($found['touchSwipe'], 18, 5);

2. In the inplace_gallery.module file, the function inplace_gallery_field_formatter_view function, for the controls html can be placed in a theme function, so that the function will be more readable.

3. In the inplace_gallery.module file, at line number 300, you added inline js to enable inplace_carousel, you can pass the entity ids to js and write the initialization code in the javascript file inplace_gallery_load.js.

4. In the inplace_gallery_load.js, using global variables activeThumbs, jcarousels is not a good idea. These global variables might get conflicts with external js includes. Eighter you prefix these variables with your module name or create a simple construction function to store these values and generate objects.

For point #3, look up Javascript Behaviors in Drupal. Using this API, your initialization code will run after the document is ready automatically. If you need to pass additional settings, use ['#attached']['js']['settings'] on any renderable element, or you can use drupal_add_js('settings', $array_of_settings). These will be available as Drupal.settings and passed to your behavior when it is launched.

http://drupal.org/node/756722 is a good resource.

Hi, I was so pleased to come across this module as a simple gallery with numbers rather than thumbnails is exactly what I'm after. However, after spending a whole day on it, I had to give it up, as I couldn't get the gallery to load the images. It just displays the alt information. When I click on the view image info, it tells me that there is no image where the path points to. I fiddled with image size, type, changed permissions etc. but no success. I would love to get this up and running, so if there is a simple solution, I'd be very grateful for a reply.

@flosuter has been contacted to ask if the application is abandoned.

After ten weeks with a status of needs work: the applicant may be contacted by a reviewer to determine whether the application was indeed abandoned. The action taken by the reviewer should be documented in the project application issue.

http://drupal.org/node/894256

Automated review

http://ventral.org/pareview/httpgitdrupalorgsandboxflosuter1277538git

Manual review

Use of untranslated strings in inplace_gallery_field_formatter_view()

In inplace_gallery.admin.inc:inplace_gallery_preset_form() $form key 'thumb_display' gets assigned translated string

'#options' => drupal_map_assoc(array(t('Fixed'), t('Adjust'), t('Buttons'), t('Numbers'))),

, but in inplace_gallery.module:inplace_gallery_field_formatter_view() these strings are used untranslated.

Over-commenting

Debugging function in theme_inplace_gallery_controls()

I believe you forgot to remove call to dsm() on first line of theme_inplace_gallery_controls().

inplace_gallery.admin.js

My editor does not complain about mismatched braces/parenthesis after

diff --git a/js/inplace_gallery.admin.js b/js/inplace_gallery.admin.js
index cb96d6e..bbc1289 100755
--- a/js/inplace_gallery.admin.js
+++ b/js/inplace_gallery.admin.js
@@ -125,7 +125,8 @@
         $('.form-item-thumbs-in-row').slideUp();
         $('.form-item-thumbs-fade').slideUp();
       });
-    });
+    }
+  };
 
     function set_fill_behavior() {
       $('.fill').click(function() {
@@ -145,5 +146,4 @@
         return false;
       });
     }
-  }
 })(jQuery);

There are undeclared variables in for loops in inplace_gallery.admin.js. direction and inplace_activeThumbs in inplace_gallery_load.js. In JavaScript they will be created in global scope and this might lead to strange bugs. You should declare every variable with var to prevent name collision in global scope.

I have to admit, I didn't look very closely, but code looks and "feels" good.

IMO such overly thorough commenting style is is a bit too much:

  // Count number of images in node.
  $number_of_items = count($items);

I've moved this point out of review, because this is really just my opinion and not something suggested in Drupal code standards.

Forgot to change status to "needs work".

@donatasp: Too much commenting could never be an issue in my book, it is not bad to explain code, even if it could be obvious for many.

@MiSc: I agree. This is a topic for a debate (without an end) and I shouldn't have put that in a review part of my post.