Merge pull request #2954 from orestisf1993/swap-for-floating

Rewrite con_swap to work only with queue operations

docs/userguide

@@ -2106,8 +2106,7 @@ using one of the following methods:
 +mark+:: A container with the specified mark, see <<vim_like_marks>>.
 
 Note that swapping does not work with all containers. Most notably, swapping
-floating containers or containers that have a parent-child relationship to one
+containers that have a parent-child relationship to one another does not work.
-another does not work.
 
 *Syntax*:
 ----------------------------------------

src/con.c

@@ -2333,11 +2333,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;
@@ -2348,132 +2343,80 @@ bool con_swap(Con *first, Con *second) {
         return false;
     }
 
-    Con *old_focus = focused;
+    Con *ws1 = con_get_workspace(first);
-
+    Con *ws2 = con_get_workspace(second);
-    Con *first_ws = con_get_workspace(first);
+    Con *restore_focus = NULL;
-    Con *second_ws = con_get_workspace(second);
+    if (ws1 == ws2 && ws1 == con_get_workspace(focused)) {
-    Con *current_ws = con_get_workspace(old_focus);
+        /* Preserve focus in the current workspace. */
-    const bool focused_within_first = (first == old_focus || con_has_parent(old_focus, first));
+        restore_focus = focused;
-    const bool focused_within_second = (second == old_focus || con_has_parent(old_focus, second));
+    } else if (first == focused || con_has_parent(focused, first)) {
-    fullscreen_mode_t first_fullscreen_mode = first->fullscreen_mode;
+        restore_focus = second;
-    fullscreen_mode_t second_fullscreen_mode = second->fullscreen_mode;
+    } else if (second == focused || con_has_parent(focused, second)) {
-
+        restore_focus = first;
-    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;
+#define SWAP_CONS_IN_TREE(headname, field)                            \
-    double second_percent = second->percent;
+    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)
 
-    /* De- and reattaching the containers will insert them at the tail of the
+    SWAP_CONS_IN_TREE(nodes_head, nodes);
-     * focus_heads. We will need to fix this. But we need to make sure first
+    SWAP_CONS_IN_TREE(focus_head, focused);
-     * and second don't get in each other's way if they share the same parent,
+    SWAP(first->parent, second->parent, Con *);
-     * 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;
+    /* Floating nodes are children of CT_FLOATING_CONs, they are listed in
-    while (second_prev_focus_head == second || second_prev_focus_head == first) {
+     * nodes_head and focus_head like all other containers. Thus, we don't need
-        second_prev_focus_head = TAILQ_PREV(second_prev_focus_head, focus_head, focused);
+     * to do anything special other than swapping the floating status and the
-    }
+     * relevant rects. */
-
+    SWAP(first->floating, second->floating, int);
-    /* We use a fake container to mark the spot of where the second container needs to go. */
+    SWAP(first->rect, second->rect, Rect);
-    Con *fake = con_new(NULL, NULL);
+    SWAP(first->window_rect, second->window_rect, Rect);
-    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));
-    }
-
-    /* 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;
-    }
-
-    /* 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);
-
-    if (second_prev_focus_head == NULL) {
-        TAILQ_INSERT_HEAD(&(first->parent->focus_head), first, focused);
-    } else {
-        TAILQ_INSERT_AFTER(&(first->parent->focus_head), second_prev_focus_head, first, focused);
-    }
-
-    if (first_prev_focus_head == NULL) {
-        TAILQ_INSERT_HEAD(&(second->parent->focus_head), second, focused);
-    } 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
+     * doesn't change during the swap. */
-     * the fake container as closing the tree will recalculate percentages. */
+    SWAP(first->percent, second->percent, double);
-    first->percent = second_percent;
-    second->percent = first_percent;
-    fake->percent = 0.0;
 
-    SWAP(first_fullscreen_mode, second_fullscreen_mode, fullscreen_mode_t);
+    if (restore_focus) {
-
+        con_focus(restore_focus);
-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);
+    /* Update new parents' & workspaces' urgency. */
+    con_set_urgency(first, first->urgent);
+    con_set_urgency(second, second->urgent);
+
+    /* 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 {
+        con_enable_fullscreen(second, first->fullscreen_mode);
+    }
+    if (second_fullscreen_mode == CF_NONE) {
+        con_disable_fullscreen(first);
+    } else {
+        con_enable_fullscreen(first, second_fullscreen_mode);
     }
 
     /* We don't actually need this since percentages-wise we haven't changed
@@ -2482,11 +2425,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;
 }

testcases/t/291-swap.t

@@ -16,20 +16,42 @@
 #
 # Tests the swap command.
 # Ticket: #917
-use i3test i3_config => <<EOT;
+use i3test i3_autostart => 0;
+
+my $config = <<EOT;
 # i3 config file (v4)
 font font -misc-fixed-medium-r-normal--13-120-75-75-C-70-iso10646-1
 
 for_window[class="mark_A"] mark A
 for_window[class="mark_B"] mark B
 EOT
+my $pid = launch_with_config($config);
 
 my ($ws, $ws1, $ws2, $ws3);
 my ($node, $nodes, $expected_focus, $A, $B, $F);
 my ($result);
 my @fullscreen_permutations = ([], ["A"], ["B"], ["A", "B"]);
+my $rect_A = [ 100, 100, 100, 100 ];
+my $rect_B = [ 200, 200, 200, 200 ];
 my @urgent;
 
+sub fullscreen_windows {
+    my $ws = shift if @_;
+
+    scalar grep { $_->{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 = <<EOT;
+# i3 config file (v4)
+font font -misc-fixed-medium-r-normal--13-120-75-75-C-70-iso10646-1
+
+workspace_layout stacking
+EOT
+$pid = launch_with_config($config);
+
+$ws = fresh_workspace;
+open_window;
+open_window;
+$B = open_window;
+
+cmd 'move right';
+cmd 'focus left, focus parent, mark a';
+cmd 'focus right';
+cmd 'swap with mark a';
+
+does_i3_live;
+
+$nodes = get_ws_content($ws);
+is($nodes->[0]->{window}, $B->{id}, 'B is on the left');
+
+exit_gracefully($pid);
+
 ###############################################################################
 
 done_testing;

testcases/t/294-focus-order.t

@@ -210,6 +210,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