remove XSync, error traps already do that

2001-08-19  Havoc Pennington  <hp@pobox.com>

	* src/display.c (meta_display_grab_window_buttons): remove XSync,
	error traps already do that
	(meta_display_grab_window_buttons): implement

	* src/keybindings.c:
	src/display.c: wire up the tab window, it rulez!

13 files changed, 548 insertions, 159 deletions

diff --git a/ChangeLog b/ChangeLog
index 2d6f609b..b595e09d 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,14 @@
 2001-08-19  Havoc Pennington  <hp@pobox.com>
 
+	* src/display.c (meta_display_grab_window_buttons): remove XSync,
+	error traps already do that
+	(meta_display_grab_window_buttons): implement
+
+	* src/keybindings.c: 
+	src/display.c: wire up the tab window, it rulez!
+
+2001-08-19  Havoc Pennington  <hp@pobox.com>
+
 	* src/tabpopup.c: add prototype thingy to display windows we're
 	cycling through with tab. Not wired up to keybindings yet.
 
diff --git a/README b/README
index 2de5ebf6..26dd5252 100644
--- a/README
+++ b/README
@@ -194,9 +194,12 @@ METACITY BUGS, NON-FEATURES, AND CAVEATS
 
  - Should support click-to-focus as an option.
 
- - Windows has a neat way of implementing Alt-Tab for 
-   window cycling that I would like to copy. (The little 
-   popup window thing.)
+ - There is some sort of race condition with Alt-Tab that can
+   sometimes result in the focus not moving.
+
+ - The focus rectangle around the icons in the Alt-Tab popup
+   is ugly, should use a GTK-like focus rectangle, or maybe
+   make it look selected, not sure.
 
  - Should Metacity support flipping in right-to-left locales?
    I don't know what window managers look like in a right-to-left
diff --git a/src/common.h b/src/common.h
index 87f830c2..3f903e24 100644
--- a/src/common.h
+++ b/src/common.h
@@ -67,9 +67,13 @@ typedef void (* MetaWindowMenuFunc) (MetaWindowMenu *menu,
                                      int             workspace,
                                      gpointer        data);
 
+/* when changing this enum, there are various switch statements
+ * you have to update
+ */
 typedef enum
 {
   META_GRAB_OP_NONE,
+
   /* Mouse ops */
   META_GRAB_OP_MOVING,
   META_GRAB_OP_RESIZING_SE,
@@ -80,6 +84,7 @@ typedef enum
   META_GRAB_OP_RESIZING_NW,
   META_GRAB_OP_RESIZING_W,
   META_GRAB_OP_RESIZING_E,
+
   /* Keyboard ops */
   META_GRAB_OP_KEYBOARD_MOVING,
   META_GRAB_OP_KEYBOARD_RESIZING_UNKNOWN,
@@ -87,6 +92,13 @@ typedef enum
   META_GRAB_OP_KEYBOARD_RESIZING_N,
   META_GRAB_OP_KEYBOARD_RESIZING_W,
   META_GRAB_OP_KEYBOARD_RESIZING_E,
+  META_GRAB_OP_KEYBOARD_RESIZING_SE,
+  META_GRAB_OP_KEYBOARD_RESIZING_NE,
+  META_GRAB_OP_KEYBOARD_RESIZING_SW,
+  META_GRAB_OP_KEYBOARD_RESIZING_NW,
+
+  META_GRAB_OP_KEYBOARD_TABBING,
+  
   /* Frame button ops */
   META_GRAB_OP_CLICKING_MINIMIZE,
   META_GRAB_OP_CLICKING_MAXIMIZE,
diff --git a/src/display.c b/src/display.c
index ee203106..bc4b819a 100644
--- a/src/display.c
+++ b/src/display.c
@@ -592,6 +592,11 @@ grab_op_is_keyboard (MetaGrabOp op)
     case META_GRAB_OP_KEYBOARD_RESIZING_N:
     case META_GRAB_OP_KEYBOARD_RESIZING_W:
     case META_GRAB_OP_KEYBOARD_RESIZING_E:
+    case META_GRAB_OP_KEYBOARD_RESIZING_SE:
+    case META_GRAB_OP_KEYBOARD_RESIZING_NE:
+    case META_GRAB_OP_KEYBOARD_RESIZING_SW:
+    case META_GRAB_OP_KEYBOARD_RESIZING_NW:
+    case META_GRAB_OP_KEYBOARD_TABBING:
       return TRUE;
       break;
 
@@ -664,7 +669,7 @@ event_callback (XEvent   *event,
                         display->grab_window->desc);
           meta_display_end_grab_op (display,
                                     event->xbutton.time);
-        }      
+        }
       else if (window && display->grab_op == META_GRAB_OP_NONE)
         {
           gboolean begin_move = FALSE;
@@ -1240,6 +1245,7 @@ meta_spew_event (MetaDisplay *display,
           name = "MappingNotify";
           break;
         default:
+          meta_verbose ("Unknown event type %d\n", event->xany.type);
           name = "Unknown event type";
           break;
         }
@@ -1453,9 +1459,9 @@ meta_display_begin_grab_op (MetaDisplay *display,
   if (pointer_already_grabbed)
     display->grab_have_pointer = TRUE;
 
-      /* We XGrabPointer even if we already have an autograb,
-       * just to set the cursor and event mask
-       */
+  /* We XGrabPointer even if we already have an autograb,
+   * just to set the cursor and event mask
+   */
       
   cursor = xcursor_for_op (display, op);
       
@@ -1480,30 +1486,19 @@ meta_display_begin_grab_op (MetaDisplay *display,
       meta_verbose ("XGrabPointer() failed\n");
       return FALSE;
     }
-  
-  switch (op)
+
+  if (grab_op_is_keyboard (op))
     {
-    case META_GRAB_OP_KEYBOARD_MOVING:
-    case META_GRAB_OP_KEYBOARD_RESIZING_UNKNOWN:
-    case META_GRAB_OP_KEYBOARD_RESIZING_S:
-    case META_GRAB_OP_KEYBOARD_RESIZING_N:
-    case META_GRAB_OP_KEYBOARD_RESIZING_W:
-    case META_GRAB_OP_KEYBOARD_RESIZING_E:
       if (meta_window_grab_all_keys (window))
         display->grab_have_keyboard = TRUE;
-
+      
       if (!display->grab_have_keyboard)
         {
-          meta_verbose ("XGrabKeyboard() failed\n");
+          meta_verbose ("grabbing all keys failed\n");
           return FALSE;
         }
-      break;
-
-    default:
-      /* non-keyboard grab ops */
-      break;
     }
-                
+  
   display->grab_op = op;
   display->grab_window = window;
   display->grab_button = button;
@@ -1519,6 +1514,10 @@ meta_display_begin_grab_op (MetaDisplay *display,
 
   g_assert (display->grab_window != NULL);
   g_assert (display->grab_op != META_GRAB_OP_NONE);
+
+  /* Do this last, after everything is set up. */
+  if (op == META_GRAB_OP_KEYBOARD_TABBING)
+    meta_screen_ensure_tab_popup (window->screen);
   
   return TRUE;
 }
@@ -1529,6 +1528,12 @@ meta_display_end_grab_op (MetaDisplay *display,
 {
   if (display->grab_op == META_GRAB_OP_NONE)
     return;
+
+  if (display->grab_op == META_GRAB_OP_KEYBOARD_TABBING)
+    {
+      meta_ui_tab_popup_free (display->grab_window->screen->tab_popup);
+      display->grab_window->screen->tab_popup = NULL;
+    }
   
   if (display->grab_have_pointer)
     XUngrabPointer (display->xdisplay, timestamp);    
@@ -1548,6 +1553,11 @@ meta_display_grab_window_buttons (MetaDisplay *display,
    * and Alt + button3 for popping up window menu.
    */
   meta_verbose ("Grabbing window buttons for 0x%lx\n", xwindow);
+
+  /* FIXME If we ignored errors here instead of spewing, we could
+   * put one big error trap around the loop and avoid a bunch of
+   * XSync()
+   */
   
   {
     int i = 1;
@@ -1562,11 +1572,10 @@ meta_display_grab_window_buttons (MetaDisplay *display,
                      PointerMotionMask | PointerMotionHintMask,
                      GrabModeAsync, GrabModeAsync,
                      False, None);
-        XSync (display->xdisplay, False);
         result = meta_error_trap_pop (display);
 
         if (result != Success)
-          meta_warning ("Failed to grab button %d with Mod1Mask for frame 0x%lx error code %d\n",
+          meta_warning ("Failed to grab button %d with Mod1Mask for window 0x%lx error code %d\n",
                         i, xwindow, result);
         
 
@@ -1582,11 +1591,10 @@ meta_display_grab_window_buttons (MetaDisplay *display,
                                   PointerMotionMask | PointerMotionHintMask,
                                   GrabModeAsync, GrabModeAsync,
                                   False, None);
-            XSync (display->xdisplay, False);
             result = meta_error_trap_pop (display);
             
             if (result != Success)
-              meta_warning ("Failed to grab button %d with ControlMask for frame 0x%lx error code %d\n",
+              meta_warning ("Failed to grab button %d with ControlMask for window 0x%lx error code %d\n",
                             i, xwindow, result);
           }
         
@@ -1599,7 +1607,32 @@ void
 meta_display_ungrab_window_buttons  (MetaDisplay *display,
                                      Window       xwindow)
 {
+  /* FIXME If we ignored errors here instead of spewing, we could
+   * put one big error trap around the loop and avoid a bunch of
+   * XSync()
+   */
+  int i = 1;
+  while (i < 4)
+    {
+      int result;
+      
+      meta_error_trap_push (display);
+      XUngrabButton (display->xdisplay, i, Mod1Mask, xwindow);
+      result = meta_error_trap_pop (display);
 
+      if (result != Success)
+        meta_warning ("Failed to ungrab button %d with Mod1Mask for window 0x%lx error code %d\n",
+                      i, xwindow, result);
 
+      meta_error_trap_push (display);
+      XUngrabButton (display->xdisplay, i, ControlMask, xwindow);
+      result = meta_error_trap_pop (display);
+      
+      if (result != Success)
+        meta_warning ("Failed to ungrab button %d with ControlMask for window 0x%lx error code %d\n",
+                      i, xwindow, result);
+      
+      ++i;
+    }
 }
 
diff --git a/src/keybindings.c b/src/keybindings.c
index 8c0f9d57..38fa3bec 100644
--- a/src/keybindings.c
+++ b/src/keybindings.c
@@ -62,9 +62,19 @@ static void handle_workspace_left     (MetaDisplay *display,
                                        XEvent      *event,
                                        gpointer     data);
 static void handle_workspace_right     (MetaDisplay *display,
-                                       MetaWindow  *window,
-                                       XEvent      *event,
-                                       gpointer     data);
+                                        MetaWindow  *window,
+                                        XEvent      *event,
+                                        gpointer     data);
+
+static gboolean process_keyboard_move_grab (MetaDisplay *display,
+                                            MetaWindow  *window,
+                                            XEvent      *event,
+                                            KeySym       keysym);
+
+static gboolean process_tab_grab           (MetaDisplay *display,
+                                            MetaWindow  *window,
+                                            XEvent      *event,
+                                            KeySym       keysym);
 
 typedef struct _MetaKeyBinding MetaKeyBinding;
 
@@ -281,10 +291,30 @@ meta_window_grab_all_keys (MetaWindow  *window)
     }
   else
     {
-      window->keys_grabbed = FALSE;
-      window->all_keys_grabbed = TRUE;
-      window->grab_on_frame = window->frame != NULL;
-      return TRUE;
+      /* Also grab the keyboard, so we get key releases and all key
+       * presses
+       */
+       meta_error_trap_push (window->display);
+       /* FIXME CurrentTime bogus */
+       XGrabKeyboard (window->display->xdisplay,
+                      grabwindow, True,
+                      GrabModeAsync, GrabModeAsync,
+                      CurrentTime);
+       
+       result = meta_error_trap_pop (window->display);
+       if (result != Success)
+         {
+           meta_verbose ("XGrabKeyboard() failed for window %s\n",
+                         window->desc);
+           return FALSE;
+         }
+       
+       meta_verbose ("Grabbed all keys on window %s\n", window->desc);
+       
+       window->keys_grabbed = FALSE;
+       window->all_keys_grabbed = TRUE;
+       window->grab_on_frame = window->frame != NULL;
+       return TRUE;
     }
 }
 
@@ -302,6 +332,8 @@ meta_window_ungrab_all_keys (MetaWindow  *window)
       XUngrabKey (window->display->xdisplay,
                   AnyKey, AnyModifier,
                   grabwindow);
+      /* FIXME CurrentTime bogus */
+      XUngrabKeyboard (window->display->xdisplay, CurrentTime);
       meta_error_trap_pop (window->display);
       
       window->grab_on_frame = FALSE;
@@ -317,29 +349,61 @@ static gboolean
 is_modifier (MetaDisplay *display,
              unsigned int keycode)
 {
-	int i;
-	int map_size;
-	XModifierKeymap *mod_keymap;
-	gboolean retval = FALSE;
-
-        /* FIXME this is ass-slow, cache the modmap */
-        
-	mod_keymap = XGetModifierMapping (display->xdisplay);
-
-	map_size = 8 * mod_keymap->max_keypermod;
-	i = 0;
-	while (i < map_size) {
-		
-		if (keycode == mod_keymap->modifiermap[i]) {
-			retval = TRUE;
-			break;
-		}
-		++i;
-	}
-
-	XFreeModifiermap (mod_keymap);
-
-	return retval;
+  int i;
+  int map_size;
+  XModifierKeymap *mod_keymap;
+  gboolean retval = FALSE;
+  
+  /* FIXME this is ass-slow, cache the modmap */
+  
+  mod_keymap = XGetModifierMapping (display->xdisplay);
+  
+  map_size = 8 * mod_keymap->max_keypermod;
+  i = 0;
+  while (i < map_size)
+    {
+      if (keycode == mod_keymap->modifiermap[i])
+        {
+          retval = TRUE;
+          break;
+        }
+      ++i;
+    }
+  
+  XFreeModifiermap (mod_keymap);
+  
+  return retval;
+}
+
+#define MOD1_INDEX 3 /* shift, lock, control, mod1 */
+static gboolean 
+is_mod1_key (MetaDisplay *display,
+             unsigned int keycode)
+{
+  int i;
+  int end;
+  XModifierKeymap *mod_keymap;
+  gboolean retval = FALSE;
+  
+  /* FIXME this is ass-slow, cache the modmap */
+  
+  mod_keymap = XGetModifierMapping (display->xdisplay);
+  
+  end = (MOD1_INDEX + 1) * mod_keymap->max_keypermod;
+  i = MOD1_INDEX * mod_keymap->max_keypermod;
+  while (i < end)
+    {
+      if (keycode == mod_keymap->modifiermap[i])
+        {
+          retval = TRUE;
+          break;
+        }
+      ++i;
+    }
+  
+  XFreeModifiermap (mod_keymap);
+  
+  return retval;
 }
 
 static void
@@ -384,57 +448,89 @@ meta_display_process_key_event (MetaDisplay *display,
                 XKeysymToString (keysym), event->xkey.state,
                 window ? window->desc : "(no window)");
 
-  if (window == NULL || !window->all_keys_grabbed)
+  if (display->grab_op == META_GRAB_OP_NONE &&
+      (window == NULL || !window->all_keys_grabbed))
     {
       /* Do the normal keybindings */
       process_event (screen_bindings, display, NULL, event, keysym);
 
       if (window)
         process_event (window_bindings, display, window, event, keysym);
+
       return;
     }
+  
+  if (display->grab_op == META_GRAB_OP_NONE)
+    return;    
 
   /* If we get here we have a global grab, because
    * we're in some special keyboard mode such as window move
    * mode.
    */
   
-  if (display->grab_op == META_GRAB_OP_NONE)
-    return;
-    
-  /* don't end grabs on modifier key presses */
-  if (is_modifier (display, event->xkey.keycode))
-    return;
-  
   handled = FALSE;
 
-  if (display->grab_op == META_GRAB_OP_KEYBOARD_MOVING &&
-      display->grab_window == window)
+  if (window == display->grab_window)
     {
-      int x, y;
-      int incr;
-      gboolean smart_snap;
-      int edge;
-      
-      if (event->type == KeyRelease)
-        return; /* don't care about releases */
-      
-      if (window == NULL)
-        meta_bug ("NULL window while doing keyboard grab op\n");
+      switch (display->grab_op)
+        {
+        case META_GRAB_OP_KEYBOARD_MOVING:
+          meta_verbose ("Processing event for keyboard move\n");
+          handled = process_keyboard_move_grab (display, window, event, keysym);
+          break;
+        case META_GRAB_OP_KEYBOARD_TABBING:
+          meta_verbose ("Processing event for keyboard tabbing\n");
+          handled = process_tab_grab (display, window, event, keysym);
+          break;
+        default:
+          break;
+        }
+    }
+  
+  /* end grab if a key that isn't used gets pressed */
+  if (!handled)
+    {
+      meta_verbose ("Ending grab op %d on key event sym %s\n",
+                    display->grab_op, XKeysymToString (keysym));
+      meta_display_end_grab_op (display, event->xkey.time);
+    }
+}
+
+static gboolean
+process_keyboard_move_grab (MetaDisplay *display,
+                            MetaWindow  *window,
+                            XEvent      *event,
+                            KeySym       keysym)
+{
+  gboolean handled;
+  int x, y;
+  int incr;
+  gboolean smart_snap;
+  int edge;
+  
+  handled = FALSE;
 
-      meta_window_get_position (window, &x, &y);
+  /* don't care about releases, but eat them, don't end grab */
+  if (event->type == KeyRelease)
+    return TRUE;
 
-      smart_snap = (event->xkey.state & ShiftMask) != 0;
+  /* don't end grab on modifier key presses */
+  if (is_modifier (display, event->xkey.keycode))
+    return TRUE;
+
+  meta_window_get_position (window, &x, &y);
+
+  smart_snap = (event->xkey.state & ShiftMask) != 0;
 
 #define SMALL_INCREMENT 1
 #define NORMAL_INCREMENT 10
 
-      if (smart_snap)
-        incr = 0;
-      else if (event->xkey.state & ControlMask)
-        incr = SMALL_INCREMENT;
-      else
-        incr = NORMAL_INCREMENT;
+  if (smart_snap)
+    incr = 0;
+  else if (event->xkey.state & ControlMask)
+    incr = SMALL_INCREMENT;
+  else
+    incr = NORMAL_INCREMENT;
 
       /* When moving by increments, we still snap to edges if the move
        * to the edge is smaller than the increment. This is because
@@ -442,71 +538,138 @@ meta_display_process_key_event (MetaDisplay *display,
        * people using just arrows shouldn't get too frustrated.
        */
       
-      switch (keysym)
-        {
-        case XK_Up:
-        case XK_KP_Up:
-          edge = meta_window_find_next_horizontal_edge (window, FALSE);
-          y -= incr;
+  switch (keysym)
+    {
+    case XK_Up:
+    case XK_KP_Up:
+      edge = meta_window_find_next_horizontal_edge (window, FALSE);
+      y -= incr;
           
-          if (smart_snap || ((edge > y) && ABS (edge - y) < incr))
-            y = edge;
+      if (smart_snap || ((edge > y) && ABS (edge - y) < incr))
+        y = edge;
           
-          handled = TRUE;
-          break;
-        case XK_Down:
-        case XK_KP_Down:
-          edge = meta_window_find_next_horizontal_edge (window, TRUE);
-          y += incr;
-
-          if (smart_snap || ((edge < y) && ABS (edge - y) < incr))
-            y = edge;
+      handled = TRUE;
+      break;
+    case XK_Down:
+    case XK_KP_Down:
+      edge = meta_window_find_next_horizontal_edge (window, TRUE);
+      y += incr;
+
+      if (smart_snap || ((edge < y) && ABS (edge - y) < incr))
+        y = edge;
           
-          handled = TRUE;
-          break;
-        case XK_Left:
-        case XK_KP_Left:
-          edge = meta_window_find_next_vertical_edge (window, FALSE);
-          x -= incr;
+      handled = TRUE;
+      break;
+    case XK_Left:
+    case XK_KP_Left:
+      edge = meta_window_find_next_vertical_edge (window, FALSE);
+      x -= incr;
           
-          if (smart_snap || ((edge > x) && ABS (edge - x) < incr))
-            x = edge;
+      if (smart_snap || ((edge > x) && ABS (edge - x) < incr))
+        x = edge;
+
+      handled = TRUE;
+      break;
+    case XK_Right:
+    case XK_KP_Right:
+      edge = meta_window_find_next_vertical_edge (window, TRUE);
+      x += incr;
+      if (smart_snap || ((edge < x) && ABS (edge - x) < incr))
+        x = edge;
+      handled = TRUE;
+      break;
+
+    case XK_Escape:
+      /* End move and restore to original position */
+      meta_window_move_resize (display->grab_window,
+                               display->grab_initial_window_pos.x,
+                               display->grab_initial_window_pos.y,
+                               display->grab_initial_window_pos.width,
+                               display->grab_initial_window_pos.height);
+      break;
+          
+    default:
+      break;
+    }
 
-          handled = TRUE;
-          break;
-        case XK_Right:
-        case XK_KP_Right:
-          edge = meta_window_find_next_vertical_edge (window, TRUE);
-          x += incr;
-          if (smart_snap || ((edge < x) && ABS (edge - x) < incr))
-            x = edge;
-          handled = TRUE;
-          break;
+  if (handled)
+    meta_window_move (window, x, y);
 
-        case XK_Escape:
-          /* End move and restore to original position */
-          meta_window_move_resize (display->grab_window,
-                                   display->grab_initial_window_pos.x,
-                                   display->grab_initial_window_pos.y,
-                                   display->grab_initial_window_pos.width,
-                                   display->grab_initial_window_pos.height);
-          break;
-          
-        default:
-          break;
-        }
+  return handled;
+}
+
+static gboolean
+process_tab_grab (MetaDisplay *display,
+                  MetaWindow  *window,
+                  XEvent      *event,
+                  KeySym       keysym)
+{
+  MetaScreen *screen;
 
-      if (handled)
-        meta_window_move (window, x, y);
-    }  
+  window = NULL; /* be sure we don't use this, it's irrelevant */
+
+  screen = display->grab_window->screen;
+
+  g_return_val_if_fail (screen->tab_popup != NULL, FALSE);
   
-  /* end grab if a key that isn't used gets pressed */
-  if (!handled)
+  if (event->type == KeyRelease &&
+      is_mod1_key (display, event->xkey.keycode))
     {
-      meta_verbose ("Ending grab op %d on key press event sym %s\n",
-                    display->grab_op, XKeysymToString (keysym));
-      meta_display_end_grab_op (display, event->xkey.time);
+      /* We're done, move to the new window. */
+      Window target_xwindow;
+      MetaWindow *target_window;
+
+      target_xwindow =
+        meta_ui_tab_popup_get_selected (screen->tab_popup);
+      target_window =
+        meta_display_lookup_x_window (display, target_xwindow);
+
+      meta_verbose ("Ending tab operation, Mod1 released\n");
+      
+      if (target_window)
+        {
+          meta_verbose ("Ending grab early so we can focus the target window\n");
+          meta_display_end_grab_op (display, event->xkey.time);
+
+          meta_verbose ("Focusing target window\n");
+          meta_window_raise (target_window);
+          meta_window_focus (target_window, event->xkey.time);
+
+          return TRUE; /* we already ended the grab */
+        }
+      
+      return FALSE; /* end grab */
+    }
+  
+  /* don't care about other releases, but eat them, don't end grab */
+  if (event->type == KeyRelease)
+    return TRUE;
+
+  /* don't end grab on modifier key presses */
+  if (is_modifier (display, event->xkey.keycode))
+    return TRUE;
+
+  switch (keysym)
+    {
+    case XK_ISO_Left_Tab:
+    case XK_Tab:
+      if (event->xkey.state & ShiftMask)
+        meta_ui_tab_popup_backward (screen->tab_popup);
+      else
+        meta_ui_tab_popup_forward (screen->tab_popup);
+
+      /* continue grab */
+      meta_verbose ("Tab key pressed, moving tab focus in popup\n");
+      return TRUE;
+      break;
+
+    default:
+      break;
     }
+
+  /* end grab */
+  meta_verbose ("Ending tabbing, uninteresting key pressed\n");
+  return FALSE;
 }
 
 static void
@@ -694,8 +857,22 @@ handle_tab_forward (MetaDisplay *display,
 
   if (window)
     {
-      meta_window_raise (window);
-      meta_window_focus (window, event->xkey.time);
+      meta_verbose ("Starting tab forward, showing popup\n");
+
+      meta_display_begin_grab_op (window->display,
+                                  display->focus_window ?
+                                  display->focus_window : window,
+                                  META_GRAB_OP_KEYBOARD_TABBING,
+                                  FALSE,
+                                  0, 0,
+                                  event->xkey.time,
+                                  0, 0);
+
+      meta_ui_tab_popup_select (window->screen->tab_popup,
+                                window->xwindow);
+      /* only after selecting proper window */
+      meta_ui_tab_popup_set_showing (window->screen->tab_popup,
+                                     TRUE);
     }
 }
 
@@ -738,8 +915,22 @@ handle_tab_backward (MetaDisplay *display,
 
   if (window)
     {
-      meta_window_raise (window);
-      meta_window_focus (window, event->xkey.time);
+      meta_verbose ("Starting tab backward, showing popup\n");
+      
+      meta_display_begin_grab_op (window->display,
+                                  display->focus_window ?
+                                  display->focus_window : window,
+                                  META_GRAB_OP_KEYBOARD_TABBING,
+                                  FALSE,
+                                  0, 0,
+                                  event->xkey.time,
+                                  0, 0);
+
+      meta_ui_tab_popup_select (window->screen->tab_popup,
+                                window->xwindow);
+      /* only after selecting proper window */
+      meta_ui_tab_popup_set_showing (window->screen->tab_popup,
+                                     TRUE);
     }
 }
 
diff --git a/src/screen.c b/src/screen.c
index e40304b1..ffad0e06 100644
--- a/src/screen.c
+++ b/src/screen.c
@@ -198,6 +198,8 @@ meta_screen_new (MetaDisplay *display,
   screen->ui = meta_ui_new (screen->display->xdisplay,
                             screen->xscreen);
 
+  screen->tab_popup = NULL;
+  
   screen->stack = meta_stack_new (screen);
   
   meta_verbose ("Added screen %d ('%s') root 0x%lx\n",
@@ -210,7 +212,7 @@ void
 meta_screen_free (MetaScreen *screen)
 {  
   meta_screen_ungrab_keys (screen);
-  
+
   meta_ui_free (screen->ui);
 
   meta_stack_free (screen->stack);
@@ -423,3 +425,45 @@ meta_screen_set_cursor (MetaScreen *screen,
   XDefineCursor (screen->display->xdisplay, screen->xroot, xcursor);
   XFreeCursor (screen->display->xdisplay, xcursor);
 }
+
+void
+meta_screen_ensure_tab_popup (MetaScreen *screen)
+{
+  MetaTabEntry *entries;
+  GSList *tab_list;
+  GSList *tmp;
+  int len;
+  int i;
+  
+  if (screen->tab_popup)
+    return;
+
+  tab_list = meta_stack_get_tab_list (screen->stack);
+  len = g_slist_length (tab_list);
+
+  entries = g_new (MetaTabEntry, len + 1);
+  entries[len].xwindow = None;
+  entries[len].title = NULL;
+  entries[len].icon = NULL;
+  
+  i = 0;
+  tmp = tab_list;
+  while (i < len)
+    {
+      MetaWindow *window;
+
+      window = tmp->data;
+      
+      entries[i].xwindow = window->xwindow;
+      entries[i].title = window->title;
+      entries[i].icon = window->icon;
+      
+      ++i;
+      tmp = tmp->next;
+    }
+  
+  screen->tab_popup = meta_ui_tab_popup_new (entries);
+  g_free (entries);
+
+  /* don't show tab popup, since proper window isn't selected yet */
+}
diff --git a/src/screen.h b/src/screen.h
index ce0a4baa..f04e5bc4 100644
--- a/src/screen.h
+++ b/src/screen.h
@@ -39,7 +39,8 @@ struct _MetaScreen
   int width;
   int height;
   MetaUI *ui;
-
+  MetaTabPopup *tab_popup;
+  
   MetaWorkspace *active_workspace;
 
   MetaStack *stack;
@@ -63,6 +64,8 @@ int           meta_screen_get_n_workspaces    (MetaScreen                 *scree
 void          meta_screen_set_cursor          (MetaScreen                 *screen,
                                                MetaCursor                  cursor);
 
+void          meta_screen_ensure_tab_popup    (MetaScreen                 *screen);
+
 #endif
 
 
diff --git a/src/stack.c b/src/stack.c
index b7de8770..91e7e372 100644
--- a/src/stack.c
+++ b/src/stack.c
@@ -902,6 +902,41 @@ meta_stack_get_tab_next (MetaStack  *stack,
     return find_tab_forward (stack, NULL, -1);
 }
 
+GSList*
+meta_stack_get_tab_list (MetaStack *stack)
+{
+  GSList *list;
+  int i;
+  
+  list = NULL;
+  
+  i = 0;
+  while (i < stack->windows->len)
+    {
+      MetaWindow *window;
+      MetaWorkspace *workspace;
+      
+      window = meta_display_lookup_x_window (stack->screen->display,
+                                             GET_XWINDOW (stack, i));
+
+      if (window)
+        workspace = window->screen->active_workspace;
+      else
+        workspace = NULL;
+      
+      if (window && IN_TAB_CHAIN (window) &&
+          (workspace == NULL ||
+           meta_workspace_contains_window (workspace, window)))
+        list = g_slist_prepend (list, window);
+
+      ++i;
+    }
+
+  list = g_slist_reverse (list);
+
+  return list;
+}
+
 int
 meta_stack_windows_cmp  (MetaStack  *stack,
                          MetaWindow *window_a,
diff --git a/src/stack.h b/src/stack.h
index 0ed30cb4..e9ad4e46 100644
--- a/src/stack.h
+++ b/src/stack.h
@@ -95,6 +95,7 @@ MetaWindow* meta_stack_get_below  (MetaStack  *stack,
 MetaWindow* meta_stack_get_tab_next (MetaStack  *stack,
                                      MetaWindow *window,
                                      gboolean    backward);
+GSList*     meta_stack_get_tab_list (MetaStack  *stack);
 /* -1 if a < b, etc. */
 int         meta_stack_windows_cmp  (MetaStack  *stack,
                                      MetaWindow *window_a,
diff --git a/src/tabpopup.c b/src/tabpopup.c
index 5db601c0..0a1b055e 100644
--- a/src/tabpopup.c
+++ b/src/tabpopup.c
@@ -40,6 +40,7 @@ struct _MetaTabPopup
   GtkWidget *label;
   GList *current;
   GList *entries;
+  GtkWidget *current_selected_widget;
 };
 
 MetaTabPopup*
@@ -52,12 +53,19 @@ meta_ui_tab_popup_new (const MetaTabEntry *entries)
   int height;
   GtkWidget *table;
   GList *tmp;
+  GtkWidget *frame;
   
   popup = g_new (MetaTabPopup, 1);
   popup->window = gtk_window_new (GTK_WINDOW_POPUP);
+  gtk_window_set_position (GTK_WINDOW (popup->window),
+                           GTK_WIN_POS_CENTER_ALWAYS);
+  /* enable resizing, to get never-shrink behavior */
+  gtk_window_set_resizable (GTK_WINDOW (popup->window),
+                            TRUE);
   popup->current = NULL;
   popup->entries = NULL;
-
+  popup->current_selected_widget = NULL;
+  
   tab_entries = NULL;
   i = 0;
   while (entries[i].xwindow != None)
@@ -84,33 +92,52 @@ meta_ui_tab_popup_new (const MetaTabEntry *entries)
     height += 1;
 
   table = gtk_table_new (height + 1, width, FALSE);
+  
+  frame = gtk_frame_new (NULL);
+  gtk_frame_set_shadow_type (GTK_FRAME (frame), GTK_SHADOW_OUT);
+  gtk_container_set_border_width (GTK_CONTAINER (table), 1);
+  gtk_container_add (GTK_CONTAINER (popup->window),
+                     frame);
+  gtk_container_add (GTK_CONTAINER (frame),
+                     table);
 
+  
   popup->label = gtk_label_new ("");
   
   gtk_table_attach (GTK_TABLE (table),
                     popup->label,
-                    0, width,   height - 1, height,
-                    0,          0,
-                    0,          0);
+                    0, width,              height, height + 1,
+                    GTK_EXPAND | GTK_FILL, GTK_EXPAND | GTK_FILL,
+                    0,                     2);
   
-  left = 0;
-  right = 1;
+
   top = 0;
   bottom = 1;
   tmp = popup->entries;
   
   while (tmp && top < height)
-    {
+    {      
+      left = 0;
+      right = 1;
+
       while (tmp && left < width)
         {
           GtkWidget *image;
+          GtkWidget *highlight;
+          
           TabEntry *te;
 
           te = tmp->data;
-          
-          image = gtk_image_new_from_pixbuf (te->icon);
 
-          te->widget = image;
+          highlight = gtk_frame_new (NULL);
+          gtk_frame_set_shadow_type (GTK_FRAME (highlight),
+                                     GTK_SHADOW_NONE);
+          image = gtk_image_new_from_pixbuf (te->icon);
+          gtk_misc_set_padding (GTK_MISC (image), 3, 3);
+          
+          gtk_container_add (GTK_CONTAINER (highlight), image);
+          
+          te->widget = highlight;
 
           gtk_table_attach (GTK_TABLE (table),
                             te->widget,
@@ -121,9 +148,11 @@ meta_ui_tab_popup_new (const MetaTabEntry *entries)
           tmp = tmp->next;
           
           ++left;
+          ++right;
         }
       
       ++top;
+      ++bottom;
     }
       
   return popup;
@@ -169,7 +198,16 @@ static void
 display_entry (MetaTabPopup *popup,
                TabEntry     *te)
 {
+  if (popup->current_selected_widget)
+    {
+      gtk_frame_set_shadow_type (GTK_FRAME (popup->current_selected_widget),
+                                 GTK_SHADOW_NONE);
+    }
+  
   gtk_label_set_text (GTK_LABEL (popup->label), te->title);
+  gtk_frame_set_shadow_type (GTK_FRAME (te->widget),
+                             GTK_SHADOW_ETCHED_IN);
+  popup->current_selected_widget = te->widget;
 }
 
 void
diff --git a/src/ui.c b/src/ui.c
index de1d8ef5..fe3de2c2 100644
--- a/src/ui.c
+++ b/src/ui.c
@@ -345,3 +345,12 @@ meta_ui_pop_delay_exposes  (MetaUI *ui)
   meta_frames_pop_delay_exposes (ui->frames);
 }
 
+GdkPixbuf*
+meta_ui_get_default_window_icon (MetaUI *ui)
+{
+  /* FIXME */
+  return gtk_widget_render_icon (GTK_WIDGET (ui->frames),
+                                 GTK_STOCK_NEW,
+                                 GTK_ICON_SIZE_LARGE_TOOLBAR,
+                                 NULL);
+}
diff --git a/src/ui.h b/src/ui.h
index b7df976e..c9ce0b04 100644
--- a/src/ui.h
+++ b/src/ui.h
@@ -117,6 +117,8 @@ GdkPixbuf* meta_gdk_pixbuf_get_from_window (GdkPixbuf   *dest,
 void      meta_ui_push_delay_exposes (MetaUI *ui);
 void      meta_ui_pop_delay_exposes  (MetaUI *ui);
 
+GdkPixbuf* meta_ui_get_default_window_icon (MetaUI *ui);
+
 typedef struct _MetaTabEntry MetaTabEntry;
 typedef struct _MetaTabPopup MetaTabPopup;
 
diff --git a/src/window.c b/src/window.c
index 65f31fbb..9e45d46f 100644
--- a/src/window.c
+++ b/src/window.c
@@ -653,6 +653,9 @@ meta_window_free (MetaWindow  *window)
   
   meta_error_trap_pop (window->display);
 
+  if (window->icon)
+    g_object_unref (G_OBJECT (window->icon));
+  
   g_free (window->sm_client_id);
   g_free (window->role);
   g_free (window->res_class);
@@ -3497,16 +3500,22 @@ update_icon_name (MetaWindow *window)
 
 static int
 update_icon (MetaWindow *window)
-{
+{  
   meta_error_trap_push (window->display);
-  
+
+#if 0
   if (window->icon)
     {
       g_object_unref (G_OBJECT (window->icon));
       window->icon = NULL;
-    }  
+    }
+#endif
   
   /* FIXME */
+
+  /* Fallback */
+  if (window->icon == NULL)
+    window->icon = meta_ui_get_default_window_icon (window->screen->ui);
   
   return meta_error_trap_pop (window->display);
 }