image CAPTCHA in PNG format: check GDlib's PNG support

This is certainly possible, however not implemented :)
using PNG instead of JPG requires a two line change in image_captcha.user.inc (around line 33):

    drupal_set_header("Content-type: image/png"); // instead of drupal_set_header("Content-type: image/jpeg");
...
    imagepng($image); // instead of imagejpeg($image);

(The choice between PNG and JPEG is pretty arbitrary, but a reason I chose JPEG is because it gives some extra noise and artifacts, which makes automatic character recognition slightly harder.)

Having a transparent background would involve some more changes however (mainly an appropriate call to imagecolortransparent() in _image_captcha_generate_image(). I don't have time to implement this for the moment, but I welcome patches :)

Hi machi27,

Thanks for your work and don't be afraid to post unfinished patches (as long as you mark them as such).
That way I can give you more feedback earlier.

Regarding the file type selection:
I think we should also have a look at file sizes (I don't have access to my Drupal development environment for the moment, so I can't test this now), but I guess the average file size of the JPGs will be smaller than the PNGs (especially if distortion and noise are enabled).
In this regard: PNG only has added value when transparency is desired.

In any case: I would avoid checkboxes, because they would make things more complicated UI-wise than needed.
I would use a select list (or radio buttons) with options:
* JPEG
* PNG with transparent background
If required/desired/requested, we then can always add an option
* PNG (without transparent background)
without much problem.

about your problem with transparency and distortion: please post your unfinished patch, so I can have a look

Indeed, the patch is not ok.

I don't use windows, so I can't help you with tortoise.
You should find more info at
http://drupal.org/patch
or more specifically at http://drupal.org/node/113172

If you keep struggling, you could also just post the files you changed, but I would prefer a patch.

There have also been some updates since you checked out the version you are working on
so you should run "cvs update".

thanks

Hi Machi27, apparently you've been busy working on this ;)

Some feedback:

• the patches like in #11 and #14 are almost good (against latest version and only showing what you changed), but best is to put everything in one patch file (so not one patch file per source file). I also have to convert the windows end of lines (carriage return + linefeed) to unix end of lines (linefeed only). What editor are you using? it seems to convert the unix end of lines to windows end of lines. Maybe there is an option in you editor to preserve the unix end of lines (drupal uses unix end of lines).
• I also noticed you changed the lines with "$Id ...$". No need to do this: those lines are automatically generated by the CVS-system, so they will be overwritten anyway.
• I noticed you use hardcoded values 1, 2 and for the image types. A better code style (better readability and code maintenance) to use constants. In image_captcha.module you should first define a constant, e.g.
  

  define('IMAGE_CAPTCHA_FILE_FORMAT_JPG', 1);
  define('IMAGE_CAPTCHA_FILE_FORMAT_PNG', 2);
  define('IMAGE_CAPTCHA_FILE_FORMAT_TRANSPARENT_PNG', 3);
  

  and then use these constants instead of hardcoded values.

• In Drupal we value our code standards very high: http://drupal.org/coding-standards. There are some indentation issues in you patch: indentation should be 2 spaces per level.
• It's a bit silly to create a fieldset for only one setting. And the feature is also rather advanced, so I would not put it that high on the settings page. I would put it in the "color settings" fieldset and change its title to "Color and image settings".
• you're mixing "jpg" and "JPG", I would do it all in capitals (they are abbreviations, not words) and "JPEG" instead of "JPG".
• in image_captcha.admin.patch: variable_get('image_captcha_type_img', 'jpg') this is wrong: the first argument should be the variable name, which is 'image_captcha_image_format' and the second should be the key of the options array (e.g. the constant IMAGE_CAPTCHA_FILE_FORMAT_JPG I defined higher)
• in image_captcha.user.patch: $img_type = variable_get('image_captcha_image_format'); this is also wrong, the first argument is ok, but there should be a second argument too
• about you getting the best results when the background is set to #fff: this is because your page background is probably white or at least a light color. On a design with a black background, you get best results if the captcha background is also set to #000. So the rule should be: set it close to the underlying background color.

This may look like a lot, but I guess this is one of your first Drupal patches (or maybe your first), and it takes some time before you get all those small details.
So no problem, I hope you've learned some things.

thanks for your work

I am using cygwin to make the patch, tortoise to download the last cvs . I am not sure about how to make "diff -up" with all the files that i want to compare.

Check if your version of diff under Cygwin has the option --strip-trailing-cr, maybe that will avoid the end of line problems. And you just have to list the files you want to create a patch for (if you don't specify files, diff will take all files that differ. So the following command should do it, I think:
diff -upw --strip-trailing-cr image_captcha.module image_captcha.admin.inc image_captcha.user.inc > pngtransparency.patch
(note the "-w" option is for ignoring changes in whitespace, which is often a good idea to add)

And again: no problem with the newbie mistakes, we've all been newbies. I hope you enjoy hacking Drupal.

Getting closer indeed.

I think you missed some spots to replace the hardcoded values to the constants
e.g. in several cases of variable_get('image_captcha_image_format', 1) and in the option list of the select widget. There are also some places where there are tabs instead of spaces for indentation.
Some UI strings need some rewording too.

I worked a bit on the patch and changed these small things myself.
be welcome to try it out

Hi,

no need to despair, you are doing fine ;)

I just wanted to tweak some small things that would be easier to do in the code than posting them here as a comment.

Anyway, about your questions:

1. drupal_set_header("Cache-Control: max-age=3600, must-revalidate"); was code for the 6.x-1.x branch that was no needed anymore, so I removed it all together.
2. about $img_type and $file_format you are right, I mixed some stuff. I wanted to harmonize the names of the constants (IMAGE_CAPTCHA_FILE_FORMAT_JPG), the drupal variable (image_captcha_file_format) and the local variables (file_format), but I forgot to change one case of "img_type". I prefer "file_format" over "img_format" (which could also refer to the image size) and "img_type" (which could also refer to color/grayscale settings). Patch in attachment should be better.
3. I didn't experience any end-of-line problems anymore whit your patches, so that should be fine, thanks
4. about the UI string rewording, you had
   

   Enter the hexadecimal code for the background color (e.g. #FFF or #FFCE90).[br/] For png-transparent-background best result set it close to the underlying background color.

   I removed the [br/], because it's best to avoid unnecessary markup and your sentence about PNG-transparency didn't look right grammatically. I changed it to
   

   When using the PNG file format with transparent background, it is recommended to set this close to the underlying background color.

   Another thing I tweaked was putting "JPEG" and "PNG" in t() (not really important) and also changed from "PNG - transparent background" to "PNG with transparent background" because the minus/dash in your version could be interpreted as "without". Another thing I did, was putting the file format option at the end of the "color and image settings" fieldset, because it is typically the least important.

I hope this gives you more insight in my changes.
Thanks anyway for your work

really annoying that previewing comments doesn't work #580564: Comment preview doesn't work anymore, I have to edit my post several times and then forget to attach the patch.
So here it is ;)

Ok, forget previous patch
this one should be more readable/understandable

yep, I committed the patch:
http://drupal.org/cvs?commit=266448
congratulations and thanks for your work and review.

There is still some follow up work to do:
now the GD-library is only checked for JPG-support (see e.g. #580364: Image CAPTCHA reports no GD, Drupal status report disagrees), I think checking for PNG support should be added too.

I think now with the addition of PNG support, things get more complicated.
But I think it's a bit out of the scope of this issue thread, so I decided to start a new one for this: #589388: refactor _image_captcha_check_setup() for GD/JPEG/PNG support and set this thread to fixed.
Thanks anyway