This is something that's been bugging me for a long time too; I made
some token efforts to try to fix it that were unsuccessful.  The
semi-asynchronous way that metacity does things like workspace
switching makes this somewhat difficult to fix.

Regardless now the behavior of this has been changed in HEAD, so we'll
have to wait for that to get sorted out before this bug can be fixed.

the problem here appears to be that switching workspaces causes
EnterNotify events that show up after all of metacity's machinations
about focusing the right window, so really all of the code to manage
the focused window after a workspace switch is simply being overridden
by the EnterNotify events that are created as a result of the
workspace switch.  This bug shows up whenever a window is both moved
to another workspace and the workspace is switched right now, but will
also be a problem when we switch from focusing the topmost to using
the MRU order for workspace switching.

I have a patch that gets around this race condition by ignoring
EnterNotify events too soon after a switch, where "too soon" is
currently defined as the current timestamp + 500. 

This is a horrible ugly hack, and doesn't really solve the problem
completely, since the weird focus changes could still occur on a slow
connection, and it means that you can't move your mouse to focus
another window right after a workspace switch.  After experimenting, I
was able to switch the workspace and then move the mouse to another
window fast enough, but it took a lot of effort.

Perhaps you can see a better solution to the race, Havoc?

Created attachment 16585 [details] [review]
my patch

My initial reaction is: add this to the long-ass list of reasons mouse
focus is inherently broken. ;-) This is hardly the only place where we 
want to make a random exception to "window containing the pointer is
focused" and thus introduce a race or hosed corner case. And if you 
don't make those exceptions mouse focus is almost unusable.

Anyhow... assuming you want to use mouse focus. ;-)

bug #90382 seems like a nice first step.

For this bug... maybe you could generate a synthetic event 
(just change some random property or something) after mapping 
all the windows, and ignore all EnterNotify received before that
synthetic event. Perhaps the general #90382 framework should 
use this technique, even.

That's a good idea.  The attached patch implements a much more general
and robust solution:

1) creates a new property _METACITY_SENTINEL which right now takes a
timestamp value but the value is currently ignored
2) the idle_calc_showing function will set the variable
ignore_enter_notify in MetaDisplay to true and change the
_METACITY_SENTINEL property on one of the windows on that display for
each display that was a window mapped in one queue processing.
3) EnterNotify events are ignored which ignore_enter_notify is true
4) receiving a _METACITY_SENTINEL property notify will set
ignore_enter_notify to false for the display.

This seems to work really well and I haven't found any problems with
it.  Patch to be attached shortly.

Created attachment 16586 [details] [review]
_METACITY_SENTINEL patch

Does this patch look to be the correct solution?  I've been using
metacity with this patch for nearly two weeks now and haven't noticed
any problems with it.

ping.

I do think we should go with this approach, here are the tweaks I'd
make to the patch:

 - I'd like to see an accessor function like
   meta_display_mark_focus_now_permitted() used to adjust the
   display->ignore_enter_notify flag and to call XChangeProperty(),
   rather than doing it inline in idle_calc_showing().
 - perhaps a counter instead of a bool for the ignore_enter_notify 
   would be more robust, as you could call mark_focus_now_permitted() 
   more than once. But you'd have to be sure to avoid going 
   negative and getting hosed if someone else modified the property.
 - missing space in "need_sentinel = g_slist_prepend(need_sentinel,
window);" 
 - let's set the property on the root window instead of some random 
   client window; otherwise there's a race where the client window 
   could be destroyed, and it's kind of odd to have code that 
   randomly picks a client window, anyhow. alternatively, could 
   use the display->no_focus_window instead of root window.
 - multiline comments have the format:
   /* blah
    * blah
    */
 - not a big fan of having meta_workspace_activate() take 
   the extra focus_this argument; easiest solution 
   I can think of is to add
   meta_workspace_activate_with_focus_window() that's 
   used in the one place where the focus_this arg is 
   not NULL. Then meta_workspace_activate() just calls 
   meta_workspace_activate_with_focus_window() with NULL.

Could you attach a patch with those changes if you agree they make
sense? Or I can clean it up and check it in when I get a chance.

Thanks

Created attachment 16977 [details] [review]
revised sentinel patch

Attached is the revised patch.  This includes your suggestions as well
as some additional code cleanups.  Some of the code in there was
mostly for an older approach that I tried and rejected, and I realized
that it wasn't really needed any more.  The new code is a fair bit
simpler.  I also needed to move some code around since I'm using the
root window now and so meta_window_property_notify is never called.

Created attachment 16978 [details] [review]
real patch (ignore previous one)

diff -u is much more readable ;-)

what?  I didn't do -pu?  What the hell?  Um, OK.  I'll attach another one.

Created attachment 16992 [details] [review]
Will the real patch please stand up?

much better ;-) (you can put the diff options in your .cvsrc, 
that's what I do)

In increment_focus_sentinel(), you have to change the property 
and increment the counter always, not just when the counter is 
0, or the counter is equivalent to a flag.

focus_sentinel_clear() has braces in the wrong spot

should activate_with_focus() use meta_window_activate() instead of 
a focus and then a raise?

in event_callback(), we could put the PropertyNotify handling inside
the screen != NULL block (screen != NULL means "event happened on root
window")

Yea I realized the counter would end up being a flag that but then we
end up generating a bunch of spurious property changes for really no
benefit.  But now that I think about it I can avoid the extra events
in another way that will be more robust, so I've changed it to do it
that way.  I added a visited flag in MetaDisplay so that I can mark
the displays as I see each one.

As for _activate instead of focus and raise: I'd considered that, but
I don't think the semantics are quite right.  First, we should be able
to move windows between workspaces if they're shaded without unshading
them.  Also, I don't think it should drop out of show_desktop_mode,
and the code to ensure that the window is visible on the workspace is
redundant.  But I suppose it doesn't really matter one way or the
other.  We could change it if you want.

Created attachment 16994 [details] [review]
This is the official BugFree (TM) patch.  I don't want to hear otherwise

Agree with you on using meta_window_activate(), good point.

The visited flag is a global variable in order to optimize one 
function, maybe instead just keep a GSList of displays and do 
"if (g_slist_find (displays, window->display) == NULL)
   displays = g_slist_prepend (displays, window->display)"

The g_slist_find is theoretically expensive, but not 
actually expensive since 2 displays is extremely uncommon or even 
impossible already, and >2 displays is just insane.

Then just iterate over the list of displays instead of the list 
of windows.

with that change it looks good to commit, let's close the bug ;-)

Thanks

change made and committed.  Funny how something so simple ended being
so complicated.

Unfortunately, this patch causes some race conditions (although not as severe as
others that already existed, which is perhaps the reason they were
unnoticed--see bug 152000 for more info; contrary to what you might expect, this
patch did NOT cause bug 123803/bug 124798).  Because of the race conditions, I
have investigated this bug further (because I wanted to make sure my patch that
I will post to bug 152000 won't cause this bug to return).  I'm posting the
information here simply because this bug seems to keep coming up for me and I
figured this was the best place so that I could refer to it when I need it...

Note that despite what I say below, I believe the infrastructure this patch adds
is very useful and that we do want to keep it (see bug 151996).  I just don't
think this particular use is warranted.

Funnily enough, it turns out that this issue was _already_ fixed before the
patch in this bug report was committed by the following change:

2003-05-03  Havoc Pennington  <hp@pobox.com>

        * src/keybindings.c (handle_move_to_workspace): when moving
        window to another workspace, don't switch to that workspace.

        * src/window.c (menu_callback): when moving window to another
        workspace, don't switch to that workspace.

Further, there was an alternate and much simpler patch that would have fixed
this particular issue (which echoes similar code in keybindings.c--search for
meta_window_change_workspace):

Index: src/window.c
===================================================================
RCS file: /cvs/gnome/metacity/src/window.c,v
retrieving revision 1.274
diff -p -u -r1.274 window.c
--- src/window.c        26 Apr 2003 17:40:32 -0000      1.274
+++ src/window.c        27 Sep 2004 18:20:39 -0000
@@ -5435,9 +5435,9 @@ menu_callback (MetaWindowMenu *menu,

             if (workspace)
               {
-                meta_workspace_activate (workspace);
                 meta_window_change_workspace (window,
                                               workspace);
+                meta_workspace_activate (workspace);
                 meta_window_raise (window);
               }
             else

(Note that this patch is against the 2003-05-01 version of CVS; I did a lot of
testing from around CVS of that time so I could figure out what the various
issues were and make sure the patch I'm making for bug 152000 won't cause this
bug to return)  I further believe the meta_window_raise was unneeded, but I
didn't bother to investigate since it's no longer relevant--it's currently #if
0'ed from Havoc's commit referenced above.