Created attachment 22194 [details] [review]
support USER_TIME

here's a first crack at a patch to support USER_TIME in gdk.  This
seems to work well for most GTK apps, but for some reason, though,
this doesn't work with gnome-terminal.  I haven't had a chance to dig
through gnome-terminal code to figure out why, but I figured Havoc
would probably know what sort of weird black magic gnome-terminal is
doing to bypass this.  Probably something to do with embedding widgets
in toplevel windows behind GDK's back.

The patch adds a new function to the API:
gdk_window_set_user_time_hint which does just that.

It's entirely possible that apps like gnome-terminal will just have to
 manually call set_user_time_hint as appropriate.

As I said in:

 http://mail.gnome.org/archives/wm-spec-list/2003-May/msg00047.html
 http://mail.gnome.org/archives/wm-spec-list/2003-June/msg00001.html

while I don't particularly *like* _NET_WM_USER_TIME, I can't think
of a better way of doing it; since I "gave the approval" there,
I think we need to implement it.

In terms of this patch:

 * The patch only implements half of the _NET_WM_USER_TIME
   scheme - it doesn't set it on newly mapped windows, which
   is the whole point. The default _NET_WM_USER_TIME value
   for newly mapped windows, should be the last 

 * User time also needs to be updated on XInput events.

 * gdk_window_set_user_time_hint() shouldn't (IMO) be in the
   cross-platform API because it's never going to make sense
   on anything but X11. 

 * I don't actually think there is anything special going
   on in the vte widget, so that needs to be debugged
   before we add the patch.

The API I'd like to see is:

 void gdk_x11_window_set_user_time (GdkWindow *window,
                                    guint32    timestamp);
  void gdk_window_set_focus_on_map (GdkWindow *window,
                                   gboolean   should_focus);

 void gtk_window_set_focus_on_map (GtkWindow *window,
                                   gboolean   should_focus);

 Property GtkWindow::focus-on-map

Where the focus-on-map settings default to TRUE and are documented
as "hints"  - should_focus = FALSE says that that focus on map
isn't useful. focus-on-map should be turned off for windows
that aren't triggered interactively, such as popups from network
activitity.

The gdk_window and gtk_window docs, since they are cross-platform,
should not reference X11 specific details, except possibly
as "On X.."

Stubs need to be added in the win32 and linux-fb buffers for the
gdk_window_set_focus_on_map(), though you'll need to file another
bug against the win32 component to actually imnplement the
functionality (if relevant)

Sorry I should have been more explicit when I first posted the patch
that I hadn't yet implemented the part about setting the hint on
initial map.

Another thing that needs to be implemented is keeping track of a sort
of global "last user input".  I'm not sure what the best way to do
that in gdk would be.  Should we use the most recent user_time of the
parent window, or use a global variable somewhere to keep track of it?

Just thought I'd post to say I'm working on this bug.  I was able to
get Rob's patch to work for gnome-terminal and all other gtk+
applications I tried which were linked against my modified gtk+.  (I
also had some issues with gnome-terminal too at first, but I think
they all turned out to be things like me running the wrong
gnome-terminal, not closing all gnome-terminals before opening a new
one to test, etc;  I believe Rob may have just been experiencing
similar problems).  I also have the hint for new mapped windows
implemented and I found it to work when I tested it with my current
patch against Metacity (see bug 118372 for more on that).

I'll post a patch sometime soon and also bring up some
questions/issues related to it...  

Created attachment 23681 [details] [review]
old version of patch; shown only so I can make reference to it

Created attachment 23682 [details] [review]
Patch to support _NET_WM_USER_TIME (do-not-focus-on-map hint)

I've attached two patches.  The first is just so I can reference it
(and was quite similar to Rob's patch), the second is a working patch.
  I have several comments/questions :

 * My gtk patch was based heavily off of Matthias' patch for bug
   64613 since it was highly similar.  However, do I really need all
   the get/set functions? I assumed yes, since get/set functions exist
   even for default size of the window.
 * Owen only mentioned a GtkWindow::focus-on-map variable; isn't
   a GdkWindow::focus-on-map variable needed too? (I didn't see how to
   implement the patch without one.)
 * I don't understand why Rob tried to update _NET_WM_USER_TIME in
   both Metacity and gtk/gdk.  I would think that one would only want
   to do it in Metacity, because the updating in gtk would only work 
   for gtk apps (and doing it in both is redundant).
 * I don't understand Rob's comment about a global variable either.
 * The KeyRelease stuff in my first patch (and in Rob's patch) seems
   to be against the wm-spec.  But I don't quite understand why the 
   spec states to not update on KeyRelease events, and updating at 
   those times would seem to make sense to me.  Comments?
 * I'm not sure how to handle XInput events.  I took a wild stab at it
   in my first patch, but I have no idea whether it's even close to 
   being correct.

I'm adding the PATCH and security keywords.  (Note that windows being
focused upon launch can cause apps such as GAIM to end up receiving
and sending a typed password.  Telsa report on #gnome that she got
Alan's password accidentally once that way.  Also, error dialogs could
be missed if the user hits the enter key right when it appears)

Also, I'm trying to keep track of all the issues (there were many that
came up, mostly related to Metacity, and I was having a hard time
keeping track of links to all the appropriate locations) at
http://www.math.utah.edu/~newren/prevent-focus-stealing.html.  That
may be useful to someone trying to track down the wm-spec files and
all the issues I'm thinking of.

Let me just note that Rob explained to me why he had the gtk updates
of _NET_WM_USER_TIME in bug 118370; I'll add that back into my patch
and attach it here.  (I still don't understand the global variable
comment, but I've asked him to explain more; hopefully that will help
me understand what I'm missing.)

I don't think this is going to make 2.4, but moving to 2.4 API because
the proposed patch adds API.

Created attachment 24500 [details] [review]
Patch to support _NET_WM_USER_TIME

I've had this newer version of the patch for a couple weeks, but I've
been sidetracked by school work and some stuff I was doing with the
bugsquad.  Anyway, I just updated the patch to apply against recent
CVS.  Differences between it and my previous patch:
  - Reincorporates setting of the timestamp upon Key/Button Press
  - Updates the timestamp upon XInput events
  - Has a 'global' user_time for last user input

I was not sure which XInput events should update the user time, so I
simply updated on all of them.  Also, I didn't know if XInput
Key/Button Release events could cause similar problems to normal
Key/Button Release events so I also updated the time for those.  

Also note that this patch has some extra asserts that were mainly
meant for debugging, but they can be removed if/when some form of this
patch gets approved.

I believe this patch is everything that is needed as far as gtk+ goes
for _NET_WM_USER_TIME support.  I've tested it with my
startup-notification and metacity patches and it works well for me. 
[My metacity patch is still missing two things (grabbing keypresses
and not raising windows on map unless they're focused as well) so I
haven't yet posted it (or my updated startup notification patch) to
bug 118372.  So if someone else wants to test it, see the patches on
my website that I posted a link to earlier.]

Comments?

More interesting than testing with metacity would be testing with kde, 
imo. KDE already supports _NET_WM_USER_TIME, right ?

Ah, good point.  I guess I need to download and compile the latest KDE...

Okay, I've now completed the testing under KDE 3.2.  Everything that I
am able to test under Kwin with this gtk+ patch works.  Since I can't
test XInput stuff, however, that does leave the possibility that it
could be wrong.  Is anyone else able to test XInput events?  Anyone
have comments on the saneness of the patch?

Ooops, I forgot to test one of the edge cases.  That one fails, which
means that these lines from gdkwindow-x11.c in the patch

  else
    gdk_x11_window_set_user_time(window,
      GDK_DISPLAY_X11 (screen_x11->display)->user_time);

are suspect.  They work in the vast majority of cases but fail in one
particular test.  So either Rob and Lubos had a communication
disconnect, or else I didn't understand Rob's explanations (which is
much more likely).  I'll go figure out what the correct thing to do
is, make an updated patch, test it, and post it here.

Note that you should NOT set the USER_TIME hint if you don't have a
value for it.  Notably, if the application has just been launched and
has had no user interaction, the USER_TIME hint should not be set at
all unless to say "please do not focus me on map".

The window manager has information from startup notification to fill
in this missing information and decide whether the window should be
mapped.  If the WM needs to map a window and has no USER_TIME hint, it
should try to find a value from startup notification and use that
instead.  If neither value is available, it should give up and focus
the newly mapped window.

Is that what you were asking about, Elijah?

Rob: Yes, that was basically my question/misunderstanding.  For some
reason I thought I needed to set it in all cases and was trying to
obtain a timestamp in gtk_init from the setting of the _NET_WM_PID
hint in order to do so.  I made sure to have my Metacity patch have
the backup timestamp be overruled by a startup notification timestamp
if both were present.  Anyway, I've removed that and I'm attaching a
fixed patch.

Created attachment 24917 [details] [review]
Corrected patch

yea, USER_TIME should only ever be set to a real, actual, verified
timestamp obtained from some sort of actual user input.

Also, you should not override USER_TIME with a startup notification
timestamp.  By definition the USER_TIME value would be more recent, so
you should always use that if it's available.

The WM algorithm is as follows:
on map of window w:
   if user_time hint is set
     w.user_time := user_time hint value for w
   else if startup notification stamp is set
     w.user_time := startup notification time stamp w
   else
     w.user_time := CurrentTime

   if (time == CurrentTime or 
      (time >= (currently focused window).user_time)
      focus and raise newly mapped window
   else
      set NEEDS_ATTENTION on w
      stack w just below currently focused window

Yeah.  Thanks for the patience as I worked to wrap my head around all
the X stuff and the standard.  What you have outlined above is what my
most current gtk patch supports.

I had moved this to 2.6 API for the set_focus_on_map addition, but 
thinking about it, we might add the backend in 2.4.x and only move the


addition of set_focus_on_map to 2.6. 2.4.x would always use the 
default value, TRUE.

The usage of assert() in this patch is incorrect 
a) there is a g_assert(), no need for assert.h
b) you've made it possible to crash the
program by XSendEvent() it an event with a 0 time field,
generally asserting on data not under the control of
the program is wrong.

Other than that, and the missing docs for 
gtk_window_set_focus_on_map(), the patch looks mostly
plausible.

Thanks for the reminder about g_assert; I guess old habits die hard.
I had left the assert's there simply for debugging purposes (I had
some mistakes in early versions of the patch that I wanted to catch);
I didn't mean for them to be in the final version.  I'll fix that and
add the relevant docs.

Created attachment 25184 [details] [review]
Remove asserts sanely

I removed the assert's.  I tried to determine whether the event
timestamp for XInput events can be depended upon to be valid, but I
couldn't find it googling so I just manually check.

Also, Owen, I'm not sure where you want documentation added.  I have a
stub in gtk-sections and gdk-sections as well as inline comments for
the  .c file.  I added a stub in the windows.sgml and gtkwindow.sgml
files, but let me know if that's not what you meant.

Elijah, the doc look fine. Can we get the patch split so that the backend stuff
goes into 2.4 and the new api into 2.6 ?

Comment on attachment 25184 [details] [review]
Remove asserts sanely

I think the usability benefits from this patch are big enough that we should
try to get it in 2.4.1

Created attachment 26661 [details] [review]
Patch with API removed

Here's the patch without the API.  This patch still adds the focus_on_map
property, but sets it to TRUE and provides no API for checking or setting its
value.	I thought this would be cleaner since it allows us to add the API later
without making changes to the existing code.  Is this what you wanted, or do
you want me to remove the focus_on_map property as well?

Technically, the property is the main api, with the setter and getter just being
C binding sugar. Therefore, the property should be a 2.6 addition as well,
but that seems to be already the case with your patch. 
You remove the PROP_FOCUS_ON_MAP constant, and your patch looks good.

I'd suggest that you attach the api portions of the patch to this bug as a
separate attachment, then we can just move the bug to the 2.6 API milestone after
committing the non-API portions. 

Soeren, will you commit this ?

Created attachment 26669 [details] [review]
Corrected patch, no PROP_FOCUS_ON_MAP constant

Ooops, yeah I mean to remove that line with the PROP_FOCUS_ON_MAP but
apparently missed it.

I committed something based on this patch. Changes:

       - made gdk_x11_window_set_user_time() internal
       - removed unused focus_on_map variables

Elijah, I agree with Matthias that it would be a good idea to attach the API
part of the patch. I'm moving it to the 2.6.0 API freeze milestone.



Sun Apr 18 16:15:15 2004  Soeren Sandmann  <sandmann@daimi.au.dk>

	Support for _NET_WM_USER_TIME (bug 115650). Patch by Elijah
	Newren.
	
	* gdk/x11/gdkwindow-x11.[ch]: Add new internal function
	_gdk_x11_set_user_time() to set the _NET_WM_USER_TIME property.

	* gdk/x11/gdkdisplay-x11.h (struct _GdkDisplayX11): Add user_time field

	* gdk/x11/gdkdisplay-x11.c: Add _NET_WM_USER_TIME to list of
	precached atoms.
	
	* gdk/x11/gdkinput-x11.c, gdk/x11/gdkevents-x11.c: Set the
	property on user interaction.

gdkwindow-x11.c defines _gdk_x11_window_set_user_time
but gdkinput-x11.c uses gdk_x11_window_set_user_time (ie, without the _).

I can't compile GTK+ from cvs HEAD. I'm getting a undefined reference to
`gdk_x11_window_set_user_time'.

Created attachment 26990 [details] [review]
fix compilation error

It appears that Soeren forgot to
s/gdk_x11_window_set_user_time/_gdk_x11_window_set_user_time/ in the
gdkinput-x11.c file as he had done elsewhere.  Here's the simple patch to fix
that.  Can I commit?  (And yes, I'll try to post the patch with the rest of the
API soon separately)

Comment on attachment 26990 [details] [review]
fix compilation error

I don't know if Matthias applied this patch and forgot to mark this bug as
committed, or discovered the compilation problem independently an fixed it. 
Anyway, it's fixed now so I'll mark this patch as obsolete.

Created attachment 27219 [details] [review]
API portion of the patch

Sorry for the delay--I wanted to wait for the other commits (Soeren's
modifications to my patch and Matthias's slight changes) before attaching the
API portion of the patch so that I could avoid future conflicts as much as
possible.  Anyway, here it is.

Comment on attachment 27219 [details] [review]
API portion of the patch

Since we've branched for 2.6 now, this is fine to commit.

I commited, so I'm marking as fixed to close the bug.

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

The patch is incomplete and leaves an important case not working.  If a user is
using an app and:
  1) The app opens another window (e.g. preferences or a confirmation dialog)
  2) The app closes the window (user is done using it or whatever)
  3) The user interacts with other window
  4) The app reopens that extra window (e.g. user wants to modify more
     preferences or whatever)
then the second time the window is reopened it will appear without focus and
behind whatever window currently has focus.  This is because the
_NET_WM_USER_TIME property for that window is that stale value from previous
usage, instead of the timestamp that caused the window to be mapped as it should
be.  So, I'm reopening this bug.  I'll attach a patch momentarily that fixes the
issue.  The patch is against the gtk-2-4 branch; a similar one will be need for
HEAD.

Created attachment 30764 [details] [review]
Patch to make sure _NET_WM_USER_TIME is updated

Some comments:
  A) We shouldn't always update _NET_WM_USER_TIME when we're about to map
     a window.	There are two cases when we shouldn't:
     1) When the app has already manually set it's value to 0 to request
	no focus
     2) When the app has set _NET_WM_USER_TIME to something more recent than
	the most recent user interaction with the app.	(see bug 150271 for
	a case where this occurs; basically whenever we have a inter-process
	communication this can happen)
     These two things are the reason for the logic in show_window_internal.
  B) The _NET_ACTIVE_WINDOW message was also using an obsolete version of the
     spec, and the newer version is closely related to the _NET_WM_USER_TIME
     stuff, so I updated that too.  However, my data.l[2] is almost certainly
     wrong.  It's supposed to be the id of the currently active window.
  C) This may affect other parts of my patch...the patch from bug 144302 
     modified _gdk_x11_window_set_user_time.  Perhaps the modifications are
     right, but it seems odd to be using a long (which could be 64 bits) to be
     passed as something which is specified to be a pointer to 32 bits.  If it
     is right, then my patch probably shouldn't be using guint32 for the new
     variable I introduct (and I need to fix Metacity...).  Could someone
     explain?

Oops, I forgot to mark this as blocking bug 149028, I'll do so now.

Please reclose this bug and open a new one. Reopening bugs is close
to never right when a fix has gone into CVS. Actually, looks like
two new bugs. A) and B) are unrelated issues.

As for C, the issue is that CARD32 server side items are represented
as longs in the Xlib API, but they still only have 32 bits of 
data in them. Your patch looks OK to me at first glance.

Okay, sorry for the confusion.  I'll split the patches for (A) and (B).  Thanks
for the clarification on (C).