Libxfce4ui api review

[Nick Schermer]

I think I've already asked for this a while ago but at that time
nobody replied. So, it would be nice if developers can take a look,
I'll upload the API docs later today, but there is not a lot of code
so reading headers will do ;-).

One crucial missing piece is a session client, the reason is simple,
I'd like to have something close to the implementation gtk will use,
so I'm still waiting what they are going to do. /Me will pick this up
soon. Other than that most of the code is there.

So please review and post your problems so I can fix them! Then
hopefully we can send gui4 to the graveyard in Xfce 4.8.

Cheers,
Nick
_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Jérôme Guelfucci-2]

Nick Schermer wrote:

> I think I've already asked for this a while ago but at that time
> nobody replied. So, it would be nice if developers can take a look,
> I'll upload the API docs later today, but there is not a lot of code
> so reading headers will do ;-).
>
> One crucial missing piece is a session client, the reason is simple,
> I'd like to have something close to the implementation gtk will use,
> so I'm still waiting what they are going to do. /Me will pick this up
> soon. Other than that most of the code is there.
>
> So please review and post your problems so I can fix them! Then
> hopefully we can send gui4 to the graveyard in Xfce 4.8.
>
> Cheers,
> Nick

Hello Nick,

I can't really review the API, but still have a question re libxfce4ui.
It may have already been discussed on this ML, but what is libxfce4ui?
It's a rewrite of gui4? It adds features? fixes bugs? Cleans code? Will
we need to port every single application using gui4 to it?

Thanks in advance,

Jérôme
_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Nick Schermer]

2009/7/6 Jérôme Guelfucci <jeromeg@...>:
> I can't really review the API, but still have a question re libxfce4ui. It
> may have already been discussed on this ML, but what is libxfce4ui? It's a
> rewrite of gui4? It adds features? fixes bugs? Cleans code? Will we need to
> port every single application using gui4 to it?

4ui is indeed a clean version of gui4 but is also contains some
complicated code that is for example shared between the panel and
thunar (xfce_execute) which is an improvement over the old xfce_exec
code. You don't need to port all the application to gui4, but it is
recommended (after the api review) to use 4ui instead of gui4.

Nick
_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Nick Schermer]

Api docs are here: http://foo-projects.org/~nick/docs/libxfce4ui/
_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Brian J. Tarricone-3]

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.

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?

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

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

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.

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

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.

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.

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

        -brian
_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Nick Schermer]

2009/7/6 Brian J. Tarricone <brian@...>:
> 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
_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Brian J. Tarricone-3]

On 2009/07/06 23:58, Nick Schermer wrote:
> 2009/7/6 Brian J. Tarricone<brian@...>:
 >
>> 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
_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Nick Schermer]

2009/7/7 Brian J. Tarricone <brian@...>:

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

Yeah good point.

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

I don't think so, most of the time the secondary text describes what
the action does (in a less specific way) for example:
Pri: Are you sure you want to remove "%s"?
Sec: This will remove the item from the configuration permanently.

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

I'll remove it, we'll see if we need it while porting the modules,
easier to add it in a later stage.

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

Will test it, see if it works (IIRC it only works during realize, but
we could handle that too in the code, print a warning or fallback to
the old way if realized). We can always change it to the way gui4
handles it.

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

Ok then we leave it out for the time being.

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

It was mostly used for the dialogs, thunar uses it in some other
internal dialogs so therefore I made it public. Probably not very
useful in public api, since we decided we use GtkDialog parents in the
xfce_dialog_ code.

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

We could add a libxfce4ui-session-private library (loooong name) using
eggsmclient so the code is in a central place, then we can drop it at
any time without having an api break.

Nick
_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Nick Schermer]

2009/7/7 Nick Schermer <nickschermer@...>:
> Probably not very useful in public api, since we decided we use GtkDialog parents in the
> xfce_dialog_ code.

Err, GtkWindow *parents...

Nick
_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Nick Schermer]

Another function that might be useful is the create_item_icon()
function in the libxfce4menu test application [1]. I think we use that
in a bunch of places to handle the (sometimes) weird Icon= values in
desktop files. On the other hand I think we should ignore values that
are not icon-names or absolute locations.

Nick

[1] http://svn.xfce.org/svn/xfce/libxfce4menu/trunk/tests/test-display-menu.c
_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Nick Schermer]

2009/7/7 Nick Schermer <nickschermer@...>:

> 2009/7/7 Brian J. Tarricone <brian@...>:
>> 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.
>
> We could add a libxfce4ui-session-private library (loooong name) using
> eggsmclient so the code is in a central place, then we can drop it at
> any time without having an api break.

Looked a bit through eggsm and I think we should use it, looks clean
and contains everything we need. Put it a private library named
libxfce4smclient-private (like libxfce4kbd-private) with the xsmp (and
maybe osx) backend. Make sure it works with xfce4-session and then sit
and wait what gtk+ is going to do.
Hopefully others agree with this. IMHO we simply don't think we have
the resources (and are in the position) of re-thinking the whole sm
backend.

Nick
_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Jannis Pohlmann-3]

On Tue, 7 Jul 2009 10:20:05 +0200
Nick Schermer <nickschermer@...> wrote:

> Another function that might be useful is the create_item_icon()
> function in the libxfce4menu test application [1]. I think we use that
> in a bunch of places to handle the (sometimes) weird Icon= values in
> desktop files. On the other hand I think we should ignore values that
> are not icon-names or absolute locations.

Not sure this still is the right approach to loading. What we could add
is a method to create a GIcon based on a string (either an icon name or
an absolute filename). Reasoning behind this is that it doesn't make
much sense to deal with pixbufs ourselves if all we have to do is
allocate meta information (GIcon) and let GTK+ resolve it into pixbufs
or whatever.

The basic implemntation could look like this:

  GIcon *
  xfce_gicon_new_for_icon_name (const gchar *icon_name)
  {
    GIcon *icon = NULL;
    GFile *file;

    if (g_path_is_absolute (icon_name))
      {
         file = g_file_new_for_path (icon_name);
         icon = g_file_icon_new (file);
         g_object_unref (file);
      }
    else
      {
         icon = g_themed_icon_new (icon_name);
      }

    return icon;
  }

If we *really* need a pixbuf, we could add another method:

  GdkPixbuf *
  xfce_gdk_pixbuf_new_from_icon (GIcon        *icon,
                                 GtkIconTheme *icon_theme,
                                 gint          size)
  {
    GInputStream *stream;
    GtkIconInfo  *icon_info;
    GdkPixbuf    *pixbuf = NULL;

    if (G_IS_THEMED_ICON (icon))
      {
        icon_info = gtk_icon_theme_lookup_by_gicon (icon_theme, icon,
                                                    size,
                                                    GTK_ICON_LOOKUP_USE_BUILTIN);
        if (icon_info != NULL)
          {
            pixbuf = gtk_icon_info_load_icon (icon_info, NULL);
            gtk_icon_info_free (icon_info);
          }
      }
    else if (G_IS_LOADABLE_ICON (icon))
      {
        stream = g_loadable_icon_load (G_LOADABLE_ICON (icon), size,
                                       NULL, NULL, NULL);
        if (stream != NULL)
          {
            pixbuf = gdk_pixbuf_new_from_stream (stream, NULL, NULL);
            g_object_unref (stream);
          }
      }
  }

The problem with this is that g_loadable_icon_load() and
gdk_pixbuf_new_from_stream() are possibly blocking function calls,
which means we'd either have to pass a GCancellable to
xfce_gdk_pixbuf_new_from_icon() or just decide not do add this function
at all and rather let GTK+ handle the loading of icons.

  - Jannis


_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Ali Abdallah-2]

Brian J. Tarricone wrote:

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

Meanwhile, are we going to keep using libxfcegui4 for the session-client
stuff, or there will be a GObject port of session- client to libxfce4ui?
i remember there were discussion about that.

>
>     -brian
Cheers,
Ali.
> _______________________________________________
> Xfce4-dev mailing list
> Xfce4-dev@...
> http://foo-projects.org/mailman/listinfo/xfce4-dev


--
Send unlimited messages for free to all destinations with DBus.

http://www.freedesktop.org/wiki/Software/dbus

_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Nick Schermer]

I think I've fixed most of the point Brian mentioned. Updated docs here:

http://foo-projects.org/~nick/docs/libxfce4ui/

Nick
_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Nick Schermer]

2009/7/7 Brian J. Tarricone <brian@...>:

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

Nick
_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Brian J. Tarricone-3]

On 2009/07/07 02:59, Nick Schermer wrote:

> 2009/7/7 Nick Schermer<nickschermer@...>:
>> 2009/7/7 Brian J. Tarricone<brian@...>:
>>> 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.
>> We could add a libxfce4ui-session-private library (loooong name) using
>> eggsmclient so the code is in a central place, then we can drop it at
>> any time without having an api break.
>
> Looked a bit through eggsm and I think we should use it, looks clean
> and contains everything we need. Put it a private library named
> libxfce4smclient-private (like libxfce4kbd-private) with the xsmp (and
> maybe osx) backend. Make sure it works with xfce4-session and then sit
> and wait what gtk+ is going to do.
> Hopefully others agree with this. IMHO we simply don't think we have
> the resources (and are in the position) of re-thinking the whole sm
> backend.

I hate having private libs, but that's probably the best idea for now.

        -b
_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Brian J. Tarricone-3]

On 2009/07/07 08:48, Nick Schermer wrote:

> 2009/7/7 Brian J. Tarricone<brian@...>:
>>>> 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
_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Nick Schermer]

2009/7/7 Brian J. Tarricone <brian@...>:
> 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?

Personally I think this will only make the api more ugly. Remember
there is still the possiblity to use the plain xfce_message_dialog
functions. Splitting the function having a *_on_screen possibility is
not ideal either. Note that only the info and warning functions take
the same arguments atm, so the enum won't make it any better too.

Using the pointer parent might look a bit ugly in the first place, but
if we properly document this and we don't crash on a gchar pointer
(all arguments are objects so we can do good type checking), I still
think this is the cleanest solution. Using a pointer is still valid
(see g_object_unref ^_^, I know for a slightly other reason) and keeps
the whole thing clean.

Nick
_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Nick Schermer]

2009/7/7 Brian J. Tarricone <brian@...>:
> I hate having private libs, but that's probably the best idea for now.

Jup I agree, but until the world has not decided what to do with this
we're better of this way, keeping 4ui clean. Do other developers agree
with this? If so I'll create the private lib containing eggsmclient
with support for xsmp.

Nick
_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Brian J. Tarricone-3]

On 2009/07/07 13:40, Nick Schermer wrote:

> 2009/7/7 Brian J. Tarricone<brian@...>:
>> 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?
>
> Personally I think this will only make the api more ugly. Remember
> there is still the possiblity to use the plain xfce_message_dialog
> functions. Splitting the function having a *_on_screen possibility is
> not ideal either. Note that only the info and warning functions take
> the same arguments atm, so the enum won't make it any better too.

Right, I forgot that the error function takes a GError, so no help there.

> Using the pointer parent might look a bit ugly in the first place, but
> if we properly document this and we don't crash on a gchar pointer
> (all arguments are objects so we can do good type checking), I still
> think this is the cleanest solution. Using a pointer is still valid
> (see g_object_unref ^_^, I know for a slightly other reason) and keeps
> the whole thing clean.

g_object_unref() is a special case used to avoid excessive cast macro
use, since you can expect to want to use it with pretty much any GObject
subclass.  Stuff that takes a GtkWidget or GtkWindow is a bit more
restricted in scope (and note that all gtk/gdk functions that have
parameters for a parent window are strongly typed as taking GtkWindow*).
  The gpointer arg to g_signal_connect() is there because there's
nothing GObject-specific about it; you really could pass a pointer to an
arbitrary struct, as long as it's a descendant of GTypeInterface (even
if it's not, you still might be able to use it).

Regardless, using a gpointer in this case really doesn't fall under
either of those cases, because you're using it effectively as an untyped
(or multi-type) parameter.  For g_object_unref(), everything that you
pass to it is a GObject.  In your case, you want to be able to pass a
GdkScreen or a GtkWindow, neither of which are the "same thing"; in fact
they are very different.  And also the functionality is very different,
and passing the different things has a different effect: one way says
"put the window on this screen" and the other says "make the window a
transient of this other window" (the fact that it also specifies the
screen is incidental).

Frankly, it's just ugly API, and documenting it well doesn't change that.

What are your objections to the _on_screen() variants, aside from just
having extra entry points?

        -brian

P.S.  I'm still not totally sold on passing GtkWidget *parent, and using
gtk_widget_get_toplevel() internally.  The argument of "but calling
GTK_WINDOW(gtk_widget_get_toplevel()) inline in the function call is too
much typing and ugly" is best answered by, "then don't use C; use
another language with a binding that doesn't suck."  But then again,
this is a library of convenience/extension functions, so I guess it's
somewhat ok.
_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev