From 0dd71674dea30076f6cda6029f624386acfe015f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ingo=20B=C3=BCrk?= Date: Sun, 27 Dec 2015 20:58:35 -0500 Subject: [PATCH 1/2] Rename tree_close() to tree_close_internal(). It should be clear for callers of this function that this is an internal function that skips certain validations which might be important. Therefore we make it clear that this is an internal function by renaming it. relates to #1761 --- include/con.h | 2 +- include/data.h | 2 +- include/tree.h | 6 +++--- src/commands.c | 2 +- src/con.c | 6 +++--- src/floating.c | 8 ++++---- src/handlers.c | 4 ++-- src/randr.c | 4 ++-- src/tree.c | 18 +++++++++--------- src/workspace.c | 4 ++-- 10 files changed, 28 insertions(+), 28 deletions(-) diff --git a/include/con.h b/include/con.h index 9ed9508a..655fde7f 100644 --- a/include/con.h +++ b/include/con.h @@ -278,7 +278,7 @@ orientation_t con_orientation(Con *con); /** * Returns the container which will be focused next when the given container - * is not available anymore. Called in tree_close and con_move_to_workspace + * is not available anymore. Called in tree_close_internal and con_move_to_workspace * to properly restore focus. * */ diff --git a/include/data.h b/include/data.h index b122dbfd..9ccc2c2e 100644 --- a/include/data.h +++ b/include/data.h @@ -62,7 +62,7 @@ typedef enum { BS_NORMAL = 0, BS_NONE = 1, BS_PIXEL = 2 } border_style_t; -/** parameter to specify whether tree_close() and x_window_kill() should kill +/** parameter to specify whether tree_close_internal() and x_window_kill() should kill * only this specific window or the whole X11 client */ typedef enum { DONT_KILL_WINDOW = 0, KILL_WINDOW = 1, diff --git a/include/tree.h b/include/tree.h index af3309e9..4a640446 100644 --- a/include/tree.h +++ b/include/tree.h @@ -57,7 +57,7 @@ bool level_down(void); void tree_render(void); /** - * Closes the current container using tree_close(). + * Closes the current container using tree_close_internal(). * */ void tree_close_con(kill_window_t kill_window); @@ -78,11 +78,11 @@ void tree_next(char way, orientation_t orientation); * recursively while deleting a containers children. * * The force_set_focus flag is specified in the case of killing a floating - * window: tree_close() will be invoked for the CT_FLOATINGCON (the parent + * window: tree_close_internal() will be invoked for the CT_FLOATINGCON (the parent * container) and focus should be set there. * */ -bool tree_close(Con *con, kill_window_t kill_window, bool dont_kill_parent, bool force_set_focus); +bool tree_close_internal(Con *con, kill_window_t kill_window, bool dont_kill_parent, bool force_set_focus); /** * Loads tree from ~/.i3/_restart.json (used for in-place restarts). diff --git a/src/commands.c b/src/commands.c index d4b2d51c..eb1834e7 100644 --- a/src/commands.c +++ b/src/commands.c @@ -1281,7 +1281,7 @@ void cmd_kill(I3_CMD, const char *kill_mode_str) { else { TAILQ_FOREACH(current, &owindows, owindows) { DLOG("matching: %p / %s\n", current->con, current->con->name); - tree_close(current->con, kill_mode, false, false); + tree_close_internal(current->con, kill_mode, false, false); } } diff --git a/src/con.c b/src/con.c index 64dbec69..f0353f97 100644 --- a/src/con.c +++ b/src/con.c @@ -1162,7 +1162,7 @@ orientation_t con_orientation(Con *con) { /* * Returns the container which will be focused next when the given container - * is not available anymore. Called in tree_close and con_move_to_workspace + * is not available anymore. Called in tree_close_internal and con_move_to_workspace * to properly restore focus. * */ @@ -1678,7 +1678,7 @@ static void con_on_remove_child(Con *con) { if (TAILQ_EMPTY(&(con->focus_head)) && !workspace_is_visible(con)) { LOG("Closing old workspace (%p / %s), it is empty\n", con, con->name); yajl_gen gen = ipc_marshal_workspace_event("empty", con, NULL); - tree_close(con, DONT_KILL_WINDOW, false, false); + tree_close_internal(con, DONT_KILL_WINDOW, false, false); const unsigned char *payload; ylength length; @@ -1699,7 +1699,7 @@ static void con_on_remove_child(Con *con) { int children = con_num_children(con); if (children == 0) { DLOG("Container empty, closing\n"); - tree_close(con, DONT_KILL_WINDOW, false, false); + tree_close_internal(con, DONT_KILL_WINDOW, false, false); return; } } diff --git a/src/floating.c b/src/floating.c index 77bc9e17..3aa42364 100644 --- a/src/floating.c +++ b/src/floating.c @@ -162,7 +162,7 @@ void floating_enable(Con *con, bool automatic) { } /* 1: detach the container from its parent */ - /* TODO: refactor this with tree_close() */ + /* TODO: refactor this with tree_close_internal() */ TAILQ_REMOVE(&(con->parent->nodes_head), con, nodes); TAILQ_REMOVE(&(con->parent->focus_head), con, focused); @@ -180,7 +180,7 @@ void floating_enable(Con *con, bool automatic) { nc->layout = L_SPLITH; /* We insert nc already, even though its rect is not yet calculated. This * is necessary because otherwise the workspace might be empty (and get - * closed in tree_close()) even though it’s not. */ + * closed in tree_close_internal()) even though it’s not. */ TAILQ_INSERT_TAIL(&(ws->floating_head), nc, floating_windows); TAILQ_INSERT_TAIL(&(ws->focus_head), nc, focused); @@ -188,7 +188,7 @@ void floating_enable(Con *con, bool automatic) { if ((con->parent->type == CT_CON || con->parent->type == CT_FLOATING_CON) && con_num_children(con->parent) == 0) { DLOG("Old container empty after setting this child to floating, closing\n"); - tree_close(con->parent, DONT_KILL_WINDOW, false, false); + tree_close_internal(con->parent, DONT_KILL_WINDOW, false, false); } char *name; @@ -333,7 +333,7 @@ void floating_disable(Con *con, bool automatic) { /* 2: kill parent container */ TAILQ_REMOVE(&(con->parent->parent->floating_head), con->parent, floating_windows); TAILQ_REMOVE(&(con->parent->parent->focus_head), con->parent, focused); - tree_close(con->parent, DONT_KILL_WINDOW, true, false); + tree_close_internal(con->parent, DONT_KILL_WINDOW, true, false); /* 3: re-attach to the parent of the currently focused con on the workspace * this floating con was on */ diff --git a/src/handlers.c b/src/handlers.c index 6f08d25c..6cbc54f2 100644 --- a/src/handlers.c +++ b/src/handlers.c @@ -503,7 +503,7 @@ static void handle_unmap_notify_event(xcb_unmap_notify_event_t *event) { goto ignore_end; } - tree_close(con, DONT_KILL_WINDOW, false, false); + tree_close_internal(con, DONT_KILL_WINDOW, false, false); tree_render(); ignore_end: @@ -878,7 +878,7 @@ static void handle_client_message(xcb_client_message_event_t *event) { if (event->data.data32[0]) last_timestamp = event->data.data32[0]; - tree_close(con, KILL_WINDOW, false, false); + tree_close_internal(con, KILL_WINDOW, false, false); tree_render(); } else { DLOG("Couldn't find con for _NET_CLOSE_WINDOW request. (window = %d)\n", event->window); diff --git a/src/randr.c b/src/randr.c index 81a33e62..6753f8a6 100644 --- a/src/randr.c +++ b/src/randr.c @@ -741,7 +741,7 @@ void randr_query_outputs(void) { if (current != next && TAILQ_EMPTY(&(current->focus_head))) { /* the workspace is empty and not focused, get rid of it */ DLOG("Getting rid of current = %p / %s (empty, unfocused)\n", current, current->name); - tree_close(current, DONT_KILL_WINDOW, false, false); + tree_close_internal(current, DONT_KILL_WINDOW, false, false); continue; } DLOG("Detaching current = %p / %s\n", current, current->name); @@ -783,7 +783,7 @@ void randr_query_outputs(void) { } DLOG("destroying disappearing con %p\n", output->con); - tree_close(output->con, DONT_KILL_WINDOW, true, false); + tree_close_internal(output->con, DONT_KILL_WINDOW, true, false); DLOG("Done. Should be fine now\n"); output->con = NULL; } diff --git a/src/tree.c b/src/tree.c index 1d06d874..e296a981 100644 --- a/src/tree.c +++ b/src/tree.c @@ -185,11 +185,11 @@ static bool _is_con_mapped(Con *con) { * recursively while deleting a containers children. * * The force_set_focus flag is specified in the case of killing a floating - * window: tree_close() will be invoked for the CT_FLOATINGCON (the parent + * window: tree_close_internal() will be invoked for the CT_FLOATINGCON (the parent * container) and focus should be set there. * */ -bool tree_close(Con *con, kill_window_t kill_window, bool dont_kill_parent, bool force_set_focus) { +bool tree_close_internal(Con *con, kill_window_t kill_window, bool dont_kill_parent, bool force_set_focus) { bool was_mapped = con->mapped; Con *parent = con->parent; @@ -219,7 +219,7 @@ bool tree_close(Con *con, kill_window_t kill_window, bool dont_kill_parent, bool for (child = TAILQ_FIRST(&(con->nodes_head)); child;) { nextchild = TAILQ_NEXT(child, nodes); DLOG("killing child=%p\n", child); - if (!tree_close(child, kill_window, true, false)) + if (!tree_close_internal(child, kill_window, true, false)) abort_kill = true; child = nextchild; } @@ -310,7 +310,7 @@ bool tree_close(Con *con, kill_window_t kill_window, bool dont_kill_parent, bool * underlying container, see ticket #660. * * Rendering has to be avoided when dont_kill_parent is set (when - * tree_close calls itself recursively) because the tree is in a + * tree_close_internal calls itself recursively) because the tree is in a * non-renderable state during that time. */ if (!dont_kill_parent) tree_render(); @@ -320,7 +320,7 @@ bool tree_close(Con *con, kill_window_t kill_window, bool dont_kill_parent, bool if (con_is_floating(con)) { DLOG("Container was floating, killing floating container\n"); - tree_close(parent, DONT_KILL_WINDOW, false, (con == focused)); + tree_close_internal(parent, DONT_KILL_WINDOW, false, (con == focused)); DLOG("parent container killed\n"); } @@ -363,7 +363,7 @@ bool tree_close(Con *con, kill_window_t kill_window, bool dont_kill_parent, bool } /* - * Closes the current container using tree_close(). + * Closes the current container using tree_close_internal(). * */ void tree_close_con(kill_window_t kill_window) { @@ -379,7 +379,7 @@ void tree_close_con(kill_window_t kill_window) { for (child = TAILQ_FIRST(&(focused->focus_head)); child;) { nextchild = TAILQ_NEXT(child, focused); DLOG("killing child=%p\n", child); - tree_close(child, kill_window, false, false); + tree_close_internal(child, kill_window, false, false); child = nextchild; } @@ -387,7 +387,7 @@ void tree_close_con(kill_window_t kill_window) { } /* Kill con */ - tree_close(focused, kill_window, false, false); + tree_close_internal(focused, kill_window, false, false); } /* @@ -773,7 +773,7 @@ void tree_flatten(Con *con) { /* 4: close the redundant cons */ DLOG("closing redundant cons\n"); - tree_close(con, DONT_KILL_WINDOW, true, false); + tree_close_internal(con, DONT_KILL_WINDOW, true, false); /* Well, we got to abort the recursion here because we destroyed the * container. However, if tree_flatten() is called sufficiently often, diff --git a/src/workspace.c b/src/workspace.c index dc0a596d..923bfc83 100644 --- a/src/workspace.c +++ b/src/workspace.c @@ -442,7 +442,7 @@ static void _workspace_show(Con *workspace) { DLOG("old = %p / %s\n", old, (old ? old->name : "(null)")); /* Close old workspace if necessary. This must be done *after* doing - * urgency handling, because tree_close() will do a con_focus() on the next + * urgency handling, because tree_close_internal() will do a con_focus() on the next * client, which will clear the urgency flag too early. Also, there is no * way for con_focus() to know about when to clear urgency immediately and * when to defer it. */ @@ -451,7 +451,7 @@ static void _workspace_show(Con *workspace) { if (!workspace_is_visible(old)) { LOG("Closing old workspace (%p / %s), it is empty\n", old, old->name); yajl_gen gen = ipc_marshal_workspace_event("empty", old, NULL); - tree_close(old, DONT_KILL_WINDOW, false, false); + tree_close_internal(old, DONT_KILL_WINDOW, false, false); const unsigned char *payload; ylength length; From 19c273a2adde5e6248566b7bd13b5cee80f84b34 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ingo=20B=C3=BCrk?= Date: Sun, 27 Dec 2015 22:11:51 -0500 Subject: [PATCH 2/2] Validate matched containers for "kill" command correctly. We now execute the validations when "kill" is executed even if match criteria are used. This prevents users from killing workspace containers, which instead kills all clients (as before when not using criteria). fixes #1761 --- include/con.h | 6 ++++ include/tree.h | 6 ---- src/commands.c | 14 +++------ src/con.c | 30 +++++++++++++++++++ src/tree.c | 28 ----------------- .../261-match-con_id-con_mark-combinations.t | 4 +++ 6 files changed, 44 insertions(+), 44 deletions(-) diff --git a/include/con.h b/include/con.h index 655fde7f..db5fcfbf 100644 --- a/include/con.h +++ b/include/con.h @@ -30,6 +30,12 @@ Con *con_new(Con *parent, i3Window *window); */ void con_focus(Con *con); +/** + * Closes the given container. + * + */ +void con_close(Con *con, kill_window_t kill_window); + /** * Returns true when this node is a leaf node (has no children) * diff --git a/include/tree.h b/include/tree.h index 4a640446..c64c173d 100644 --- a/include/tree.h +++ b/include/tree.h @@ -56,12 +56,6 @@ bool level_down(void); */ void tree_render(void); -/** - * Closes the current container using tree_close_internal(). - * - */ -void tree_close_con(kill_window_t kill_window); - /** * Changes focus in the given way (next/previous) and given orientation * (horizontal/vertical). diff --git a/src/commands.c b/src/commands.c index eb1834e7..579b5e0e 100644 --- a/src/commands.c +++ b/src/commands.c @@ -1258,7 +1258,6 @@ void cmd_split(I3_CMD, const char *direction) { void cmd_kill(I3_CMD, const char *kill_mode_str) { if (kill_mode_str == NULL) kill_mode_str = "window"; - owindow *current; DLOG("kill_mode=%s\n", kill_mode_str); @@ -1273,16 +1272,11 @@ void cmd_kill(I3_CMD, const char *kill_mode_str) { return; } - HANDLE_INVALID_MATCH; + HANDLE_EMPTY_MATCH; - /* check if the match is empty, not if the result is empty */ - if (match_is_empty(current_match)) - tree_close_con(kill_mode); - else { - TAILQ_FOREACH(current, &owindows, owindows) { - DLOG("matching: %p / %s\n", current->con, current->con->name); - tree_close_internal(current->con, kill_mode, false, false); - } + owindow *current; + TAILQ_FOREACH(current, &owindows, owindows) { + con_close(current->con, kill_mode); } cmd_output->needs_tree_render = true; diff --git a/src/con.c b/src/con.c index f0353f97..591419e4 100644 --- a/src/con.c +++ b/src/con.c @@ -220,6 +220,36 @@ void con_focus(Con *con) { } } +/* + * Closes the given container. + * + */ +void con_close(Con *con, kill_window_t kill_window) { + assert(con != NULL); + DLOG("Closing con = %p.\n", con); + + /* We never close output or root containers. */ + if (con->type == CT_OUTPUT || con->type == CT_ROOT) { + DLOG("con = %p is of type %d, not closing anything.\n", con, con->type); + return; + } + + if (con->type == CT_WORKSPACE) { + DLOG("con = %p is a workspace, closing all children instead.\n", con); + Con *child, *nextchild; + for (child = TAILQ_FIRST(&(con->focus_head)); child;) { + nextchild = TAILQ_NEXT(child, focused); + DLOG("killing child = %p.\n", child); + tree_close_internal(child, kill_window, false, false); + child = nextchild; + } + + return; + } + + tree_close_internal(con, kill_window, false, false); +} + /* * Returns true when this node is a leaf node (has no children) * diff --git a/src/tree.c b/src/tree.c index e296a981..5642c5c7 100644 --- a/src/tree.c +++ b/src/tree.c @@ -362,34 +362,6 @@ bool tree_close_internal(Con *con, kill_window_t kill_window, bool dont_kill_par return true; } -/* - * Closes the current container using tree_close_internal(). - * - */ -void tree_close_con(kill_window_t kill_window) { - assert(focused != NULL); - - /* There *should* be no possibility to focus outputs / root container */ - assert(focused->type != CT_OUTPUT); - assert(focused->type != CT_ROOT); - - if (focused->type == CT_WORKSPACE) { - DLOG("Workspaces cannot be close, closing all children instead\n"); - Con *child, *nextchild; - for (child = TAILQ_FIRST(&(focused->focus_head)); child;) { - nextchild = TAILQ_NEXT(child, focused); - DLOG("killing child=%p\n", child); - tree_close_internal(child, kill_window, false, false); - child = nextchild; - } - - return; - } - - /* Kill con */ - tree_close_internal(focused, kill_window, false, false); -} - /* * Splits (horizontally or vertically) the given container by creating a new * container which contains the old one and the future ones. diff --git a/testcases/t/261-match-con_id-con_mark-combinations.t b/testcases/t/261-match-con_id-con_mark-combinations.t index b255558e..61ff1ff0 100644 --- a/testcases/t/261-match-con_id-con_mark-combinations.t +++ b/testcases/t/261-match-con_id-con_mark-combinations.t @@ -27,9 +27,11 @@ $ws = fresh_workspace; open_window(wm_class => 'matchme'); cmd '[con_id=__focused__ class=doesnotmatch] kill'; +sync_with_i3; is(@{get_ws($ws)->{nodes}}, 1, 'window was not killed'); cmd '[con_id=__focused__ class=matchme] kill'; +sync_with_i3; is(@{get_ws($ws)->{nodes}}, 0, 'window was killed'); ############################################################################### @@ -41,9 +43,11 @@ open_window(wm_class => 'matchme'); cmd 'mark marked'; cmd '[con_mark=marked class=doesnotmatch] kill'; +sync_with_i3; is(@{get_ws($ws)->{nodes}}, 1, 'window was not killed'); cmd '[con_mark=marked class=matchme] kill'; +sync_with_i3; is(@{get_ws($ws)->{nodes}}, 0, 'window was killed'); ###############################################################################