GNOME Bugzilla – Bug 641395
Add more data about the origin application to the "Launched" signal
Last modified: 2011-02-16 18:44:21 UTC
Created attachment 180012 [details] [review] The patch The newly introduced "Launched" signal (https://bugzilla.gnome.org/show_bug.cgi?id=606960) provides useful information about applications being launched, but it lacks information about the origin (the application that is doing the launching), like its PID, prgname, or its desktop file. The attached patch fixes this. Even though PID could be read using dbus' GetConnectionUnixProcessID(), this is more reliable if the origin disappears from the bus shortly after the signal is emitted and also avoids another roundtrip to dbus.
When writing commit messages, it's very helpful for other people to know ***WHY*** you're making a change. I can see from the code *what* you changed, but again not *why*. "The attached patch fixes this." Fixes what - why do you want this information, and what is another process going to do with it? That helps us evaluate whether it makes sense for us to carry the patch.
For example you can see my commit message for a patch I just posted: https://bugzilla.gnome.org/show_bug.cgi?id=640790#c5 I explicitly mention the name of the consuming code "gnome-shell" and briefly explain why it's better than just JS_MaybeGC().
Created attachment 180029 [details] [review] The patch Amended additional info to the commit msg: This will help applications such as zeitgeist's datahub to collect more complete information about application launches, as the "actor" of a launch is important for zeitgeist's magic to work properly.
Created attachment 180297 [details] [review] The patch Sorry, attached the unupdated patch...
Review of attachment 180297 [details] [review]: ::: gio/gdesktopappinfo.c @@ +964,3 @@ + "origin-desktop-file", + g_variant_new ("s", + gio_desktop_file)); One problematic corner case here is if the path in GIO_LAUNCHED_DESKTOP_FILE isn't UTF-8; think e.g. a GB-2312 encoded username in /home/$username/.config/share/applications. In this case what would happen is the process would hit an assertion inside GVariant; I think it will crash then. You could either: 1) Add && g_utf8_validate (gio_desktop_file, -1) to the if statement 2) Send the desktop path as "ay" (array of bytes) We do send it as an array of bytes in notify_desktop_launch, so I suggest you do the same here. You can use g_variant_new_bytestring(). @@ +968,3 @@ + "origin-prgname", + g_variant_new ("s", + g_get_prgname ())); This has a similar problem if the executable path happens to be non-UTF8. I'd also use g_variant_new_bytestring() here.
Created attachment 180334 [details] [review] The patch Updated patch, now uses byte arrays for the desktop file path and prgname.
Review of attachment 180334 [details] [review]: Looks OK now, thanks!
the patch created the issue on https://bugzilla.gnome.org/show_bug.cgi?id=642490