For displaying very large images, a map-style interface is useful. This module provides a formatter to display images in the OpenLayers mapping interface, and a drush command for batch preprocessing the images into tiles at the necessary different zoom levels.

Dependencies:
http://drupal.org/project/openlayers
http://drupal.org/project/media

Project page: http://drupal.org/sandbox/noahadler/1787024

Git clone command:
git clone --recursive --branch 7.x-1.x http://git.drupal.org/sandbox/noahadler/1787024.git media_imagezoom

This is for Drupal 7.

Comments

killua99’s picture

killua99 commented

All the code is really good. Don't see any problem here.

bripatand’s picture

bripatand commented

You should remove your name from the above git command and use:
git clone http://git.drupal.org/sandbox/noahadler/1787024.git media_imagezoom

Anyway, when running the command, I got the error message:
warning: remote HEAD refers to nonexistent ref, unable to checkout.

noahadler’s picture

noahadler commented

@bripatand, thanks, not sure how I missed that! I've updated the original post. There is no master branch though, so it does need to retain the --branch 7.x-1.x, otherwise you'd have to git checkout 7.x-1.x after you clone. I think that's the cause of that error.

roynilanjan’s picture

roynilanjan commented
Status: Needs review » Needs work

Have a look few code observations,

In _media_imagezoom_tile function,

  1. What about variable $x,$y please give some meaning full name according to naming convention & in the comments give some @param description
  2. public://tiles/tms/1.0.0/ should not be hard coded should some configurable from admin!
  3. @drupal_mkdir -=> why want to suppress some warning & error during create of directory>
  4. if I disabled my watchdog(to minimize the database volume) in my application what about your watchdog() function execution line?
  5. Put your
    drupal_set_message('Cannot tile non-image media');

    in the else part & if you want to return some debuging information of an object then should have some option(do you want debug the problem? if yes then enable the devel module & use it's debug functions (dpm,dvm etc..) instead of direct print_r other wise if user get an error & long object format will be confused

vineet.osscube’s picture

vineet.osscube commented

Status - Needs Work

Manual Review :

When you are emitting information which is important for the logs, use drush_log() instead of drush_print(). This is helpful because command can be executed remotely and drush_log() insures that your log info gets passed back to the caller correctly.

For example, the 'package downloaded' log info should be:

OLD: drush_print("{$this->project->name} downloaded from {$this->project->download['url']}.");
NEW: drush_log("{$this->project->name} downloaded from {$this->project->download['url']}.", 'ok');

noahadler’s picture

noahadler commented

Thanks for the feedback! I've updated the code to try to meet most of the issues encountered above, including some comments where necessary.

@roynilanjan, afaict, watchdog() is a function defined in the bootstrap.inc part of core, so is always available to call, even if you disable the dblog module.

I've replaced the hard-coded tile paths with a variable_get for the path to the tiles, and I'll work on creating an admin interface to make it easy to change this. I didn't include the 1.0.0 portion, because it's a convention for these TMS-style tiles that the OpenLayers viewer automatically checks for.

I'll get that admin interface going asap!

roynilanjan’s picture

roynilanjan commented

One question if we use hook_watchdog instead of direct watchdog function - then it can be treated as logger according to severity!
- what do you think?

if you want to return some debuging information of an object then should have some option(do you want debug the problem? if yes then enable the devel module & use it's debug functions (dpm,dvm etc..) instead of direct print_r other wise if user get an error & long object format will be confused

=> what about this?

noahadler’s picture

noahadler commented

Hi roynilanjan,

I'm not sure what you mean about hook_watchdog. I think the watchdog function calls all the modules that implement hook_watchdog, such as dblog, or log4php. So it's already taken care of!

I did remove the print_r functions and replace them with dpm in the latest commits, and check that function_exists('dpm') too.

Still got to knock out the admin interface, then I'll set back to 'needs review'! Thanks for the help everyone.

roynilanjan’s picture

roynilanjan commented

Have some questions,

- If DB log module disable then where watchdog will log the information?
using hook_watchdog i can send email to site administrator (Error log information)even my dblog module disabled!

Please have a look http://api.drupal.org/api/drupal/modules%21system%21system.api.php/funct...
http://www.calebthorne.com/blog/2012/Jun/understanding-drupal-hookmail

noahadler’s picture

noahadler commented
Status: Needs work » Needs review

Update: I've added an admin interface that lets you set the 'base URI' for tile storage. This way you can change it from 'public://tiles' to private storage or different location.

@roynilanjan, I maintain that this module should not implement hook_watchdog, nor call it directly. This module simply calls watchdog to log diagnostic information about this module's functionality, which is deep image zooming, not system logging. The watchdog function, in turn, calls any module functions which implement hook_watchdog. This is standard behavior for Drupal hook system, which is well documented, and a rough equivalent of the Observer pattern. So in short, if you disable all your modules which do implement hook_watchdog, you will by design not get any logging from any systems through watchdog. Hope this make sense!

roynilanjan’s picture

roynilanjan commented

Sorry but conceptually wrong idea you have regarding,

watchdog calls internally hook_watchdog by using module_invoke but this is not a observation pattern this normal drupal hook behavior module_invoke or module_invokeall(this is implemented by PHP basic callback mechanism call_user_function-array)
dependency which we provide normally through info file actually this is a good example of observer pattern

One more thing I can't understand=>This module simply calls watchdog to log diagnostic information about this module's functionality, which is deep image zooming... what is difference between this and system loging? can you please explain..

Although i'll go thorough your module shortly & let you know if anything in my view

scotthorn’s picture

scotthorn commented
Status: Needs review » Reviewed & tested by the community

The code looks great to me. Especially your inclusion of drush commands for admins to pre-cut all the tiles if necessary. Nice.

@roynilanjan, I don't believe modules that use the watchdog function have to implement hook_watchdog. Just in core, there are ~16 modules that use the function, but only four that implement the hook:
http://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/calls...
http://api.drupal.org/api/drupal/modules%21system%21system.api.php/funct...

I think if you used a watchdog call at line 137 it would be sufficient. The only other small thing I noticed was what looks like a leftover print function from development on line 216. IMO after those it's ready to go!

noahadler’s picture

noahadler commented

Thanks for the review, @scotthorn! I've removed that print command-- it was, as you say, a leftover from development at some point.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

jthorson’s picture

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

One other comment ...

media_imagezoom.info
Remove these lines from the bottom of your info file:
; Information added by drupal.org packaging script on 2011-10-12
core = "7.x"

Otherwise, looks good!

Thanks for your contribution, PERSONNAME!

I updated your account to let you 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:

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get 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.

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

(not verified) commented
Issue summary: View changes

Change URL to remove username