imwrite parameter framework patch (with full gif support)

[John W. Eaton]

On  1-Oct-2010, John Swensen wrote:

| You may want to make some mods.  I have been trying to look at the
| changes you make to my patches to see if they are stylistic things
| that I can start doing to save core maintainer's time on future
| patches.  Let me know if you see any glaring bugs/holes.  I plan on
| starting to work my way through each of the format types and
| submitting another patch each time I complete an image format. 

There are some style guidelines in the Octave manual in the current
sources.

Here are some additional specific comments.

+2010-09-39  John P. Swensen <address@hidden>
+
+       * image/imwrite.m: Added function for default image options.

Please write the name of the function, for example:

        * image/imwrite.m (default_options_for_format): New subfunction.
        Use it to set default formats.

+function options = default_options_for_format (fmt,img)

Please put a space after commas in argument lists.

+  options = {};
+  switch (fmt)
+   case 'bmp'
+    # Do nothing; no default options, but not a warning.

Please use ## for indented comments, for the benefit of Emacs Octave
mode indenting.

+   case 'gif'

Please use "" to quote strings unless there is some good reason not
to.

+    warning (["imwrite - No default options exist for format: " fmt]);

Please take advantage of the format possibilities of the warning and
error functions when appropriate, and don't captitalize error
messages.  For example:

    warning ("imwrite: no default options exist for the %s format", fmt);

+  end

Please always use specific "end" tokens.  In this case, "endswitch"
instead of end.

+2010-09-30  John P. Swensen <address@hidden>
+
+       * DLD-FUNCTIONS/__magick_read__.cc (write_image): Added
+       framework for all supported image types.
+       (<format>_settings): Implemented functions for each supported
+       image format with skeleton code.
+       (gif_settings): Completely implemented all GIF settings.
+

Please include entries for all new functions.  Say what was added or
changed, not simply that something was added or changed.

+std::string ptolower (std::string str)
+{
+  std::transform(str.begin(), str.end(), str.begin(), tolower);
+  return str;
+}

If this function is only used locally, it should be static.  If you
intend to make a copy of the argument, I would prefer that it be
explicit.  Also, please follow the whitespace rules as described in
the style guide in the manual (and generally, the GNU coding
standards).  For example:

static std::string
ptolower (const std::string& s)
{
  std::string r = s;
  std::transform (r.begin (), r.end (), r.begin (), tolower);
  return r;
}

However, if you need caseless strings for property names or values,
maybe it would be better to use the caseless string class defined in
graphics.h.in?  Maybe we should split that class out of graphics.h and
put it in its own file?  It seems likely that it would be useful in
more places than just graphics.h or graphics.cc, but it doesn't make
sense to require you to include all of graphics.h just to get caseless
character strings.

+  /*
+   * BackgroundColor
+   * Comment
+   * DelayTime
+   * DisposalMethod
+   * Location
+   * LoopCount
+   * ScreenSize
+   * TransparentColor
+   * WriteMode
+   */

Please use C++ comments.

+  // NOTE: for future coders, the order in which properties are processed is 
important.

Please limit the length of lines, including comments, to less than 80
characters if possible.

+  bool isAppendMode = false;

Please don't use CamelCase.  Instead, write  is_append_mode.

+  try {

Please follow the GNU coding style and put braces on lines by themselves.

+        else
+          {
+            error ("imwrite: invalid type for 'gif' property '%s' (expects a 
string)", "writemode"); 
+          }

If there is only one statement following an else, there is generally
no need to use braces.  Also, in this error message, there is no need
to use a format specifier when you have a literal string corresponding
to it.  Instead, write

        else
          error ("imwrite: invalid type for 'gif' property 'writemode' (expects 
a string)"); 

+        if (!((dimensions (0) == 2 && dimensions (1) == 1) || (dimensions (0) 
== 1 && dimensions (1) == 2)))

Please split long conditionals before the || and && operators, like
this:

         if (!((dimensions (0) == 2 && dimensions (1) == 1)
             || (dimensions (0) == 1 && dimensions (1) == 2)))

In the comment

+            // Actually, ML makes it sound like they use values of the colormap
+            // for the background color.  Maybe this is so they don't actually 
introduce
+            // another color to the colormap.  This may be problematic as I 
don't think
...

it is OK to say Matlab (I don't know what ML is) but it doesn't make
sense to say "they" when referring to Matlab.  Maybe you mean to say
that "The Matlab docs seem to imply that colormap values are used for
the background color.  Maybe this is so that it is not necessary to
introduce another color into the colormap. ..."?

Would you please make these changes and resubmit the patch?

Thanks,

jwe