This needs to be resolved, else it's going to seriously break some things. I'm
not sure whether it's going to be more useful to call a
  panel_applet_focus_me (applet)
or some sort of
  panel_applet_send_message (applet, _NET_ACTIVE_WINDOW)

(please note, exact namespace of libpanel-applet was not consulted when writing
this comment, or is it a particularly informed comment in itself).

I'll wait for Mark to come with some wise input as to how it should be done.

Elijah: the panel will probably need to send _NET_ACTIVE_WINDOW too so we can
navigate with the keyboard from a panel object to the next one. Or maybe I don't
understand anything :-)

I would think panel_applet_focus_me() is right (it should take a GtkWidget arg
for the widget inside the applet to be focused, since an applet could presumably
have two widgets... and it should take a timestamp ;-))

/me wonders where the comment went that he submitted about 14 hours ago...

A _NET_ACTIVE_WINDOW message can be sent by calling
gdk_window_focus(window_id,timestamp), so I think we probably don't need any
extra API.  The applet just needs to know the id of the containing toplevel
panel window (in order to pass to gdk_window_focus; note that the window manager
doesn't recognize the applet toplevel window which is why the GDK_WINDOW_XID of
the toplevel panel applet is needed).  The applet can get the timestamp from the
relevant button_press event or gtk_get_current_event_time() call.

Vincent: No, I don't think the panel will need to send a _NET_ACTIVE_WINDOW
message.  Metacity will focus the panel on a Ctrl-Alt-Tab, just not on a click.
 And, of course, the panel-menu is an override-redirect window that has keyboard
grabs so we don't need to do anything special for it.  I think that only
keyboard text-entry applets will need to be changed (i.e. gdict and mini-commander).

Can't you not do gdk_window_focus (gtk_widget_get_toplevel (applet)->window) ?

I think it would be nice to wrap that in:

  void
  panel_applet_activate (PanelApplet *applet,
                         guint32      activate_time)
  {
    GtkWidget *toplevel;

    g_return_if_fail (PANEL_IS_APPLET (applet));

    toplevel = gtk_widget_get_toplevel (applet);
    if (!toplevel || !GTK_WIDGET_REALIZED (toplevel))
      return;
      
    gdk_window_focus (toplevel->window, activate_time)
  }

I tried that, but the applet's toplevel window is not the same as the panel's
toplevel window, and metacity does not recognize the applet's toplevel window.

/me should dig out the patch and attach it here in case he messed something
simple up...

Created attachment 35453 [details] [review]
Metacity patch, for debugging/testing purposes

Created attachment 35454 [details] [review]
gdict patch which demonstrates the problem, for debugging/testing purposes

I couldn't find my old patches, so I generated some new ones.

The Metacity one assists in debugging, to see if the _NET_ACTIVE_WINDOW message
(sent by gdk_window_focus) is working properly or not.  Just recompile metacity
and run from a terminal ("./src/metacity --replace").

The gdict one demonstrates two things: (1) using the toplevel window of the
applet, which does not work, and (2) an ultra-hack which does work...given some
minor (*cough*) intervention--you have to find the XID of the appropriate panel
window yourself using xprop commands, stick that in the code to replace
0xe00024, recompile and then restart gdict.

If you're curious, here's the debugging output I get when running with the above
patches:

From the gdict applet:
gtk_widget_get_toplevel (widget)->window = 0xa22db10
GDK_WINDOW_XWINDOW (gtk_widget_get_toplevel (widget)->window) = 0xc00004

From the metacity process:
Window manager warning: _NET_ACTIVE_WINDOW request for unknown window 0xc00004
Window manager warning: Successful _NET_ACTIVE_WINDOW request for window
'0xe00024 (Bottom Pan)'

So, in summary, we need the panel to tell the applet what
  GDK_WINDOW_XWINDOW (gtk_widget_get_toplevel (panel_widget)->window)
is equal to, so that the user doesn't have to go searching for 0xe00024 and
recompile and all that stuff like I did.

Created attachment 35539 [details] [review]
Send the xid of the panel xwindow to the applet, provide a focus_request api for the applet

Created attachment 35540 [details] [review]
New, simpler gdict patch--call focus_request api on button press

Comment on attachment 35453 [details] [review]
Metacity patch, for debugging/testing purposes

Since this is meant for testing purposes only (i.e. can't be applied to
gnome-panel), I'm marking this as rejected.

Comment on attachment 35540 [details] [review]
New, simpler gdict patch--call focus_request api on button press

Since this is for testing purposes only (I'll submit the patch against
gnome-utils in bug 161737 as soon as Mark and others point out any required
code cleanups), I'm marking this patch as rejected too.

Elijah: I'll let Mark review the patch. I'm just wondering if it's still working
after you move your gdict applet to another panel.

It is not that hard to find the xid of the toplevel window containing a X
window, although it does require several roundtrips. You can look at the code of
gdk_window_get_deskrelative_origin() for inspiration.

Vincent: No, it doesn't work worth beans if you move the applet to another
panel--the panel the applet was originally on gets activated instead.  I'll take
a look.

Matthias: Okay, I'll take a look at this method too.  Would several roundtrips
everytime the user clicks on the applet be preferable to getting the information
once from the panel when it starts up (and again only if the applet is ever
moved to another panel or is restarted)?  Obviously, it'd be slower (though I
doubt anyone could notice), but I'm having a hard time judging whether "a little
slower" or "possibly a little simpler code" is more important...

Okay, I think I prefer the XQueryTree rather than using moniker items.

Firstly, its just gut instinct that using a much convoluted bonobo based method
to communicate something to the applet which the applet can determine on its own
from the Xserver is just ugly.

Secondly, I don't like the idea of race conditions involving applets being
re-parented and the property not being updated by the time the applet requests
focus. How would you guarantee ordering there?

Finally, in theory, applets could be embedded in other containers, or we could
be embedding other windows (e.g. notification area icons) which also need to
request focus and I'd like them all to use the same mechanism. For notification
area icons, we'd also need to proxy the toplevel id to the icons.

So, panel_applet_request_focus() should use XQueryTree to iterate over its
ancestors until it finds a _NET_WM_WINDOW_TYPE_DOCK window. You'd start from
gtk_widget_get_toplevel() as that should return you the dock window if its an
in-process applet.

What I don't like about all this is you're hardcoding into applets some
knowledge about window manager policy. Now, I guess it does no harm if applets
do this and the WM focuses the panel on click anyway but ... I dunno, it feels
like we're putting a hack into applets "because Metacity and KWin" need it
rather than it being specified behaviour.

Created attachment 35589 [details] [review]
Use XQueryTree and XGetWindowProperty to find containing panel's XID

Okay, here's a patch using the new method (works great even when the applet is
moved to a different panel).  It keeps the same API as before, so the gdict
applet patch remains the same.

To address your overall concerns with the method, though, what kind of general
method would you like?	Having the WM send the DOCK window some specified
message and require the DOCK window to respond?  Have another message type that
is basically identical to _NET_ACTIVE_WINDOW but with a different name?  Or
something more simple like have the EWMH explicitly specify that apps who set a
DOCK hint on their windows are required to send _NET_ACTIVE_WINDOW messages if
they want input focus on click?  When I proposed a solution somewhat along the
lines of the first of those possibilites (which actually would have been _far_
harder to implement), Lubos mentioned that he already did things the way this
proposed patch implements, but added "I'm right now not quite sure how clean
solution this is, but it's been there since ages, and Kicker is much nicer to
use than other panels."  Anyway, we went with his method, but I'm sure we could
suggest doing it another way  ........ of course, perhaps this should be
discussed on wm-spec-list instead.  :-)

To put my general concern more simply - what happens if we have a policy changes
again to not focus *other* types of windows on click? Do we then add this hack
to all apps with windows of that type?

(Discussion about this would be better on wm-spec-list, I think. I don't mind
going ahead with the current course, but I would like to see a discussion around
whether there are alternatives)

Patch looks about right to me. I've some nitpicks, but they're not hugely
important - I can fix after you've committed. Lets get this stuff in so we can
start fixing the applets.

okay, committed.