Created attachment 51330 [details] [review]
A proposed fix

This patch establishes the current timestamp and presents the window with it.

Don't know if this is the right approach to take, but I couldn't find a better
way

Comments?

wow a 4 month old patch that still applies to HEAD. Yay!

DonS, this fixes the issue here.  Commit it I say!

yelp-base.c: In function ‘slowly_and_stupidly_obtain_timestamp’:
yelp-base.c:231: warning: pointer targets in passing argument 7 of ‘XChangeProperty’ differ in signedness
yelp-base.c: At top level:

correct way to do this is here:

http://mail.gnome.org/archives/desktop-devel-list/2004-December/msg00306.html

but perhaps a solution to bug 326008 would fix this as well.

(Hmm... midair collision, but perhaps something in my long comments is useful anyway, so I'll submit despite duping Brent's comments a little)

The patch may provide better behavior than the problem, but it turns out to be a hack that will fail in various race-condition like ways (making yelp likely to steal focus when it shouldn't and in ways that are hard to measure under normal testing but which would constantly bug people like me who can trigger things like the stuff in bug 152000).  It's not too surprising that it's a hack since the gnome-terminal/nautilus code copied is a hack as well, but there are two important differences between this patch and the code in those modules:
  - gnome-terminal & nautilus first try to obtain the timestamp with which the additional instance was launched and then forwarded that to the first instance instead of resorting to the hack
  - failing that (meaning the application was not correctly launched, i.e. wasn't launched with startup notification), the _second instance_ would ping the xserver for a timestamp as early as possible so that it could get a timestamp as close as possible to its "launch time" and then forward that timestamp on to the first instance.

Oh, there is actually a third difference: gnome-terminal/nautilus use gdk_x11_window_set_user_time() after explicitly realizing the window and before showing it, instead of using gtk_window_present*.  That saves an unnecessary round trip and avoids another (though quite unlikely) race condition.

But, I think that this is just yet-another-demonstration of the fact that we _really_ need a nice lib for apps so that we don't have a zillion and one roll-your-own (likely buggy) single instance code handling versions around.  So bug 326008 looks very relevant.  :)  In the mean time, though, http://mail.gnome.org/archives/desktop-devel-list/2004-December/msg00306.html explains the details; and http://mail.gnome.org/archives/nautilus-list/2005-February/msg00045.html may be a useful thread.

I may be able to help codewise if needed; if not I can at least answer questions.  :)

Created attachment 56969 [details] [review]
Update

ok, here is an updated (well, rewritten) patch that might be somewhat better.  I'm really not sure about this stuff so, let me know if I'm on even vaguely the right track.

The patch now trys to find the timestamp from the DESKTOP_STARTUP_ID env. variable.  failing that, it falls back on the slowly_and_stupidly_get_timestamp thing (which is now called by the second instance).  It does this in the first possible opportunity in main().  This is then passed to the other instance (hence the mucking about with the idl stuff).

From http://mail.gnome.org/archives/desktop-devel-list/2004-December/msg00306.html
it says the DESKTOP_STARTUP_ID is of the form:
<unique identifier>_TIME<timestamp of launch event>
Instead of dragging in libsn as a dependancy, I have just extracted the timestamp from the timestamp string.  Is this ok, or does libsn need to be involved?  The user_time is set from that and then the window is shown.  If no timestamp is passed in (i.e. another window is asking for a new window), none of this happens - does anything need to be done in this case?

Is this on the right track?  Cheers

Created attachment 57404 [details] [review]
Update to the update

Makes it apply cleanly to current CVS HEAD

Comment on attachment 57404 [details] [review]
Update to the update

The timestamp handling seems good for the most part but you have a number of edge case bugs not handled.  Some of these bugs also cover up startup notification problems; you probably want to use the startup notification library to handle some of this work for you (e.g. functions like sn_launchee_context_get_startup_id(), sn_launchee_context_setup_window(), sn_launchee_context_get_id_has_timestamp(), etc., in addition to the gtk function gtk_window_set_auto_startup_notification(FALSE)), much like nautilus or gnome-terminal does.

+	if (startup_id != NULL && *startup_id != '\0') {
+		timestamp = g_strdup (startup_id);
+		putenv ("DESKTOP_STARTUP_ID=");

This should all done _before_ gtk_init(); gtk_init() will unset DESKTOP_STARTUP_ID for you which makes startup_id undefined for you; you're lucky it just didn't happen to get overwritten.  In fact, since you use gnome_program_init(), you shouldn't add a call to gtk_init().

>+	if (timestamp) {
>+		gchar *time = strstr (timestamp, "_TIME");

The timestamp string is of the form <unique>_TIME<number>, so this will fail if <unique> contains "_TIME".  I would suggest using g_strrstr() instead.

>+		time+=5;
>+		real_time = g_ascii_strtod (time, NULL);

Two problems:
  - time may be 0x00000005 (NULL + 5), which would be an invalid location to try to convert to a double.  Old startup notification launchers using an ancient version of the spec didn't have timestamp parts of the launch id string (i.e. the _TIME<number> part), they only had the <unique> part of it.  You'll need to check if g_strrstr() returns NULL or not.
  - While startup notification launchers likely won't be buggy, we probably shouldn't on them providing a valid timestamp.  So, you'll probably need to add error checking instead of using g_ascii_strtod() without it.

>+		gdk_x11_window_set_user_time (window->window, real_time);
>+	}

There's a function in the startup-notification library that can do the timestamp parsing for you; again, see nautilus & gnome-terminal for examples (grep for sn_launchee* or maybe just sn_*).

Created attachment 57430 [details] [review]
Third times the charm

Handles startup notification in pretty much exactly the same way as gnome-terminal.  It adds a dependancy of libstartup-notification.  Almost all the code dealing with this is now taken from gnome-terminal ;)

It fixes all the issues Elijah brought up with the last patch.

It does fix all the issues I brought up, but it introduces two new ones both kinda trivial:

- It seems weird to refer to the string as "timestamp" in a few places, since the string does include more and you now use the other parts too (even though the most important of these uses is kind of hidden in a sn_launchee_* function).  "startup_id" would seem to make more sense

- The patch doesn't compile.  You appear to have missed the yelp-base.h portion of the patch resulting in the declaration and definition of yelp_base_new_window() not matching.

Anyway, other than that it looks great to me.  :)

I've committed the patch above (with a couple of changed to fix Elijahs comments).

Closing the bug.

2006-01-16  Don Scorgie  <dscorgie@cvs.gnome.org>

        * configure.in:
        * src/yelp-base.h:
        * idl/GNOME_Yelp.idl:
        * src/yelp-main.c:
        * src/yelp-base.c:
        Add startup notification stuff, fixes #310237