From d0ab51db85f0176d51bafe4d03f9c7c1621cf53c Mon Sep 17 00:00:00 2001 From: Orestis Floros Date: Thu, 21 Mar 2019 22:48:39 +0200 Subject: [PATCH 1/4] workspace_move_to_output: Make stylistic changes --- src/workspace.c | 39 +++++++++++++++++++++------------------ 1 file changed, 21 insertions(+), 18 deletions(-) diff --git a/src/workspace.c b/src/workspace.c index 5c2c48fa..b36885ee 100644 --- a/src/workspace.c +++ b/src/workspace.c @@ -964,7 +964,7 @@ Con *workspace_encapsulate(Con *ws) { * This returns true if and only if moving the workspace was successful. */ bool 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) { @@ -973,7 +973,7 @@ bool workspace_move_to_output(Con *ws, Output *output) { } 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); Con *previously_visible_ws = TAILQ_FIRST(&(content->focus_head)); if (previously_visible_ws) { @@ -984,7 +984,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 +999,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 +1008,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 +1018,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 next one in the focus-stack. */ + /* The workspace which we just detached was visible, so focus the next + * 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) - floating_fix_coordinates(floating_con, &(old_content->rect), &(content->rect)); + TAILQ_FOREACH(floating_con, &(ws->floating_head), floating_windows) { + floating_fix_coordinates(floating_con, &(old_content->rect), &(content->rect)); + } ipc_send_workspace_event("move", ws, NULL); if (workspace_was_visible) { @@ -1040,18 +1042,19 @@ bool workspace_move_to_output(Con *ws, Output *output) { return true; } - /* NB: We cannot simply work with previously_visible_ws since it might - * have been cleaned up by workspace_show() already, depending on the - * focus order/number of other workspaces on the output. - * Instead, we loop through the available workspaces and only work with - * previously_visible_ws if we still find it. */ + /* NB: We cannot simply work with previously_visible_ws since it might have + * been cleaned up by workspace_show() already, depending on the focus + * order/number of other workspaces on the output. Instead, we loop through + * the available workspaces and only work with previously_visible_ws if we + * 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 - * visible, it might need to get cleaned up. */ + * was visible on the destination output. Since it is no longer visible, + * it might need to get cleaned up. */ CALL(previously_visible_ws, on_remove_child); break; } From 351d891f4cfdc97189898025ce4eed3eec31afab Mon Sep 17 00:00:00 2001 From: Orestis Floros Date: Thu, 21 Mar 2019 23:51:57 +0200 Subject: [PATCH 2/4] get_output_for_con: Assert result != NULL - The result from con_get_output was always not NULL because con_get_output asserts so - get_output_by_name should always be able to get an output from the corresponding container - workspace_move_to_output doesn't return bool anymore since it can't fail --- include/workspace.h | 3 +-- src/commands.c | 11 +---------- src/con.c | 2 -- src/output.c | 10 +--------- src/workspace.c | 12 ++---------- 5 files changed, 5 insertions(+), 33 deletions(-) diff --git a/include/workspace.h b/include/workspace.h index db544ce8..69974a2e 100644 --- a/include/workspace.h +++ b/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); diff --git a/src/commands.c b/src/commands.c index 778b4a1a..b7fbce90 100644 --- a/src/commands.c +++ b/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); - if (!success) { - yerror("Failed to move workspace to output."); - return; - } + workspace_move_to_output(ws, target_output); } cmd_output->needs_tree_render = true; diff --git a/src/con.c b/src/con.c index f904bfe8..a88909e1 100644 --- a/src/con.c +++ b/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); diff --git a/src/output.c b/src/output.c index ebba5c77..3d931b1b 100644 --- a/src/output.c +++ b/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) { - ELOG("Could not get output from name \"%s\".\n", output_con->name); - return NULL; - } + assert(output != NULL); return output; } diff --git a/src/workspace.c b/src/workspace.c index b36885ee..0aeecb45 100644 --- a/src/workspace.c +++ b/src/workspace.c @@ -961,17 +961,11 @@ 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) { 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); DLOG("got output %p with content %p\n", output, content); @@ -1039,7 +1033,7 @@ 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 have @@ -1058,6 +1052,4 @@ bool workspace_move_to_output(Con *ws, Output *output) { CALL(previously_visible_ws, on_remove_child); break; } - - return true; } From 7fc3bf660e58ad11908be60d7df048f920e74689 Mon Sep 17 00:00:00 2001 From: Orestis Floros Date: Thu, 21 Mar 2019 23:57:24 +0200 Subject: [PATCH 3/4] cmd_focus_output: Avoid assertion crash Happened when the command criteria didn't match any windows. For example: `[con_mark=doesnotexist] focus output left`. --- src/commands.c | 21 +++++++-------------- testcases/t/502-focus-output.t | 29 +++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 14 deletions(-) diff --git a/src/commands.c b/src/commands.c index b7fbce90..3cf5a57c 100644 --- a/src/commands.c +++ b/src/commands.c @@ -1626,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 */ - Output *current_output = NULL; - Output *output; + if (TAILQ_EMPTY(&owindows)) { + ysuccess(true); + return; + } - TAILQ_FOREACH(current, &owindows, owindows) - current_output = get_output_for_con(current->con); - assert(current_output != NULL); - - output = get_output_from_string(current_output, name); + Output *current_output = get_output_for_con(TAILQ_FIRST(&owindows)->con); + Output *output = get_output_from_string(current_output, name); if (!output) { - yerror("No such output found."); + yerror("Output %s not found.", name); return; } @@ -1658,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); } diff --git a/testcases/t/502-focus-output.t b/testcases/t/502-focus-output.t index 01d8db33..118aba16 100644 --- a/testcases/t/502-focus-output.t +++ b/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; From d4e4cbfd2504fee3a2570bac5b1c1104cb5f2e2b Mon Sep 17 00:00:00 2001 From: Orestis Floros Date: Fri, 22 Mar 2019 00:55:30 +0200 Subject: [PATCH 4/4] workspace_move_to_output: Avoid operations when workspace already at destination Closes #3635. Probably the bug can still happen when a tree_close_internal happens inside a workspace_show but modifying the code to avoid them seems to not be worth it. --- src/workspace.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/workspace.c b/src/workspace.c index 0aeecb45..3cf74754 100644 --- a/src/workspace.c +++ b/src/workspace.c @@ -969,6 +969,11 @@ void workspace_move_to_output(Con *ws, Output *output) { Con *content = output_get_content(output->con); 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) { DLOG("Previously visible workspace = %p / %s\n", previously_visible_ws, previously_visible_ws->name);