Created attachment 11194 [details] [review]
patch fixing the problems

Created attachment 11195 [details]
variant with some additional optimization

Created attachment 11196 [details]
even more compact variant

Created attachment 11197 [details]
some testcases

Hmm, not to dump on you problems that were there before
with color parse, but:

 - I'd probably make i size_t, even if being able to 
   pass a 2gig string here is highly improbably.

 - r,g,b need to be unsigned - the current result will
   produce incorrect results for four digits entries
   of the form #aaaabbbbccccc, if I'm not mistaken

 - The function should have a g_return_if_fail (spec != NULL, FALSE);

 - Input like "#ff aa bb" should not be valid, but I think
   it will be with the current patch; I suspect the code
   isn't going to be able to rely on scanf and probably needs
   to use strtod directly.

 - Better to write (i * 4) than (i << 2) when multiplication 
   by 4 is what is intended. The latter is no more efficient
   and much less clear.

 - I think the results are subtly "wrong" for i == 3 as 
   compared to usual practice ... I'd expect 
   #100100100 to be equivalent to #100110011001 not 
   to #100010001000. The generic scale algorithm from
   len bits to 16 bits I'd recommend is:

    c = c << (16 - len);
    while (len < 16)
      {
         c |= (c >> len);
         len *= 2;  
      }

    It's probably best to encapsulate this in a subroutine;
    if speed (and maybe even clarity) is important, it
    might be better to simply special case for len == 4,8,12,16
    rather than writing the loop as above.

 - the test cases need to test the resulting values, not
   just incorrect/correct.

I guess it depends on what you would call "correct". Here is what
man XParseColor has to say about #rrggbb and friends:

       An RGB Device specification is identified by the prefix
       ``rgb:'' and conforms to the following syntax:

       rgb:<red>/<green>/<blue>

           <red>, <green>, <blue> := h | hh | hhh | hhhh
           h := single hexadecimal digits (case insignificant)

       Note that h indicates the value scaled in 4 bits, hh the
       value scaled in 8 bits, hhh the value scaled in 12 bits,
       and hhhh the value scaled in 16 bits, respectively.

       For backward compatibility, an older syntax for RGB Device
       is supported, but its continued use is not encouraged.
       The syntax is an initial sharp sign character followed by
       a numeric specification, in one of the following formats:

       #RGB                (4 bits each)
       #RRGGBB             (8 bits each)
       #RRRGGGBBB          (12 bits each)
       #RRRRGGGGBBBB       (16 bits each)

       The R, G, and B represent single hexadecimal digits.  When
       fewer than 16 bits each are specified, they represent the
       most significant bits of the value (unlike the ``rgb:''
       syntax, in which values are scaled).  For example, the
       string ``#3a7'' is the same as ``#3000a0007000''.

So if we want to be compatible with that, we should go with the even
simpler

	  x = 16 - 4*i;
	  color->red = r << x;
	  color->green = g << x;
	  color->blue = b << x;

Note that this would contradict the doc comment about #ffffff being
white (which probably a lot of people would expect from HTML). Thus it
is probably not a good idea. We should note the incompatibility to 
XParseColor in the docs, though. 

Maybe we should additionally support rgb:rr/gg/bb ?

Created attachment 11205 [details]
sscanf-less version which scales properly

Created attachment 11206 [details]
more tests; also check values

The incompatibility with XParseColor() is intentional -- the
XParseColor() behavior is a bug that the X people thought
they couldn't fix for compat reasons without introducing
a new syntax, but my feeling was that people would expect
correct behavior not one that was bit-per-bit identical
to XParseColor().

I'd rather not open the rgb: can of worms... would we also
support rfbi? TekHVC:? (Note that nautilus actually did
use (IIRC) rgbi:, so broke when we switched to pango_color_parse()
in 2.0, but that has been since fixed / worked around.)

Two quibbles

* The parameters for hex() should be broken into multiple lines

* I think it would read better with:

   int bits = i * 4;

  Than i *= 4; (Less like assembly with a limited number of 
  registers :-)

Other than that, looks great. If you could put the
implementation into Pango (both branches) and the test
(without the g_print()s) into pango/tests (HEAD only
would be fine), that would be wonderful.

Thanks.

I agree that rgb: would be redundant if # does the right thing and
anything beyond that (rgbi:, cieluv:) would require calibration and 
is definitively too much for pango.

Interestingly, the copy of this code in gdk-pixbuf/io-xpm.c should
perhaps be "fixed" in the other direction, ie changed to behave like
Xlib, since XParseColor is probably what most existing xpms were
developed against...

Committed to both branches (accidentally added the tests to stable
as well, hope you don't mind).