Libxfce4ui api review

[Nick Schermer]

2009/7/6 Brian J. Tarricone <brian at tarricone.org>:
> Random stuff in no particular order:
>
> 1.  xfce_message_dialog_vnew() -- please use the 'standard' naming
> convention as used in glib/gtk: xfce_message_dialog_new_valist(). "_vnew"
> isn't used at all; if you were to use "_newv", that would be for a function
> that takes an array length and an array of parameters.

Will do that.

> 2.  xfce_message_dialog_run() -- I'm a little skeptical of this, since it
> doesn't really line up with (e.g.) gtk_dialog_run().  If you're just
> changing it for the sake of changing it, please don't: why waste time doing
> s/xfce_message_dialog/xfce_message_dialog_run/g for a cosmetic difference
> when it doesn't really make the function use clearer?

True, duno why I did that actually. Probably to make clear is does
gtk_dialog_run for you... whatever.

> 3.  xfce_message_dialog_show_{info,warning,error}() -- I'm happy to be rid
> of xfce_{info,warn,err}(), but we should make these HIG-compliant by taking
> separate primary and secondary text.  Aside from changing the ugly naming,
> this doesn't really have much of a benefit over the old ones.  Ditto for
> _confirm().

xfce_dialog_show_error takes the error message as secondary argument.
For the xfce_dialog_show_{info,warning,confirm} functions we could do
add a secondary text and use the format for the primary text, like:

void xfce_dialog_show_warning (gpointer     parent,
                          const gchar *secondary_text,
                          const gchar *format,
                          ...);

Are you ok with that?

> 4.  xfce-dialogs.h in general -- why are the 'parent' arguments gpointers?
>  They should be GtkWindows.  Yes, I know, more typing for the cast, but...
> well, if we didn't care about type-safety, then we might as well just have
> everything take a gpointer argument and be done with it.  (See #8 below; I
> realised why you did this, but it's still bad.)

It is kinda handy, but you're right, might not be so suitable for
public api. Will change those to GtkWindow's.

> 5.  xfce-execute.h -- if we're going to rename them, we might as well use
> the glib/gdk convention of using "spawn" in the name.  Nice idea on
> providing a startup timestamp.  Any reason for dropping the 'in_terminal'
> stuff?  That's still useful for anything that executes a .desktop file.

I was planning to add some convenient functions here.
xfce_execute_on_screen has the in_terminal boolean, for
xfce_execute_argv_on_screen you probably did a lot of parsing, so
there you should add it yourself. The startup timestamp is very
important for focus stealing.
For renaming it to xfce_spawn-*, duno, not a strong opinion about that.

> 6.  xfce_gdk_pixbuf_new_from_inline_at_size() -- I forget why we need
> this... is using gdk directly much more difficult?

I searched through the code at the time and IIRC it was still used at
2 or more modules. Could be convenient.

> 7.  We could also use a xfce_gtk_window_center_on_screen() that takes a
> GdkScreen* and monitor number.  I know I use the analogous libxfcegui4
> function in places.

Function now uses gtk_window_set_position() so gtk centers the screen,
the old xfce_gtk_window_center_on_monitor in gui4 calculated the
center by using the size requisition, which is a bit of a hack.
Haven't tried gtk_window_set_position() in a while but IIRC is does
the right thing.

Is there a place where xfce_gtk_window_center_on_screen is preferred
over xfce_gtk_window_center_on_active_screen?

> 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.

> Yeah, I'm worried about the session client stuff too.  GNOME seems to be
> attempting to move away from XSMP; I'm not yet convinced that doing the same
> makes sense for us.  We won't be able to kill libxfcegui4 in Xfce core
> entirely without a working session client, though :-( .

Jup, bit of a dilemma here. But on the other hand, if we have a good
gobject api, we should be able to change the internal code without
bothering anyone else. Maybe we can take the layout of the exo session
client as a base and expand it a bit? That allows us to deprecate the
exo client too, so we have 1 implementation in Xfce.

Thanks for your comments,
Nick