Let image.inc be an api, and move GD toolkit code into image.gd.inc

It appears that the patch in #3 does not include the new file - I think you need to put the -N flag before your -F^f.

The patch is straight forward, but:

  function image_gd_info() {
    return array('name' => 'gd', 'title' => 'Build-in GD2 toolkit.');
  }

You could change it to: 'title' => t('Built-in GD2 toolkit')

  if ($toolkit = variable_get('image_toolkit', 'gd')) {
    drupal_set_message("De huidige toolkit is $toolkit");
    include_once './includes/image.'. $toolkit .'.inc';
  }

Could just be changed to:

  $toolkit = variable_get('image_toolkit', 'gd')) {
  include_once './includes/image.'. $toolkit .'.inc';

Removed the message; and the if () check, because it's always going to have a value, either the one set by the user, or the default i.e. 'gd'.

Some more features could be introduced so that contributed modules can make use of introducing new libraries, that can be a separate patch though.

simple patch. improves consistency. Allows cleaner imagemagic integration. works.

Humm, I couldn't apply the patch...

patching file includes/image.gd.inc
patching file includes/image.inc
patch unexpectedly ends in middle of line
patch: **** malformed patch at line 385:

I'll give it a try on another machine.

I think this is an excellent idea. It makes for a much cleaner interface.

I made most of the changes Gurpartap Singh pointed out but ended up restoring a good chunk of the original image_get_toolkit() function. The primary reason being that users can delete the toolkit files. I get bug reports on the image module from users who'd deleted the imagemagik toolkit and couldn't get the setting to switch back. If the file's missing there's no toolkit.

Steef, I'm unclear why you changed the name from gd to gd2... It is more accurate but you will need an update to change the image_toolkit variable's value from gd to gd2.

okay so steef and i talked on IRC and decided that the patch on #9 is the way to go.

Wow, this is really one of those "You mean it didn't already work like this?!" patches. It makes far more sense for gd-specific functions to be in image.gd.inc than in image.inc. +1.

Functionality-wise, I tested the two places in core where the image API is called: user pictures and upload.module (when max definitions are specified). In both cases, everything worked as expected.

Patch #9 is RTBC.

/subscribing (late)

Reviewed the code:

- The gd file should have a @file comment, it does not have this phpdoc tag there now
- The image_gd_info() function could use a short explanation. Is it a hook implementation?
- The t() change I see with the watdog() call is definitely not right. Watchdog() calls *should not* have t()-ed strings (possibly except their last link parameter) in Drupal 6, as they are localized on display, not on logging.

Otherwise this seems like a logical code cleanup (so IMHO it is on track to be committed) but I don't see how this provides any new possibilities, unlike how others pointed out.

Since there's a chance that this might get committed, I've gone through and modified all the comments to follow the coding standards.

I made one small change to image_rotate(). It now lets you pass a background color to the toolkit. The GD toolkit has always accepted the parameter which defaulted to 0.

@Stefan: well, dikini pointed out that this allows cleaner imagemagic integration, which I don't see. Anyway, this does not make this patch better or worse ;)

Seems like the latest patch has file comments misplaced. It has a GD2 file comment on top of image.inc, and a generic one on top of gd.inc.

Let's get reviews on the patch, so we have more eyes seeing it works, and I can commit it.

This patch seems to introduce API changes (i.e. addition of background color) and should probably be postponed to D7 instead? Please motivate this change, and I'll consider to commit it.

Please rename $bg_color to $background.

Dries, technically, it is a change. All it does though is to correct one annoying wart in the image API. It does so by adding an additional, optional, parameter that defaults to its current behavior.

image_gd_rotate() already accepts the background color parameter but image_rotate() isn't providing anything to it. So if you wanted a white background on a rotation you had to callimage_toolkit_invoke('rotate', ...)); and pass it in yourself.

I'm OK with committing this patch for D6 -- it doesn't break anything. However, I'd like to see us add some code comments to the top of image.inc, and maybe extend the code comments on top of image.gd2.inc to reference image.inc.

At the top of image.inc, it would be good to explain the purpose of this file, and the basic design of the image API. Specifically, it is worth explaining why it is important to use Drupal's API rather than using PHP's functions directly. Why would you _not_ want to use GD2 -- it's part of a standard PHP installation. Explain why other toolkits might be better?

It's also useful to explain that one can swap in different image backends, how that is done, what backends are supported out of the box. For developers, it's really useful to know this all works from a high-level point of view. Furthermore, it might also be useful to explain certain limitations -- can I have more than one active backend at the same time?

If we can add one or two paragraphs of text to the top of image.inc, then this makes for a good commit.

Thanks.

first for anyone who's testing at home, here's an imagemagick toolkit that works with D6. re-roll to follow.

poked through the database.inc to get an idea of how to add a @defgroup. i haven't run it through the api.module but it looks like it should be alright.

when you know how something works docs are hard to write, i never know what questions to answer. hopefully someone can take a look at those and make sure they make sense.

Committed to CVS HEAD. Good job.

It looks like you only committed part of the patch... if you didn't like my docs you could have just said so ;)

Committed to CVS HEAD. Thanks.