Merge pull request #4023 from orestisfl/ws-assignment

Fix 2 workspace assignment bugs

11 files changed, 116 insertions, 183 deletions

diff --git a/RELEASE-NOTES-next b/RELEASE-NOTES-next
index da5a014b..2da81372 100644
--- a/RELEASE-NOTES-next
+++ b/RELEASE-NOTES-next
@@ -46,3 +46,5 @@ working. Please reach out to us in that case!
   • fix a bug with i3-nagbar not starting after it has already started once
   • fix conflict when moving parent of fullscreen window to workspace
   • fix Xorg memory leak with i3bar
+  • fix named workspace assignments on output changes
+  • fix named workspace assignment precedence on workspace renames
diff --git a/include/handlers.h b/include/handlers.h
index d2c79c59..81012e7b 100644
--- a/include/handlers.h
+++ b/include/handlers.h
@@ -47,22 +47,3 @@ void handle_event(int type, xcb_generic_event_t *event);
  *
  */
 void property_handlers_init(void);
-
-#if 0
-/**
- * Configuration notifies are only handled because we need to set up ignore
- * for the following enter notify events
- *
- */
-int handle_configure_event(void *prophs, xcb_connection_t *conn, xcb_configure_notify_event_t *event);
-#endif
-
-#if 0
-/**
- * Handles _NET_WM_WINDOW_TYPE changes
- *
- */
-int handle_window_type(void *data, xcb_connection_t *conn, uint8_t state,
-                       xcb_window_t window, xcb_atom_t atom,
-                       xcb_get_property_reply_t *property);
-#endif
diff --git a/include/workspace.h b/include/workspace.h
index 69974a2e..2193ed0b 100644
--- a/include/workspace.h
+++ b/include/workspace.h
@@ -46,6 +46,19 @@ Con *get_existing_workspace_by_name(const char *name);
 Con *get_existing_workspace_by_num(int num);
 
 /**
+ * Returns the first output that is assigned to a workspace specified by the
+ * given name or number. Returns NULL if no such output exists.
+ *
+ * If an assignment matches by number but there is an assignment later that
+ * matches by name, the second one is preferred.
+ * The order of the 'ws_assignments' queue is respected: if multiple
+ * assignments match the criteria, the first one is returned.
+ * 'name' is ignored when NULL, 'parsed_num' is ignored when it is -1.
+ *
+ */
+Con *get_assigned_output(const char *name, long parsed_num);
+
+/**
  * Returns true if the first output assigned to a workspace with the given
  * workspace assignment is the same as the given output.
  *
@@ -57,11 +70,8 @@ bool output_triggers_assignment(Output *output, struct Workspace_Assignment *ass
  * creating the workspace if necessary (by allocating the necessary amount of
  * memory and initializing the data structures correctly).
  *
- * If created is not NULL, *created will be set to whether or not the
- * workspace has just been created.
- *
  */
-Con *workspace_get(const char *num, bool *created);
+Con *workspace_get(const char *num);
 
 /**
  * Extracts workspace names from keybindings (e.g. “web” from “bindsym $mod+1
@@ -136,51 +146,6 @@ void workspace_back_and_forth(void);
  */
 Con *workspace_back_and_forth_get(void);
 
-#if 0
-/**
- * Assigns the given workspace to the given screen by correctly updating its
- * state and reconfiguring all the clients on this workspace.
- *
- * This is called when initializing a screen and when re-assigning it to a
- * different screen which just got available (if you configured it to be on
- * screen 1 and you just plugged in screen 1).
- *
- */
-void workspace_assign_to(Workspace *ws, Output *screen, bool hide_it);
-
-/**
- * Initializes the given workspace if it is not already initialized. The given
- * screen is to be understood as a fallback, if the workspace itself either
- * was not assigned to a particular screen or cannot be placed there because
- * the screen is not attached at the moment.
- *
- */
-void workspace_initialize(Workspace *ws, Output *screen, bool recheck);
-
-/**
- * Gets the first unused workspace for the given screen, taking into account
- * the preferred_screen setting of every workspace (workspace assignments).
- *
- */
-Workspace *get_first_workspace_for_output(Output *screen);
-
-/**
- * Unmaps all clients (and stack windows) of the given workspace.
- *
- * This needs to be called separately when temporarily rendering a workspace
- * which is not the active workspace to force reconfiguration of all clients,
- * like in src/xinerama.c when re-assigning a workspace to another screen.
- *
- */
-void workspace_unmap_clients(xcb_connection_t *conn, Workspace *u_ws);
-
-/**
- * Maps all clients (and stack windows) of the given workspace.
- *
- */
-void workspace_map_clients(xcb_connection_t *conn, Workspace *ws);
-#endif
-
 /**
  * Goes through all clients on the given workspace and updates the workspace’s
  * urgent flag accordingly.
diff --git a/src/commands.c b/src/commands.c
index 3f8c2695..5bf4cef6 100644
--- a/src/commands.c
+++ b/src/commands.c
@@ -359,7 +359,7 @@ void cmd_move_con_to_workspace_name(I3_CMD, const char *name, const char *no_aut
 
     LOG("should move window to workspace %s\n", name);
     /* get the workspace */
-    Con *ws = workspace_get(name, NULL);
+    Con *ws = workspace_get(name);
 
     if (no_auto_back_and_forth == NULL) {
         ws = maybe_auto_back_and_forth_workspace(ws);
@@ -390,7 +390,7 @@ void cmd_move_con_to_workspace_number(I3_CMD, const char *which, const char *no_
 
     Con *ws = get_existing_workspace_by_num(parsed_num);
     if (!ws) {
-        ws = workspace_get(which, NULL);
+        ws = workspace_get(which);
     }
 
     if (no_auto_back_and_forth == NULL) {
@@ -1399,7 +1399,7 @@ void cmd_focus(I3_CMD) {
 
     CMD_FOCUS_WARN_CHILDREN;
 
-    Con *__i3_scratch = workspace_get("__i3_scratch", NULL);
+    Con *__i3_scratch = workspace_get("__i3_scratch");
     owindow *current;
     TAILQ_FOREACH (current, &owindows, owindows) {
         Con *ws = con_get_workspace(current->con);
@@ -2026,26 +2026,9 @@ void cmd_rename_workspace(I3_CMD, const char *old_name, const char *new_name) {
     con_attach(workspace, parent, false);
     ipc_send_workspace_event("rename", workspace, NULL);
 
-    /* Move the workspace to the correct output if it has an assignment */
-    struct Workspace_Assignment *assignment = NULL;
-    TAILQ_FOREACH (assignment, &ws_assignments, ws_assignments) {
-        if (assignment->output == NULL)
-            continue;
-        if (strcmp(assignment->name, workspace->name) != 0 && (!name_is_digits(assignment->name) || ws_name_to_number(assignment->name) != workspace->num)) {
-            continue;
-        }
-
-        Output *target_output = get_output_by_name(assignment->output, true);
-        if (!target_output) {
-            LOG("Could not get output named \"%s\"\n", assignment->output);
-            continue;
-        }
-        if (!output_triggers_assignment(target_output, assignment)) {
-            continue;
-        }
-        workspace_move_to_output(workspace, target_output);
-
-        break;
+    Con *assigned = get_assigned_output(workspace->name, workspace->num);
+    if (assigned) {
+        workspace_move_to_output(workspace, get_output_for_con(assigned));
     }
 
     bool can_restore_focus = previously_focused != NULL;
diff --git a/src/con.c b/src/con.c
index 3621c662..19685073 100644
--- a/src/con.c
+++ b/src/con.c
@@ -1370,7 +1370,7 @@ bool con_move_to_mark(Con *con, const char *mark) {
     }
 
     /* For target containers in the scratchpad, we just send the window to the scratchpad. */
-    if (con_get_workspace(target) == workspace_get("__i3_scratch", NULL)) {
+    if (con_get_workspace(target) == workspace_get("__i3_scratch")) {
         DLOG("target container is in the scratchpad, moving container to scratchpad.\n");
         scratchpad_move(con);
         return true;
diff --git a/src/handlers.c b/src/handlers.c
index b74e589d..eba5fe29 100644
--- a/src/handlers.c
+++ b/src/handlers.c
@@ -727,7 +727,7 @@ static void handle_client_message(xcb_client_message_event_t *event) {
             return;
         }
 
-        if (con_is_internal(ws) && ws != workspace_get("__i3_scratch", NULL)) {
+        if (con_is_internal(ws) && ws != workspace_get("__i3_scratch")) {
             DLOG("Workspace is internal but not scratchpad, ignoring _NET_ACTIVE_WINDOW\n");
             return;
         }
diff --git a/src/manage.c b/src/manage.c
index 50fec469..13e67d94 100644
--- a/src/manage.c
+++ b/src/manage.c
@@ -291,7 +291,7 @@ void manage_window(xcb_window_t window, xcb_get_window_attributes_cookie_t cooki
             /* A_TO_WORKSPACE type assignment or fallback from A_TO_WORKSPACE_NUMBER
              * when the target workspace number does not exist yet. */
             if (!assigned_ws) {
-                assigned_ws = workspace_get(assignment->dest.workspace, NULL);
+                assigned_ws = workspace_get(assignment->dest.workspace);
             }
 
             nc = con_descend_tiling_focused(assigned_ws);
@@ -322,7 +322,7 @@ void manage_window(xcb_window_t window, xcb_get_window_attributes_cookie_t cooki
         } else if (startup_ws) {
             /* If it was started on a specific workspace, we want to open it there. */
             DLOG("Using workspace on which this application was started (%s)\n", startup_ws);
-            nc = con_descend_tiling_focused(workspace_get(startup_ws, NULL));
+            nc = con_descend_tiling_focused(workspace_get(startup_ws));
             DLOG("focused on ws %s: %p / %s\n", startup_ws, nc, nc->name);
             if (nc->type == CT_WORKSPACE)
                 nc = tree_open_con(nc, cwindow);
diff --git a/src/randr.c b/src/randr.c
index 1ea3a062..8e000ca6 100644
--- a/src/randr.c
+++ b/src/randr.c
@@ -438,34 +438,29 @@ void init_ws_for_output(Output *output) {
     Con *content = output_get_content(output->con);
     Con *previous_focus = con_get_workspace(focused);
 
-    /* go through all assignments and move the existing workspaces to this output */
-    struct Workspace_Assignment *assignment;
-    TAILQ_FOREACH (assignment, &ws_assignments, ws_assignments) {
-        if (!output_triggers_assignment(output, assignment)) {
+    /* Iterate over all workspaces and check if any of them should be assigned
+     * to this output. */
+    Con *output_con;
+    TAILQ_FOREACH (output_con, &(croot->nodes_head), nodes) {
+        if (con_is_internal(output_con)) {
             continue;
         }
-        Con *workspace = get_existing_workspace_by_name(assignment->name);
-        if (workspace == NULL)
-            continue;
 
-        /* check that this workspace is not already attached (that means the
-         * user configured this assignment twice) */
-        Con *workspace_out = con_get_output(workspace);
-        if (workspace_out == output->con) {
-            LOG("Workspace \"%s\" assigned to output \"%s\", but it is already "
-                "there. Do you have two assignment directives for the same "
-                "workspace in your configuration file?\n",
-                workspace->name, output_primary_name(output));
-            continue;
-        }
+        Con *workspace;
+        TAILQ_FOREACH (workspace, &(output_get_content(output_con)->nodes_head), nodes) {
+            Con *workspace_out = get_assigned_output(workspace->name, workspace->num);
+            if (output->con != workspace_out) {
+                continue;
+            }
 
-        DLOG("Moving workspace \"%s\" from output \"%s\" to \"%s\" due to assignment\n",
-             workspace->name, workspace_out->name, output_primary_name(output));
-        /* Need to copy output's rect since content is not yet rendered. We
-         * can't call render_con here because render_output only proceeds if a
-         * workspace exists. */
-        content->rect = output->con->rect;
-        workspace_move_to_output(workspace, output);
+            DLOG("Moving workspace \"%s\" from output \"%s\" to \"%s\" due to assignment\n",
+                 workspace->name, workspace_out->name, output_primary_name(output));
+            /* Need to copy output's rect since content is not yet rendered. We
+             * can't call render_con here because render_output only proceeds
+             * if a workspace exists. */
+            content->rect = output->con->rect;
+            workspace_move_to_output(workspace, output);
+        }
     }
 
     /* Temporarily set the focused container, might not be initialized yet. */
@@ -485,6 +480,7 @@ void init_ws_for_output(Output *output) {
     }
 
     /* otherwise, we create the first assigned ws for this output */
+    struct Workspace_Assignment *assignment;
     TAILQ_FOREACH (assignment, &ws_assignments, ws_assignments) {
         if (!output_triggers_assignment(output, assignment)) {
             continue;
diff --git a/src/scratchpad.c b/src/scratchpad.c
index e8d70be7..24cde612 100644
--- a/src/scratchpad.c
+++ b/src/scratchpad.c
@@ -32,7 +32,7 @@ void scratchpad_move(Con *con) {
     }
     DLOG("should move con %p to __i3_scratch\n", con);
 
-    Con *__i3_scratch = workspace_get("__i3_scratch", NULL);
+    Con *__i3_scratch = workspace_get("__i3_scratch");
     if (con_get_workspace(con) == __i3_scratch) {
         DLOG("This window is already on __i3_scratch.\n");
         return;
@@ -84,7 +84,7 @@ void scratchpad_move(Con *con) {
  */
 bool scratchpad_show(Con *con) {
     DLOG("should show scratchpad window %p\n", con);
-    Con *__i3_scratch = workspace_get("__i3_scratch", NULL);
+    Con *__i3_scratch = workspace_get("__i3_scratch");
     Con *floating;
 
     /* If this was 'scratchpad show' without criteria, we check if the
@@ -245,7 +245,7 @@ static int _lcm(const int m, const int n) {
  *
  */
 void scratchpad_fix_resolution(void) {
-    Con *__i3_scratch = workspace_get("__i3_scratch", NULL);
+    Con *__i3_scratch = workspace_get("__i3_scratch");
     Con *__i3_output = con_get_output(__i3_scratch);
     DLOG("Current resolution: (%d, %d) %d x %d\n",
          __i3_output->rect.x, __i3_output->rect.y,
diff --git a/src/workspace.c b/src/workspace.c
index f48599c7..f3ddf01c 100644
--- a/src/workspace.c
+++ b/src/workspace.c
@@ -72,21 +72,22 @@ static void _workspace_apply_default_orientation(Con *ws) {
 
 /*
  * Returns the first output that is assigned to a workspace specified by the
- * given name or number or NULL if no such output exists. If there is a
- * workspace with a matching name and another workspace with a matching number,
- * the output assigned to the first one is returned.
- * The order of the 'ws_assignments' queue is respected: if multiple assignments
- * match the specified workspace, the first one is returned.
- * If 'name' is NULL it will be ignored.
- * If 'parsed_num' is -1 it will be ignored.
+ * given name or number. Returns NULL if no such output exists.
+ *
+ * If an assignment matches by number but there is an assignment later that
+ * matches by name, the second one is preferred.
+ * The order of the 'ws_assignments' queue is respected: if multiple
+ * assignments match the criteria, the first one is returned.
+ * 'name' is ignored when NULL, 'parsed_num' is ignored when it is -1.
  *
  */
-static Con *get_assigned_output(const char *name, long parsed_num) {
+Con *get_assigned_output(const char *name, long parsed_num) {
     Con *output = NULL;
     struct Workspace_Assignment *assignment;
     TAILQ_FOREACH (assignment, &ws_assignments, ws_assignments) {
         if (name && strcmp(assignment->name, name) == 0) {
-            DLOG("Found workspace name assignment to output \"%s\"\n", assignment->output);
+            DLOG("Found workspace name=\"%s\" assignment to output \"%s\"\n",
+                 name, assignment->output);
             Output *assigned_by_name = get_output_by_name(assignment->output, true);
             if (assigned_by_name) {
                 /* When the name matches exactly, skip numbered assignments. */
@@ -96,7 +97,8 @@ static Con *get_assigned_output(const char *name, long parsed_num) {
                    parsed_num != -1 &&
                    name_is_digits(assignment->name) &&
                    ws_name_to_number(assignment->name) == parsed_num) {
-            DLOG("Found workspace number assignment to output \"%s\"\n", assignment->output);
+            DLOG("Found workspace number=%ld assignment to output \"%s\"\n",
+                 parsed_num, assignment->output);
             Output *assigned_by_num = get_output_by_name(assignment->output, true);
             if (assigned_by_num) {
                 output = assigned_by_num->con;
@@ -122,52 +124,45 @@ bool output_triggers_assignment(Output *output, struct Workspace_Assignment *ass
  * memory and initializing the data structures correctly).
  *
  */
-Con *workspace_get(const char *num, bool *created) {
+Con *workspace_get(const char *num) {
     Con *workspace = get_existing_workspace_by_name(num);
+    if (workspace) {
+        return workspace;
+    }
 
-    if (workspace == NULL) {
-        LOG("Creating new workspace \"%s\"\n", num);
-
-        /* We set workspace->num to the number if this workspace’s name begins
-         * with a positive number. Otherwise it’s a named ws and num will be
-         * -1. */
-        long parsed_num = ws_name_to_number(num);
+    LOG("Creating new workspace \"%s\"\n", num);
 
-        Con *output = get_assigned_output(num, parsed_num);
-        /* if an assignment is not found, we create this workspace on the current output */
-        if (!output) {
-            output = con_get_output(focused);
-        }
+    /* We set workspace->num to the number if this workspace’s name begins with
+     * a positive number. Otherwise it’s a named ws and num will be 1. */
+    const long parsed_num = ws_name_to_number(num);
 
-        Con *content = output_get_content(output);
-        LOG("got output %p with content %p\n", output, content);
-        /* We need to attach this container after setting its type. con_attach
-         * will handle CT_WORKSPACEs differently */
-        workspace = con_new(NULL, NULL);
-        char *name;
-        sasprintf(&name, "[i3 con] workspace %s", num);
-        x_set_name(workspace, name);
-        free(name);
-        workspace->type = CT_WORKSPACE;
-        FREE(workspace->name);
-        workspace->name = sstrdup(num);
-        workspace->workspace_layout = config.default_layout;
-        workspace->num = parsed_num;
-        LOG("num = %d\n", workspace->num);
-
-        workspace->parent = content;
-        _workspace_apply_default_orientation(workspace);
-
-        con_attach(workspace, content, false);
-
-        ipc_send_workspace_event("init", workspace, NULL);
-        ewmh_update_desktop_properties();
-        if (created != NULL)
-            *created = true;
-    } else if (created != NULL) {
-        *created = false;
+    Con *output = get_assigned_output(num, parsed_num);
+    /* if an assignment is not found, we create this workspace on the current output */
+    if (!output) {
+        output = con_get_output(focused);
     }
 
+    /* No parent because we need to attach this container after setting its
+     * type. con_attach will handle CT_WORKSPACEs differently. */
+    workspace = con_new(NULL, NULL);
+
+    char *name;
+    sasprintf(&name, "[i3 con] workspace %s", num);
+    x_set_name(workspace, name);
+    free(name);
+
+    FREE(workspace->name);
+    workspace->name = sstrdup(num);
+    workspace->workspace_layout = config.default_layout;
+    workspace->num = parsed_num;
+    workspace->type = CT_WORKSPACE;
+
+    con_attach(workspace, output_get_content(output), false);
+    _workspace_apply_default_orientation(workspace);
+
+    ipc_send_workspace_event("init", workspace, NULL);
+    ewmh_update_desktop_properties();
+
     return workspace;
 }
 
@@ -552,9 +547,7 @@ void workspace_show(Con *workspace) {
  *
  */
 void workspace_show_by_name(const char *num) {
-    Con *workspace;
-    workspace = workspace_get(num, NULL);
-    workspace_show(workspace);
+    workspace_show(workspace_get(num));
 }
 
 /*
@@ -821,10 +814,7 @@ Con *workspace_back_and_forth_get(void) {
         return NULL;
     }
 
-    Con *workspace;
-    workspace = workspace_get(previous_workspace_name, NULL);
-
-    return workspace;
+    return workspace_get(previous_workspace_name);
 }
 
 static bool get_urgency_flag(Con *con) {
@@ -1011,7 +1001,7 @@ void workspace_move_to_output(Con *ws, Output *output) {
 
             /* so create the workspace referenced to by this assignment */
             DLOG("Creating workspace from assignment %s.\n", assignment->name);
-            workspace_get(assignment->name, NULL);
+            workspace_get(assignment->name);
             used_assignment = true;
             break;
         }
diff --git a/testcases/t/297-assign-workspace-to-output.t b/testcases/t/297-assign-workspace-to-output.t
index eb8f24b2..4e251e5d 100644
--- a/testcases/t/297-assign-workspace-to-output.t
+++ b/testcases/t/297-assign-workspace-to-output.t
@@ -41,7 +41,7 @@ my $pid = launch_with_config($config);
 
 sub check_output {
     my ($workspace, $output, $msg) = @_;
-    is(get_output_for_workspace($workspace), $output, $msg);
+    is(get_output_for_workspace($workspace), $output, "[$workspace->$output] " . $msg);
 }
 
 check_output('9', '', 'Numbered workspace with a big number that is assigned to output that does not exist is not used');
@@ -82,6 +82,10 @@ workspace 4 output whitespace                    fake-0
 workspace foo output doesnotexist1 doesnotexist2 doesnotexist3
 workspace bar output doesnotexist
 workspace bar output fake-0
+workspace 5 output fake-0
+workspace 5:xxx output fake-1
+workspace 6:xxx output fake-0
+workspace 6 output fake-1
 EOT
 
 $pid = launch_with_config($config);
@@ -90,8 +94,11 @@ do_test('1', 'fake-0', 'Multiple assignments do not override a single one');
 do_test('2', 'fake-3', 'First output out of multiple assignments is used');
 do_test('3', 'fake-0', 'First existing output is used');
 do_test('4', 'fake-0', 'Excessive whitespace is ok');
-do_test('5', 'fake-1', 'Numbered initialization for fake-1');
-do_test('6', 'fake-2', 'Numbered initialization for fake-2');
+do_test('5', 'fake-0', 'Numbered assignment ok');
+do_test('5:xxx', 'fake-1', 'Named assignment overrides number');
+do_test('6', 'fake-1', 'Numbered assignment ok');
+do_test('6:xxx', 'fake-0', 'Named assignment overrides number');
+do_test('7', 'fake-2', 'Numbered initialization for fake-2');
 
 cmd 'focus output fake-0, workspace foo';
 check_output('foo', 'fake-0', 'Workspace with only non-existing assigned outputs opened in current output');
@@ -103,5 +110,14 @@ check_output('bar', 'fake-0', 'Second workspace assignment line ignored');
 cmd 'workspace 2, move workspace to output left';
 check_output('2', 'fake-2', 'Moved assigned workspace up');
 
+# Specific name overrides assignment by number after renaming
+# See #4021
+cmd 'workspace 5, rename workspace to 5:xxx';
+check_output('5:xxx', 'fake-1', 'workspace triggered correct, specific assignment after renaming');
+
+# Same but opposite order
+cmd 'workspace 6, rename workspace to 6:xxx';
+check_output('6:xxx', 'fake-0', 'workspace triggered correct, specific assignment after renaming');
+
 exit_gracefully($pid);
 done_testing;