1) This patch fixes typo.

Created attachment 12663 [details] [review]
gtk+-head-gdk_x11_colormap_foreign_new.patch

2) This patch fixes default colormap size problem.

Created attachment 12664 [details] [review]
gtk+-head-gdk_colormap_new.patch

Created attachment 12665 [details] [review]
gtk+-2-0-gdk_colormap_new.patch

3) This patch fixes color store problem in
gdk_colormap_alloc_colors_private().

Created attachment 12666 [details] [review]
gtk+-head-gdk_colormap_alloc_colors_private.patch

Created attachment 12667 [details] [review]
gtk+-2-0-gdk_colormap_alloc_colors_private.patch

3) gdk_colormap_alloc_colors() is still broken. Please ignore
above gdk_colormap_alloc_colors_private patches.

3) This patch fixes color allocation problems in PseudoColor visual.

  a) gdk_colormap_alloc_colors_writeable():
     Rewritten.

  b) gdk_colormap_alloc_colors_shared():
     Renamed to gdk_colormap_alloc_colors_read_only().
     Fixed return value.

  c) gdk_colormap_alloc_colors_pseudocolor():
     Call gdk_colormap_alloc_colors_writeable() if the 
     colormap is private or writable color allocation is
     requested, calls gdk_colormap_alloc_colors_read_only()
     otherwise.

  d) gdk_colormap_alloc_colors():
     Call new gdk_colormap_alloc_colors_pseudocolor().

  e) gdk_colormap_free_colors():
     Don't call XFreeColors() if the colormap is private.

Now, color allocation in PseudoColor visual works well.

Created attachment 12680 [details] [review]
gtk+-head-gdk_colormap_alloc_colors.patch

Created attachment 12681 [details] [review]
gtk+-2-0-gdk_colormap_alloc_colors.patch

4) gtk 2.1 gdk_colormap_new() should set private->xdisplay.
The follwing patch fixes it.

Created attachment 12683 [details] [review]
gtk+-head-gdk_colormap_new-xdisplay.patch

The following patch includes all above patches against the latest cvs
head.

Created attachment 12729 [details] [review]
gtk+-head-gdkcolor-x11.c.patch

The following attachment is a simple color allocation test program.

Please try this program in PseudoColor visual display.

  $ ./testcolor --colormap new-alloc \
                [--no-writable] [--best-match]
  $ ./testcolor --colormap new \
                [--no-writable] [--best-match]
  $ ./testcolor --colormap system \
                [--no-writable] [--best-match]

Created attachment 12730 [details]
testcolor.c

Owen, is gtk+-head-gdkcolor-x11.c.patch OK?
Please let me know.
I really need this GdkColormap bugfix.

It can take a while to get bug fixes in the tree sometimes...
there is nothing much I can do about it. Time is finite.

I'll try to go through these patches before the next release.

I understand. Thanks.

Still working on understanding the changes here, but some 
procedural comments:

1) It helps me a lot if you can do:
  
    1 bug
    1 bug reports
    1 patch
     
   Multiple bugs in a single bug report, or a single patch
   fixing multiple independent bugs makes reviewing the
   patch much harder.

2) It's really important to keep patches as small as possible.
   For example, the section of your patch:

===
@@ -1042,13 +1047,15 @@
    */
   if (nremaining > 0)
     {
-      if (private->private_val)
-       return gdk_colormap_alloc_colors_private (colormap, colors,
ncolors, writeable, best_match, success);
+      if (private->private_val || writeable)
+       nremaining = gdk_colormap_alloc_colors_writeable (colormap,
colors, ncolors,
+                                                          best_match,
success);
       else
-       return gdk_colormap_alloc_colors_shared (colormap, colors,
ncolors, writeable, best_match, success);
+       nremaining = gdk_colormap_alloc_colors_read_only (colormap,
colors, ncolors,
+                                                          best_match,
success);
     }
-  else
-    return 0;
+
+  return nremaining;
 }

 /**
===
   
Could be:

===
@@ -1042,10 +1042,10 @@ gdk_colormap_alloc_colors_pseudocolor (G
    */
   if (nremaining > 0)
     {
-      if (private->private_val)
-       return gdk_colormap_alloc_colors_private (colormap, colors,
ncolors, writeable, best_match, success);
+      if (private->private_val || writeable)
+       return gdk_colormap_alloc_colors_writeable (colormap, colors,
ncolors, writeable, best_match, success);
       else
-       return gdk_colormap_alloc_colors_shared (colormap, colors,
ncolors, writeable, best_match, success);
+       return gdk_colormap_alloc_colors_read_only (colormap, colors,
ncolors, writeable, best_match, success);
     }
   else
     return 0;
===

The effect is the same, but by maintaining the same indentation
and changing the way the return value works, it's a lot easier
to see what changed.

Committing some of the easier parts of the above changes:

Sun Dec  8 20:19:22 2002  Owen Taylor  <otaylor@redhat.com>

        Fixes for GdkColormapX11 (#100023, Naofumi Yasufuku)

        * gdk/x11/gdkcolor-x11.c (gdk_x11_colormap_foreign_new):
        Fix typo that caused us to always return the system
        colormap.

        * gdk/x11/gdkcolor-x11.c (gdk_colormap_new): When allocating
        a private colormap and copying the system palette
        to prevent flashing, handle colormap->size greater than
        system_colormap->size.

        * gdk/x11/gdkcolor-x11.c (gdk_x11_colormap_get_xdisplay):
        Get rid of the last remains of private->xdisplay
        in favor of using private->screen.

More obvious stuff.

Sun Dec  8 20:32:23 2002  Owen Taylor  <otaylor@redhat.com>

        More fixes for GdkColormapX11 (#100023, Naofumi Yasufuku)

        * gdk/x11/gdkcolor-x11.c (gdk_colors_free)
        (gdk_colormap_free_colors): Don't call XFreeColors() for
        private colorsmaps.

        * gdk/x11/gdkcolor-x11.c (gdk_colormap_alloc_colors_writeable,
        gdk_colormap_colors_private, gdk_colormap_alloc_colors_shared):
        Fix return values to return number remaining not number
        allocated.

        * gdk/x11/gdkcolor-x11.c (gdk_colormap_alloc_colors_shared,
        gdk_colormap_alloc_colors_private): Clean up some a stray
        initializations.

Sun Dec  8 21:22:46 2002  Owen Taylor  <otaylor@redhat.com>

        * gdk/x11/gdkcolor-x11.c (gdk_colormap_alloc_colors_private):
        When allocating non-writeable colors, fill in the flags
        field of the XColor, and insert the returned color into
        our color hash. (#100023, Naofumi Yasufuku)

        * gdk/x11/gdkcolor-x11.c (gdk_colormap_free_colors):
        Fix a typo (my fault) that slipped in in the last patch.

Some bugs I found when reading through the code:

Sun Dec  8 21:29:10 2002  Owen Taylor  <otaylor@redhat.com>

        * gdk/x11/gdkcolor-x11.c (gdk_colormap_alloc1): Fix
        a lost refcount in the case where we get a duplicate
        back from the X server.

        * gdk/x11/gdkcolor-x11.c (gdk_colormap_alloc_colors_writeable):
        Set success[] for all colors when allocation of colors
        cells via XAllocColorCells succeeds.

        * gdk/x11/gdkcolor-x11.c (gdk_colormap_alloc_colors_private):
        Don't match against colors cells allocated writeable.

Sun Dec  8 21:43:31 2002  Owen Taylor  <otaylor@redhat.com>

        * gdk/x11/gdkcolor-x11.c: Fix spacing in a bunch of
        for (i=0; i<ncolors; i++).

        * gdk/x11/gdkcolor-x11.c: Various g_return_if_fail()
        cleanup.

====

Regarding the parts of your patch I didn't apply: I think
it's worth clarifying the distinction between the
concepts of a private colormap, and the concept of a
writeable color cell.

 private colormap: GDK owns all cells in the colormap
   and does allocations internally.
 non-private colormap: The colormap is potentially shared
   with other applications; the server does allocations.

 writeable color cell: The application has complete control
   over the color cells contents and may change it at
   any time. GDK makes no assumptions about what is held
   in the color cell.
 non-writeable color cell: The color cell is fixed once
   allocated; other users of the color within the same
   application (for a private color map) or from other
   applicaitons (for shared color maps) may share the
   color cell.

So these are "orthogonal" concepts. You can have both
writeable and non-writeable color cells in both private
and non-private color maps. 

====

The above changes are revisions 1.49 through 1.53 of gdkcolor-x11.c,
so you can get the individual diffs with:

 cvs diff -r 1.48 -r 1.49 gdkcolor-x11.c
 [...]
 cvs diff -r 1.52 -r 1.53 gdkcolor-x11.c

I've have not applied these changes to the 2.0 branch; most of 
the bugs are really old (except for the gdk_colormap_foreign_new()
one, which doesn't affect the 2.0 branch). So, I think it's
safer to leave them as is rather than risk breaking something.

I haven't tried out your test program, since I dont' have a 
pseudocolor display around to test with. If you could test
what I've applied, I would appreciate it.

Thanks for your work on this code... as you can tell by the 
number of bugs you found, although gdk_colormap_alloc_colors()
is over 4 years old, it hasn't been tested much.

> writeable color cell: The application has complete control
>   over the color cells contents and may change it at
>   any time. GDK makes no assumptions about what is held
>   in the color cell.

I understand. Thank you, Owen.

A simple bug still remains in alloc_colors_private().
This is the patch (I couldn't generate cvs diff because of cvs server
error).

Created attachment 12864 [details] [review]
gdk_colormap_alloc_colors_private.patch

Mon Dec  9 10:44:59 2002  Owen Taylor  <otaylor@redhat.com>

        * gdk/x11/gdkcolor-x11.c (gdk_colormap_alloc_colors_private):
        Fix a bug in one of my earlier changes. (From Naofumi Yasufuku,
        #100023)

Applied, thanks. If you are happy with the code now, please resolve
this bug FIXED.

Closing to clean up my list of current bugs, but please reopen
if you find remaining problems.