Libxfce4ui api review

[Brian J. Tarricone]

On 2009/07/06 23:58, Nick Schermer wrote:
> 2009/7/6 Brian J. Tarricone<brian at tarricone.org>:
 >
>> 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.

Yeah, xfce_message_dialog() isn't necessarily 100% clear, but... eh, 
whatever.  I don't feel too strongly about it either way.

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

Hmm, yeah, that might work pretty well.  Maybe call the last arg 
'primary_format' instead.

Does it make more sense to use the format string capability for the 
primary or secondary string?  I'm not sure...

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

Cool, sounds good.

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

All right.  Yeah, I know I've used it before, but I wasn't sure how 
useful it is.  Really, tho, looking at the code, it's a pretty simple 
function.  It's basically just:

pixbuf = gdk_pixbuf_new_from_inline(...);
if(pixbuf && (width != pixbuf_width && height != pixbuf_height)) {
     scaled = gdk_pixbuf_scale_simple(...);
     g_object_unref(pixbuf);
     pixbuf = scaled;
}
return pixbuf;

I mean, it's effectively a 6-line function.  I dunno... I'm just 
attempting to push throwing away everything that's possible so at least 
the new lib starts out small.  But I guess it's useful.

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

I feel like the reason we had this function was because 
gtk_window_set_position() doesn't work in some instances.  Unfortunately 
the revision log for libgui's xfce-gtk-extensions.c doesn't provide any 
clues.

> Is there a place where xfce_gtk_window_center_on_screen is preferred
> over xfce_gtk_window_center_on_active_screen?

Probably not, good point.

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

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

The exo client really serves a different purpose: to have really 
lightweight session management using the old X11R5 protocol when you 
don't want to use the heavyweight XSMP.  However, it looks like gtk now 
(I'm not sure when they stared doing this) automatically sets up the 
X11R5 protocol for all apps, though I don't believe there's an easy way 
to set the restart command.  So maybe the exo client could go away 
regardless in favor of a single function to set the restart command... 
or maybe just not replace it at all in favor of just always using the 
newer protocol.

Anyhow, I'm *very* hesitant to go our own way here with a new session 
client.  For reference see here:

http://live.gnome.org/SessionManagement/EggSMClient

EggSMClient itself is pretty well done and even has non-unix support 
(not that that's entirely useful for the Xfce core desktop).  As much as 
I don't want to create a new libnetk, it might make sense to base work 
on our session client on EggSMClient.  Or maybe just import it as-is. 
Or maybe even just bite the bullet and copy the sources into the core 
Xfce apps that use it, and don't have a session client at all.  That way 
we could just wait for gtk to have one.

Of course, there's even still a bit of controversy around EggSMClient, 
and around session management on the X desktop in general.  I'm strongly 
considering suggesting we just punt on the session client problem until 
4.10, or later, even.

	-brian