Remanage window after property updates (like titles) (#3759)

docs/layout-saving

@@ -261,27 +261,3 @@ container:
     ]
 }
 --------------------------------------------------------------------------------
-
-=== Placeholders using window title matches don't swallow the window
-
-If you use the +title+ attribute to match a window and find that it doesn't
-work or only works sometimes, the reason might be that the application sets the
-title only after making the window visible. This will be especially true for
-programs running inside terminal emulators, e.g., +urxvt -e irssi+ when
-matching on +title: "irssi"+.
-
-One way to deal with this is to not rely on the title, but instead use, e.g.,
-the +instance+ attribute and running the program to set this window instance to
-that value:
-
---------------------------------------------------------------------------------
-# Run irssi via
-# urxvt -name "irssi-container" -e irssi
-
-"swallows": [
-    {
-        "class": "URxvt",
-        "instance": "irssi-container"
-    }
-]
---------------------------------------------------------------------------------

include/con.h

@@ -533,3 +533,11 @@ bool con_swap(Con *first, Con *second);
  *
  */
 uint32_t con_rect_size_in_orientation(Con *con);
+
+/**
+ * Merges container specific data that should move with the window (e.g. marks,
+ * title format, and the window itself) into another container, and closes the
+ * old container.
+ *
+ */
+void con_merge_into(Con *old, Con *new);

include/data.h

@@ -489,6 +489,10 @@ struct Window {
     bool shaped;
     /** The window has a nonrectangular input shape. */
     bool input_shaped;
+
+    /* Time when the window became managed. Used to determine whether a window
+     * should be swallowed after initial management. */
+    time_t managed_since;
 };
 
 /**

include/manage.h

@@ -37,3 +37,10 @@ void restore_geometry(void);
 void manage_window(xcb_window_t window,
                    xcb_get_window_attributes_cookie_t cookie,
                    bool needs_to_be_mapped);
+
+/**
+ * Remanages a window: performs a swallow check and runs assignments.
+ * Returns con for the window regardless if it updated.
+ *
+ */
+Con *remanage_window(Con *con);

include/startup.h

@@ -69,3 +69,9 @@ struct Startup_Sequence *startup_sequence_get(i3Window *cwindow,
  *
  */
 char *startup_workspace_for_window(i3Window *cwindow, xcb_get_property_reply_t *startup_id_reply);
+
+/**
+ * Deletes the startup sequence for a window if it exists.
+ *
+ */
+void startup_sequence_delete_by_window(i3Window *win);

include/window.h

@@ -22,14 +22,14 @@ void window_free(i3Window *win);
  * given window.
  *
  */
-void window_update_class(i3Window *win, xcb_get_property_reply_t *prop, bool before_mgmt);
+void window_update_class(i3Window *win, xcb_get_property_reply_t *prop);
 
 /**
  * Updates the name by using _NET_WM_NAME (encoded in UTF-8) for the given
  * window. Further updates using window_update_name_legacy will be ignored.
  *
  */
-void window_update_name(i3Window *win, xcb_get_property_reply_t *prop, bool before_mgmt);
+void window_update_name(i3Window *win, xcb_get_property_reply_t *prop);
 
 /**
  * Updates the name by using WM_NAME (encoded in COMPOUND_TEXT). We do not
@@ -38,7 +38,7 @@ void window_update_name(i3Window *win, xcb_get_property_reply_t *prop, bool befo
  * window_update_name()).
  *
  */
-void window_update_name_legacy(i3Window *win, xcb_get_property_reply_t *prop, bool before_mgmt);
+void window_update_name_legacy(i3Window *win, xcb_get_property_reply_t *prop);
 
 /**
  * Updates the CLIENT_LEADER (logical parent window).
@@ -62,7 +62,7 @@ void window_update_strut_partial(i3Window *win, xcb_get_property_reply_t *prop);
  * Updates the WM_WINDOW_ROLE
  *
  */
-void window_update_role(i3Window *win, xcb_get_property_reply_t *prop, bool before_mgmt);
+void window_update_role(i3Window *win, xcb_get_property_reply_t *prop);
 
 /**
  * Updates the _NET_WM_WINDOW_TYPE property.

src/con.c

@@ -1258,34 +1258,17 @@ static bool _con_move_to_con(Con *con, Con *target, bool behind_focused, bool fi
 
     /* 8. If anything within the container is associated with a startup sequence,
      * delete it so child windows won't be created on the old workspace. */
-    struct Startup_Sequence *sequence;
-    xcb_get_property_cookie_t cookie;
-    xcb_get_property_reply_t *startup_id_reply;
-
     if (!con_is_leaf(con)) {
         Con *child;
         TAILQ_FOREACH(child, &(con->nodes_head), nodes) {
             if (!child->window)
                 continue;
-
+            startup_sequence_delete_by_window(child->window);
-            cookie = xcb_get_property(conn, false, child->window->id,
-                                      A__NET_STARTUP_ID, XCB_GET_PROPERTY_TYPE_ANY, 0, 512);
-            startup_id_reply = xcb_get_property_reply(conn, cookie, NULL);
-
-            sequence = startup_sequence_get(child->window, startup_id_reply, true);
-            if (sequence != NULL)
-                startup_sequence_delete(sequence);
         }
     }
 
     if (con->window) {
-        cookie = xcb_get_property(conn, false, con->window->id,
+        startup_sequence_delete_by_window(con->window);
-                                  A__NET_STARTUP_ID, XCB_GET_PROPERTY_TYPE_ANY, 0, 512);
-        startup_id_reply = xcb_get_property_reply(conn, cookie, NULL);
-
-        sequence = startup_sequence_get(con->window, startup_id_reply, true);
-        if (sequence != NULL)
-            startup_sequence_delete(sequence);
     }
 
     /* 9. If the container was marked urgent, move the urgency hint. */
@@ -2401,3 +2384,40 @@ bool con_swap(Con *first, Con *second) {
 uint32_t con_rect_size_in_orientation(Con *con) {
     return (con_orientation(con) == HORIZ ? con->rect.width : con->rect.height);
 }
+
+/*
+ * Merges container specific data that should move with the window (e.g. marks,
+ * title format, and the window itself) into another container, and closes the
+ * old container.
+ *
+ */
+void con_merge_into(Con *old, Con *new) {
+    new->window = old->window;
+    old->window = NULL;
+
+    if (old->title_format) {
+        FREE(new->title_format);
+        new->title_format = old->title_format;
+        old->title_format = NULL;
+    }
+
+    if (old->sticky_group) {
+        FREE(new->sticky_group);
+        new->sticky_group = old->sticky_group;
+        old->sticky_group = NULL;
+    }
+
+    new->sticky = old->sticky;
+
+    con_set_urgency(new, old->urgent);
+
+    mark_t *mark;
+    TAILQ_FOREACH(mark, &(old->marks_head), marks) {
+        TAILQ_INSERT_TAIL(&(new->marks_head), mark, marks);
+        ipc_send_window_event("mark", new);
+    }
+    new->mark_changed = (TAILQ_FIRST(&(old->marks_head)) != NULL);
+    TAILQ_INIT(&(old->marks_head));
+
+    tree_close_internal(old, DONT_KILL_WINDOW, false);
+}

src/handlers.c

@@ -565,7 +565,9 @@ static bool handle_windowname_change(void *data, xcb_connection_t *conn, uint8_t
 
     char *old_name = (con->window->name != NULL ? sstrdup(i3string_as_utf8(con->window->name)) : NULL);
 
-    window_update_name(con->window, prop, false);
+    window_update_name(con->window, prop);
+
+    con = remanage_window(con);
 
     x_push_changes(croot);
 
@@ -590,7 +592,9 @@ static bool handle_windowname_change_legacy(void *data, xcb_connection_t *conn,
 
     char *old_name = (con->window->name != NULL ? sstrdup(i3string_as_utf8(con->window->name)) : NULL);
 
-    window_update_name_legacy(con->window, prop, false);
+    window_update_name_legacy(con->window, prop);
+
+    con = remanage_window(con);
 
     x_push_changes(croot);
 
@@ -612,7 +616,9 @@ static bool handle_windowrole_change(void *data, xcb_connection_t *conn, uint8_t
     if ((con = con_by_window_id(window)) == NULL || con->window == NULL)
         return false;
 
-    window_update_role(con->window, prop, false);
+    window_update_role(con->window, prop);
+
+    con = remanage_window(con);
 
     return true;
 }
@@ -1158,7 +1164,9 @@ static bool handle_class_change(void *data, xcb_connection_t *conn, uint8_t stat
             return false;
     }
 
-    window_update_class(con->window, prop, false);
+    window_update_class(con->window, prop);
+
+    con = remanage_window(con);
 
     return true;
 }

src/manage.c

@@ -13,6 +13,34 @@
 
 #include <yajl/yajl_gen.h>
 
+/*
+ * Match frame and window depth. This is needed because X will refuse to reparent a
+ * window whose background is ParentRelative under a window with a different depth.
+ *
+ */
+static xcb_window_t _match_depth(i3Window *win, Con *con) {
+    xcb_window_t old_frame = XCB_NONE;
+    if (con->depth != win->depth) {
+        old_frame = con->frame.id;
+        con->depth = win->depth;
+        x_con_reframe(con);
+    }
+    return old_frame;
+}
+
+/*
+ * Remove all match criteria, the first swallowed window wins. 
+ *
+ */
+static void _remove_matches(Con *con) {
+    while (!TAILQ_EMPTY(&(con->swallow_head))) {
+        Match *first = TAILQ_FIRST(&(con->swallow_head));
+        TAILQ_REMOVE(&(con->swallow_head), first, matches);
+        match_free(first);
+        free(first);
+    }
+}
+
 /*
  * Go through all existing windows (if the window manager is restarted) and manage them
  *
@@ -174,13 +202,13 @@ void manage_window(xcb_window_t window, xcb_get_window_attributes_cookie_t cooki
     FREE(buttons);
 
     /* update as much information as possible so far (some replies may be NULL) */
-    window_update_class(cwindow, xcb_get_property_reply(conn, class_cookie, NULL), true);
+    window_update_class(cwindow, xcb_get_property_reply(conn, class_cookie, NULL));
-    window_update_name_legacy(cwindow, xcb_get_property_reply(conn, title_cookie, NULL), true);
+    window_update_name_legacy(cwindow, xcb_get_property_reply(conn, title_cookie, NULL));
-    window_update_name(cwindow, xcb_get_property_reply(conn, utf8_title_cookie, NULL), true);
+    window_update_name(cwindow, xcb_get_property_reply(conn, utf8_title_cookie, NULL));
     window_update_leader(cwindow, xcb_get_property_reply(conn, leader_cookie, NULL));
     window_update_transient_for(cwindow, xcb_get_property_reply(conn, transient_cookie, NULL));
     window_update_strut_partial(cwindow, xcb_get_property_reply(conn, strut_cookie, NULL));
-    window_update_role(cwindow, xcb_get_property_reply(conn, role_cookie, NULL), true);
+    window_update_role(cwindow, xcb_get_property_reply(conn, role_cookie, NULL));
     bool urgency_hint;
     window_update_hints(cwindow, xcb_get_property_reply(conn, wm_hints_cookie, NULL), &urgency_hint);
     border_style_t motif_border_style = BS_NORMAL;
@@ -341,24 +369,13 @@ void manage_window(xcb_window_t window, xcb_get_window_attributes_cookie_t cooki
             DLOG("Uh?! Container without a placeholder, but with a window, has swallowed this to-be-managed window?!\n");
         } else {
             /* Remove remaining criteria, the first swallowed window wins. */
-            while (!TAILQ_EMPTY(&(nc->swallow_head))) {
+            _remove_matches(nc);
-                Match *first = TAILQ_FIRST(&(nc->swallow_head));
-                TAILQ_REMOVE(&(nc->swallow_head), first, matches);
-                match_free(first);
-                free(first);
-            }
         }
     }
     xcb_window_t old_frame = XCB_NONE;
     if (nc->window != cwindow && nc->window != NULL) {
         window_free(nc->window);
-        /* Match frame and window depth. This is needed because X will refuse to reparent a
+        old_frame = _match_depth(cwindow, nc);
-         * window whose background is ParentRelative under a window with a different depth. */
-        if (nc->depth != cwindow->depth) {
-            old_frame = nc->frame.id;
-            nc->depth = cwindow->depth;
-            x_con_reframe(nc);
-        }
     }
     nc->window = cwindow;
     x_reinit(nc);
@@ -594,6 +611,8 @@ void manage_window(xcb_window_t window, xcb_get_window_attributes_cookie_t cooki
     }
     render_con(croot);
 
+    cwindow->managed_since = time(NULL);
+
     /* Send an event about window creation */
     ipc_send_window_event("new", nc);
 
@@ -670,3 +689,57 @@ geom_out:
 out:
     free(attr);
 }
+
+/*
+ * Remanages a window: performs a swallow check and runs assignments.
+ * Returns con for the window regardless if it updated.
+ *
+ */
+Con *remanage_window(Con *con) {
+    Match *match;
+    Con *nc = con_for_window(croot, con->window, &match);
+    if (nc == NULL || nc->window == con->window) {
+        run_assignments(con->window);
+        return con;
+    }
+    /* Make sure the placeholder that wants to swallow this window didn't spawn
+     * after the window to follow current behavior: adding a placeholder won't
+     * swallow windows currently managed. */
+    if (nc->window->managed_since > con->window->managed_since) {
+        run_assignments(con->window);
+        return con;
+    }
+
+    if (!restore_kill_placeholder(nc->window->id)) {
+        DLOG("Uh?! Container without a placeholder, but with a window, has swallowed this managed window?!\n");
+    } else {
+        _remove_matches(nc);
+    }
+    window_free(nc->window);
+
+    xcb_window_t old_frame = _match_depth(con->window, nc);
+
+    x_reparent_child(nc, con);
+
+    bool moved_workpaces = (con_get_workspace(nc) != con_get_workspace(con));
+
+    con_merge_into(con, nc);
+
+    /* Destroy the old frame if we had to reframe the container. This needs to be done
+     * after rendering in order to prevent the background from flickering in its place. */
+    if (old_frame != XCB_NONE) {
+        xcb_destroy_window(conn, old_frame);
+    }
+
+    run_assignments(nc->window);
+
+    if (moved_workpaces) {
+        /* If the window is associated with a startup sequence, delete it so
+         * child windows won't be created on the old workspace. */
+        startup_sequence_delete_by_window(nc->window);
+
+        ewmh_update_wm_desktop();
+    }
+
+    return nc;
+}

src/startup.c

@@ -365,3 +365,22 @@ char *startup_workspace_for_window(i3Window *cwindow, xcb_get_property_reply_t *
 
     return sequence->workspace;
 }
+
+/*
+ * Deletes the startup sequence for a window if it exists.
+ *
+ */
+void startup_sequence_delete_by_window(i3Window *win) {
+    struct Startup_Sequence *sequence;
+    xcb_get_property_cookie_t cookie;
+    xcb_get_property_reply_t *startup_id_reply;
+
+    cookie = xcb_get_property(conn, false, win->id, A__NET_STARTUP_ID,
+                              XCB_GET_PROPERTY_TYPE_ANY, 0, 512);
+    startup_id_reply = xcb_get_property_reply(conn, cookie, NULL);
+
+    sequence = startup_sequence_get(win, startup_id_reply, true);
+    if (sequence != NULL) {
+        startup_sequence_delete(sequence);
+    }
+}

src/window.c

@@ -26,7 +26,7 @@ void window_free(i3Window *win) {
  * given window.
  *
  */
-void window_update_class(i3Window *win, xcb_get_property_reply_t *prop, bool before_mgmt) {
+void window_update_class(i3Window *win, xcb_get_property_reply_t *prop) {
     if (prop == NULL || xcb_get_property_value_length(prop) == 0) {
         DLOG("WM_CLASS not set.\n");
         FREE(prop);
@@ -52,9 +52,6 @@ void window_update_class(i3Window *win, xcb_get_property_reply_t *prop, bool bef
         win->class_instance, win->class_class);
 
     free(prop);
-    if (!before_mgmt) {
-        run_assignments(win);
-    }
 }
 
 /*
@@ -62,7 +59,7 @@ void window_update_class(i3Window *win, xcb_get_property_reply_t *prop, bool bef
  * window. Further updates using window_update_name_legacy will be ignored.
  *
  */
-void window_update_name(i3Window *win, xcb_get_property_reply_t *prop, bool before_mgmt) {
+void window_update_name(i3Window *win, xcb_get_property_reply_t *prop) {
     if (prop == NULL || xcb_get_property_value_length(prop) == 0) {
         DLOG("_NET_WM_NAME not specified, not changing\n");
         FREE(prop);
@@ -89,9 +86,6 @@ void window_update_name(i3Window *win, xcb_get_property_reply_t *prop, bool befo
     win->uses_net_wm_name = true;
 
     free(prop);
-    if (!before_mgmt) {
-        run_assignments(win);
-    }
 }
 
 /*
@@ -101,7 +95,7 @@ void window_update_name(i3Window *win, xcb_get_property_reply_t *prop, bool befo
  * window_update_name()).
  *
  */
-void window_update_name_legacy(i3Window *win, xcb_get_property_reply_t *prop, bool before_mgmt) {
+void window_update_name_legacy(i3Window *win, xcb_get_property_reply_t *prop) {
     if (prop == NULL || xcb_get_property_value_length(prop) == 0) {
         DLOG("WM_NAME not set (_NET_WM_NAME is what you want anyways).\n");
         FREE(prop);
@@ -134,9 +128,6 @@ void window_update_name_legacy(i3Window *win, xcb_get_property_reply_t *prop, bo
     win->name_x_changed = true;
 
     free(prop);
-    if (!before_mgmt) {
-        run_assignments(win);
-    }
 }
 
 /*
@@ -218,7 +209,7 @@ void window_update_strut_partial(i3Window *win, xcb_get_property_reply_t *prop)
  * Updates the WM_WINDOW_ROLE
  *
  */
-void window_update_role(i3Window *win, xcb_get_property_reply_t *prop, bool before_mgmt) {
+void window_update_role(i3Window *win, xcb_get_property_reply_t *prop) {
     if (prop == NULL || xcb_get_property_value_length(prop) == 0) {
         DLOG("WM_WINDOW_ROLE not set.\n");
         FREE(prop);
@@ -233,9 +224,6 @@ void window_update_role(i3Window *win, xcb_get_property_reply_t *prop, bool befo
     LOG("WM_WINDOW_ROLE changed to \"%s\"\n", win->role);
 
     free(prop);
-    if (!before_mgmt) {
-        run_assignments(win);
-    }
 }
 
 /*

testcases/t/542-layout-restore-remanage.t

@@ -0,0 +1,86 @@
+#!perl
+# vim:ts=4:sw=4:expandtab
+#
+# Please read the following documents before working on tests:
+# • https://build.i3wm.org/docs/testsuite.html
+#   (or docs/testsuite)
+#
+# • https://build.i3wm.org/docs/lib-i3test.html
+#   (alternatively: perldoc ./testcases/lib/i3test.pm)
+#
+# • https://build.i3wm.org/docs/ipc.html
+#   (or docs/ipc)
+#
+# • http://onyxneon.com/books/modern_perl/modern_perl_a4.pdf
+#   (unless you are already familiar with Perl)
+#
+# Tests that swallowing still works after a window gets managed and its property
+# updated.
+use i3test;
+use File::Temp qw(tempfile);
+use IO::Handle;
+use X11::XCB qw(PROP_MODE_REPLACE);
+
+sub change_window_title {
+    my ($window, $title, $length) = @_;
+    my $atomname = $x->atom(name => '_NET_WM_NAME');
+    my $atomtype = $x->atom(name => 'UTF8_STRING');
+    $length ||= length($title) + 1;
+    $x->change_property(
+        PROP_MODE_REPLACE,
+        $window->id,
+        $atomname->id,
+        $atomtype->id,
+        8,
+        $length,
+        $title
+    );
+    sync_with_i3;
+}
+
+my $ws = fresh_workspace;
+
+my @content = @{get_ws_content($ws)};
+is(@content, 0, 'no nodes on the new workspace yet');
+
+my ($fh, $filename) = tempfile(UNLINK => 1);
+print $fh <<EOT;
+{
+    "layout": "splitv",
+    "nodes": [
+        {
+            "swallows": [
+                {
+                    "title": "different_title"
+                }
+            ]
+        }
+    ]
+}
+EOT
+$fh->flush;
+cmd "append_layout $filename";
+
+@content = @{get_ws_content($ws)};
+is(@content, 1, 'one node on the workspace now');
+
+my $window = open_window(
+    name => 'original_title',
+    wm_class => 'a',
+);
+
+@content = @{get_ws_content($ws)};
+is(@content, 2, 'two nodes on the workspace now');
+
+change_window_title($window, "different_title");
+
+does_i3_live;
+
+@content = @{get_ws_content($ws)};
+my @nodes = @{$content[0]->{nodes}};
+is(@content, 1, 'only one node on the workspace now');
+is($nodes[0]->{name}, 'different_title', 'test window got swallowed');
+
+close($fh);
+
+done_testing;