Revise workspace next/prev

See the issue #1798 (http://github.com/i3/i3/issues/1798).

+workspace_next+ as-is cycles through either numbered or named workspaces,
but when it reaches the last numbered/named workspace, it only looks for
named workspaces. This commit changes it: look for named workspaces after
exhausting numbered ones, but also for numbered ones after exhausting
named ones.

Also add a test case 528-workspace-next-prev.t (numbered workspaces and named
workspaces on 2 outputs) for testing this.

docs/userguide

@@ -1849,6 +1849,10 @@ container to workspace next+, +move container to workspace prev+ to move a
 container to the next/previous workspace and +move container to workspace current+
 (the last one makes sense only when used with criteria).
 
++workspace next+ cycles through either numbered or named workspaces. But when it
+reaches the last numbered/named workspace, it looks for named workspaces after
+exhausting numbered ones and looks for numbered ones after exhausting named ones.
+
 See <<move_to_outputs>> for how to move a container/workspace to a different
 RandR output.
 

src/workspace.c

@@ -506,12 +506,33 @@ void workspace_show_by_name(const char *num) {
  */
 Con *workspace_next(void) {
     Con *current = con_get_workspace(focused);
-    Con *next = NULL;
+    Con *next = NULL, *first = NULL, *first_opposite = NULL;
     Con *output;
 
     if (current->num == -1) {
         /* If currently a named workspace, find next named workspace. */
-        next = TAILQ_NEXT(current, nodes);
+        if ((next = TAILQ_NEXT(current, nodes)) != NULL)
+            return next;
+        bool found_current = false;
+        TAILQ_FOREACH(output, &(croot->nodes_head), nodes) {
+            /* Skip outputs starting with __, they are internal. */
+            if (con_is_internal(output))
+                continue;
+            NODES_FOREACH(output_get_content(output)) {
+                if (child->type != CT_WORKSPACE)
+                    continue;
+                if (!first)
+                    first = child;
+                if (!first_opposite && child->num != -1)
+                    first_opposite = child;
+                if (child == current) {
+                    found_current = true;
+                } else if (child->num == -1 && found_current) {
+                    next = child;
+                    return next;
+                }
+            }
+        }
     } else {
         /* If currently a numbered workspace, find next numbered workspace. */
         TAILQ_FOREACH(output, &(croot->nodes_head), nodes) {
@@ -521,6 +542,10 @@ Con *workspace_next(void) {
             NODES_FOREACH(output_get_content(output)) {
                 if (child->type != CT_WORKSPACE)
                     continue;
+                if (!first)
+                    first = child;
+                if (!first_opposite && child->num == -1)
+                    first_opposite = child;
                 if (child->num == -1)
                     break;
                 /* Need to check child against current and next because we are
@@ -532,41 +557,9 @@ Con *workspace_next(void) {
         }
     }
 
-    /* Find next named workspace. */
-    if (!next) {
-        bool found_current = false;
-        TAILQ_FOREACH(output, &(croot->nodes_head), nodes) {
-            /* Skip outputs starting with __, they are internal. */
-            if (con_is_internal(output))
-                continue;
-            NODES_FOREACH(output_get_content(output)) {
-                if (child->type != CT_WORKSPACE)
-                    continue;
-                if (child == current) {
-                    found_current = 1;
-                } else if (child->num == -1 && (current->num != -1 || found_current)) {
-                    next = child;
-                    goto workspace_next_end;
-                }
-            }
-        }
-    }
+    if (!next)
+        next = first_opposite ? first_opposite : first;
 
-    /* Find first workspace. */
-    if (!next) {
-        TAILQ_FOREACH(output, &(croot->nodes_head), nodes) {
-            /* Skip outputs starting with __, they are internal. */
-            if (con_is_internal(output))
-                continue;
-            NODES_FOREACH(output_get_content(output)) {
-                if (child->type != CT_WORKSPACE)
-                    continue;
-                if (!next || (child->num != -1 && child->num < next->num))
-                    next = child;
-            }
-        }
-    }
-workspace_next_end:
     return next;
 }
 
@@ -576,7 +569,7 @@ workspace_next_end:
  */
 Con *workspace_prev(void) {
     Con *current = con_get_workspace(focused);
-    Con *prev = NULL;
+    Con *prev = NULL, *first_opposite = NULL, *last = NULL;
     Con *output;
 
     if (current->num == -1) {
@@ -584,6 +577,28 @@ Con *workspace_prev(void) {
         prev = TAILQ_PREV(current, nodes_head, nodes);
         if (prev && prev->num != -1)
             prev = NULL;
+        if (!prev) {
+            bool found_current = false;
+            TAILQ_FOREACH_REVERSE(output, &(croot->nodes_head), nodes_head, nodes) {
+                /* Skip outputs starting with __, they are internal. */
+                if (con_is_internal(output))
+                    continue;
+                NODES_FOREACH_REVERSE(output_get_content(output)) {
+                    if (child->type != CT_WORKSPACE)
+                        continue;
+                    if (!last)
+                        last = child;
+                    if (!first_opposite && child->num != -1)
+                        first_opposite = child;
+                    if (child == current) {
+                        found_current = true;
+                    } else if (child->num == -1 && found_current) {
+                        prev = child;
+                        goto workspace_prev_end;
+                    }
+                }
+            }
+        }
     } else {
         /* If numbered workspace, find previous numbered workspace. */
         TAILQ_FOREACH_REVERSE(output, &(croot->nodes_head), nodes_head, nodes) {
@@ -591,7 +606,13 @@ Con *workspace_prev(void) {
             if (con_is_internal(output))
                 continue;
             NODES_FOREACH_REVERSE(output_get_content(output)) {
-                if (child->type != CT_WORKSPACE || child->num == -1)
+                if (child->type != CT_WORKSPACE)
+                    continue;
+                if (!last)
+                    last = child;
+                if (!first_opposite && child->num == -1)
+                    first_opposite = child;
+                if (child->num == -1)
                     continue;
                 /* Need to check child against current and previous because we
                  * are traversing multiple lists and thus are not guaranteed
@@ -602,40 +623,8 @@ Con *workspace_prev(void) {
         }
     }
 
-    /* Find previous named workspace. */
-    if (!prev) {
-        bool found_current = false;
-        TAILQ_FOREACH_REVERSE(output, &(croot->nodes_head), nodes_head, nodes) {
-            /* Skip outputs starting with __, they are internal. */
-            if (con_is_internal(output))
-                continue;
-            NODES_FOREACH_REVERSE(output_get_content(output)) {
-                if (child->type != CT_WORKSPACE)
-                    continue;
-                if (child == current) {
-                    found_current = true;
-                } else if (child->num == -1 && (current->num != -1 || found_current)) {
-                    prev = child;
-                    goto workspace_prev_end;
-                }
-            }
-        }
-    }
-
-    /* Find last workspace. */
-    if (!prev) {
-        TAILQ_FOREACH_REVERSE(output, &(croot->nodes_head), nodes_head, nodes) {
-            /* Skip outputs starting with __, they are internal. */
-            if (con_is_internal(output))
-                continue;
-            NODES_FOREACH_REVERSE(output_get_content(output)) {
-                if (child->type != CT_WORKSPACE)
-                    continue;
-                if (!prev || child->num > prev->num)
-                    prev = child;
-            }
-        }
-    }
+    if (!prev)
+        prev = first_opposite ? first_opposite : last;
 
 workspace_prev_end:
     return prev;
@@ -675,7 +664,7 @@ Con *workspace_next_on_output(void) {
             if (child->type != CT_WORKSPACE)
                 continue;
             if (child == current) {
-                found_current = 1;
+                found_current = true;
             } else if (child->num == -1 && (current->num != -1 || found_current)) {
                 next = child;
                 goto workspace_next_on_output_end;

testcases/t/528-workspace-next-prev.t

@@ -0,0 +1,127 @@
+#!perl
+# vim:ts=4:sw=4:expandtab
+#
+# Please read the following documents before working on tests:
+# • http://build.i3wm.org/docs/testsuite.html
+#   (or docs/testsuite)
+#
+# • http://build.i3wm.org/docs/lib-i3test.html
+#   (alternatively: perldoc ./testcases/lib/i3test.pm)
+#
+# • http://build.i3wm.org/docs/ipc.html
+#   (or docs/ipc)
+#
+# • http://onyxneon.com/books/modern_perl/modern_perl_a4.pdf
+#   (unless you are already familiar with Perl)
+#
+# Tests whether 'workspace next' works correctly.
+#
+use List::Util qw(first);
+use i3test i3_autostart => 0;
+
+sub assert_next {
+    my ($expected) = @_;
+
+    cmd 'workspace next';
+    # We need to sync after changing focus to a different output to wait for the
+    # EnterNotify to be processed, otherwise it will be processed at some point
+    # later in time and mess up our subsequent tests.
+    sync_with_i3;
+
+    is(focused_ws, $expected, "workspace $expected focused");
+}
+
+
+my $config = <<EOT;
+# i3 config file (v4)
+font -misc-fixed-medium-r-normal--13-120-75-75-C-70-iso10646-1
+
+fake-outputs 1024x768+0+0,1024x768+1024+0
+EOT
+my $pid = launch_with_config($config);
+
+################################################################################
+# Setup workspaces so that they stay open (with an empty container).
+# Have only named workspaces in the 1st output and numbered + named workspaces
+# in the 2nd output.
+################################################################################
+
+sync_with_i3;
+$x->root->warp_pointer(0, 0);
+sync_with_i3;
+
+cmd 'workspace A';
+# ensure workspace A stays open
+open_window;
+
+cmd 'workspace B';
+# ensure workspace B stays open
+open_window;
+
+cmd 'workspace D';
+# ensure workspace D stays open
+open_window;
+
+cmd 'workspace E';
+# ensure workspace E stays open
+open_window;
+
+cmd 'focus output right';
+
+cmd 'workspace 1';
+# ensure workspace 1 stays open
+open_window;
+
+cmd 'workspace 2';
+# ensure workspace 2 stays open
+open_window;
+
+cmd 'workspace 3';
+# ensure workspace 3 stays open
+open_window;
+
+cmd 'workspace 4';
+# ensure workspace 4 stays open
+open_window;
+
+cmd 'workspace 5';
+# ensure workspace 5 stays open
+open_window;
+
+cmd 'workspace C';
+# ensure workspace C stays open
+open_window;
+
+cmd 'workspace F';
+# ensure workspace F stays open
+open_window;
+
+cmd 'focus output right';
+
+################################################################################
+# Use workspace next and verify the correct order.
+################################################################################
+
+# The current order should be:
+# output 1: A, B, D, E
+# output 2: 1, 2, 3, 4, 5, C, F
+
+cmd 'workspace A';
+is(focused_ws, 'A', 'back on workspace A');
+
+assert_next('B');
+assert_next('D');
+assert_next('E');
+assert_next('C');
+assert_next('F');
+assert_next('1');
+assert_next('2');
+assert_next('3');
+assert_next('4');
+assert_next('5');
+assert_next('A');
+assert_next('B');
+
+exit_gracefully($pid);
+
+done_testing;