Libxfce4ui api review

[Brian J. Tarricone]

On 2009/07/07 08:48, Nick Schermer wrote:
> 2009/7/7 Brian J. Tarricone<brian at tarricone.org>:
>>>> 8.  I don't really understand what xfce_gtk_dialog_parse_parent() does
>>>> based
>>>> on the API description.  Can you explain it better?  Also I very much
>>>> dislike how 'parent' can be a GtkWidget or GdkScreen -- ohhhh, I see why
>>>> the
>>>> xfce_message_dialog*() functions take a gpointer now.  Every time I think
>>>> about doing something like that for *private* internal API, I cringe and
>>>> try
>>>> to find a better way.  Absolutely we shouldn't do something like this for
>>>> *public* API.
>>> Ok, will remove it.
>> Can you explain what those functions are for exactly?  I'm curious... If
>> they're useful, we should keep them, but maybe make the API a little less
>> weird.
>
> Mm after updating the git panel to the new api, I'd still like to
> reconsider this. I'll explain:
>
> There are a couple of cases there you just want to show an error
> dialog without having a transient widget (for example a plugin that
> failed to start). you can simply do this:
>
> xfce_dialog_show_error (gtk_widget_get_screen (GTK_WIDGET (plugin)), .....);
>
> or when you show an error on a button action that failed:
>
> xfce_dialog_show_error (button, ....) instead of
> xfce_dialog_show_error (GTK_WINDOW (gtk_widget_get_toplevel (button)),
> ...)
>
> or you simply set a GtkWindow as parent. The last get_toplevel thing
> is more convenient, but for the GdkScreen case you now pass NULL which
> might result in a dialog showing up at the wrong location.

Ok, now I understand.

I think for API non-ugliness, I'd prefer one of two things:

a)  It just takes an extra argument, and you can leave one or both NULL:

void xfce_dialog_show_error(GtkWindow *parent,
                             GdkScreen *screen,
                             const gchar *secondary,
                             const gchar *primary_format,
                             ...);

b) Two separate functions:

void xfce_dialog_show_error(GtkWindow *parent,
                             const gchar *secondary,
                             const gchar *primary_format,
                             ...);
void xfce_dialog_show_error_on_screen(GdkScreen *screen,
                                       const gchar *secondary,
                                       const gchar *primary_format,
                                       ...);

I think I'd prefer (b).  For convenience's sake I guess I'd be ok if it 
were GtkWidget *parent instead, so you can pass an arbitrary widget, and 
the function can do gtk_widget_get_toplevel() for you.  Though still, 
it's a little odd.

There's also another option to consider overall that reduces the number 
of entry points:

typedef enum
{
     XFCE_DIALOG_TYPE_INFO = 0,
     XFCE_DIALOG_TYPE_WARNING,
     XFCE_DIALOG_TYPE_ERROR,
} XfceDialogType;

void xfce_dialog_show(GtkWindow *parent,
                       XfceDialogType type,
                       const gchar *secondary,
                       const gchar *primary_format,
                       ...);
void xfce_dialog_show_on_screen(GtkWindow *parent,
                                 XfceDialogType type,
                                 const gchar *secondary,
                                 const gchar *primary_format,
                                 ...);

Or instead of the enum, could also abuse the GTK_STOCK_DIALOG_* strings. 
  Personally I like this better because of fewer entry points, and if we 
want to add other dialog types in the future (no idea what, though, so 
this might be a silly consideration), it's easy to do without having to 
add more functions.  Yes, it's a little bit more typing, but not a lot, 
and IMHO it makes the API cleaner, especially given the _on_screen() 
variant and the need for 2x the number of functions if you do it the 
other way.

Thoughts?

	-brian