Actually, there were some other focus oddities going on.  I played with it a bit more, and I can describe the problem better.  It's not just that unshading doesn't give keyboard focus to the window.  Rather, unshading puts the window into some sort of quasi-sloppy-focus mode.  This persists even after clicking in the newly-unshaded window, and doesn't stop until another window is explicitly focused and the previously unshaded window is focused again.

Basically, unshading puts the window into a mode where it only has keyboard focus when the mouse is over the window.  Putting the mouse anywhere else removes focus, though it doesn't send focus anywhere else.  My original impression that it just didn't get focus was because I unshade by double-clicking the titlebar, and the titlebar is not in the window.

One more data point: out of curiosity, I added a keybinding to toggle the shaded state.  Oddly enough, this behavior does not happen when using that keybinding.

the quasi-sloppy is probably caused by RevertToPointerRoot (hint for anyone debugging this)

So I'm able to fix this by inserting the following at line 1244 of frames.c, inside the part of meta_frames_button_press_event that handles shade toggling:

meta_core_end_grab_op (gdk_display, event->time);

Based on my observation from comment #2, I just tried to see what was different between double-clicking and using a keybinding, and it pretty much came down to *something* in that function.  I thought the META_GRAB_OP_MOVING grab op might be interfering, so I tried releasing it, and it works.

Note that I don't know all the inracacies of window management, and I have no idea if this is the right fix.  Somebody who knows what he's doing should run with this.

Created attachment 84741 [details] [review]
possible fix

Attaching a patch per my previous comment.  It's moved because of the patches from Linus that were applied.

I've spent a long time on this and still can't figure it out.  I have one main question stumping me:

  How is it possible that the focus window is becoming invalid during
  an *unshade* operation?  I assume that is what is happening, since
  we're essentially seeing RevertToPointerRoot behavior.  But the
  revert_to argument of XSetInputFocus shouldn't be used unless the
  focus window becomes unviewable; I can see why that would happen for
  a shade operation but not for an unshade operation.

So, here's some notes I took while trying to figure things out:

1) We have meta_window_focus() calls all over the place.  A double-click on the titlebar results in *six* calls to meta_window_focus().  (The user clicked on the titlebar, so we focus the window.  We're about to grab the pointer/keyboard to do a move/resize/whatever operation, so we focus the window.  We're going to unshade the window, so we focus the window. etc.)

2) gtk+/gdk/x11/gdkwindow-x11.c contains an interesting tidbit; apparently they have a focus_window separate from the rest of their app for focus:
  /* We use an extra X window for toplevel windows that we XSetInputFocus()
   * to in order to avoid getting keyboard events redirected to subwindows
   * that might not even be part of this app
   */
  Window focus_window;
Is something funny happening to us as well when we focus the frame window?

3) How Shaun's patch works around the issue: If any window grab is in effect, meta_window_focus() bails early without bothering to try to focus any window (the assumption seems to be that a different window is grabbed, but no special casing exists for when the grabbed window is the window that was requested to be focused).  With Shaun's patch, the last of the 6 meta_window_focus() calls comes after the grab has ended so the call isn't ignored and the client window is focused.

4) Why this never showed up earlier:  It wouldn't surprise me if we used to have 10 calls to meta_window_focus().  We probably had one or two triggered directly from meta_display_end_grab_op().

Ping?  I only remember this bug when I install/upgrade a system, and then have to build metacity by hand with my patch.  I imagine there's a few people out there who are just living (annoyed) with this bug.

I understand that there's probably some insidious goings-on that manifest themselves in this bug, and that my patch is a cheap bandage.  But I have been running with my patch for about seven months now.  It does fix the symptom, and seemingly with no other ill effects.

Um, go ahead and apply the patch for now but please add a FIXME comment above it and point to this bug (and in particular, comment #6).

Thanks Shaun.

Was this patch ever applied?

Nope, sorry.  I always completely forget about it until I upgrade.

I don't see this in 2.25.0 (using right-click bound to toggle_shade).  Shaun, do you still see it?

I haven't tried 2.25 yet.  I can confirm it's still a problem in Fedora 9, but that's only metacity 2.22, I think.

Note that I'm not sure the problem would be triggered by using right-click to unshade.  I know I didn't have any problems using a keybinding to unshade.  The problem was that the left click induced a grab operation, which caused some sort of odd conflict.  I never quite figured out the conflict, but I was able to get around it by ending the grab operation.  I think it went something like this:

click ->
  start grab op
  double-click ->
    double-click handler ->
      unshade
      focus
      end grab op

And the "focus" step failed because there was a grab operation.  My quick fix was to insert a step as follows:

click ->
  start grab op
  double-click ->
    end grab op ** NEW **
    double-click handler ->
      unshade
      focus
      end grab op

If right clicks don't induce a grab operation, they wouldn't trigger this problem.

That said, I took a glance at the code to see if my reasoning would stand up, and it's all changed.  I'm not even sure how to apply this patch anymore.

Created attachment 118407 [details] [review]
possible fix

Well, here's a version of the same thing which applies now.  Let us know whether this gives you ideas.

I still get the bug with SVN trunk (rev 3938).  Your patch fixes it.

I want to point out that I've found a consistent crasher in the build of metacity I'm running.  I need to check whether it's caused by this patch.  If so, this patch shouldn't be applied as is.  Otherwise, I'll file a separate bug.

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

OK, I don't know what was up with that crasher I was experiencing before, but I can't reproduce it with SVN trunk either with or without the patch.

This patch fixes the problem for me.  Thanks for updating it.

Committed, then.  Thanks.