Re: imwrite parameter framework patch (with full gif support)

[John Swensen]

On Oct 1, 2010, at 2:49 AM, John W. Eaton wrote:

> 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

Oh my!.  Sorry for all the problems.  I didn't realize I was making you fix 
quite that much stuff in my previous patches.  I guess most of my previous 
patches were much smaller.  I will get everything fixed and re-submit.

John