[D7] Histogram

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

There are some warnings reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxmatthewmessmer2184823git

In histogram.php -
Line 85 and 89: Functions should be called with no spaces between the function name and opening parentheses.

or die ("Cannot Initialize new GD image stream");
or die ("Cannot Initialize new GD image stream");

See the coding standard - https://drupal.org/coding-standards#functcall

Matthew,

Interesting looking module. This my first project review so bear that in mind :)

• On the project page you might want to provide some use cases.
• Does your module only support JPEG image types? (i.e. your use of imagecreatefromjpeg in your histogram_make_histogram function). If so, should warn users in help file and on project page.
• I don't think you need to add field_ui in the requirements section of the README.txt. given that it is enabled in standard D7 installations.
• I would only display the help text via the help menu and not also on the config page as you have it currently set up.
• The help text could use some sub headings for the pages it applies to (ie config, manage fields, node add/edit etc.).
• What effect does the choice of configured bundles have on the function on the module? The widget still seems to be available in bundles that have not been enabled to work with the module.
• When updating nodes with the histogram widget the option to regenerate the histogram should be more obvious. Users (well at least I did) tend to miss or gloss over options and expect it to work with default settings.
• Try multi-line PHP comments if you want to appease code sniffer. You probably want to add an explanation for why you commented out code in the first place. Maybe add a @todo tag anyway.

Hope this helps

Hi machostache,
yes you have to move that commented part to dev branch, and release only stable version for now.

Hello, machostache!

Thanks for an awesome idea and module. It might be useful in some cases.
I have reviewed the module and I find some minor issues with it. But It works quite well and I can see possibilities on how it can be extended in the future.

I think the module can be promoted to full project, after small fixes:

1) histogram.php should be renamed to histogram.inc. php files by default executable, so it not safe.

2) Use theme_image in theme_histogram_formatter_default. Maybe somebody want to add additional class for your image or etc...

/**
 * Histogram theme formatter.
 */
function theme_histogram_formatter_default($item) {

  $output = '';
  $output .= '<img class="histogram" src="';
  $output .= url($item['name']);
  $output .= '" >';

  return $output;
}

3) Why you work only with nodes? We should provide functions for all entities. For now you can describe those limitations and fix it in the future.
if ($instance['entity_type'] == 'node') {

4) set_time_limit(50000) it's not great solution because we will have problems with performance. If it is a very difficult operation (need a lot of time) we should move it to the background (cron).

/**
 * Makes PNG file histogram.
 */
function histogram_make_histogram($image, $color = "#000000", $force_bw = FALSE, $hist_type = 1, $hist_width = 256) {
  set_time_limit(50000);
  if (!isset($image)) {
    drupal_set_message(t('An error occurred and processing did not complete.'), 'error');
  }

5) Please remove comments:

  // $histogram_r_compiled = array();
  // $histogram_g_compiled = array();
  // $histogram_b_compiled = array();

6) On uninstall process you should remove all variables that you created before:
foreach (variable_get('histogram_nodetypes', array()) as $type) {

7) I found that png, gif files not work because you use only $img = imagecreatefromjpeg($source_file).
Could you handle other cases imagecreatefrompng(), imagecreatefromgif()?

Now about global approach:
I want to know why you use additional settings form admin/config/media/histogram/settings? I didn't get it.

I add field to that content type, so I want to render histogram. Why I should set double settings "Select nodetypes which should be enabled for histogram generation." here admin/config/media/histogram/settings.

In my opinion we didn't need create new field type. Why user should each time select "Histogram Type", "Force Single Channel Histogram" for each node? Are you realy need that fields?
I think you can just implement field formatter for standart image fields:
1) Move that settings to field formatter form
2) "Histogram Type", "Force Single Channel Histogram" it just VIEW, but not buisness logic.
3) We can just implement 2 different View Mode for that node and output "Stacked" or "Combined" histogram.

Other approach implement new image effect (image style effect) hook_image_effect_info:
1) Implement your settings in effect form
2) if you want you can create two image styles with different settings:
"Stacked"
"Combined"
"Image overlay (image + histogram)"
etc.
3) Image style will handle and store all files! You just implement effect
4) Apply that image style to standard image field. Image -> effect -> call your histogram_make_histogram with image as argument (see) -> your calculate histogram and override $image->resource.

As result you can implement histogram for all Entities, Multiple fields, etc. in more common drupal way.

Altogether a great module and it's working now.

This is just my thoughts on how it could be improved... If you want I can help you with it.

Best regards,
WebMorozov

Hi,

For the 2nd point, you are calling theme function directly. For best practice instead of theme_image($variables) you should use theme('image', $variables).

Change is ok and the module works.

But when i generate a histo, i have 2 warnings:
Warning: Division by zero in _histogram_make_histogram() (line 115 of C:\wamp\www\neurones_import\sites\all\modules\histogram\histogram.inc).
Warning: Division by zero in _histogram_make_histogram() (line 116 of C:\wamp\www\neurones_import\sites\all\modules\histogram\histogram.inc).

The code in function _histogram_make_histogram() is too long, maybe can you create sub functions ?

I'm approving this live in the mentor booth at DrupalCon.

Thanks for the review deekayen! Usually we only approve RTBC applications that have been verified by an additional pair of eyes.

And you should also post our nice welcome message from https://groups.drupal.org/node/184389#promote :

Thanks for your contribution, machostache!

deekayen updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.

@klausi, yes, I understand there should be more eyes and I counted myself as those extra eyes. I did a code review in person, gave him my commentary, promoted him, and stuffed the remaining issues into the project issue queue. I didn't see any reason to keep him waiting any longer.