Created attachment 31794 [details] [review]
gtk proof of concept patch (against gtk-2-4 branch)

Created attachment 31795 [details] [review]
metacity proof of concept patch

The above two patches are a proof of concept implementing _NET_WM_TAKE_ACTIVITY
support.  I consider them to just be proof of concept, because while they work,
there were something things I didn't understand about the spec (so I may have
implemented it incorrectly), and there's some other issues or ugliness in the
patches.  (I'm filing the gtk+ patches here because of this; once I understand
some of the below questions better, I'll file a real gtk+ bug).  In particular,
I'm wondering:

From the spec: "data.l[2] = the respective client window", and "The Window
Manager can uniquely identify the message by the timestamp and the data.l[2]
field if necessary.  The Client MUST NOT alter any field in the event other than
the window".  How can the WM uniquely identify the message using the data.l[2]
field, if that field is allowed to change?  Am I missing something?  

I don't see why the window field (data.l[2]) is useful at all.  Although I pass
something for that field, both my Metacity and Gtk+ patches ignore it.

Again, from the spec: "Note however that that the Window Manager should not do
anything if such operations have meanwhile taken place for another window (e.g.
if a new Client window has been mapped as a result of the click)."  I'm not sure
how this comes into play and I haven't taken it into account.  I guess the
example given makes sense, but what other cases are relevant?

My Gtk+ patches hard code the drag and drop threshold.  That's kind of lame, but
I thought calling a gtk function, like gtk_drag_check_threshold, from gdk was
taboo.  Besides, I haven't a clue what I'd pass for the widget parameter.

Should we only raise windows on ButtonRelease for frame clicks?  That would seem
to be more consistent to do that, but I haven't bothered with that yet.

For applications that don't support this spec, it'd be nice to do something
clever to emulate it.  Unfortunately, metacity doesn't get ButtonRelease events
for apps (I'm guessing due to a grab).  This also means we can't even fall back
to the dumb behavior of always raise on ButtonRelease.  We have to use the old
behavior of always raise on ButtonPress for such apps.  It makes the interface a
little inconsistent, but that's probably unavoidable.

In case anyone is trying this out:  In doing further stress testing of my patch
for bug 152000 (which I still haven't posted yet), I happened to be running with
these patches too--as I have been doing  since I created them.  I haven't run
across any glitches for mouse or sloppy focus--they work great.  However, I did
find two issues for click-to-focus:

1) If a window is focused but not raised (which is possible since my patch
focuses on ButtonPress and then conditionally raises later depending on whether
a take_activity return message is received), then subsequent clicks won't raise
(apparently because take activity messages aren't sent in click-to-focus when
the window has focus).

2) If a window is focused but not raised, it's location in the MRU list changes.
 It looks odd to have a window at the front of the MRU list but not be  on top
of the other windows.

I'll fix these issues in the next couple days.

Rob, Havoc: do either of you have any answers to my questions in comment 3?

Created attachment 32039 [details] [review]
Don't focus window upon ButtonPress; wait for take-activity pong message

This fixes the two issues I mentioned above that affected click-to-focus.

Note that this protocol also has an additional possible use for us.  There are a
number of times when we don't want clicking on the panel to focus it, yet we do
need to focus it on click in at least a few cases (e.g. dictionary or command
line applets).  This _NET_WM_TAKE_ACTIVITY stuff allows the client to determine
not only when to raise but also when to focus (because we don't do either unless
we receive a pong message from the app).  So, if gdk provides a function to set
display_x11->tracking_take_activity_info to FALSE, then all we'd have left to do
is to patch gnome-panel to call that gdk function whenever we don't want clicks
to result in the panel getting focus.

So, Rob & Havoc--any comments on this?  In order to do that, we'd have to get it
done before the gtk+ API freeze (the "API slush-freeze" is a little over two
weeks away).  I can put in the time, but I'd like to hear your comments on
whether I should try that and on any thoughts you might have regarding my
questions in comment 3.

That sounds like a great idea to me, assuming Mark agrees.  Though of course we
can put the support into gtk and metacity and the panel can support it later, so
I guess it's if gtk maintainers agree, which they will since its in the spec.

Maybe you could send me an email with a list of your pending patches (in the
order that you want them reviewed) and we can get those in there as necessary,
then branch, then try to add the take_activity stuff and the focus stealing
prevention stuff.

This is a really busy time for me right now at work though so I don't think I'll
be able to write any code to help you out but I think I'll be able to be pretty
responsive on patches at least.  So if you can put in the work I think it would
be a great improvement.  We also have those remaining focus stealing problems
left, since it was a disappointment to everyone, and most especially you Elijah,
that we couldn't get that into 2.8.

also, do you have a gtk bug open about this somewhere?

Comment on attachment 31794 [details] [review]
gtk proof of concept patch (against gtk-2-4 branch)

I just submitted an updated version against gtk+ HEAD as bug 154260, so I'm
marking this patch as obsolete now.

> How can the WM uniquely identify the message using the data.l[2]
> field, if that field is allowed to change?  Am I missing something?

The "window field" is the window the event is sent to, rather than 
data.l[2]

> I don't see why the window field (data.l[2]) is useful at all.  Although I pass
> something for that field, both my Metacity and Gtk+ patches ignore it.

It would be used if you had two events with the same timestamp, perhaps
theoretically possible. In current metacity the process_pong function should
probably be matching both window and timestamp instead of just timestamp.

> I guess the
> example given makes sense, but what other cases are relevant?

I would say that if we call meta_window_focus() again while a TAKE_ACTIVITY is
pending, we should cancel the pending one, as a start.

> Should we only raise windows on ButtonRelease for frame clicks? 

There was some old bug suggesting this. I don't think this is the effect we want
in GTK; we want to raise on button press almost always (gtk would normally
immediately reply to the TAKE_ACTIVITY), but raise on button release *if you
click on an area that may be dragged*. That's what Windows does too.
It will seem clunky to users I believe if we always wait for the release,
instead of only waiting for release when we aren't sure whether we'll focus.

> The "window field" is the window the event is sent to, rather than 
> data.l[2]

If the ButtonPress and ButtonRelease events aren't in the same window, why are
we raising it?  And what is "it" (i.e. which of the two windows do we raise)?

> It would be used if you had two events with the same timestamp

What if both events occurred for the same window?  Just throw one away?

> I would say that if we call meta_window_focus() again while a TAKE_ACTIVITY
> is pending, we should cancel the pending one, as a start.

Would that introduce a race condition?  Consider the following:
  Basic idea: User clicks and releases in window A and then in window B
  More detail:
  1) User clicks in window A
  2) Metacity sends take-activity message to app running window A
  3) User releases mouse button just barely after clicking
  4) User quickly moves mouse to window B
  5) User clicks on window B
  6) Metacity sends take-activity message to app runing window B
  7) Metacity finally receives take-activity pong from app runing window A
  8) Metacity raises and focuses window A, and deletes take-activity
     message for B as a side effect of meta_window_focus

> raise on button release *if you click on an area that may be dragged*

The window can be dragged around.  I guess my question was whether we should
have the window raised when it's being moved.  Yeah, that does make sense. 
Thinking of it in those terms, I totally agree with you for the general case
because the vast majority of users (i.e. people who aren't weird like me) don't
like working with an obscured object.

> The "window field" is the window the event is sent to, rather than 
> data.l[2]

Um, I just noticed something else.  Isn't the event sent from the client to the
root window?  That's how I'm reading what Lubos wrote...

Created attachment 32476 [details] [review]
Support _NET_WM_MOUSE_ACTION (instead of TAKE_ACTIVITY)

See the 14th additional comment to bug 154260 and/or
http://mail.gnome.org/archives/wm-spec-list/2004-October/msg00008.html to learn
more about why I prefer this method to TAKE_ACTIVITY.

Ooops, forgot to mention two things about the MOUSE_ACTION stuff:

1) This patch uses Olivier's idea from the wm-spec-list (for text selection,
raise on ButtonPress, then lower back to normal position upon ButtonRelease). 
I'm not sure I like that, but this patch exists partially to show how easy
things like this are to do.  (Though my implementation is slightly incomplete;
for sheer simplicity since it's proof-of-concept, I just lowered the window to
the bottom of the stack instead of lowering it to its original position)

2) Perhaps I should only queue a mouse-action thingy if the window isn't raised;
the patch above doesn't do that.

Created attachment 32483 [details] [review]
New version of support for _NET_WM_MOUSE_ACTION with a small fix

I found a small bug.  This patch is almost the same as the previous one; the
only difference is that I replaced the second occurrence of
  else if (button_release_event_with_motion)
in the SELECTION section of process_mouse_action_message with
  else if (button_release_event_without_motion)

I believe I figured out my confusion regarding the usage of the window in the
TAKE_ACTIVITY spec.  The words "the respective client window" appeared in two
places in the spec and Lubos was only referring to changing one of them (namely
the window to which the XSendEvent message should be sent).

Created attachment 32529 [details] [review]
Updated support for _NET_WM_MOUSE_ACTION

The MOUSE_ACTION proposal has undergone some changes due to comments from
Lubos.	The newest proposal, at
  http://mail.gnome.org/archives/wm-spec-list/2004-October/msg00014.html
hasn't been commented on yet, but I'm guessing it won't be too different from
the final version.  This patch implements the newest proposal fully and
provides debugging information (mostly to aid in making patches to various
widgets and to make sure those patches are correct).

Oh, I forgot to mention one thing.  Bug 152952 has a new patch that helps with
debugging, especially with finding widgets that aren't calling
gdk_window_begin_mouse_action() and friends but need to.

Doh!  Sorry, I meant to paste that last comment in bug 154260...

Created attachment 32565 [details] [review]
Provide just a little more debugging information

This patch makes it easier to catch widgets that aren't yet supported or to
track down bugs in their support.  I'm using this while trying to generate the
necessary gtk+, vte, nautilus, etc. patches and debugging them.  I thought I'd
attach it here because gtk+ API slush freeze is today--I'm guessing due to the
lack of response so far (in both bugzilla and on wm-spec-list) that these
patches won't be able to go in so I'm just going to attach the latest of
everything I have before this all gets shelved for gtk+-2.8

Oh, one more thing I forgot.  Neither TAKE_ACTIVITY nor MOUSE_ACTION allow for
distinguishing between a drop with the target window equal to the source window
and a drop with the target and source windows being different.  That effectively
means that drag and drops to and from the same window result in that window not
being raised.  Is that what we want, or do we need a more thorough proposal?

I'm actually assuming this is fine, because we need a separate proposal that
allows for raising a window when it's a potential drop target.  We would just
need to make sure that it can raise on EnterNotify if the window is different
than the source window, and raise on ButtonRelease if the window equals the
source window.  Not that I know how to accomplish that or know if it's feasible...

Oh, I was wrong.  gtk+ API slush-freeze is still two days away...

Also, I'm going to mark this bug as depending on 154260.  Technically it doesn't
depend on it, but there probably isn't much point in adopting this patch if gtk+
doesn't support the feature.

Comment on attachment 32039 [details] [review]
Don't focus window upon ButtonPress; wait for take-activity pong message

This patch no longer applies, and further the _NET_WM_TAKE_ACTIVITY proposal
puts clients in charge of policy, which isn't what we want.

Comment on attachment 32565 [details] [review]
Provide just a little more debugging information

This patch needs updating, as it no longer applies to HEAD.

Created attachment 65057 [details] [review]
Patch updated to HEAD

See extra notes in bug 154260 comment 28 for changes that should probably affect this patch too

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

Pulling it off the patch-review list.

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

I'd like to help fix this; is there anything I can do?

Hi dsargeant, good to have your help!

Per Elijah's comments at

http://blogs.gnome.org/metacity/2008/06/11/drag-and-drop/

his patch in comment 25 should implement the "sane subset" which is as much as is needed of the spec.  It has probably rotted, however, so a first step would be to clean it up and apply it to trunk.  Once that's done I'll check it in and we'll see about getting the equivalent done to GTK.