Created attachment 8511 [details] [review]
Proposed patch

+
+      red = green = blue = 0;
+      red1 = LOWBIT (xvisual->red_mask);
+      green1 = LOWBIT (xvisual->green_mask);
+      blue1 = LOWBIT (xvisual->blue_mask);
+
+      for (i=0; i<nlookup; i++)
+      {
+        xpalette[i].pixel = red|green|blue;
+        xpalette[i].pad = 0;
+        red += red1;
+        if (red > xvisual->red_mask)
+          red = 0;
+          green += green1;
+        if (green > xvisual->green_mask)
+          green = 0;
+          blue += blue1;
+        if (blue > xvisual->blue_mask)
+          blue = 0;
+      }
     }

Could you explain this code?

Also, 

 * You have GDK_VISUAL_TRUE_COLOR listed in gdk_colormap_sync()
   where it can't happen and makes no sense.
 * I'd prefer to see the code for querying direct color
   separated entirely (probably differnet function)
   from the code for querying psuedo-color, since the
   logic is really entirely different.

The case statement for GDK_VISUAL_TRUE_COLOR is not needed and i will
be removing it.
I had put the logic for DirectColor in gdk_colormap_sync() itself
because we query the colors for other visual classes in that function.
Reading the comments, yes the direct color logic should be moved to
another function. 

Now for the explanation of the code. I can try although bad at
explaining  :-).
Basically what the code does that, is that for each component (red,
green and blue) we get all possible colors and call XQueryColors. 
I am better off explaining with an example.
On my machine the green mask is 7e0 i.e 1111100000.
The macro LOWBIT() would give me the value for green1 as 32 basically
it is 100000. Similarly for other components.
On the next iteration the value for green1 is 64 i.e is 1000000.
This goes till i complete the iteration.
Hope i am clear with the explanation...

I am going to attach the entire dump of the red, green and blue values
before i call XQueryColors(). This dump should be explain better of
what i intend to do.

One more thing, what shall be the name of the new function where the
direct color logic goes ? Shall i call it
gdk_colormap_direct_color_sync () ?

Created attachment 8539 [details]
dump of red, green and blue value prior to calling XQueryColors()

Created attachment 9207 [details] [review]
Patch I'm applying

I've attached the patch I'm applying to CVS; it's basically
your patch with the changes I suggested above. I replaced
the LOWBIT() stuff with what I think is equivalent code:

  for (i = 0; i < colormap->size; i++)
    {
      xpalette[i].pixel =
	(((i << visual->red_shift)   & visual->red_mask)   |
	 ((i << visual->green_shift) & visual->green_mask) |
	 ((i << visual->blue_shift)  & visual->blue_mask));
    }

Less efficient, but I think clearer. (Thanks for the explanation!)

Testing of the change I'm committing would be appreciated,
since I don't have a suitable display here to test on.

hu Jun 13 18:48:14 2002  Owen Taylor  <otaylor@redhat.com>

        * gdk/x11/gdkcolor-x11.c: Make work
        gdk_colormap_sync() work for DirectColor visuals
        as well. Fill in the ->colors array in
        gdk_screen_get_system_colormap () for DirectColor visuals.
        (#81954, Based on a patch from shivaram.upadhyayula@wipro.com)

Tested the changes on a Solaris box with DirectVisual class as the 
default visual. The changes work fine. Thanks Owen !