Comments on clipping fix (118317)

[Owen Taylor <otaylor redhat com>]

http://bugzilla.gnome.org/show_bug.cgi?id=118317
Describes a problem with nested paints being buggy, and has
a patch from Soeren to change the GdkWindow paint stack to
use a set of pixmaps instead of sharing a single pixmap
for all nested paints; I'm posting the comments here since
they ended up being long - I had to figure out what I was 
thinking several years ago in order to evaluate the patch.


I'll first describe the motivation for how nest paints work.

gdk_window_begin_paint() can be thought of as a contract
between GDK and the application. GDK promises:

 - I'll clear the area to the window background
 - I'll clip your drawing to the area
 - I won't show anything to the user until you
   call end_paint()

In exchange, the application is promising:

 - When I call end_paint(), I'm finished drawing
   the area, and it's ready to be shown to the 
   user.

The most obvious way of doing nested begin_paints/end_paints
would be to make the outer paint clip the inner paint.
But this isn't a particularly *useful* interpretation;
when someone calls begin_paint() they are
promising to draw the region completely, so there is
no point in clipping their drawing to someone elses
earlier clip region.

And we might imagine a situation where:
 
 - Expose handler calls update method
 - Update method wants to update some non-exposed part
   of the widget, so calls begin_paint/end_paint.

If apps are well behaved and draw everything out of expose
handlers, this won't come up, but GDK/GTK+ doesn't make
that a hard requirement.

So that's why every paint gets a new clip region, that
can be larger than the outer clip region. Why is this
region subtracted from the region of the outer paint?
When the inner paint is finished, that area of the 
window ought to be in its final state, so there is no point
in the outer paint drawing it again when it finishes.

A consideration that we have to keep in mind when touching
this subsystem is that its something that we need to
eventually virtualize for various backends. GDK/DirectFB
already does this - as I understand it gdk_window_begin_paint() 
simply turns off updating of the screen from the per-window
buffer, clears the area and sets a clip on drawing.
When the outermost paint ends, updating is turned 
back on. A OS/X port of GDK would take a similar approach.

(No real effects on this bug that I can think of, but worth
keeping in mind.)

I think your fix makes sense for the default implementation;
it simplifies things a lot, and as you say, nested
clips are rare to vanishing.

I'm not so sure that setting GC clip regions is a bad idea:

 - As mentioned in my comments below, the region for
   a paint often occuppies only a tiny region of its 
   bounding box.

 - It's not a clip per operation, it's at most setting
   the clip once per GC per paint in addition to
   what we do know.
   
 - We already set the clip in many cases when drawing
   things like labels. For such cases, there is little
   extra X overhead in intersecting the clip with the 
   paint clip - we already have to delay setting the
   clip until we draw to deal with offsets.

However, that's something we can worry about separately.

---------------------------------------------------------

Comments on the patch:

===
       gc_values.fill = GDK_TILED;
       gc_values.tile = private->bg_pixmap;
-      gc_values.ts_x_origin = - paint->x_offset;
-      gc_values.ts_y_origin = - paint->y_offset;
+      gc_values.ts_x_origin = -paint->x_offset;
+      gc_values.ts_y_origin = -paint->y_offset;
===
   
Probably wouldn't complain if this was new code, but the spaces
are definitely supposed to be there in the GTK+ coding style.
(For unary minus, not for negative numbers, of course.)

====
+  for (list = private->paint_stack; list != NULL; list = list->next)
+    {
+      GdkRectangle tmp_box;
+      GdkWindowPaint *tmp_paint = list->data;
+
+      gdk_region_subtract (tmp_paint->region, paint->region);
+      gdk_region_get_clipbox (tmp_paint->region, &tmp_box);
+      tmp_paint->x_offset = tmp_box.x;
+      tmp_paint->y_offset = tmp_box.y;
     }
===

The change in the x_offset/y_offset for the other paints doesn't 
make sense to me - the offset is an offset between the backing
pixmap and the window. It's the amount we offset our drawing 
operations by.

===
-  if (!gdk_region_empty (init_region))
-    gdk_window_paint_init_bg (window, paint, init_region);
-  
-  gdk_region_destroy (init_region);
   
   private->paint_stack = g_slist_prepend (private->paint_stack, paint);
+
+  if (!gdk_region_empty (region))
+    {
+      gdk_window_clear_backing_rect (window,
+				     clip_box.x, clip_box.y,
+				     clip_box.width, clip_box.height);
+    }
 #endif /* USE_BACKING_STORE */
 }
===

I think clipping the clear to the actual region is worthwhile - the
regions are not infrequently very sparse, for an opaque move resize,
we might frequently have a 500x500 region of which only 
two 5x500 stripes are there.

=== 
   gdk_draw_drawable (private->impl, tmp_gc, paint->pixmap,
-                     clip_box.x - paint->x_offset,
-                     clip_box.y - paint->y_offset,
+                     0, 0,
                      clip_box.x - x_offset, clip_box.y - y_offset,
                      clip_box.width, clip_box.height);
===

This change looks like compensation for the change I commented
on above.

====
+      gdk_region_get_clipbox (paint->region, &paint_clipbox);
+      
+      intersection = gdk_region_rectangle (&rect);
+      gdk_region_intersect (intersection, paint->region);
+
+      gdk_region_offset (intersection, -x, -y);
+      gdk_region_get_clipbox (intersection, &clipbox);
+      gdk_gc_set_clip_region (tmp_gc, intersection);
+      
+      gdk_draw_drawable (tmp_pixmap, tmp_gc, paint->pixmap,
+			 clipbox.x + x - paint_clipbox.x,
+			 clipbox.y + y - paint_clipbox.y,
+			 clipbox.x, clipbox.y,
+			 clipbox.width, clipbox.height);
====

Having the paint_clipbox involved here looks wrong to me -
I would expect:

    gdk_draw_drawable (tmp_pixmap, tmp_gc, paint->pixmap,
		       clipbox.x + paint->x_offset,
		       clipbox.y + paint->y_offset,
		       clipbox.x, clipbox.y,
		       clipbox.width, clipbox.height);