From 8e1687a317bdde3217dba895bc8f05975690896c Mon Sep 17 00:00:00 2001 From: Orestis Floros Date: Wed, 6 Sep 2017 05:44:09 +0300 Subject: [PATCH] Rewrite con_swap to work only with queue operations Benefits are that we don't open a fake container and don't call many complicated functions that can lead to redraws (x_push_changes calls) as discussed in #2954. Fixes #2810: Windows exchange floating mode & window rects. Swap will still not work with CT_FLOATING_CONs but this doesn't make much sense. Fixes #3280: The behaviour is not very user friendly but swap behaves exactly as it should. The rest is a tree_flatten issue. Attached pictures in #2954. --- docs/userguide | 3 +- src/con.c | 194 +++++++++++------------------- testcases/t/291-swap.t | 219 +++++++++++++++++++++++++++++++++- testcases/t/294-focus-order.t | 39 ++++++ 4 files changed, 324 insertions(+), 131 deletions(-) diff --git a/docs/userguide b/docs/userguide index da5d9873..f92b4bea 100644 --- a/docs/userguide +++ b/docs/userguide @@ -2092,8 +2092,7 @@ using one of the following methods: +mark+:: A container with the specified mark, see <>. Note that swapping does not work with all containers. Most notably, swapping -floating containers or containers that have a parent-child relationship to one -another does not work. +containers that have a parent-child relationship to one another does not work. *Syntax*: ---------------------------------------- diff --git a/src/con.c b/src/con.c index d50c29be..436ce776 100644 --- a/src/con.c +++ b/src/con.c @@ -2335,11 +2335,6 @@ bool con_swap(Con *first, Con *second) { return false; } - if (con_is_floating(first) || con_is_floating(second)) { - ELOG("Floating windows cannot be swapped.\n"); - return false; - } - if (first == second) { DLOG("Swapping container %p with itself, nothing to do.\n", first); return false; @@ -2350,132 +2345,80 @@ bool con_swap(Con *first, Con *second) { return false; } - Con *old_focus = focused; - - Con *first_ws = con_get_workspace(first); - Con *second_ws = con_get_workspace(second); - Con *current_ws = con_get_workspace(old_focus); - const bool focused_within_first = (first == old_focus || con_has_parent(old_focus, first)); - const bool focused_within_second = (second == old_focus || con_has_parent(old_focus, second)); - fullscreen_mode_t first_fullscreen_mode = first->fullscreen_mode; - fullscreen_mode_t second_fullscreen_mode = second->fullscreen_mode; - - if (first_fullscreen_mode != CF_NONE) { - con_disable_fullscreen(first); - } - if (second_fullscreen_mode != CF_NONE) { - con_disable_fullscreen(second); - } - - double first_percent = first->percent; - double second_percent = second->percent; + Con *ws1 = con_get_workspace(first); + Con *ws2 = con_get_workspace(second); + Con *restore_focus = NULL; + if (ws1 == ws2 && ws1 == con_get_workspace(focused)) { + /* Preserve focus in the current workspace. */ + restore_focus = focused; + } else if (first == focused || con_has_parent(focused, first)) { + restore_focus = second; + } else if (second == focused || con_has_parent(focused, second)) { + restore_focus = first; + } + +#define SWAP_CONS_IN_TREE(headname, field) \ + do { \ + struct headname *head1 = &(first->parent->headname); \ + struct headname *head2 = &(second->parent->headname); \ + Con *first_prev = TAILQ_PREV(first, headname, field); \ + Con *second_prev = TAILQ_PREV(second, headname, field); \ + if (second_prev == first) { \ + TAILQ_SWAP(first, second, head1, field); \ + } else if (first_prev == second) { \ + TAILQ_SWAP(second, first, head1, field); \ + } else { \ + TAILQ_REMOVE(head1, first, field); \ + TAILQ_REMOVE(head2, second, field); \ + if (second_prev == NULL) { \ + TAILQ_INSERT_HEAD(head2, first, field); \ + } else { \ + TAILQ_INSERT_AFTER(head2, second_prev, first, field); \ + } \ + if (first_prev == NULL) { \ + TAILQ_INSERT_HEAD(head1, second, field); \ + } else { \ + TAILQ_INSERT_AFTER(head1, first_prev, second, field); \ + } \ + } \ + } while (0) + + SWAP_CONS_IN_TREE(nodes_head, nodes); + SWAP_CONS_IN_TREE(focus_head, focused); + SWAP(first->parent, second->parent, Con *); + + /* Floating nodes are children of CT_FLOATING_CONs, they are listed in + * nodes_head and focus_head like all other containers. Thus, we don't need + * to do anything special other than swapping the floating status and the + * relevant rects. */ + SWAP(first->floating, second->floating, int); + SWAP(first->rect, second->rect, Rect); + SWAP(first->window_rect, second->window_rect, Rect); - /* De- and reattaching the containers will insert them at the tail of the - * focus_heads. We will need to fix this. But we need to make sure first - * and second don't get in each other's way if they share the same parent, - * so we select the closest previous focus_head that isn't involved. */ - Con *first_prev_focus_head = first; - while (first_prev_focus_head == first || first_prev_focus_head == second) { - first_prev_focus_head = TAILQ_PREV(first_prev_focus_head, focus_head, focused); - } - - Con *second_prev_focus_head = second; - while (second_prev_focus_head == second || second_prev_focus_head == first) { - second_prev_focus_head = TAILQ_PREV(second_prev_focus_head, focus_head, focused); - } - - /* We use a fake container to mark the spot of where the second container needs to go. */ - Con *fake = con_new(NULL, NULL); - fake->layout = L_SPLITH; - _con_attach(fake, first->parent, first, true); - - bool result = true; - /* Swap the containers. We set the ignore_focus flag here because after the - * container is attached, the focus order is not yet correct and would - * result in wrong windows being focused. */ - - /* Move first to second. */ - result &= _con_move_to_con(first, second, false, false, false, true, false); - /* If swapping the containers didn't work we don't need to mess with the focus. */ - if (!result) { - goto swap_end; - } - - /* If we moved the container holding the focused window to another - * workspace we need to ensure the visible workspace has the focused - * container. - * We don't need to check this for the second container because we've only - * moved the first one at this point.*/ - if (first_ws != second_ws && focused_within_first) { - con_activate(con_descend_focused(current_ws)); - } + /* We need to copy each other's percentages to ensure that the geometry + * doesn't change during the swap. */ + SWAP(first->percent, second->percent, double); - /* Move second to where first has been originally. */ - result &= _con_move_to_con(second, fake, false, false, false, true, false); - if (!result) { - goto swap_end; + if (restore_focus) { + con_focus(restore_focus); } - /* Swapping will have inserted the containers at the tail of their parents' - * focus head. We fix this now by putting them in the position of the focus - * head the container they swapped with was in. */ - TAILQ_REMOVE(&(first->parent->focus_head), first, focused); - TAILQ_REMOVE(&(second->parent->focus_head), second, focused); + /* Update new parents' & workspaces' urgency. */ + con_set_urgency(first, first->urgent); + con_set_urgency(second, second->urgent); - if (second_prev_focus_head == NULL) { - TAILQ_INSERT_HEAD(&(first->parent->focus_head), first, focused); + /* Exchange fullscreen modes, can't use SWAP because we need to call the + * correct functions. */ + fullscreen_mode_t second_fullscreen_mode = second->fullscreen_mode; + if (first->fullscreen_mode == CF_NONE) { + con_disable_fullscreen(second); } else { - TAILQ_INSERT_AFTER(&(first->parent->focus_head), second_prev_focus_head, first, focused); + con_enable_fullscreen(second, first->fullscreen_mode); } - - if (first_prev_focus_head == NULL) { - TAILQ_INSERT_HEAD(&(second->parent->focus_head), second, focused); + if (second_fullscreen_mode == CF_NONE) { + con_disable_fullscreen(first); } else { - TAILQ_INSERT_AFTER(&(second->parent->focus_head), first_prev_focus_head, second, focused); - } - - /* If the focus was within any of the swapped containers, do the following: - * - If swapping took place within a workspace, ensure the previously - * focused container stays focused. - * - Otherwise, focus the container that has been swapped in. - * - * To understand why fixing the focus_head previously wasn't enough, - * consider the scenario - * H[ V[ A X ] V[ Y B ] ] - * with B being focused, but X being the focus_head within its parent. If - * we swap A and B now, fixing the focus_head would focus X, but since B - * was the focused container before it should stay focused. - */ - if (focused_within_first) { - if (first_ws == second_ws) { - con_activate(old_focus); - } else { - con_activate(con_descend_focused(second)); - } - } else if (focused_within_second) { - if (first_ws == second_ws) { - con_activate(old_focus); - } else { - con_activate(con_descend_focused(first)); - } - } - - /* We need to copy each other's percentages to ensure that the geometry - * doesn't change during the swap. This needs to happen _before_ we close - * the fake container as closing the tree will recalculate percentages. */ - first->percent = second_percent; - second->percent = first_percent; - fake->percent = 0.0; - - SWAP(first_fullscreen_mode, second_fullscreen_mode, fullscreen_mode_t); - -swap_end: - /* The two windows exchange their original fullscreen status */ - if (first_fullscreen_mode != CF_NONE) { - con_enable_fullscreen(first, first_fullscreen_mode); - } - if (second_fullscreen_mode != CF_NONE) { - con_enable_fullscreen(second, second_fullscreen_mode); + con_enable_fullscreen(first, second_fullscreen_mode); } /* We don't actually need this since percentages-wise we haven't changed @@ -2484,11 +2427,8 @@ swap_end: con_fix_percent(first->parent); con_fix_percent(second->parent); - /* We can get rid of the fake container again now. */ - con_close(fake, DONT_KILL_WINDOW); - con_force_split_parents_redraw(first); con_force_split_parents_redraw(second); - return result; + return true; } diff --git a/testcases/t/291-swap.t b/testcases/t/291-swap.t index da2d564d..8705c1ec 100644 --- a/testcases/t/291-swap.t +++ b/testcases/t/291-swap.t @@ -16,20 +16,42 @@ # # Tests the swap command. # Ticket: #917 -use i3test i3_config => < 0; + +my $config = <{fullscreen_mode} != 0 } @{get_ws_content($ws)} +} + +sub cmp_floating_rect { + my ($window, $rect, $prefix) = @_; + my ($absolute, $top) = $window->rect; + + is($absolute->{width}, $rect->[2], "$prefix: width matches"); + is($absolute->{height}, $rect->[3], "$prefix: height matches"); + + is($top->{x}, $rect->[0], "$prefix: x matches"); + is($top->{y}, $rect->[1], "$prefix: y matches"); +} + ############################################################################### # Invalid con_id should not crash i3 # See issue #2895. @@ -261,7 +283,6 @@ sync_with_i3; cmd '[con_mark=B] swap container with mark A'; -sync_with_i3; does_i3_live; $nodes = get_ws_content($ws1); @@ -502,6 +523,142 @@ cmp_float($nodes->[1]->{nodes}->[0]->{percent}, 0.75, 'A has 75% height'); kill_all_windows; +############################################################################### +# Swap floating windows in the same workspace. Check that they exchange rects. +############################################################################### +$ws = fresh_workspace; + +$A = open_floating_window(wm_class => 'mark_A', rect => $rect_A); +$B = open_floating_window(wm_class => 'mark_B', rect => $rect_B); + +cmd '[con_mark=B] swap container with mark A'; +cmp_floating_rect($A, $rect_B, 'A got B\'s rect'); +cmp_floating_rect($B, $rect_A, 'B got A\'s rect'); + +kill_all_windows; + +############################################################################### +# Swap a fullscreen floating and a normal floating window. +############################################################################### +$ws1 = fresh_workspace; +$A = open_floating_window(wm_class => 'mark_A', rect => $rect_A, fullscreen => 1); +$ws2 = fresh_workspace; +$B = open_floating_window(wm_class => 'mark_B', rect => $rect_B); + +cmd '[con_mark=B] swap container with mark A'; + +cmp_floating_rect($A, $rect_B, 'A got B\'s rect'); + +$nodes = get_ws($ws1); +$node = $nodes->{floating_nodes}->[0]->{nodes}->[0]; +is($node->{window}, $B->{id}, 'B is on the first workspace'); +is($node->{fullscreen_mode}, 1, 'B is now fullscreened'); + +$nodes = get_ws($ws2); +$node = $nodes->{floating_nodes}->[0]->{nodes}->[0]; +is($node->{window}, $A->{id}, 'A is on the second workspace'); +is($node->{fullscreen_mode}, 0, 'A is not fullscreened anymore'); + +kill_all_windows; + +############################################################################### +# Swap a floating window which is in a workspace that has another, regular +# window with a regular window in another workspace. A & B focused. +# +# Before: +# +-----------+ +# | F | +# | +---+ | +# | | A | | +# | +---+ | +# | | +# +-----------+ +# +# +-----------+ +# | | +# | | +# | B | +# | | +# | | +# +-----------+ +# +# After: Same as above but A <-> B. A & B focused. +############################################################################### +$ws1 = fresh_workspace; +open_window; +$A = open_floating_window(wm_class => 'mark_A', rect => $rect_A); +$ws2 = fresh_workspace; +$B = open_window(wm_class => 'mark_B'); +$expected_focus = get_focused($ws2); + +cmd '[con_mark=B] swap container with mark A'; + +$nodes = get_ws($ws1); +$node = $nodes->{floating_nodes}->[0]->{nodes}->[0]; +is($node->{window}, $B->{id}, 'B is floating on the first workspace'); +is(get_focused($ws1), $expected_focus, 'B is focused'); +cmp_floating_rect($B, $rect_A, 'B got A\'s rect'); + +$nodes = get_ws_content($ws2); +$node = $nodes->[0]; +is($node->{window}, $A->{id}, 'A is on the second workspace'); + +kill_all_windows; + +################################################################### +# Test that swapping a floating window maintains the correct +# floating order. +################################################################### +$ws = fresh_workspace; +$A = open_floating_window(wm_class => 'mark_A', rect => $rect_A); +$B = open_window(wm_class => 'mark_B'); +$F = open_floating_window; +$expected_focus = get_focused($ws); + +cmd '[con_mark=B] swap container with mark A'; + +$nodes = get_ws($ws); + +$node = $nodes->{floating_nodes}->[0]->{nodes}->[0]; +is($node->{window}, $B->{id}, 'B is floating, bottom'); +cmp_floating_rect($B, $rect_A, 'B got A\'s rect'); + +$node = $nodes->{floating_nodes}->[1]->{nodes}->[0]; +is($node->{window}, $F->{id}, 'F is floating, top'); +is(get_focused($ws), $expected_focus, 'F still focused'); + +$node = $nodes->{nodes}->[0]; +is($node->{window}, $A->{id}, 'A is tiling'); + +kill_all_windows; + +############################################################################### +# Swap a sticky, floating container A and a floating fullscreen container B. +# A should remain sticky and floating and should be fullscreened. +############################################################################### +$ws1 = fresh_workspace; +open_window; +$A = open_floating_window(wm_class => 'mark_A', rect => $rect_A); +$expected_focus = get_focused($ws1); +cmd 'sticky enable'; +$B = open_floating_window(wm_class => 'mark_B', rect => $rect_B); +cmd 'fullscreen enable'; + +cmd '[con_mark=B] swap container with mark A'; + +is(@{get_ws($ws1)->{floating_nodes}}, 2, '2 fullscreen containers on first workspace'); +is(get_focused($ws1), $expected_focus, 'A is focused'); + +$ws2 = fresh_workspace; +cmd 'fullscreen disable'; +cmp_floating_rect($A, $rect_B, 'A got B\'s rect'); +is(@{get_ws($ws2)->{floating_nodes}}, 1, 'only A in new workspace'); + +cmd "workspace $ws1"; # TODO: Why does rect check fails without switching workspaces? +cmp_floating_rect($B, $rect_A, 'B got A\'s rect'); + +kill_all_windows; + ############################################################################### # Swapping containers moves the urgency hint correctly. ############################################################################### @@ -527,6 +684,64 @@ is(get_ws($ws2)->{urgent}, 0, 'the second workspace is not marked urgent'); kill_all_windows; +############################################################################### +# Swapping an urgent container from another workspace with the focused +# container removes the urgency hint from both the container and the previous +# workspace. +############################################################################### + +$ws2 = fresh_workspace; +$B = open_window(wm_class => 'mark_B'); +$ws1 = fresh_workspace; +$A = open_window(wm_class => 'mark_A'); + +$B->add_hint('urgency'); +sync_with_i3; + +cmd '[con_mark=B] swap container with mark A'; + +@urgent = grep { $_->{urgent} } @{get_ws_content($ws1)}; +is(@urgent, 0, 'B is not marked urgent'); +is(get_ws($ws1)->{urgent}, 0, 'the first workspace is not marked urgent'); + +@urgent = grep { $_->{urgent} } @{get_ws_content($ws2)}; +is(@urgent, 0, 'A is not marked urgent'); +is(get_ws($ws2)->{urgent}, 0, 'the second workspace is not marked urgent'); + +kill_all_windows; + +exit_gracefully($pid); + +############################################################################### +# Test that swapping with workspace_layout doesn't crash i3. +# See issue #3280. +############################################################################### + +$config = <[0]->{window}, $B->{id}, 'B is on the left'); + +exit_gracefully($pid); + ############################################################################### done_testing; diff --git a/testcases/t/294-focus-order.t b/testcases/t/294-focus-order.t index c818f1d4..2058e1d8 100644 --- a/testcases/t/294-focus-order.t +++ b/testcases/t/294-focus-order.t @@ -176,6 +176,45 @@ cmd '[id=' . $windows[2]->id . '] move to workspace ' . $ws; cmd '[id=' . $windows[1]->id . '] move to workspace ' . $ws; confirm_focus('\'move to workspace\' focus order when moving containers from other workspace'); +###################################################################### +# Swapping sibling containers correctly swaps focus order. +###################################################################### + +sub swap_with_ids { + my ($idx1, $idx2) = @_; + cmd '[id=' . $windows[$idx1]->id . '] swap container with id ' . $windows[$idx2]->id; +} + +$ws = fresh_workspace; +$windows[2] = open_window; +$windows[0] = open_window; +$windows[3] = open_window; +$windows[1] = open_window; + +# If one of the swapees is focused we deliberately preserve its focus, switch +# workspaces to move focus away from the windows. +fresh_workspace; + +# 2 0 3 1 <- focus order in this direction +swap_with_ids(3, 1); +# 2 0 1 3 +swap_with_ids(2, 3); +# 3 0 1 2 +swap_with_ids(0, 2); +# 3 2 1 0 + +# Restore input focus for confirm_focus +cmd "workspace $ws"; + +# Also confirm swap worked +$ws = get_ws($ws); +for my $i (0 .. 3) { + my $node = $ws->{nodes}->[3 - $i]; + is($node->{window}, $windows[$i]->id, "window $i in correct position after swap"); +} + +confirm_focus('\'swap container with id\' focus order'); + ###################################################################### # Test focus order with floating and tiling windows. # See issue: 1975 -- 2.39.5