More work on refreshing the focus cycle dialog when windows are added/removed from the valid focus order

8 files changed, 147 insertions, 136 deletions

diff --git a/openbox/client.c b/openbox/client.c
index 6799654e..afea324b 100644
--- a/openbox/client.c
+++ b/openbox/client.c
@@ -34,6 +34,7 @@
 #include "grab.h"
 #include "prompt.h"
 #include "focus.h"
+#include "focus_cycle.h"
 #include "stacking.h"
 #include "openbox.h"
 #include "group.h"
@@ -1996,6 +1997,8 @@ void client_update_wmhints(ObClient *self)
 
         XFree(hints);
     }
+
+    focus_cycle_addremove(self, TRUE);
 }
 
 void client_update_title(ObClient *self)
@@ -3254,7 +3257,7 @@ static void client_iconify_recursive(ObClient *self,
                 self->iconic = iconic;
 
                 /* update the focus lists.. iconic windows go to the bottom of
-                   the list */
+                   the list. this will also call focus_cycle_addremove(). */
                 focus_order_to_bottom(self);
 
                 changed = TRUE;
@@ -3266,9 +3269,10 @@ static void client_iconify_recursive(ObClient *self,
                 self->desktop != DESKTOP_ALL)
                 client_set_desktop(self, screen_desktop, FALSE, FALSE);
 
-            /* this puts it after the current focused window */
-            focus_order_remove(self);
-            focus_order_add_new(self);
+            /* this puts it after the current focused window, this will
+               also cause focus_cycle_addremove() to be called for the
+               client */
+            focus_order_like_new(self);
 
             changed = TRUE;
         }
@@ -3601,6 +3605,8 @@ static void client_set_desktop_recursive(ObClient *self,
             /* the new desktop's geometry may be different, so we may need to
                resize, for example if we are maximized */
             client_reconfigure(self, FALSE);
+
+        focus_cycle_addremove(self, FALSE);
     }
 
     /* move all transients */
@@ -3616,6 +3622,8 @@ void client_set_desktop(ObClient *self, guint target,
 {
     self = client_search_top_direct_parent(self);
     client_set_desktop_recursive(self, target, donthide, dontraise);
+
+    focus_cycle_addremove(NULL, TRUE);
 }
 
 gboolean client_is_direct_child(ObClient *parent, ObClient *child)
@@ -3864,6 +3872,8 @@ void client_set_state(ObClient *self, Atom action, glong data1, glong data2)
         client_hilite(self, demands_attention);
 
     client_change_state(self); /* change the hint to reflect these changes */
+
+    focus_cycle_addremove(self, TRUE);
 }
 
 ObClient *client_focus_target(ObClient *self)
diff --git a/openbox/focus.c b/openbox/focus.c
index c82c4f62..36d754f4 100644
--- a/openbox/focus.c
+++ b/openbox/focus.c
@@ -90,6 +90,9 @@ void focus_set_client(ObClient *client)
         push_to_top(client);
         /* remove hiliting from the window when it gets focused */
         client_hilite(client, FALSE);
+
+        /* make sure the focus cycle popup shows things in the right order */
+        focus_cycle_reorder();
     }
 
     /* set the NET_ACTIVE_WINDOW hint, but preserve it on shutdown */
@@ -98,9 +101,6 @@ void focus_set_client(ObClient *client)
         PROP_SET32(RootWindow(ob_display, ob_screen),
                    net_active_window, window, active);
     }
-
-    /* make sure the focus cycle popup shows things in the right order */
-    focus_cycle_reorder();
 }
 
 static ObClient* focus_fallback_target(gboolean allow_refocus,
@@ -201,7 +201,7 @@ void focus_order_add_new(ObClient *c)
         focus_order_to_top(c);
     else {
         g_assert(!g_list_find(focus_order, c));
-        /* if there are any iconic windows, put this above them in the order,
+        /* if there are only iconic windows, put this above them in the order,
            but if there are not, then put it under the currently focused one */
         if (focus_order && ((ObClient*)focus_order->data)->iconic)
             focus_order = g_list_insert(focus_order, c, 0);
@@ -209,14 +209,20 @@ void focus_order_add_new(ObClient *c)
             focus_order = g_list_insert(focus_order, c, 1);
     }
 
-    focus_cycle_add(c);
+    focus_cycle_addremove(c, TRUE);
 }
 
 void focus_order_remove(ObClient *c)
 {
     focus_order = g_list_remove(focus_order, c);
 
-    focus_cycle_remove(c);
+    focus_cycle_addremove(c, TRUE);
+}
+
+void focus_order_like_new(struct _ObClient *c)
+{
+    focus_order = g_list_remove(focus_order, c);
+    focus_order_add_new(c);
 }
 
 void focus_order_to_top(ObClient *c)
@@ -232,6 +238,8 @@ void focus_order_to_top(ObClient *c)
              it && !((ObClient*)it->data)->iconic; it = g_list_next(it));
         focus_order = g_list_insert_before(focus_order, it, c);
     }
+
+    focus_cycle_reorder();
 }
 
 void focus_order_to_bottom(ObClient *c)
@@ -247,6 +255,8 @@ void focus_order_to_bottom(ObClient *c)
              it && !((ObClient*)it->data)->iconic; it = g_list_next(it));
         focus_order = g_list_insert_before(focus_order, it, c);
     }
+
+    focus_cycle_reorder();
 }
 
 ObClient *focus_order_find_first(guint desktop)
@@ -292,6 +302,10 @@ gboolean focus_valid_target(ObClient *ft,
                             gboolean desktop_windows,
                             gboolean user_request)
 {
+    /* NOTE: if any of these things change on a client, then they should call
+       focus_cycle_addremove() to make sure the client is not shown/hidden
+       when it should not be */
+
     gboolean ok = FALSE;
 
     /* see if the window is still managed or is going away */
diff --git a/openbox/focus.h b/openbox/focus.h
index 19ab406e..c8af1101 100644
--- a/openbox/focus.h
+++ b/openbox/focus.h
@@ -58,6 +58,10 @@ void focus_order_remove(struct _ObClient *c);
 /*! Move a client to the top of the focus order */
 void focus_order_to_top(struct _ObClient *c);
 
+/*! Move a client to where it would be if it was newly added to the focus order
+ */
+void focus_order_like_new(struct _ObClient *c);
+
 /*! Move a client to the bottom of the focus order (keeps iconic windows at the
   very bottom always though). */
 void focus_order_to_bottom(struct _ObClient *c);
diff --git a/openbox/focus_cycle.c b/openbox/focus_cycle.c
index 07cc2e83..40436683 100644
--- a/openbox/focus_cycle.c
+++ b/openbox/focus_cycle.c
@@ -30,8 +30,14 @@
 #include <X11/Xlib.h>
 #include <glib.h>
 
+typedef enum {
+    OB_CYCLE_NONE = 0,
+    OB_CYCLE_NORMAL,
+    OB_CYCLE_DIRECTIONAL
+} ObCycleType;
+
 ObClient       *focus_cycle_target = NULL;
-static gboolean focus_cycle_directional = FALSE;
+static ObCycleType focus_cycle_type = OB_CYCLE_NONE;
 static gboolean focus_cycle_iconic_windows;
 static gboolean focus_cycle_all_desktops;
 static gboolean focus_cycle_dock_windows;
@@ -52,64 +58,43 @@ void focus_cycle_shutdown(gboolean reconfig)
     if (reconfig) return;
 }
 
-void focus_cycle_add(ObClient *ifclient)
-{
-    if (!(focus_cycle_target && ifclient && !focus_cycle_directional))
-        return;
-
-    if (focus_valid_target(ifclient, TRUE,
-                           focus_cycle_iconic_windows,
-                           focus_cycle_all_desktops,
-                           focus_cycle_dock_windows,
-                           focus_cycle_desktop_windows,
-                           FALSE))
-        focus_cycle_popup_refresh(focus_cycle_target,
-                                  focus_cycle_iconic_windows,
-                                  focus_cycle_all_desktops,
-                                  focus_cycle_dock_windows,
-                                  focus_cycle_desktop_windows); 
-}
-
-void focus_cycle_remove(ObClient *ifclient)
+void focus_cycle_addremove(ObClient *c, gboolean redraw)
 {
-    if (!(focus_cycle_target && ifclient))
+    if (!focus_cycle_type)
         return;
 
-    if (focus_cycle_directional) {
-        if (focus_cycle_target == ifclient) {
+    if (focus_cycle_type == OB_CYCLE_DIRECTIONAL) {
+        if (c && focus_cycle_target == c) {
             focus_directional_cycle(0, TRUE, TRUE, TRUE, TRUE,
                                     TRUE, TRUE, TRUE);
         }
     }
-    else {
-        if (!focus_valid_target(ifclient, TRUE,
-                                focus_cycle_iconic_windows,
-                                focus_cycle_all_desktops,
-                                focus_cycle_dock_windows,
-                                focus_cycle_desktop_windows,
-                                FALSE)) {
-            if (focus_cycle_target == ifclient) {
-                focus_cycle_target =
-                    focus_cycle_popup_revert(focus_cycle_target);
-                focus_cycle_update_indicator(focus_cycle_target);
-            }
-            focus_cycle_popup_refresh(focus_cycle_target,
-                                      focus_cycle_iconic_windows,
-                                      focus_cycle_all_desktops,
-                                      focus_cycle_dock_windows,
-                                      focus_cycle_desktop_windows); 
+    else if (c && redraw) {
+        gboolean v, s;
+
+        v = focus_cycle_valid(c);
+        s = focus_cycle_popup_is_showing(c);
+
+        if (v != s) {
+            focus_cycle_target =
+                focus_cycle_popup_refresh(focus_cycle_target, redraw);
         }
     }
+    else if (redraw) {
+        focus_cycle_reorder();
+    }
 }
 
 void focus_cycle_reorder()
 {
-    if (focus_cycle_target && !focus_cycle_directional)
-        focus_cycle_popup_refresh(focus_cycle_target,
-                                  focus_cycle_iconic_windows,
-                                  focus_cycle_all_desktops,
-                                  focus_cycle_dock_windows,
-                                  focus_cycle_desktop_windows); 
+    if (focus_cycle_type == OB_CYCLE_NORMAL) {
+        focus_cycle_target = focus_cycle_popup_refresh(focus_cycle_target,
+                                                       TRUE);
+        focus_cycle_update_indicator(focus_cycle_target);
+        if (!focus_cycle_target)
+            focus_cycle(TRUE, TRUE, TRUE, TRUE, TRUE,
+                        TRUE, TRUE, TRUE, TRUE, TRUE);
+    }
 }
 
 ObClient* focus_cycle(gboolean forward, gboolean all_desktops,
@@ -126,7 +111,6 @@ ObClient* focus_cycle(gboolean forward, gboolean all_desktops,
     if (interactive) {
         if (cancel) {
             focus_cycle_target = NULL;
-            focus_cycle_directional = FALSE;
             goto done_cycle;
         } else if (done)
             goto done_cycle;
@@ -164,17 +148,11 @@ ObClient* focus_cycle(gboolean forward, gboolean all_desktops,
             if (it == NULL) it = g_list_last(list);
         }
         ft = it->data;
-        if (focus_valid_target(ft, TRUE,
-                               focus_cycle_iconic_windows,
-                               focus_cycle_all_desktops,
-                               focus_cycle_dock_windows,
-                               focus_cycle_desktop_windows,
-                               FALSE))
-        {
+        if (focus_cycle_valid(ft)) {
             if (interactive) {
                 if (ft != focus_cycle_target) { /* prevents flicker */
                     focus_cycle_target = ft;
-                    focus_cycle_directional = FALSE;
+                    focus_cycle_type = OB_CYCLE_NORMAL;
                     focus_cycle_draw_indicator(showbar ? ft : NULL);
                 }
                 if (dialog)
@@ -187,7 +165,7 @@ ObClient* focus_cycle(gboolean forward, gboolean all_desktops,
                 return focus_cycle_target;
             } else if (ft != focus_cycle_target) {
                 focus_cycle_target = ft;
-                focus_cycle_directional = FALSE;
+                focus_cycle_type = OB_CYCLE_NORMAL;
                 done = TRUE;
                 break;
             }
@@ -198,7 +176,7 @@ done_cycle:
     if (done && !cancel) ret = focus_cycle_target;
 
     focus_cycle_target = NULL;
-    focus_cycle_directional = FALSE;
+    focus_cycle_type = OB_CYCLE_NONE;
     g_list_free(order);
     order = NULL;
 
@@ -238,8 +216,7 @@ static ObClient *focus_find_directional(ObClient *c, ObDirection dir,
         /* the currently selected window isn't interesting */
         if (cur == c)
             continue;
-        if (!focus_valid_target(it->data, TRUE, FALSE, FALSE, dock_windows,
-                                desktop_windows, FALSE))
+        if (!focus_cycle_valid(it->data))
             continue;
 
         /* find the centre coords of this window, from the
@@ -318,7 +295,6 @@ ObClient* focus_directional_cycle(ObDirection dir, gboolean dock_windows,
 
     if (cancel) {
         focus_cycle_target = NULL;
-        focus_cycle_directional = FALSE;
         goto done_cycle;
     } else if (done && interactive)
         goto done_cycle;
@@ -344,17 +320,15 @@ ObClient* focus_directional_cycle(ObDirection dir, gboolean dock_windows,
         GList *it;
 
         for (it = focus_order; it; it = g_list_next(it))
-            if (focus_valid_target(it->data, TRUE,
-                                   focus_cycle_iconic_windows,
-                                   focus_cycle_all_desktops,
-                                   focus_cycle_dock_windows,
-                                   focus_cycle_desktop_windows, FALSE))
+            if (focus_cycle_valid(it->data)) {
                 ft = it->data;
+                break;
+            }
     }
 
     if (ft && ft != focus_cycle_target) {/* prevents flicker */
         focus_cycle_target = ft;
-        focus_cycle_directional = TRUE;
+        focus_cycle_type = OB_CYCLE_DIRECTIONAL;
         if (!interactive)
             goto done_cycle;
         focus_cycle_draw_indicator(showbar ? ft : NULL);
@@ -373,10 +347,20 @@ done_cycle:
 
     first = NULL;
     focus_cycle_target = NULL;
-    focus_cycle_directional = FALSE;
+    focus_cycle_type = OB_CYCLE_NONE;
 
     focus_cycle_draw_indicator(NULL);
     focus_cycle_popup_single_hide();
 
     return ret;
 }
+
+gboolean focus_cycle_valid(struct _ObClient *client)
+{
+    return focus_valid_target(client, TRUE,
+                              focus_cycle_iconic_windows,
+                              focus_cycle_all_desktops,
+                              focus_cycle_dock_windows,
+                              focus_cycle_desktop_windows,
+                              FALSE);
+}
diff --git a/openbox/focus_cycle.h b/openbox/focus_cycle.h
index c074a52b..4a97c103 100644
--- a/openbox/focus_cycle.h
+++ b/openbox/focus_cycle.h
@@ -47,8 +47,10 @@ struct _ObClient* focus_directional_cycle(ObDirection dir,
                                           gboolean dialog,
                                           gboolean done, gboolean cancel);
 
-void focus_cycle_add(struct _ObClient *ifclient);
-void focus_cycle_remove(struct _ObClient *ifclient);
+/*! Set @redraw to FALSE if there are more clients to be added/removed first */
+void focus_cycle_addremove(struct _ObClient *ifclient, gboolean redraw);
 void focus_cycle_reorder();
 
+gboolean focus_cycle_valid(struct _ObClient *client);
+
 #endif
diff --git a/openbox/focus_cycle_popup.c b/openbox/focus_cycle_popup.c
index b97bc97b..16522802 100644
--- a/openbox/focus_cycle_popup.c
+++ b/openbox/focus_cycle_popup.c
@@ -18,6 +18,7 @@
 */
 
 #include "focus_cycle_popup.h"
+#include "focus_cycle.h"
 #include "popup.h"
 #include "client.h"
 #include "screen.h"
@@ -78,11 +79,7 @@ static ObIconPopup *single_popup;
 static gchar   *popup_get_name (ObClient *c);
 static gboolean popup_setup    (ObFocusCyclePopup *p,
                                 gboolean create_targets,
-                                gboolean refresh_targets,
-                                gboolean iconic_windows,
-                                gboolean all_desktops,
-                                gboolean dock_windows,
-                                gboolean desktop_windows);
+                                gboolean refresh_targets);
 static void     popup_render   (ObFocusCyclePopup *p,
                                 const ObClient *c);
 
@@ -172,9 +169,7 @@ static void popup_target_free(ObFocusCyclePopupTarget *t)
 }
 
 static gboolean popup_setup(ObFocusCyclePopup *p, gboolean create_targets,
-                            gboolean refresh_targets,
-                            gboolean iconic_windows, gboolean all_desktops,
-                            gboolean dock_windows, gboolean desktop_windows)
+                            gboolean refresh_targets)
 {
     gint maxwidth, n;
     GList *it;
@@ -206,13 +201,7 @@ static gboolean popup_setup(ObFocusCyclePopup *p, gboolean create_targets,
     for (it = g_list_last(focus_order); it; it = g_list_previous(it)) {
         ObClient *ft = it->data;
 
-        if (focus_valid_target(ft, TRUE,
-                               iconic_windows,
-                               all_desktops,
-                               dock_windows,
-                               desktop_windows,
-                               FALSE))
-        {
+        if (focus_cycle_valid(ft)) {
             GList *rit;
 
             /* reuse the target if possible during refresh */
@@ -268,8 +257,6 @@ static gboolean popup_setup(ObFocusCyclePopup *p, gboolean create_targets,
         while (rtargets) {
             popup_target_free(rtargets->data);
             rtargets = g_list_delete_link(rtargets, rtargets);
-
-            popup.targets = g_list_delete_link(popup.targets, popup.targets);
         }
     }
 
@@ -523,8 +510,7 @@ void focus_cycle_popup_show(ObClient *c, gboolean iconic_windows,
 
     /* do this stuff only when the dialog is first showing */
     if (!popup.mapped)
-        popup_setup(&popup, TRUE, FALSE, iconic_windows, all_desktops,
-                    dock_windows, desktop_windows);
+        popup_setup(&popup, TRUE, FALSE);
     g_assert(popup.targets != NULL);
 
     popup_render(&popup, c);
@@ -571,8 +557,7 @@ void focus_cycle_popup_single_show(struct _ObClient *c,
     if (!popup.mapped) {
         Rect *a;
 
-        popup_setup(&popup, FALSE, FALSE, iconic_windows, all_desktops,
-                    dock_windows, desktop_windows);
+        popup_setup(&popup, FALSE, FALSE);
         g_assert(popup.targets == NULL);
 
         /* position the popup */
@@ -597,54 +582,67 @@ void focus_cycle_popup_single_hide(void)
     icon_popup_hide(single_popup);
 }
 
-GList* focus_cycle_popup_is_showing(ObClient *client)
+gboolean focus_cycle_popup_is_showing(ObClient *c)
 {
     if (popup.mapped) {
         GList *it;
 
         for (it = popup.targets; it; it = g_list_next(it)) {
             ObFocusCyclePopupTarget *t = it->data;
-            if (t->client == client)
-                return it;
+            if (t->client == c)
+                return TRUE;
         }
     }
-    return NULL;
+    return FALSE;
 }
 
-ObClient* focus_cycle_popup_revert(ObClient *target)
+static ObClient* popup_revert(ObClient *target)
 {
-    GList *it;
-
-    if (!popup.mapped) return NULL;
+    GList *it, *itt;
 
     for (it = popup.targets; it; it = g_list_next(it)) {
         ObFocusCyclePopupTarget *t = it->data;
         if (t->client == target) {
-            if (it->prev)
-                return ((ObFocusCyclePopupTarget*)it->prev->data)->client;
-            else if (it->next)
-                return ((ObFocusCyclePopupTarget*)it->next->data)->client;
-            else
-                return NULL;
+            /* move to a previous window if possible */
+            for (itt = it->prev; itt; itt = g_list_previous(itt)) {
+                ObFocusCyclePopupTarget *t2 = it->data;
+                if (focus_cycle_valid(t2->client))
+                    return t2->client;
+            }
+
+            /* otherwise move to a following window if possible */
+            for (itt = it->next; itt; itt = g_list_next(itt)) {
+                ObFocusCyclePopupTarget *t2 = it->data;
+                if (focus_cycle_valid(t2->client))
+                    return t2->client;
+            }
+
+            /* otherwise, we can't go anywhere there is nowhere valid to go */
+            return NULL;
         }
     }
-    g_assert_not_reached();
+    return NULL;
 }
 
-void focus_cycle_popup_refresh(ObClient *target,
-                               gboolean iconic_windows,
-                               gboolean all_desktops,
-                               gboolean dock_windows,
-                               gboolean desktop_windows)
+ObClient* focus_cycle_popup_refresh(ObClient *target,
+                                    gboolean redraw)
 {
-    if (!popup.mapped) return;
+    if (!popup.mapped) return NULL;
 
-    if (popup_setup(&popup, TRUE, TRUE, iconic_windows, all_desktops,
-                    dock_windows, desktop_windows))
-    {
+    if (!focus_cycle_valid(target))
+        target = popup_revert(target);
+
+    redraw = popup_setup(&popup, TRUE, TRUE) && redraw;
+
+    if (!target && popup.targets)
+        target = ((ObFocusCyclePopupTarget*)popup.targets->data)->client;
+
+    if (target && redraw) {
         popup.mapped = FALSE;
         popup_render(&popup, target);
         XFlush(ob_display);
         popup.mapped = TRUE;
     }
+
+    return target;
 }
diff --git a/openbox/focus_cycle_popup.h b/openbox/focus_cycle_popup.h
index b4c9e350..934d9a67 100644
--- a/openbox/focus_cycle_popup.h
+++ b/openbox/focus_cycle_popup.h
@@ -39,13 +39,13 @@ void focus_cycle_popup_single_show(struct _ObClient *c,
                                    gboolean desktop_windows);
 void focus_cycle_popup_single_hide();
 
-/*! Reverts from the current @target to a new focus cycle target window */
-struct _ObClient* focus_cycle_popup_revert(struct _ObClient *target);
-/*! Redraws the focus cycle popup */
-void focus_cycle_popup_refresh(struct _ObClient *target,
-                               gboolean iconic_windows,
-                               gboolean all_desktops,
-                               gboolean dock_windows,
-                               gboolean desktop_windows);
+gboolean focus_cycle_popup_is_showing(struct _ObClient *c);
+
+/*! Redraws the focus cycle popup, and returns the current target.  If
+    the target given to the function is no longer valid, this will return
+    a different target that is valid, and which should be considered the
+    current focus cycling target. */
+struct _ObClient *focus_cycle_popup_refresh(struct _ObClient *target,
+                                            gboolean redraw);
 
 #endif
diff --git a/openbox/screen.c b/openbox/screen.c
index 5ae07907..55fd58d6 100644
--- a/openbox/screen.c
+++ b/openbox/screen.c
@@ -713,8 +713,7 @@ void screen_set_desktop(guint num, gboolean dofocus)
     for (it = stacking_list; it; it = g_list_next(it)) {
         if (WINDOW_IS_CLIENT(it->data)) {
             ObClient *c = it->data;
-            if (client_show(c))
-                focus_cycle_add(c);
+            client_show(c);
         }
     }
 
@@ -725,8 +724,6 @@ void screen_set_desktop(guint num, gboolean dofocus)
         if (WINDOW_IS_CLIENT(it->data)) {
             ObClient *c = it->data;
             if (client_hide(c)) {
-                focus_cycle_remove(c);
-
                 if (c == focus_client) {
                     /* c was focused and we didn't do fallback clearly so make
                        sure openbox doesnt still consider the window focused.
@@ -742,6 +739,8 @@ void screen_set_desktop(guint num, gboolean dofocus)
         }
     }
 
+    focus_cycle_addremove(NULL, TRUE);
+
     event_end_ignore_all_enters(ignore_start);
 
     if (event_curtime != CurrentTime)