Libxfce4ui spawn api break

[Nick Schermer]

People,

While i was adding startup notification support to the thunar uca
plugin, i discovered some applications might need to have the pid of
the spawned process [to setup a g_child_watch_add(_full)] to monitor
when it exits (in case of the uca plugin: it refreshed the working
directory of the action to make sure file changes are visible, even on
volumes that don't support file monitoring or when file monitoring is
somehow disabled).

So I'd like to add a pid return field like this:

gboolean
xfce_spawn_on_screen (GdkScreen    *screen,
                      const gchar  *working_directory,
                      gchar       **argv,
                      gchar       **envp,
                      GSpawnFlags   flags,
                      gboolean      startup_notify,
                      guint32       startup_timestamp,
                      const gchar  *icon_name,
                      GPid         *child_pid, /* <-- new variable */
                      GError      **error);

The xfce_spawn_command_line_on_screen() function won't be touched.

Any objections, better ideas?

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

[Jannis Pohlmann-3]

On Sat, 31 Oct 2009 18:45:04 +0100
Nick Schermer <nickschermer@...> wrote:

> People,
>
> While i was adding startup notification support to the thunar uca
> plugin, i discovered some applications might need to have the pid of
> the spawned process [to setup a g_child_watch_add(_full)] to monitor
> when it exits (in case of the uca plugin: it refreshed the working
> directory of the action to make sure file changes are visible, even on
> volumes that don't support file monitoring or when file monitoring is
> somehow disabled).
>
> So I'd like to add a pid return field like this:
>
> gboolean
> xfce_spawn_on_screen (GdkScreen    *screen,
>                       const gchar  *working_directory,
>                       gchar       **argv,
>                       gchar       **envp,
>                       GSpawnFlags   flags,
>                       gboolean      startup_notify,
>                       guint32       startup_timestamp,
>                       const gchar  *icon_name,
>                       GPid         *child_pid, /* <-- new variable */
>                       GError      **error);
>
> The xfce_spawn_command_line_on_screen() function won't be touched.
>
> Any objections, better ideas?

It's ok from my perspective. As I said on IRC I think we should add
this to the release model document:

  The API of a component may be broken during the development cycle if
  the symbols to be broken were introduced in that same cycle.

Since everyone is advised to keep core components in sync and in a
release ready state, this would allows for more experiments within
development cycles. Once a development cycle ends and the final release
is made, all new symbols are frozen.

If we can agree on that, go ahead. I personally think
xfce_spawn_on_screen() should be at least as powerful as
gdk_spawn_on_screen(). And if it misses the PID, then we should add it
as long as we can.

  - Jannis


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

[Brian J. Tarricone-3]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 10/31/2009 10:45 AM, Nick Schermer wrote:

>
> So I'd like to add a pid return field like this:
>
> gboolean
> xfce_spawn_on_screen (GdkScreen    *screen,
>                       const gchar  *working_directory,
>                       gchar       **argv,
>                       gchar       **envp,
>                       GSpawnFlags   flags,
>                       gboolean      startup_notify,
>                       guint32       startup_timestamp,
>                       const gchar  *icon_name,
>                       GPid         *child_pid, /* <-- new variable */
>                       GError      **error);

Sure, go for it.  Just please fix up existing users of this function (if
there are any) in git.

        -brian

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.11 (GNU/Linux)

iEYEARECAAYFAkrsiUoACgkQ6XyW6VEeAnsbRACeKvMN//aFzgX7PY41LSko4CiP
/ZQAoLIVWHaId4u5DcYEMe+BfNeUvm6m
=aXoa
-----END PGP SIGNATURE-----
_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Nick Schermer]

2009/10/31 Brian J. Tarricone <brian@...>:
> Sure, go for it.  Just please fix up existing users of this function (if
> there are any) in git.

Yes I'll do that. After the API change and a bit of testing on my own
pc, I'll release a new development version of 4ui, fix the core
components and make them depend on the new release.

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

[Nick Schermer]

2009/10/31 Jannis Pohlmann <jannis@...>:
> If we can agree on that, go ahead. I personally think
> xfce_spawn_on_screen() should be at least as powerful as
> gdk_spawn_on_screen(). And if it misses the PID, then we should add it
> as long as we can.

In that case we should also add (GSpawnChildSetupFunc child_setup,
gpointer user_data), does anybody think we needs this?

Nick

http://library.gnome.org/devel/glib/2.22/glib-Spawning-Processes.html#GSpawnChildSetupFunc
_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Brian J. Tarricone-3]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 10/31/2009 04:21 PM, Nick Schermer wrote:
> 2009/10/31 Jannis Pohlmann <jannis@...>:
>> If we can agree on that, go ahead. I personally think
>> xfce_spawn_on_screen() should be at least as powerful as
>> gdk_spawn_on_screen(). And if it misses the PID, then we should add it
>> as long as we can.
>
> In that case we should also add (GSpawnChildSetupFunc child_setup,
> gpointer user_data), does anybody think we needs this?

Nah, from what I'm told the child setup function is generally a bad
idea.  I can't think of any use cases offhand, aside from calling
setsid(), which is rarely necessary (and should be done by default
anyway, unless G_SPAWN_DO_NOT_REAP_CHILD is passed).

        -b
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.11 (GNU/Linux)

iEYEARECAAYFAkrsyywACgkQ6XyW6VEeAns+HgCg6JhmwgWYzF+8a/HiMBw8wGnj
HokAnRKj8X+gk/lEBmujDXotLedtzkxG
=12Cu
-----END PGP SIGNATURE-----
_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Nick Schermer]

2009/11/1 Brian J. Tarricone <brian@...>:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 10/31/2009 04:21 PM, Nick Schermer wrote:
>> 2009/10/31 Jannis Pohlmann <jannis@...>:
>>> If we can agree on that, go ahead. I personally think
>>> xfce_spawn_on_screen() should be at least as powerful as
>>> gdk_spawn_on_screen(). And if it misses the PID, then we should add it
>>> as long as we can.
>>
>> In that case we should also add (GSpawnChildSetupFunc child_setup,
>> gpointer user_data), does anybody think we needs this?
>
> Nah, from what I'm told the child setup function is generally a bad
> idea.  I can't think of any use cases offhand, aside from calling
> setsid(), which is rarely necessary (and should be done by default
> anyway, unless G_SPAWN_DO_NOT_REAP_CHILD is passed).

Ok, that's what I thought. We always add reap-child to watch the
process for startup notification, so no setsid for us.

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

[Nick Schermer]

2009/11/1 Nick Schermer <nickschermer@...>:

> 2009/11/1 Brian J. Tarricone <brian@...>:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> On 10/31/2009 04:21 PM, Nick Schermer wrote:
>>> 2009/10/31 Jannis Pohlmann <jannis@...>:
>>>> If we can agree on that, go ahead. I personally think
>>>> xfce_spawn_on_screen() should be at least as powerful as
>>>> gdk_spawn_on_screen(). And if it misses the PID, then we should add it
>>>> as long as we can.

It turns out returning the pid is not useful, because you can create
only 1 child watch per pid and we already need a child watch for
handling startup notification.
I though about adding a GChildWatchFunc and gpointer, which could be
called from the child watch we connect, but then there is not way to
disconnect the function, so that is not really a solution either.

Other ideas?

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

[Brian J. Tarricone-3]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11/01/2009 02:41 PM, Nick Schermer wrote:
>
> It turns out returning the pid is not useful, because you can create
> only 1 child watch per pid and we already need a child watch for
> handling startup notification.
> I though about adding a GChildWatchFunc and gpointer, which could be
> called from the child watch we connect, but then there is not way to
> disconnect the function, so that is not really a solution either.

Oh hmm, yeah, that kinda sucks.  Stupid glib.

Well, I guess it depends on how much work you want to do...  We could
create a xfce_add_child_watch() that *does* allow more than one child
watch (just store a list of callbacks in a hash table keyed by pid), and
put that in libxfce4util.

Not sure that's really a good idea, though... I hate adding new API for
a one-off need.

Your approach sounds fine, though as you suggest, you'd have to add
extra API to allow disconnecting the child watch function (and again,
you'd have to keep track of a pid->callback mapping anyway).

So I dunno, maybe the xfce_add_child_watch()/xfce_remove_child_watch()
idea is a better one?  It's at least cleaner and more generic, but it
might be a little over-engineered/bloaty/crack-like.   Hmm...

        -brian
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.11 (GNU/Linux)

iEYEARECAAYFAkruEpQACgkQ6XyW6VEeAntIUACfZgtLVEwPUHjRZB5kz5GFGCdY
6tsAoIsMdSDeavqs1EWXsRIqQ3DzXJVQ
=uEhe
-----END PGP SIGNATURE-----
_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Nick Schermer]

2009/11/1 Brian J. Tarricone <brian@...>:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 11/01/2009 02:41 PM, Nick Schermer wrote:
>>
>> It turns out returning the pid is not useful, because you can create
>> only 1 child watch per pid and we already need a child watch for
>> handling startup notification.
>> I though about adding a GChildWatchFunc and gpointer, which could be
>> called from the child watch we connect, but then there is not way to
>> disconnect the function, so that is not really a solution either.
>
> Oh hmm, yeah, that kinda sucks.  Stupid glib.
>
> Well, I guess it depends on how much work you want to do...  We could
> create a xfce_add_child_watch() that *does* allow more than one child
> watch (just store a list of callbacks in a hash table keyed by pid), and
> put that in libxfce4util.
>
> Not sure that's really a good idea, though... I hate adding new API for
> a one-off need.

Well right now it's only 1 place where we need it, but it's not
uncommon to watch a child. Not that I agree we should add a lot of api
for it, but still.

Another option would be a function that returns the XfceSpawnData on
succeed. Normally we free the data when the spawn is completed, but as
an alternative we could not free the data and leave that to the owner
(when the _full function is called).

XfceSpawnData *
xfce_spawn_on_screen_full (...);

void
xfce_spawn_set_callback (XfceSpawnData *data,
                                        GChildWatchFunc function,
                                        gpointer data,
                                        GDestroyNotify notify);

void
xfce_spawn_disconnect (XfceSpawnData *data);

Not a free function since the spawn could still be running, so
disconnect will unset the users' callbacks and enable auto-freeing
again or directly free if the spawn was completed. That is not a lot
of work, more like splitting the current code a bit.

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

[Nick Schermer]

2009/11/2 Nick Schermer <nickschermer@...>:
> Well right now it's only 1 place where we need it, but it's not
> uncommon to watch a child. Not that I agree we should add a lot of api
> for it, but still.
>
> Another option would be a function that returns the XfceSpawnData on
> succeed. Normally we free the data when the spawn is completed, but as
> an alternative we could not free the data and leave that to the owner
> (when the _full function is called).

I've put this into a more readable version here:
http://foo-projects.org/~nick/tmp/xfce-spawn-api-sample.c

Note that i removed the envp from xfce_spawn_on_screen, this is something
we rarely use, not sure, passing NULL is not a lot of work either.

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

[Brian J. Tarricone-3]

On Mon, Nov 2, 2009 at 03:41, Nick Schermer <nickschermer@...> wrote:

> Note that i removed the envp from xfce_spawn_on_screen, this is something
> we rarely use, not sure, passing NULL is not a lot of work either.

I need to check what I'm using in my local tree, but I've switched
xfce4-session to using some of the spawn functions, and I wanted to
include startup  notification as well.  For that, I do need the
ability to set envp to follow the xsmp spec properly.

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

[Nick Schermer]

2009/11/2 Brian J. Tarricone <brian@...>:
> On Mon, Nov 2, 2009 at 03:41, Nick Schermer <nickschermer@...> wrote:
>
>> Note that i removed the envp from xfce_spawn_on_screen, this is something
>> we rarely use, not sure, passing NULL is not a lot of work either.
>
> I need to check what I'm using in my local tree, but I've switched
> xfce4-session to using some of the spawn functions, and I wanted to
> include startup  notification as well.  For that, I do need the
> ability to set envp to follow the xsmp spec properly.

A, ok, well we can leave it in, not a big deal.

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

[Nick Schermer]

I pushed my idea of the new api to the nick/spawn-child-watch branch.
There is also a nick/uca-startup-notification branch in thunar to show
how this would work in the real world.

Also did some cleaning in the spawn code to use second timeouts and
use environ again.

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

[Nick Schermer]

2009/11/4 Nick Schermer <nickschermer@...>:
> I pushed my idea of the new api to the nick/spawn-child-watch branch.
> There is also a nick/uca-startup-notification branch in thunar to show
> how this would work in the real world.

We could also put this in a gobject with a signal to tell the child
exits, duno if that makes it more useful? Maybe a bit too complicated
with a lot of properties and data copying.

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

[Brian J. Tarricone-3]

On Wed, Nov 4, 2009 at 15:02, Nick Schermer <nickschermer@...> wrote:
> 2009/11/4 Nick Schermer <nickschermer@...>:
>> I pushed my idea of the new api to the nick/spawn-child-watch branch.
>> There is also a nick/uca-startup-notification branch in thunar to show
>> how this would work in the real world.
>
> We could also put this in a gobject with a signal to tell the child
> exits, duno if that makes it more useful? Maybe a bit too complicated
> with a lot of properties and data copying.

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

[Nick Schermer]

2009/11/5 Brian J. Tarricone <brian@...>:
> On Wed, Nov 4, 2009 at 15:02, Nick Schermer <nickschermer@...> wrote:
>> 2009/11/4 Nick Schermer <nickschermer@...>:
>>> I pushed my idea of the new api to the nick/spawn-child-watch branch.
>>> There is also a nick/uca-startup-notification branch in thunar to show
>>> how this would work in the real world.

Any comments? Better ideas?

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