Re: more patches

[Havoc Pennington <hp redhat com>]

Owen Taylor <otaylor redhat com> writes:
> > +  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)).

That would be wrong, because there's no guarantee that get_toplevel()
even returns a GtkWindow. ;-) The problem is confusion on my part
about what get_toplevel() does. I was taking it to be the same as
get_ancestor(GTK_TYPE_WINDOW) (even though just a couple weeks ago I
documented get_toplevel() with a special note that it was not
equivalent to get_ancestor (GTK_TYPE_WINDOW), my memory is short ;-)

So using (GtkWindow *) there was deliberate, to handle the NULL return
case I thought existed. The issue is whether the pointer can be NULL,
not which kind of cast is conventional.

I think the right fix is to simply replace get_toplevel with the
get_ancestor() call. At which point (GtkWindow *) is correct, because
the return value can be NULL and is otherwise guaranteed to be a
window.
 
>  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.)
> 

I don't like set_activate_default() too much either, but I find the
enum kind of more confusing than a bool, if there's only one possible
setting. It adds an extra concept to the API and an extra
cross-reference you have to follow in the docs. So that's why I didn't
go with the enum option.

Would rather someone think of a better name...

Havoc