Merge pull request #3658 from orestisf1993/workspace_move_to_output

Workspace move to output

include/workspace.h

@@ -218,7 +218,6 @@ Con *workspace_encapsulate(Con *ws);
 
 /**
  * Move the given workspace to the specified output.
- * This returns true if and only if moving the workspace was successful.
  *
  */
-bool workspace_move_to_output(Con *ws, Output *output);
+void workspace_move_to_output(Con *ws, Output *output);

src/commands.c

@@ -1114,22 +1114,13 @@ void cmd_move_workspace_to_output(I3_CMD, const char *name) {
         }
 
         Output *current_output = get_output_for_con(ws);
-        if (current_output == NULL) {
-            yerror("Cannot get current output. This is a bug in i3.");
-            return;
-        }
-
         Output *target_output = get_output_from_string(current_output, name);
         if (!target_output) {
             yerror("Could not get output from string \"%s\"", name);
             return;
         }
 
-        bool success = workspace_move_to_output(ws, target_output);
+        workspace_move_to_output(ws, target_output);
-        if (!success) {
-            yerror("Failed to move workspace to output.");
-            return;
-        }
     }
 
     cmd_output->needs_tree_render = true;
@@ -1635,24 +1626,18 @@ void cmd_open(I3_CMD) {
  *
  */
 void cmd_focus_output(I3_CMD, const char *name) {
-    owindow *current;
-
-    DLOG("name = %s\n", name);
-
     HANDLE_EMPTY_MATCH;
 
-    /* get the output */
+    if (TAILQ_EMPTY(&owindows)) {
-    Output *current_output = NULL;
+        ysuccess(true);
-    Output *output;
+        return;
+    }
 
-    TAILQ_FOREACH(current, &owindows, owindows)
+    Output *current_output = get_output_for_con(TAILQ_FIRST(&owindows)->con);
-    current_output = get_output_for_con(current->con);
+    Output *output = get_output_from_string(current_output, name);
-    assert(current_output != NULL);
-
-    output = get_output_from_string(current_output, name);
 
     if (!output) {
-        yerror("No such output found.");
+        yerror("Output %s not found.", name);
         return;
     }
 
@@ -1667,7 +1652,6 @@ void cmd_focus_output(I3_CMD, const char *name) {
     workspace_show(ws);
 
     cmd_output->needs_tree_render = true;
-    // XXX: default reply for now, make this a better reply
     ysuccess(true);
 }
 

src/con.c

@@ -1400,8 +1400,6 @@ void con_move_to_output(Con *con, Output *output, bool fix_coordinates) {
  */
 bool con_move_to_output_name(Con *con, const char *name, bool fix_coordinates) {
     Output *current_output = get_output_for_con(con);
-    assert(current_output != NULL);
-
     Output *output = get_output_from_string(current_output, name);
     if (output == NULL) {
         ELOG("Could not find output \"%s\"\n", name);

src/output.c

@@ -54,16 +54,8 @@ char *output_primary_name(Output *output) {
 
 Output *get_output_for_con(Con *con) {
     Con *output_con = con_get_output(con);
-    if (output_con == NULL) {
-        ELOG("Could not get the output container for con = %p.\n", con);
-        return NULL;
-    }
-
     Output *output = get_output_by_name(output_con->name, true);
-    if (output == NULL) {
+    assert(output != NULL);
-        ELOG("Could not get output from name \"%s\".\n", output_con->name);
-        return NULL;
-    }
 
     return output;
 }

src/workspace.c

@@ -961,19 +961,18 @@ Con *workspace_encapsulate(Con *ws) {
 
 /*
  * Move the given workspace to the specified output.
- * This returns true if and only if moving the workspace was successful.
  */
-bool workspace_move_to_output(Con *ws, Output *output) {
+void workspace_move_to_output(Con *ws, Output *output) {
-    LOG("Trying to move workspace %p / %s to output %p / \"%s\".\n", ws, ws->name, output, output_primary_name(output));
+    DLOG("Moving workspace %p / %s to output %p / \"%s\".\n", ws, ws->name, output, output_primary_name(output));
 
     Output *current_output = get_output_for_con(ws);
-    if (current_output == NULL) {
-        ELOG("Cannot get current output. This is a bug in i3.\n");
-        return false;
-    }
-
     Con *content = output_get_content(output->con);
-    LOG("got output %p with content %p\n", output, content);
+    DLOG("got output %p with content %p\n", output, content);
+
+    if (ws->parent == content) {
+        DLOG("Nothing to do, workspace already there\n");
+        return;
+    }
 
     Con *previously_visible_ws = TAILQ_FIRST(&(content->focus_head));
     if (previously_visible_ws) {
@@ -984,7 +983,7 @@ bool workspace_move_to_output(Con *ws, Output *output) {
 
     bool workspace_was_visible = workspace_is_visible(ws);
     if (con_num_children(ws->parent) == 1) {
-        LOG("Creating a new workspace to replace \"%s\" (last on its output).\n", ws->name);
+        DLOG("Creating a new workspace to replace \"%s\" (last on its output).\n", ws->name);
 
         /* check if we can find a workspace assigned to this output */
         bool used_assignment = false;
@@ -999,7 +998,7 @@ bool workspace_move_to_output(Con *ws, Output *output) {
             }
 
             /* so create the workspace referenced to by this assignment */
-            LOG("Creating workspace from assignment %s.\n", assignment->name);
+            DLOG("Creating workspace from assignment %s.\n", assignment->name);
             workspace_get(assignment->name, NULL);
             used_assignment = true;
             break;
@@ -1008,8 +1007,9 @@ bool workspace_move_to_output(Con *ws, Output *output) {
         /* if we couldn't create the workspace using an assignment, create it on
          * the output. Workspace init IPC events are sent either by
          * workspace_get or create_workspace_on_output. */
-        if (!used_assignment)
+        if (!used_assignment) {
             create_workspace_on_output(current_output, ws->parent);
+        }
     }
     DLOG("Detaching\n");
 
@@ -1017,18 +1017,19 @@ bool workspace_move_to_output(Con *ws, Output *output) {
     Con *old_content = ws->parent;
     con_detach(ws);
     if (workspace_was_visible) {
-        /* The workspace which we just detached was visible, so focus
+        /* The workspace which we just detached was visible, so focus the next
-         * the next one in the focus-stack. */
+         * one in the focus-stack. */
         Con *focus_ws = TAILQ_FIRST(&(old_content->focus_head));
-        LOG("workspace was visible, focusing %p / %s now\n", focus_ws, focus_ws->name);
+        DLOG("workspace was visible, focusing %p / %s now\n", focus_ws, focus_ws->name);
         workspace_show(focus_ws);
     }
     con_attach(ws, content, false);
 
     /* fix the coordinates of the floating containers */
     Con *floating_con;
-    TAILQ_FOREACH(floating_con, &(ws->floating_head), floating_windows)
+    TAILQ_FOREACH(floating_con, &(ws->floating_head), floating_windows) {
-    floating_fix_coordinates(floating_con, &(old_content->rect), &(content->rect));
+        floating_fix_coordinates(floating_con, &(old_content->rect), &(content->rect));
+    }
 
     ipc_send_workspace_event("move", ws, NULL);
     if (workspace_was_visible) {
@@ -1037,24 +1038,23 @@ bool workspace_move_to_output(Con *ws, Output *output) {
     }
 
     if (!previously_visible_ws) {
-        return true;
+        return;
     }
 
-    /* NB: We cannot simply work with previously_visible_ws since it might
+    /* NB: We cannot simply work with previously_visible_ws since it might have
-     * have been cleaned up by workspace_show() already, depending on the
+     * been cleaned up by workspace_show() already, depending on the focus
-     * focus order/number of other workspaces on the output.
+     * order/number of other workspaces on the output. Instead, we loop through
-     * Instead, we loop through the available workspaces and only work with
+     * the available workspaces and only work with previously_visible_ws if we
-     * previously_visible_ws if we still find it. */
+     * still find it. */
     TAILQ_FOREACH(ws, &(content->nodes_head), nodes) {
-        if (ws != previously_visible_ws)
+        if (ws != previously_visible_ws) {
             continue;
+        }
 
         /* Call the on_remove_child callback of the workspace which previously
-         * was visible on the destination output. Since it is no longer
+         * was visible on the destination output. Since it is no longer visible,
-         * visible, it might need to get cleaned up. */
+         * it might need to get cleaned up. */
         CALL(previously_visible_ws, on_remove_child);
         break;
     }
-
-    return true;
 }

testcases/t/502-focus-output.t

@@ -86,4 +86,33 @@ is(focused_output, 'fake-1', 'focus on second output');
 cmd 'focus output fake-0';
 is(focused_output, 'fake-0', 'focus on first output');
 
+################################################################################
+# use 'focus output' with command criteria and verify that i3 does not crash
+# when they don't match any window
+################################################################################
+
+is(focused_output, 'fake-0', 'focus on first output');
+
+cmd '[con_mark=doesnotexist] focus output right';
+does_i3_live;
+is(focused_output, 'fake-0', 'focus remained on first output');
+
+################################################################################
+# use 'focus output' with command criteria and verify that focus gets changed
+# appropriately
+################################################################################
+
+is(focused_output, 'fake-0', 'focus on first output');
+
+my $window = open_window;
+
+cmd 'focus output right';
+is(focused_output, 'fake-1', 'focus on second output');
+
+cmd '[id= . ' . $window->id . '] focus output right';
+is(focused_output, 'fake-1', 'focus on second output after command with criteria');
+
+cmd 'focus output right';
+is(focused_output, 'fake-0', 'focus on first output after command without criteria');
+
 done_testing;