After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 606960 - gio: Add extension point for informing parties of launched application data
gio: Add extension point for informing parties of launched application data
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: gio
2.23.x
Other Linux
: Normal enhancement
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2010-01-14 14:41 UTC by Jason Smith
Modified: 2011-01-05 17:17 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
GIO patch (7.44 KB, patch)
2010-01-14 14:41 UTC, Jason Smith
none Details | Review
New extension point with appinfo, uri, launchctx, and pid (9.06 KB, patch)
2010-06-18 21:09 UTC, Mikkel Kamstrup Erlandsen
none Details | Review
New extension point with appinfo, uri, launchctx, and pid (9.13 KB, patch)
2010-08-18 10:06 UTC, Mikkel Kamstrup Erlandsen
none Details | Review
gdesktopappinfo: Send out a session bus signal when launching .desktop file (3.65 KB, patch)
2010-12-20 18:13 UTC, Colin Walters
none Details | Review
gdesktopappinfo: Send out a session bus signal when launching .desktop file (3.68 KB, patch)
2010-12-20 20:21 UTC, Colin Walters
none Details | Review
gdesktopappinfo: Add g_desktop_app_info_launch_uris_lowlevel() (6.90 KB, patch)
2010-12-20 20:21 UTC, Colin Walters
none Details | Review
gdesktopappinfo: Send out a session bus signal when launching .desktop file (5.27 KB, patch)
2010-12-21 14:19 UTC, Colin Walters
none Details | Review
gdesktopappinfo: Add g_desktop_app_info_launch_uris_lowlevel() (7.67 KB, patch)
2010-12-21 14:19 UTC, Colin Walters
none Details | Review
gdesktopappinfo: Send out a session bus signal when launching .desktop file (5.29 KB, patch)
2010-12-24 22:35 UTC, Colin Walters
committed Details | Review
gdesktopappinfo: Add g_desktop_app_info_launch_uris_as_manager() (7.62 KB, patch)
2010-12-24 22:35 UTC, Colin Walters
committed Details | Review

Description Jason Smith 2010-01-14 14:41:32 UTC
Created attachment 151399 [details] [review]
GIO patch

I am providing here a patch for GIO which opens up a new extension point. This extension point is useful to do quite a few things, however its primary target is a window matching library called wncksync. Wncksync is used to match windows by the XID to the .desktop file on the system they were launched from. With this patch wncksync becomes incredibly accurate at doing this, having very few flaws. Additionally the Zeitgeist developers are aware of this patch and intend on using it to improve their application as well.

I will keep this short as I am sure there will need to be a conversation here about the patch, its implications, if and how to include it, and of course the code itself.

Cheers,
Jason
Comment 1 Colin Walters 2010-05-13 21:03:16 UTC
I'd actually like a patch which delegates launching of a .desktop file to another process entirely.  Just check whether say "org.gnome.Shell" exists on the bus - if it does, call org.gnome.Shell.LaunchApp(foo.desktop).  Your wncksync daemon could implement this interface.
Comment 2 Colin Walters 2010-05-13 21:10:34 UTC
The code in gio could check g_getenv ("DBUS_SESSION_BUS_ADDRESS"), or it could be conditionally compiled (--enable-launch-tracking)?
Comment 3 Colin Walters 2010-05-13 21:35:04 UTC
(Basically, we shouldn't be afraid of putting GNOME UI-specific code directly in the GTK+ stack, any more than we should be afraid of putting Windows UI specific code in)
Comment 4 JasSmith 2010-05-13 22:07:01 UTC
I am not quite sure if you are being coy or not... However I don't think delegating the task of launching desktop files to a running process over dbus is a very good idea, primarily because that requires the remote process parse and launch the desktop file on its own. Additionally this breaks having multiple consumers of the service.
Comment 5 Colin Walters 2010-05-17 13:36:28 UTC
Not being coy.  We already have the problem of multiple processes parsing desktop files, so there's no efficiency loss.  Having one well known process launch apps under e.g. a well-known environment will make things more reliable. 

As far as multiple consumers - nothing stops the launcher process exporting a DBus service.
Comment 6 Dan Winship 2010-05-17 13:42:59 UTC
ftr, KDE does this (has a single app-launching process)
Comment 7 JasSmith 2010-05-17 14:11:55 UTC
Delegating out to a process is possible, however I am not convinced it is the proper solution however. It requires the creation of a new app launching process as well as exporting a well known interface on it which other applications may add hooks into. I am willing to do this work if I can get a general nod that upstream would indeed be interested in merging this idea.

There will of course need to remain the normal fallback there is today, and I guess some code to throw an error if there are multiple subscribers to the extension point. I will be willing to provide a reference implementation of a launching process along with this work, however the standardization of its API and dbus location are probably best left for a future debate, or the gnome shell project.
Comment 8 Colin Walters 2010-05-17 17:32:02 UTC
It doesn't require the creation of a new process.  We could put it in gnome-settings-daemon or gnome-session.  (Probably the latter).
Comment 9 Colin Walters 2010-06-18 17:32:34 UTC
Actually after looking at some of the other GIO extension points, I'm now in favor of just doing this as an extension point.

Fair warning though - I plan to ship an extension point with gnome-session which delegates launch handling to that process.  The way I suggest your wncksync extension point work is that it disables itself if gnome-session 2.32 is running, and picks up the signals that will be emitted by that process.
Comment 10 JasSmith 2010-06-18 17:54:31 UTC
Splendid, please keep me in the loop. Just so you know wncksync has been renamed BAMF (BAMF Application Matching Framework) and is now shipping in Maverick for usage by Unity, global menus, and app indicators. As far as I know Zeitgeist will begin using this extension point as well soon.
Comment 11 David Zeuthen (not reading bugmail) 2010-06-18 17:55:41 UTC
I'd like to strongly object to this - there's absolutely no reason to make this
something like this pluggable and doing so will only increase the load on
downstreams since it will make it hard to figure out what implementation people
are using.

FWIW, we're already seeing problems like this with XFCE's libfm that does this
with /usr/lib64/gio/modules/libgiofm.so which provides the
gio-desktop-app-info-lookup extension. Which causes different behavior in GNOME
just because you have XFCE installed.

Instead, GIO proper should ship the best possible implementation.
Comment 12 Colin Walters 2010-06-18 18:17:44 UTC
(In reply to comment #11)

> Instead, GIO proper should ship the best possible implementation.

Well...then we'd need to spec out what that implementation is.  As I said above, I'd really prefer the shell to be in control of process launching, since it gives us more feedback (for example, if a process terminates before creating an X window we can know that, rather than having the information lost to the SIGCHLD void in some other random process, and waiting for the startup-notification timeout).

So a stub proposal is:

static gboolean looked_for_launcher = FALSE;
static GDBusConnection *session_bus = NULL;
static GDBusProxy *launcher_proxy = NULL;
if (!looked_for_launcher)
{
  session_bus = g_bus_get_sync (G_BUS_TYPE_SESSION, NULL, NULL);
  if (session_bus)
    {
      launcher_proxy = g_dbus_proxy_new_sync (conn, "org.freedesktop.AppLauncher", ...);
      if (launcher_proxy)
        {
          g_dbus_proxy_call_async (launcher_proxy, "Launch", g_variant_new ("s", desktop_path));
        }
      else
        {
          g_object_unref (session_bus);
          session_bus = NULL;
        }
    }
}
if (!launcher_proxy)
  do_exec();
Comment 13 David Zeuthen (not reading bugmail) 2010-06-18 19:09:31 UTC
(In reply to comment #12)
> (In reply to comment #11)
> 
> > Instead, GIO proper should ship the best possible implementation.
> 
> Well...then we'd need to spec out what that implementation is.  As I said
> above, I'd really prefer the shell to be in control of process launching, since
> it gives us more feedback (for example, if a process terminates before creating
> an X window we can know that, rather than having the information lost to the
> SIGCHLD void in some other random process, and waiting for the
> startup-notification timeout).

Sure. My objection is mainly to the non-deterministic behavior inherent when using GIO extension points and secondary the "avoid plugins etc. when the problem is better solved in the core code" party line.

Anyway, using D-Bus for this sounds fine - that way GIO apps running under a desktop environment will simply just do the right thing (GNOME 2.x: use default; GNOME 3.x: use gnome-shell; XFCE: use whatever these guys want etc. etc.).

Btw, if you're doing this it would probably make sense to also make GIO use the same D-Bus interface for the gio-desktop-app-info-lookup extension [1] to avoid already existing problems pointed out in comment 11.

[1] : http://library.gnome.org/devel/gio/unstable/gio-Desktop-file-based-GAppInfo.html
Comment 14 JasSmith 2010-06-18 19:57:43 UTC
I think its worth mentioning that my original proposal had an extension point that was purely informational. No responsibility was delegated out and therefor behavior would still be deterministic.
Comment 15 Mikkel Kamstrup Erlandsen 2010-06-18 20:59:05 UTC
So if I understand correctly the consensus is that a DBus API like

  org.gnome.AppLauncher.Launch(in s app_id, in as uris)

available on org.gnome.AppLauncher, falling back to some in-process launching logic if no one owns that name. All of this async of course.

So the questions that pop to mind are:

 * Is this done under the hood of g_app_info_launch() - which seems to be synchronous in nature?

 * How about startup notification ids, setting environment variables like DISPLAY and launching on some particular virtual desktop? Do we need to account for that somehow? Generally everything you can set via  GdkAppLaunchContext/GAppLaunchContext.
Comment 16 Mikkel Kamstrup Erlandsen 2010-06-18 21:09:29 UTC
Created attachment 164044 [details] [review]
New extension point with appinfo, uri, launchctx, and pid

It's not that I am not paying attention to the discussion, but I have this patch lying around anyway.

It's a rework of Jason's patch that extends the callback signature to include everything known at the callsite - most notably the URI list.

The callback signature is now:

   void (* on_launched) (GDesktopAppInfoLaunchHandler *launch_handler,
                         GDesktopAppInfo              *app_info,
                         GList                        *uris,
                         GAppLaunchContext            *launch_ctx,
                         gint                          pid);

And quite frankly the more I think about it, the more I feal like a whole new framework around this seems a bit overkill... Why is it that the GAppInfo/GLaunchContext isn't good enough? It's pretty nice as it is now imho.
Comment 17 JasSmith 2010-06-18 21:17:05 UTC
Mikkels rework provides even more context and better support for different uses.
Comment 18 Matthias Clasen 2010-06-18 23:11:25 UTC
I'm with David on this one. I don't want random libraries to hook into central pieces of gio functionality like this. A dbus interface that is implemented by the desktop infrastructure and used by gio sounds much better to me.
Comment 19 Mikkel Kamstrup Erlandsen 2010-06-19 09:22:55 UTC
I don't see how a DBus service prevents this in any way. Any old process can own the well known name (unless we hardwire the name to some of the existing Gnome service names), and anyone can install a match rule to sniff the data off the bus.

The only reason I see for the dbus service would be that it could (maybe) provide some "pid 1 for the desktop"-like capabilities although I can't clearly see what these capabilities would be in this situation.

I think the DBus idea is interesting, but what is the technical argument here? With my patch we get the monitoring and it prevents anyone from hijacking g_app_info_launch().
Comment 20 Matthias Clasen 2010-06-24 00:27:04 UTC
an extension point means that each glib-based desktop environment will feel compelled to install its own module for this. You will end up with applaunchmodule-gnome.so, applaunchmodule-xfce.so and applaunchmodule-someotherdesktop.so installed. 

What priorities are you going to give each one to make the right one win in each desktop ? ...you can't.

With a dbus interface, it is the responsibility of the desktop env to provide an implementation of that, so the right implementation will be naturally present if you run yourfavouritedesktop.
Comment 21 Mikkel Kamstrup Erlandsen 2010-06-24 08:47:25 UTC
But the proposed patch doesn't tinker with the launching mechanism. It's purely a monitor. I agree with your observations on the extension point for app launching. Bad idea.

But I still haven't seen the technical arguments on why we need on out of process app launching scheme in the first place. Why is the current GIO code not good enough?
Comment 22 Mikkel Kamstrup Erlandsen 2010-08-18 10:06:50 UTC
Created attachment 168184 [details] [review]
New extension point with appinfo, uri, launchctx, and pid

Updated patch to apply against git master as of today
Comment 23 Matthias Clasen 2010-12-03 22:06:35 UTC
So, just to remove any doubt: I don't want an extension point for this, regardless if it is launching or monitoring.
Comment 24 Adam Williamson 2010-12-06 17:20:00 UTC
mclasen: okay, and if I'm following the thread right, your preferred method would be:

"A dbus interface that is implemented by the desktop infrastructure and used by gio sounds much better to me."

viz comment 18, right?

Mikkel, Jason, what do you think of that?
Comment 25 Neil Jagdish Patel 2010-12-06 17:35:32 UTC
This patch was never about intercepting how applications were launched, it's simply about being able to see what application has been launched and with what arguments through an infrastructure that works today.

Matthias, can I confirm why, if this patch is taken in the form presented and if the comments about an application server are discarded, this extension point would be rejected? It doesn't seem more or less harmful that a system library knows what application has launched, it's pid and the arguments it was launched with than the functionality any other GIO extension point provides today.

I feel like the comments on this bug ventured off into a completely different direction than what the original patch was about, this patch *does not* alter the launching of any application. What it does do is allow something like Zeitgeist or BAMF to know when an app is launched, it's pid, and with what args.
Comment 26 Colin Walters 2010-12-06 17:56:29 UTC
(In reply to comment #25)
> What it does do is allow something like
> Zeitgeist or BAMF to know when an app is launched, it's pid, and with what
> args.

Here's the problem again - the "or" in your sentence is *exclusive*.  Only one thing can sanely be implementing an extension point (yeah, there's priority, but it doesn't really make sense in practice).
Comment 27 Colin Walters 2010-12-06 17:58:15 UTC
(In reply to comment #21)
> But the proposed patch doesn't tinker with the launching mechanism. It's purely
> a monitor. I agree with your observations on the extension point for app
> launching. Bad idea.
> 
> But I still haven't seen the technical arguments on why we need on out of
> process app launching scheme in the first place. Why is the current GIO code
> not good enough?

Because a centralized place would allow the UI shell to know the status of applications more reliably, in cases like where the application crashes during startup.
Comment 28 Colin Walters 2010-12-06 17:59:47 UTC
As far as an alternative approach, I think we probably want both say:

/usr/bin/xdg-launch --timestamp 12345 mozilla-firefox  (for mozilla-firefox.desktop, timestamp is X server timestamp)

and the corresponding DBus to avoid the exec/etc. overhead.
Comment 29 Neil Jagdish Patel 2010-12-06 18:37:27 UTC
(In reply to comment #26)
> (In reply to comment #25)
> > What it does do is allow something like
> > Zeitgeist or BAMF to know when an app is launched, it's pid, and with what
> > args.
> 
> Here's the problem again - the "or" in your sentence is *exclusive*.  Only one
> thing can sanely be implementing an extension point (yeah, there's priority,
> but it doesn't really make sense in practice).

Why? Why does a *read-only* extension point require only one thing implementing it? Why can't BAMF be notified of Firefox being run a few milliseconds after Zeitgeist?

I'm pretty sure we shipped both a libbamf.so and libgiozeitgiest.so implementation of this extension point in Maverick. They worked fine together and continue to do so.
Comment 30 Colin Walters 2010-12-06 18:47:20 UTC
(In reply to comment #29)
> (In reply to comment #26)
> > (In reply to comment #25)
> > > What it does do is allow something like
> > > Zeitgeist or BAMF to know when an app is launched, it's pid, and with what
> > > args.
> > 
> > Here's the problem again - the "or" in your sentence is *exclusive*.  Only one
> > thing can sanely be implementing an extension point (yeah, there's priority,
> > but it doesn't really make sense in practice).
> 
> Why? Why does a *read-only* extension point require only one thing implementing
> it? Why can't BAMF be notified of Firefox being run a few milliseconds after
> Zeitgeist?

Well, the patch attached here clearly only instantiates one point and calls into it.  I guess if one required the points to only be "read-only" in that sense, yes, you could have every GIO-using process make N different dbus calls...

Actually, what is the Zeitgeist one for?  Is it just interested in the URLs?
Comment 31 JasSmith 2010-12-06 19:24:24 UTC
The zeitgeist plugin is intended to provide information about what applications are launcher with what files as well as when they were launched. Similar to what BAMF uses it for.

As for making N different dbus class, opening a .desktop file is a very uncommon thing in comparison to other things. Considering you are creating a new process each time you do this, the extra dbus call overhead feels rather light. If the patch submitted here only instantiates one object this is a mistake. Mikkel and I will ensure that it works properly with many consumers as is intended. I am not sure if this is the most recent version or not, I need to speak with Mikkel.
Comment 32 Colin Walters 2010-12-06 21:44:04 UTC
(In reply to comment #31)
> The zeitgeist plugin is intended to provide information about what applications
> are launcher with what files as well as when they were launched.

You're saying the zeitgeist one also does application/XID matching?

> Similar to
> what BAMF uses it for.

What does BAMF care about the file list though?

> As for making N different dbus class, opening a .desktop file is a very
> uncommon thing in comparison to other things. Considering you are creating a
> new process each time you do this, the extra dbus call overhead feels rather
> light. 

Oh we clearly have lots more stupid stuff in the desktop and apps; I'm not saying performance is a blocker or even an issue.  

> If the patch submitted here only instantiates one object this is a
> mistake. Mikkel and I will ensure that it works properly with many consumers as
> is intended. I am not sure if this is the most recent version or not, I need to
> speak with Mikkel.

But how much sense does it make to keep iterating on the patch instead of moving forward on a standard app-launching API, where we can be a lot more flexible than having a bunch of random bits of code hook into every single process.
Comment 33 JasSmith 2010-12-06 21:58:08 UTC
Zeitgeist does not do XID/application matching, however it is interested in observing behavior to determine trends. It does weird things with the information, but I know they are interested in it.

BAMF cares about the file list as a way to know what files an application has started to manage. BAMF is quickly moving to enabling both an application centric and a document centric view of the linux desktop. How this information is then represented to the user is up the the user interface. BAMF however will provide information such as what URI's each application is managing, what URL's web browsers are looking at, previews of those documents/pages, and a method of activating them within those programs. Being able to watch this information go out, see who launched what with what files is important.

As for a central app-launching API, most solutions proposed tend to focus around GNOME-Shell or are very GNOME centric, and dont always exist in the reality of the cross-desktop world we live in. I would be happy if there was one point where KDE/GNOME/XFCE/E17/etc all would do application launching from, however there is not and not likely to be one ever. From the point of view of a person writing BAMF, it makes no difference if this is in GIO or if GNOME makes everything launch through a service, except for one thing. It is dramatically easier to do it this way and requires zero change to existing applications and launchers, which is a major selling point. If GNOME moves into an app launching service, thats fine, it is just one more data provider to me, to Zeitgeist, and to any application looking to gather this sort of information.
Comment 34 Matthias Clasen 2010-12-07 15:01:12 UTC
I'm actually a lot more negative on the whole monitoring idea than I am on centralized app launching. It just seems fundamentally the wrong approach. 

Whats next, a patch to gnome-terminal to catch apps that are launched on the 
commandline ?

We don't just add extension points because it is a convenient way for you to snoop at some things. There really should be some justification like avoiding a linking problem or license issues.
Comment 35 Colin Walters 2010-12-16 14:49:39 UTC
(In reply to comment #33)
> 
> As for a central app-launching API, most solutions proposed tend to focus
> around GNOME-Shell or are very GNOME centric, and dont always exist in the
> reality of the cross-desktop world we live in. I would be happy if there was
> one point where KDE/GNOME/XFCE/E17/etc all would do application launching from,
> however there is not and not likely to be one ever.

But that has nothing to do with this bug.  KDE for example doesn't use GIO for application launching currently as far as I know.  So your patch is irrelevant there too.

> From the point of view of a
> person writing BAMF, it makes no difference if this is in GIO or if GNOME makes
> everything launch through a service, except for one thing. It is dramatically
> easier to do it this way

The complexity here is debatable, and mostly irrelevant anyways.  Correctness is more important.

> and requires zero change to existing applications and
> launchers, which is a major selling point.

If GIO used a launch service, there would also be zero change to applications.

> If GNOME moves into an app launching
> service, thats fine, it is just one more data provider to me, to Zeitgeist, and
> to any application looking to gather this sort of information.

Well, let's not call these things "applications"; they're more extensions or frameworks.
Comment 36 Colin Walters 2010-12-16 14:57:35 UTC
(In reply to comment #34)
> I'm actually a lot more negative on the whole monitoring idea than I am on
> centralized app launching. It just seems fundamentally the wrong approach. 
> 
> Whats next, a patch to gnome-terminal to catch apps that are launched on the 
> commandline ?

That would have to be to bash, actually; well, short of ptracing everything by default.  

The terminal case is a huge problem and always has been - not just for this but e.g. we've never gotten timestamps right.  Long term, I think we will have to end up with some sort of GNOME bash extension too.

> We don't just add extension points because it is a convenient way for you to
> snoop at some things. There really should be some justification like avoiding a
> linking problem or license issues.

Okay so...what we need to scope out here is how a service would work.  There are two broad options:

1) Add a private GNOME interface
2) Spec out a freedesktop standard

Honestly, given how little time we have left before the stable GLib release, and how long it takes things to go through fd.org, I suggest we simply add a patch to call into org.gnome.something.  This does mean we'd also take a patch to dynamically check for KDE's launching service too.
Comment 37 Colin Walters 2010-12-20 18:13:56 UTC
Created attachment 176775 [details] [review]
gdesktopappinfo: Send out a session bus signal when launching .desktop file

This signal contains the full path of the .desktop file, along with
the process id, which allows interested parties such as GNOME Shell
to better know the state of the system (which processes correspond
to which .desktop files).
Comment 38 Colin Walters 2010-12-20 18:15:42 UTC
I realized there's a simple change that will satisfy everyone:

1) Send out a DBus signal on the session bus with the data

Having various interested parties supply separate purely informational extension points which in turn invoke DBus or IP over avian carrier or whatever is silly when we can just emit one well-known signal.

2) Optionally check for org.gnome.Shell on the bus, and use it to launch.

This would be a private interface; we could later spec out something "standard", but it's not a big deal.
Comment 39 JasSmith 2010-12-20 18:57:43 UTC
Looks like this would work for me.
Comment 40 Colin Walters 2010-12-20 20:21:43 UTC
Created attachment 176786 [details] [review]
gdesktopappinfo: Send out a session bus signal when launching .desktop file

This signal contains the full path of the .desktop file, along with
the process id, which allows interested parties such as GNOME Shell
to better know the state of the system (which processes correspond
to which .desktop files).

V2: Memory leak fix
Comment 41 Colin Walters 2010-12-20 20:21:49 UTC
Created attachment 176787 [details] [review]
gdesktopappinfo: Add g_desktop_app_info_launch_uris_lowlevel()

A new GDesktopAppInfo specific function which provides more control
over launched processes.  Intended basically only for use in GNOME
Shell, where we want:

1) To avoid recursively calling ourself via DBus
2) To know the GPid for each launched program
3) Possibly control over the process environment; for example,
   we may want to call setsid() or redirect file descriptors.
Comment 42 Colin Walters 2010-12-21 14:19:16 UTC
Created attachment 176825 [details] [review]
gdesktopappinfo: Send out a session bus signal when launching .desktop file

This signal contains the full path of the .desktop file, along with
the process id, which allows multiple interested components (like
GNOME Shell) to better know the state of the system (which processes
correspond to which .desktop files).

V2: Memory leak fix
V3: Ensure we only broadcast URIs which are actually passed to each
    process.
Comment 43 Colin Walters 2010-12-21 14:19:30 UTC
Created attachment 176826 [details] [review]
gdesktopappinfo: Add g_desktop_app_info_launch_uris_lowlevel()

A new GDesktopAppInfo specific function which provides more control
over launched processes.  Intended basically only for use in GNOME
Shell, where we want:

*) To directly know the GPid for each launched program, without
   having to listen to a DBus signal emitted in our own process
*) Possibly control over the process environment; for example,
   we may want to call setsid() or redirect file descriptors.

And in the future:
*) To avoid recursively calling ourself via DBus, when a later
   patch causes g_app_info_launch() to indirect via the shell.

V2: Add GSpawnFlags argument, add user data for each callback
Comment 44 Colin Walters 2010-12-21 14:27:38 UTC
See corresponding shell patch at https://bugzilla.gnome.org/show_bug.cgi?id=637745
Comment 45 Matthias Clasen 2010-12-21 15:03:27 UTC
'lowlevel' is not a great name. It's long, and not really accurate (if it was really lowlevel, I would expect launch_uris to be implemented in terms of it).

How about 'full' ? Not a great name either, but shorter, and we have prior art in calling 'just the same with more args' variants that...
Comment 46 Matthias Clasen 2010-12-21 15:10:17 UTC
I like the dbus signal idea. We need to add some documentation around it, though.
Comment 47 Matthias Clasen 2010-12-21 15:14:20 UTC
I wonder why you chose a callback for the in-process notification. Wouldn't it be a nice match to use a signal for that, too ?
Comment 48 Colin Walters 2010-12-21 15:15:53 UTC
(In reply to comment #45)
> 'lowlevel' is not a great name. It's long, and not really accurate (if it was
> really lowlevel, I would expect launch_uris to be implemented in terms of it).

launch_uris *is* implemented in terms of it.

> How about 'full' ? Not a great name either, but shorter, and we have prior art
> in calling 'just the same with more args' variants that...

Hmm, I thought someone told me not to use "full" in general, but to use descriptive names; "full" was for "like the original, but with GDestroyNotify", but that's not true here.

It's definitely not the same thing though, because _lowlevel *also* skips the DBus stuff.  Hmm, though I should probably change it to still emit the signal, but what I wanted to avoid was what I said in the commit message; gnome-shell trying to recursively invoke itself when a later patch adds centralized launching.
Comment 49 Dan Winship 2010-12-21 16:00:39 UTC
Comment on attachment 176825 [details] [review]
gdesktopappinfo: Send out a session bus signal when launching .desktop file

I don't really know the gvariant/gdbus stuff, so I don't know if you're doing that right, but:

>+  session_bus = g_bus_get_sync (G_BUS_TYPE_SESSION, NULL, NULL);

>+      g_dbus_connection_flush_sync (session_bus, NULL, NULL);

You can't do that; it will make existing apps (including gnome-shell) block in their UI threads when launching apps sometimes.

I think you need to launch the apps first, saving up the URIs/pids, and then fork (twice), create a new (non-shared) dbus connection, and signal over that.
Comment 50 Dan Winship 2010-12-21 16:36:11 UTC
Comment on attachment 176826 [details] [review]
gdesktopappinfo: Add g_desktop_app_info_launch_uris_lowlevel()

> Hmm, I thought someone told me not to use "full" in general, but to use
> descriptive names; "full" was for "like the original, but with GDestroyNotify",

That would have been Owen. Not sure how widespread that meme is.

>+      data.user_setup = user_setup;

need to set user_setup_data too

>+ * @pid_callback: (scope call): Callback for child processes
>+ * @pid_callback_data: (closure callback): User data for @callback

(closure pid_callback)

>+ * This function performs the equivalent of g_app_info_launch_uris(),
>+ * but provides additional control over the exact environment of the
>+ * child processes via a setup function @setup, as well as the process
>+ * identifier of each one via @pid_callback.  In some environments,
>+ * g_app_info_launch() may call a manager process instead of launching
>+ * sub processes as children; this function also skips any such
>+ * functionality.

If there are reasons why it is Good to have a manager process and Bad for apps to launch other apps directly themselves, then the documentation should say "Don't call this method unless you are the launch manager for a desktop environment." (Or maybe even "Don't call this method unless you are holding the name 'org.gtk.gio.LaunchManager' on the session bus".) Otherwise people will use it to get the setup/pid callback functionality, and end up messing other things up in the process.

And then you can call it "g_desktop_app_info_launch_as_manager()" or something.

>+  return _g_desktop_app_info_launch_uris_internal ((GAppInfo*)appinfo,
>+						   _LAUNCH_FLAGS_NO_SESSION_BUS,

But what if there were other processes listening to the signal?


Also, see attachment 176623 [details] [review] (in particular, trySpawn()) and bug 635089 comment 20. For purposes of that patch, it would be nice if there was a g_desktop_app_info_spawn() that took a raw argv rather than a list of files/URIs, but then handled startup notification, launch signalling, etc. (Although this could be fixed differently by just ensuring that there was a .desktop file corresponding to every argv we wanted to launch, eg "gnome-control-center sound", etc. Many of them already exist.)
Comment 51 Colin Walters 2010-12-21 17:01:14 UTC
(In reply to comment #49)
> (From update of attachment 176825 [details] [review])
> I don't really know the gvariant/gdbus stuff, so I don't know if you're doing
> that right, but:
> 
> >+  session_bus = g_bus_get_sync (G_BUS_TYPE_SESSION, NULL, NULL);
> 
> >+      g_dbus_connection_flush_sync (session_bus, NULL, NULL);
> 
> You can't do that; it will make existing apps (including gnome-shell) block in
> their UI threads when launching apps sometimes.

Tradeoff - we could remove the _sync, and allow the signal to be possibly lost if the launching application terminates immediately.

> I think you need to launch the apps first, saving up the URIs/pids, and then
> fork (twice), create a new (non-shared) dbus connection, and signal over that.

Eeek...forking without an exec() is going to be very messy; I'd have to try resetting the gdbus connection state, etc.  

We could possibly push this down into gdbus itself.  I wonder if we can sanely check inside an atexit handler whether we have pending messages, and if so, fork() and finish flushing inside there.  Oh god, the evil.

So okay, if we want to handle reliably sending the signal if the process exits, I think what we probably need is to ship /usr/libexec/gio-launch-desktop-app.
Comment 52 Colin Walters 2010-12-21 17:12:43 UTC
(In reply to comment #50)
> (From update of attachment 176826 [details] [review])
> > Hmm, I thought someone told me not to use "full" in general, but to use
> > descriptive names; "full" was for "like the original, but with GDestroyNotify",
> 
> That would have been Owen. Not sure how widespread that meme is.

I think he's right.

> If there are reasons why it is Good to have a manager process and Bad for apps
> to launch other apps directly themselves, then the documentation should say
> "Don't call this method unless you are the launch manager for a desktop
> environment." (Or maybe even "Don't call this method unless you are holding the
> name 'org.gtk.gio.LaunchManager' on the session bus".) Otherwise people will
> use it to get the setup/pid callback functionality, and end up messing other
> things up in the process.

"_launch_as_process_manager" ?

> >+  return _g_desktop_app_info_launch_uris_internal ((GAppInfo*)appinfo,
> >+						   _LAUNCH_FLAGS_NO_SESSION_BUS,
> 
> But what if there were other processes listening to the signal?

Yes, I will change that.

> Also, see attachment 176623 [details] [review] (in particular, trySpawn()) and bug 635089 comment
> 20. For purposes of that patch, it would be nice if there was a
> g_desktop_app_info_spawn() that took a raw argv rather than a list of
> files/URIs, but then handled startup notification, launch signalling, etc.
> (Although this could be fixed differently by just ensuring that there was a
> .desktop file corresponding to every argv we wanted to launch, eg
> "gnome-control-center sound", etc. Many of them already exist.)

Hmmm; yes, I think that it makes sense to just create .desktop files for them; they could be "private" like gnome-shell/data/gnome-shell-foo-config-private.desktop, and you can use shell_app_system_load_from_desktop_file().
Comment 53 Colin Walters 2010-12-21 18:10:46 UTC
(In reply to comment #47)
> I wonder why you chose a callback for the in-process notification. Wouldn't it
> be a nice match to use a signal for that, too ?

Signals are weird for this in that the state change *only* happens during function invocation.  To be correct you'd have to

int id = g_signal_connect (appinfo, "launchinfo", my_callback, mydata)
g_desktop_app_info_launch_lowlevel (appinfo);
g_signal_disconnect (appinfo, id);

Which is just cumbersome.
Comment 54 Dan Winship 2010-12-21 19:11:09 UTC
(In reply to comment #51)
> > I think you need to launch the apps first, saving up the URIs/pids, and then
> > fork (twice), create a new (non-shared) dbus connection, and signal over that.
> 
> Eeek...forking without an exec() is going to be very messy; I'd have to try
> resetting the gdbus connection state, etc.  

That's why I was suggesting creating a new connection rather than trying to share the default one.

> We could possibly push this down into gdbus itself.  I wonder if we can sanely
> check inside an atexit handler whether we have pending messages, and if so,
> fork() and finish flushing inside there.  Oh god, the evil.

I believe the answer to "Can we sanely.*atexit handler.*\?" is "no". And anyway, it's not just the flushing that's potentially blocking, g_bus_get_sync() can block too, so you can't even queue up the messages from launch_uris().

> So okay, if we want to handle reliably sending the signal if the process exits,
> I think what we probably need is to ship /usr/libexec/gio-launch-desktop-app.

It would suck for gvfs-open to have to just immediately spawn gio-launch-desktop-app though. If we went this way maybe it would be worth adding explicit launch_uris_sync/async/finish APIs as well, which apps could use to avoid the need for using the helper.
Comment 55 Colin Walters 2010-12-21 19:33:56 UTC
(In reply to comment #54)
> (In reply to comment #51)
> > > I think you need to launch the apps first, saving up the URIs/pids, and then
> > > fork (twice), create a new (non-shared) dbus connection, and signal over that.
> > 
> > Eeek...forking without an exec() is going to be very messy; I'd have to try
> > resetting the gdbus connection state, etc.  
> 
> That's why I was suggesting creating a new connection rather than trying to
> share the default one.

I haven't looked in detail, but I really don't want to even try keeping gio in general working across a fork(); who knows what locks are held, etc.

> I believe the answer to "Can we sanely.*atexit handler.*\?" is "no". And
> anyway, it's not just the flushing that's potentially blocking,
> g_bus_get_sync() can block too, so you can't even queue up the messages from
> launch_uris().

True, but for g_bus_get_sync() in the common case we'll already have a bus connection, so it will be an immediate return.  In the uncommon case, yes, we'll bounce to dbus-daemon twice, but it's going to be fairly deterministic.

But on the other hand, let's be realistic here - g_spawn_* also does a lot of blocking on the filesystem.  Most of those inodes are in core, probably, yes...but still.

> It would suck for gvfs-open to have to just immediately spawn
> gio-launch-desktop-app though.

Suck in terms of code complexity, or overhead?  I'm not concerned about the latter, and actually having a separate binary interface here would be great (I started prototyping out xdg-launch-desktop-app).

> If we went this way maybe it would be worth
> adding explicit launch_uris_sync/async/finish APIs as well, which apps could
> use to avoid the need for using the helper.

But under your criteria, is the current g_app_info_launch() sync or async?  It has some of both (sync filesystem blocking, but it certainly doesn't pause until the app has actually done anything interesting).
Comment 56 Dan Winship 2010-12-21 21:30:15 UTC
(In reply to comment #55)
> Suck in terms of code complexity, or overhead?

Overhead/architecture. "Run this program to have it run a program to run your program"

> But under your criteria, is the current g_app_info_launch() sync or async?

Neither. It's (theoretically) non-blocking.  g_app_info_launch_sync() would be the current API plus a GCancellable. (Even if the cancellable didn't actually work, its presence in the API would indicate "might block; don't call from the UI thread".) g_app_info_launch_async() would do the same thing from a thread or something; the new API wouldn't have different return value semantics from the current API, it would just guarantee that one way or another, the caller has to wait for the dbus call to complete, and so we didn't need to do the out-of-process thing.

But if you have additional reasons for liking the out-of-process idea, then there's no reason to do this.
Comment 57 Colin Walters 2010-12-24 22:35:26 UTC
Created attachment 177027 [details] [review]
gdesktopappinfo: Send out a session bus signal when launching .desktop file

This signal contains the full path of the .desktop file, along with
the process id, which allows multiple interested components (like
GNOME Shell) to better know the state of the system (which processes
correspond to which .desktop files).

V2: Memory leak fix
V3: Ensure we only broadcast URIs which are actually passed to each
    process.
V4: Don't flush
Comment 58 Colin Walters 2010-12-24 22:35:40 UTC
Created attachment 177028 [details] [review]
gdesktopappinfo: Add g_desktop_app_info_launch_uris_as_manager()

A new GDesktopAppInfo specific function which provides more control
over launched processes.  Intended basically only for use in GNOME
Shell, where we want:

*) To directly know the GPid for each launched program, without
   having to listen to a DBus signal emitted in our own process
*) Possibly control over the process environment; for example,
   we may want to call setsid() or redirect file descriptors.

And in the future:
*) To avoid recursively calling ourself via DBus, when a later
   patch causes g_app_info_launch() to indirect via the shell.

V2: Add GSpawnFlags argument, add user data for each callback
V3: Rename to g_desktop_app_info_launch_uris_as_manager().
    Tweak docs, add missing handling for user_setup_data.
Comment 59 Colin Walters 2010-12-24 22:41:59 UTC
I think we can go with these patches for now; I've dropped the g_dbus_connection_sync().   We can later add a "gio-desktop-app-launch" binary (and an _async variant if that's desired).
Comment 60 Dan Winship 2010-12-27 17:29:45 UTC
Comment on attachment 177027 [details] [review]
gdesktopappinfo: Send out a session bus signal when launching .desktop file

> True, but for g_bus_get_sync() in the common case we'll already have a bus
> connection, so it will be an immediate return.  In the uncommon case, yes,
> we'll bounce to dbus-daemon twice, but it's going to be fairly deterministic.

And the most common case of not already having a connection would probably be something like gvfs-open, which doesn't have a UI to block anyway. OK.

>V2: Memory leak fix
>V3: Ensure we only broadcast URIs which are actually passed to each
>    process.
>V4: Don't flush

(make sure to remove these from the commit message before merging/pushing)
Comment 61 Dan Winship 2010-12-27 17:33:19 UTC
Comment on attachment 177028 [details] [review]
gdesktopappinfo: Add g_desktop_app_info_launch_uris_as_manager()

>+enum _LaunchFlags {
>+  _LAUNCH_FLAGS_NONE = 0
>+};

you can just get rid of this. It's internal API. If we need to change something later we can change it later.

>+ * During invocation, g_desktop_app_info_launch_uris_lowlevel() may

fix name
Comment 62 Matthias Clasen 2010-12-28 04:56:56 UTC
Review of attachment 177028 [details] [review]:

::: gio/gdesktopappinfo.c
@@ +1179,3 @@
+ * This function performs the equivalent of g_app_info_launch_uris(),
+ * but is intended primarily for operating system components that
+ * launch applications.  Ordinary applications should use

Wouldn't hurt to add a 'like gnome-shell' here.

@@ +1184,3 @@
+ * Specifically, this function provides additional control over the
+ * exact environment of the child processes via a setup function
+ * @setup, as well as the process identifier of each one via

'each one' confused me for a second here. I think it would be clearer to say 'of each child process' here.

@@ +1186,3 @@
+ * @setup, as well as the process identifier of each one via
+ * @pid_callback.  In contrast to g_app_info_launch_uris(), the
+ * process will always be run directly.

What does 'the process will always be run directly' mean ? First of all, 'the process' seems slightly contradictory to the prior talk about multiple processes, and second, the launch_uris() docs don't talk about running something 'indirectly'...

::: gio/gdesktopappinfo.h
@@ +100,3 @@
+ * @user_data: User data
+ *
+ * During invocation, g_desktop_app_info_launch_uris_lowlevel() may

The reference should be updated to the new function name, I guess ?
Comment 63 Colin Walters 2011-01-05 16:58:43 UTC
Attachment 177027 [details] pushed as bb6c44b - gdesktopappinfo: Send out a session bus signal when launching .desktop file
Attachment 177028 [details] pushed as e6546de - gdesktopappinfo: Add g_desktop_app_info_launch_uris_as_manager()
Comment 64 JasSmith 2011-01-05 17:17:30 UTC
Looks great, thanks Colin