Re: more patches

[Owen Taylor <otaylor redhat com>]

Havoc Pennington <hp redhat com> writes:

> Hi,
> 
> Here's some assorted bug-closing stuff.
> 
> - add gdk_window_shape_combine_region() - exports XShapeCombineRegion()
> 
> - add gdk_region_get_rectangles() - this is to allow you to 
>   write your own region operations, such as draw_region() or 
>   the like
> 
> - add gtk_entry_get_layout_offsets() - this is to support
>   using the entry for editable sheet cells and other such 
>   hacks, by allowing you to line up the entry text with the 
>   non-editable text the entry drops on top of. Also you can 
>   use it to convert widget coordinates to layout coordinates,
>   in order to e.g. find out which word the user clicked on.
> 
> - gtk_entry_set_activate_default(), I guess I already sent in this
>   patch yesterday.

Looks mostly fine to commit, with the following comments.

> +      GdkRegionBox rect;
> +      rect = region->rects[i];
> +      (*rectangles)[i].x = MIN (rect.x1, rect.x2);
> +      (*rectangles)[i].y = MIN (rect.y1, rect.y2);
> +      (*rectangles)[i].width = ABS (rect.x1 - rect.x2);
> +      (*rectangles)[i].height = ABS (rect.y1 - rect.y2);

Do you have evidence that the MIN and ABS are necessary? I don't
really want to dig into the GdkRegion code at the moment,
but I would be suprised if that were the case...

> +void
> +gdk_window_shape_combine_region (GdkWindow *window,
> +                                 GdkRegion *shape_region,
> +                                 gint       offset_x,
> +                                 gint       offset_y)
> +{  
> +  g_return_if_fail (GDK_IS_WINDOW (window));

Hmmm, there is a small problem here in that you don't
handle the 32-bit coordinates stuff here.

Either we need to:

 - Handle the 32 bit coordinate stuff (not hard - I think 
   you just have adjust offset_x and offset_y by the result of 
   _gdk_windowing_window_get_offsets () and do some clipping
   when converting the region to an X reg

 - Only allow this for toplevels

 - Not handle the offset case, warn about it with a docs, and
   put a check for non zero offset_x/offset_y here.
> +  
> +#ifdef HAVE_SHAPE_EXT
> +  if (GDK_WINDOW_DESTROYED (window))
> +    return;
> +
> +  if (shape_region == NULL)
> +    {
> +      /* Use NULL mask to unset the shape */
> +      gdk_window_shape_combine_mask (window, NULL, 0, 0);
> +      return;
> +    }
> +  
> +  if (gdk_window_have_shape_ext ())
> +    {
> +      GdkRectangle *rects = NULL;
> +      gint n_rects = 0;
> +      XRectangle *xrects = NULL;
> +      gint i;
> +
> +      if (shape_region)

This check doesn't make sense, nor does the initialization above.

> +        gdk_region_get_rectangles (shape_region,
> +                                   &rects, &n_rects);


> +      xrects = g_new (XRectangle, n_rects);
> +
> +      i = 0;
> +      while (i < n_rects)
> +        {
> +          xrects[i].x = rects[i].x;
> +          xrects[i].y = rects[i].y;
> +          xrects[i].width = rects[i].width;
> +          xrects[i].height = rects[i].height;
> +          ++i;
> +        }

Hmmm, perhaps we we should have a gdkx11.h private function that
does the conversion of a GdkRegion to X rectangles: That
would:

 - Reduce code duplication _gdk_x11_gc_flush (GdkGC *gc)

 - Make it more reasonable to have the function poke inside
   GdkRegion structure and avoid the double copy.

> +      XShapeCombineRectangles (GDK_WINDOW_XDISPLAY (window),
> +                               GDK_WINDOW_XID (window),
> +                               ShapeBounding,
> +                               offset_x, offset_y,
> +                               xrects, n_rects,
> +                               ShapeSet,
> +                               YXBanded);

Shouldn't you free xrects? 

> +							 _("Activate default"),
> +							 _("Whether to activate the default widget when Enter is pressed, such as the default button in a dialog"),

I'd suggest:

 the default widget (such as the default button in a dialog) when Enter is pressed

Or something like that, which doesn't dangle the phrase off the end.

> +static void
> +gtk_entry_real_activate (GtkEntry *entry)
> +{
> +  GtkWindow *window;
> +  GtkWidget *widget;
> +
> +  widget = GTK_WIDGET (entry);
> +
> +  if (entry->activate_default)
> +    {
> +      window = (GtkWindow *) gtk_widget_get_toplevel (widget);

Here, I agree 100% with Tim that this should be 
GTK_WINDOW (gtk_widget_get_toplevel (widget)).

And in fact, to catch (slightly odd) the case where this gets called
on an entry not added into a complete toplevel, you really should
have:
         
        GtkWidget *window = gtk_widget_get_toplevel (widget);

        if (window && GTK_IS_WINDOW (window) && ...)

Your usage would be more appropriate for 
gtk_widget_get_ancestor (widget, GTK_TYPE_WINDOW).

>  /**
> + * gtk_entry_set_activate_default:

would it make sense to make this instead:

 gtk_entry_set_activate_action (GtkEntry         *entry, 
                                GtkActivateAction action);

 GTK_ACTIVATE_NONE
 GTK_ACTIVATE_DEFAULT_WIDGET

Even though the effect is the same, it somehow seems clearer
to me. (Mostly, I just don't like the name 'set_activate_default'
very much.)

Regards,
                                        Owen