Created attachment 11580 [details]
implementation

Created attachment 11581 [details]
header

*** Bug 99461 has been marked as a duplicate of this bug. ***

Really sort of torn on this one. On one hand, it *is*
something that is needed in various places. On the
other hand, it's ugly API (because g_spawn_* is ugly)
and doesn't fit in very well with what's in GDK currently.

Would getting rid of the synchronous variants make sense?
I don't think wanting to run a GUI app synchronously
from another GUI app is very common or ever correct (repaints
on the first app will not work)

Some comments reading through the patch:

 - Maybe keep egg_make_spawn_environment_for_screen
   private? It strikes me as an implementation detail.

 - We don't put in extra spaces *everywhere* - 
   'envp[i]', not 'envp [i]'

 - egg_make_spawn_environment_for_screen() cannot return
   NULL in correctly functioning program, so it should
   not be documented as returning NULL, and handling
   NULL returns isn't necessary.

- A helper function for:

  default_screen = gdk_display_get_default_screen (
				gdk_screen_get_display (screen));
  if (screen != default_screen)
    new_envp = egg_make_spawn_environment_for_screen (screen, envp);

  Would be a good idea.

Other thing that needs dealing with is portability. 
On non-X11, this should just be a pass-through, so
probably best put in the port directories, with a
trivial implementation for win32 and linux-fb.

Owen: yeah, I can understand why you're torn. But I think its going to
be pretty essential for most multiscreen apps ...

I agree that removing the sync versions make sense.

We could also make the function names a little less crufty:

gdk_spawn(), gdk_spawn_with_pipes and gdk_spawn_command_line().

But there are a couple of problems with that:

1) If we ever decide later to add the sync versions we'll end up with
gdk_spawn() and gdk_spawn_sync() instead of
gdk_spawn_async()/gdk_spawn_async(). I suppose that wouldn't be too
bad, though - it would give a clear indication that the async version
is the one you mostly want to use.

2) Moving from gspawn to gdkspawn is no longer a s/g/gdk/. But I think
that's fine - its not a sed-job as the extra argument is always going
to require more thought.


I can do all this pretty trivially (along with your other comments) as
I commit, if you think I should go ahead. Just let me know :-)

Created attachment 19544 [details] [review]
updated patch

I've updated the patch. Changes are:

 - privatise make_environment_for_screen
 - make_environment not documented as returning NULL
 - remove async() methods
 - fix the envp [i] vs. envp[i] stylistic issue
 - win32 and linux-fb implementations
 - rename functions removing _async and _on_screen bits
 - removed the 

     if (screen != default_screen)
        new_envp = make_env_for_screen ();

   bits as that breaks when you run with --screen or
   --display. What you really want to check is whether
   the current $DISPLAY value matches the return value
   from gdk_screen_make_display_name(), but after implenting
   that I decided it just complicates matters and we should
   just always pass in a modified environment.


I'm slightly worried now that its not obvious from looking
at the prototypes what gdk_spawn and co. are for. Initially
GdkScreen * was the first argument to each of these functions
but I changed that as it looks to much like a GdkScreen operation.
In retrospect I think it would go some way to making the API
more clear.

Let's go ahead and add these.

 * The API docs should only be in the x11/ directory - they
   don't need to be duplicated (and that can actually cause
   problems for gtk-doc)

 * The names I'd like to see are:

   gdk_spawn_on_screen()
   gdk_spawn_on_screen_with_pipes()
   gdk_spawn_command_line_on_screen()

 * I would make the GdkScreen the first parameter; compare
   how all the _for_display() functions take the GdkDisplay
   as the first parameter.
 

Created attachment 22291 [details] [review]
final patch

Okay, I've gone ahead committed after making those changes. 

Thanks Owen.