GNOME Bugzilla – Bug 151245
Application startup notification forwarding
Last modified: 2004-12-22 21:47:04 UTC
Okay, I have put together some proof-of-concept patches demonstrating the ability to do application startup notification forwarding for apps which request already-running instances to open another window. Currently, I only have it working for gnome-terminal and I'm not sure that this is even the right way to go about accomplishing this; it may be that I need to massively rework these patches (or that someone else does). But, I figured this will at least get the discussion going. The basic ideas are: 1) Have startup notification launchers set a DESKTOP_STARTUP_TIMESTAMP environment variable equal to the value of the TIMESTAMP field passed in the X messages 2) Have the app read this environment variable, if it exists 3) Have the app forward this environment variable along with the rest of the information in its request to the other instance to open a new window 4) Have the other instance of the app call copy_of_gdk_x11_window_set_user_time on the window it opens so that metacity has a correct _NET_WM_USER_TIME to compare. In order to understand my patches, let me list some of the problems I was trying to overcome: 1) I have a gtk+ patch, but it's much to late to add API for gtk+-2.4. So for now, apps that need this extended functionality (gnome-terminal, nautilus, epiphany, etc.) will need to read the environment variables directly instead of having gtk+ handle it. 2) because of (1) there's a fairly likely possibility of a DESKTOP_STARTUP_TIMESTAMP environment variable being left over from previous activity since most apps won't unset this. So, for now, apps should only trust this environment variable if the DESKTOP_STARTUP_ID environment variable is set and valid. 3) apparently the two gnome modules that utilize startup-notification (gnome-desktop and nautilus) do their own little thing as far as creating the environment for apps that get launched. So, all three must be patched. 4) It seemed like I did an awful lot of unncessary work in order to accomplish step 3 for gnome-terminal--but I wasn't very familiar with the code and just changed things until something worked. Suggestions on simplifying that step would be much appreciated. 5) Because of (4), my patch for gnome-terminal adds a string which would break string freeze. The string is totally unimportant too. This is just another reason to cut down on the size of the gnome-terminal patch. But I don't yet understand popt very thoroughly, so I haven't done that at this point. I'll attach the five patches in a minute
Created attachment 31033 [details] [review] gtk patch to handle DESKTOP_STARTUP_TIMESTAMP This patch is against the gtk-2-4 branch, even though it can't be applied there due to the API addition. But it could be applied other than the new gdk_x11_display_get_startup_timestamp function, and the patch should apply fairly easy to HEAD. Because of these issues, this patch isn't necessary to see my proof-of-concept work.
Created attachment 31034 [details] [review] startup-notification patch to support DESKTOP_STARTUP_TIMESTAMP This patch provides support for DESKTOP_STARTUP_TIMESTAMP in the launcher and launchee, and also updates the spec in doc/startup-notification.txt
Created attachment 31035 [details] [review] gnome-desktop patch to support launching with DESKTOP_STARTUP_TIMESTAMP
Created attachment 31036 [details] [review] nautilus patch to support launching with DESKTOP_STARTUP_TIMESTAMP
Created attachment 31037 [details] [review] gnome-terminal patch implementing startup notification forwarding Just to reiterate: Getting the environment variables in one instance and setting the user time in the other to the passed desktop-startup-timestamp only take a few lines of code. Most of the lines of code in this oversized patch are for passing the desktop-startup-timestamp from one instance being launched to another instance that is already running.
Ok, now I'm reassigning to metacity-maint so I can get feedback from them on these patches. I'm also marking this as blocking 149028 while I'm at it...
Created attachment 31039 [details] [review] gnome-terminal patch, the real version Oops, I accidentally uploaded an old version of the gnome-terminal patch that has a bug. Here's the corrected version.
Comment on attachment 31033 [details] [review] gtk patch to handle DESKTOP_STARTUP_TIMESTAMP Storing startup_notification_timestamp as a malloc'd gulong is pretty weird, usually if you want a "NULL" value with an int you'd either use 0 or -1, or if both of those are valid values, create a "guint startup_notification_timestamp_set : 1" flag Returning a pointer from x11_display_get_startup_timestamp(): definitely weird. Looks good to me otherwise, though of course Owen and Matthias are the guys you need to approve this.
Comment on attachment 31034 [details] [review] startup-notification patch to support DESKTOP_STARTUP_TIMESTAMP We need to post on xdg@freedesktop or wm-spec-list of course, and get Lubos to comment. Same comment about using a pointer to int as for the gtk patch. Looks right to me otherwise.
Comment on attachment 31035 [details] [review] gnome-desktop patch to support launching with DESKTOP_STARTUP_TIMESTAMP Looks good to me.
Comment on attachment 31036 [details] [review] nautilus patch to support launching with DESKTOP_STARTUP_TIMESTAMP Nautilus patch also looks good
Comment on attachment 31039 [details] [review] gnome-terminal patch, the real version Terminal patch - same thing about Time* vs. just Time, this just isn't normally done in GNOME code. Patch does look pretty much right to me though, has the right changes.
OK, per discussion in email maybe the gtk patch and other aspects here reflect misunderstanding on my part.
Per further discussion in email, it appears that the remaining confusion we had was regarding similarities and differences between DESKTOP_STARTUP_ID and DESKTOP_STARTUP_TIMESTAMP: ' > I think this may have been the thing that caused all the confusion, > because the initial idea is to "do the same thing for STARTUP_TIMESTAMP > that we did for STARTUP_ID", yet that may not be the right thing to do > (i.e. if STARTUP_ID is for all windows, then it can't be right because > STARTUP_TIMESTAMP is only relevant to the mapping of a single window) Right, this is probably the key thing I screwed up. I was just thinking "everywhere we have startup ID add startup timestamp" but that's busted, we only want the startup timestamp for the single window. ' My patches don't attempt to do the same thing for the STARTUP_TIMESTAMP as for the STARTUP_ID, so they should be mostly okay as they currently stand, modulo the coding style issues Havoc points out. I'll email the wm-spec list to see if they like this basic method.
Just so that I don't forget, the gnome-terminal patch has a bug in that it sets the _NET_WM_USER_TIME on the very first window launched, when it should only be set on subsequent windows (see also bug 152030).
Above patches are all invalid since Lubos introduced a different proposal to the spec to handle this and his proposal is the version that was accepted. The cool thing is that his proposal requires far less work. We only need to patch startup-notification and gnome-terminal to get things to work. I'll attach the new patches in a minute.
Created attachment 32944 [details] [review] Startup-notification patch to handle new spec Here's the startup notification patch to handle making the DESKTOP_STARTUP_ID's have the format <unique>_TIME<timestamp>
Created attachment 32945 [details] [review] gnome-terminal patch using new DESKTOP_STARTUP_ID format The gnome-terminal patch is also much shorter, because DESKTOP_STARTUP_ID is already forwarded by gnome-terminal...
Comment on attachment 32945 [details] [review] gnome-terminal patch using new DESKTOP_STARTUP_ID format Ooops, sorry--I forgot to mark this as a patch.
Comment on attachment 32944 [details] [review] Startup-notification patch to handle new spec I'm too lazy to look at the full code, but should check that assert(!sequence->timestamp_set) (or inverse assertion) can't be triggered by a broken sn implementation. e.g. one that has both _TIME in the ID and also sets TIMESTAMP property, or one that sets neither.
Comment on attachment 32945 [details] [review] gnome-terminal patch using new DESKTOP_STARTUP_ID format I think g_error on invalid startup ID is probably over the top; warning would be fine. g_error should be a bug in the program at hand, not a bug in input received. Couldn't this be encapsulated in a launchee_context_get_timestamp()? Or even be put in that libsn setup_window() call? I guess it depends on whether setup_window() is defined to be used only on the first window, or on all windows in the launchee, and I don't remember the answer there.
Created attachment 32956 [details] [review] Remove the two asserts, add some code so that the gnome-terminal patch is smaller The assert(!sequence->timestamp_set) definitely can be triggered by a broken sn implementation. I put it there on purpose as a debugging measure to make sure that I correctly converted everything over to the new system and them, um, forgot to pull it out. :-) There was another assert in there too, which can also be triggered. I ripped them both out in this new patch. I also added some functions to make the gnome-terminal patch smaller. The work can't be completely encapsulated--because we can't call gdk_x11_window_set_user_time from startup_notification--but it still looks much nicer. I don't have a cvs account on freedesktop.org. I pinged on irc, applied for one, got a response requesting more info, replied with the needed info, pinged again later when nothing was done for a while, and eventually gave up. So I can't apply the startup notification patch. Can you do that--or else help me get a cvs account there?
Created attachment 32957 [details] [review] Newer, much smaller gnome-terminal patch, just for reference. This is just for reference; I'll file a gnome-terminal bug with this patch soon.
Comment on attachment 32957 [details] [review] Newer, much smaller gnome-terminal patch, just for reference. This patch has been filed in bug 156413.
Comment on attachment 32956 [details] [review] Remove the two asserts, add some code so that the gnome-terminal patch is smaller "last_occurrence(" space before paren! ;-) Looks good, thanks. You should bump the libsn version and make the configure script of metacity/etc. require the newer version probably.
As I stated above, I don't have cvs commit access at freedesktop.org. Can you commit?
Mark seems to have incremented the version to 0.8 right after the last release, so requiring that version when using the new API is correct.