Improve startup sequence termination conditions

If a window with _NET_STARTUP_ID set is moved to another workspace, it
will delete any associated startup sequence immediately. This will also
occur if a window has a leader with _NET_STARTUP_ID set, if the leader
has no container (never been mapped).

A startup sequence may also be deleted if it's matched by
startup_workspace_for_window() and its 30-second timeout has elapsed.

include/startup.h

@@ -31,12 +31,27 @@
  */
 void start_application(const char *command, bool no_startup_id);
 
+/**
+ * Deletes a startup sequence, ignoring whether its timeout has elapsed.
+ * Useful when e.g. a window is moved between workspaces and its children
+ * shouldn't spawn on the original workspace.
+ *
+ */
+void startup_sequence_delete(struct Startup_Sequence *sequence);
+
 /**
  * Called by libstartup-notification when something happens
  *
  */
 void startup_monitor_event(SnMonitorEvent *event, void *userdata);
 
+/**
+ * Gets the stored startup sequence for the _NET_STARTUP_ID of a given window.
+ *
+ */
+struct Startup_Sequence *startup_sequence_get(i3Window *cwindow,
+    xcb_get_property_reply_t *startup_id_reply, bool ignore_mapped_leader);
+
 /**
  * Checks if the given window belongs to a startup notification by checking if
  * the _NET_STARTUP_ID property is set on the window (or on its leader, if it’s

src/con.c

@@ -769,6 +769,38 @@ void con_move_to_workspace(Con *con, Con *workspace, bool fix_coordinates, bool
             con_focus(con_descend_focused(focus_next));
     }
 
+    /* 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;
+
+            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,
+            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);
+    }
+
     CALL(parent, on_remove_child);
 }
 

src/startup.c

@@ -58,7 +58,7 @@ static void startup_timeout(EV_P_ ev_timer *w, int revents) {
 }
 
 /*
- * Some applications (such as Firefox) mark a startup sequence as completede
+ * Some applications (such as Firefox) mark a startup sequence as completed
  * *before* they even map a window. Therefore, we cannot entirely delete the
  * startup sequence once it’s marked as complete. Instead, we’ll mark it for
  * deletion in 30 seconds and use that chance to delete old sequences.
@@ -68,15 +68,10 @@ static void startup_timeout(EV_P_ ev_timer *w, int revents) {
  * the root window cursor.
  *
  */
-static int _delete_startup_sequence(struct Startup_Sequence *sequence) {
+static int _prune_startup_sequences(void) {
     time_t current_time = time(NULL);
     int active_sequences = 0;
 
-    /* Mark the given sequence for deletion in 30 seconds. */
-    sequence->delete_at = current_time + 30;
-    DLOG("Will delete startup sequence %s at timestamp %ld\n",
-         sequence->id, sequence->delete_at);
-
     /* Traverse the list and delete everything which was marked for deletion 30
      * seconds ago or earlier. */
     struct Startup_Sequence *current, *next;
@@ -94,20 +89,35 @@ static int _delete_startup_sequence(struct Startup_Sequence *sequence) {
         if (current_time <= current->delete_at)
             continue;
 
-        DLOG("Deleting startup sequence %s, delete_at = %ld, current_time = %ld\n",
+        startup_sequence_delete(current);
-             current->id, current->delete_at, current_time);
-
-        /* Unref the context, will be free()d */
-        sn_launcher_context_unref(current->context);
-
-        /* Delete our internal sequence */
-        TAILQ_REMOVE(&startup_sequences, current, sequences);
     }
 
     return active_sequences;
 
 }
 
+/**
+ * Deletes a startup sequence, ignoring whether its timeout has elapsed.
+ * Useful when e.g. a window is moved between workspaces and its children
+ * shouldn't spawn on the original workspace.
+ *
+ */
+void startup_sequence_delete(struct Startup_Sequence *sequence) {
+    assert(sequence != NULL);
+    DLOG("Deleting startup sequence %s, delete_at = %ld, current_time = %ld\n",
+         sequence->id, sequence->delete_at, time(NULL));
+
+    /* Unref the context, will be free()d */
+    sn_launcher_context_unref(sequence->context);
+
+    /* Delete our internal sequence */
+    TAILQ_REMOVE(&startup_sequences, sequence, sequences);
+
+    free(sequence->id);
+    free(sequence->workspace);
+    FREE(sequence);
+}
+
 /*
  * Starts the given application by passing it through a shell. We use double fork
  * to avoid zombie processes. As the started application’s parent exits (immediately),
@@ -233,7 +243,13 @@ void startup_monitor_event(SnMonitorEvent *event, void *userdata) {
         case SN_MONITOR_EVENT_COMPLETED:
             DLOG("startup sequence %s completed\n", sn_startup_sequence_get_id(snsequence));
 
-            if (_delete_startup_sequence(sequence) == 0) {
+            /* Mark the given sequence for deletion in 30 seconds. */
+            time_t current_time = time(NULL);
+            sequence->delete_at = current_time + 30;
+            DLOG("Will delete startup sequence %s at timestamp %ld\n",
+                 sequence->id, sequence->delete_at);
+
+            if (_prune_startup_sequences() == 0) {
                 DLOG("No more startup sequences running, changing root window cursor to default pointer.\n");
                 /* Change the pointer of the root window to indicate progress */
                 if (xcursor_supported)
@@ -247,32 +263,44 @@ void startup_monitor_event(SnMonitorEvent *event, void *userdata) {
     }
 }
 
-/*
+/**
- * Checks if the given window belongs to a startup notification by checking if
+ * Gets the stored startup sequence for the _NET_STARTUP_ID of a given window.
- * the _NET_STARTUP_ID property is set on the window (or on its leader, if it’s
- * unset).
- *
- * If so, returns the workspace on which the startup was initiated.
- * Returns NULL otherwise.
  *
  */
-char *startup_workspace_for_window(i3Window *cwindow, xcb_get_property_reply_t *startup_id_reply) {
+struct Startup_Sequence *startup_sequence_get(i3Window *cwindow,
+    xcb_get_property_reply_t *startup_id_reply, bool ignore_mapped_leader) {
     /* The _NET_STARTUP_ID is only needed during this function, so we get it
      * here and don’t save it in the 'cwindow'. */
     if (startup_id_reply == NULL || xcb_get_property_value_length(startup_id_reply) == 0) {
         FREE(startup_id_reply);
-        DLOG("No _NET_STARTUP_ID set on this window\n");
+        DLOG("No _NET_STARTUP_ID set on window 0x%08x\n", cwindow->id);
         if (cwindow->leader == XCB_NONE)
             return NULL;
 
-        xcb_get_property_cookie_t cookie;
+        /* This is a special case that causes the leader's startup sequence
-        cookie = xcb_get_property(conn, false, cwindow->leader, A__NET_STARTUP_ID, XCB_GET_PROPERTY_TYPE_ANY, 0, 512);
+         * to only be returned if it has never been mapped, useful primarily
+         * when trying to delete a sequence.
+         *
+         * It's generally inappropriate to delete a leader's sequence when
+         * moving a child window, but if the leader has no container, it's
+         * likely permanently unmapped and the child is the "real" window. */
+        if (ignore_mapped_leader && con_by_window_id(cwindow->leader) != NULL) {
+            DLOG("Ignoring leader window 0x%08x\n", cwindow->leader);
+            return NULL;
+        }
+
         DLOG("Checking leader window 0x%08x\n", cwindow->leader);
+
+        xcb_get_property_cookie_t cookie;
+
+        cookie = xcb_get_property(conn, false, cwindow->leader,
+            A__NET_STARTUP_ID, XCB_GET_PROPERTY_TYPE_ANY, 0, 512);
         startup_id_reply = xcb_get_property_reply(conn, cookie, NULL);
 
-        if (startup_id_reply == NULL || xcb_get_property_value_length(startup_id_reply) == 0) {
+        if (startup_id_reply == NULL ||
-            DLOG("No _NET_STARTUP_ID set on the leader either\n");
+            xcb_get_property_value_length(startup_id_reply) == 0) {
             FREE(startup_id_reply);
+            DLOG("No _NET_STARTUP_ID set on the leader either\n");
             return NULL;
         }
     }
@@ -304,5 +332,31 @@ char *startup_workspace_for_window(i3Window *cwindow, xcb_get_property_reply_t *
 
     free(startup_id);
     free(startup_id_reply);
+
+    return sequence;
+}
+
+/*
+ * Checks if the given window belongs to a startup notification by checking if
+ * the _NET_STARTUP_ID property is set on the window (or on its leader, if it’s
+ * unset).
+ *
+ * If so, returns the workspace on which the startup was initiated.
+ * Returns NULL otherwise.
+ *
+ */
+char *startup_workspace_for_window(i3Window *cwindow, xcb_get_property_reply_t *startup_id_reply) {
+    struct Startup_Sequence *sequence = startup_sequence_get(cwindow, startup_id_reply, false);
+    if (sequence == NULL)
+        return NULL;
+
+    /* If the startup sequence's time span has elapsed, delete it. */
+    time_t current_time = time(NULL);
+    if (sequence->delete_at > 0 && current_time > sequence->delete_at) {
+        DLOG("Deleting expired startup sequence %s\n", sequence->id);
+        startup_sequence_delete(sequence);
+        return NULL;
+    }
+
     return sequence->workspace;
 }

testcases/t/175-startup-notification.t

@@ -136,16 +136,49 @@ is_num_children($second_ws, 0, 'still no containers on the second workspace');
 is_num_children($first_ws, 2, 'two containers on the first workspace');
 
 ######################################################################
-# 2) open another window after the startup process is completed
+# verifies that finishing startup doesn't immediately stop windows
-# (should be placed on the current workspace)
+# from being placed on the sequence's workspace, but that moving
+# the leader actually deletes the startup sequence mapping
 ######################################################################
 
 complete_startup();
 sync_with_i3;
 
-my $otherwin = open_window;
+# Startup has completed but the 30-second deletion time hasn't elapsed,
+# so this window should still go on the leader's initial workspace.
+$win = open_window({ dont_map => 1, client_leader => $leader });
+$win->map;
+sync_with_i3;
+
+is_num_children($first_ws, 3, 'three containers on the first workspace');
+
+# Switch to the first workspace and move the focused window to the
+# second workspace.
+cmd "workspace $first_ws";
+cmd "move workspace $second_ws";
+
 is_num_children($second_ws, 1, 'one container on the second workspace');
 
+# Create and switch to a new workspace, just to be safe.
+my $third_ws = fresh_workspace;
+
+# Moving the window between workspaces should have immediately
+# removed the startup workspace mapping. New windows with that
+# leader should be created on the current workspace.
+$win = open_window({ dont_map => 1, client_leader => $leader });
+$win->map;
+sync_with_i3;
+
+is_num_children($third_ws, 1, 'one container on the third workspace');
+
+######################################################################
+# 2) open another window after the startup process is completed
+# (should be placed on the current workspace)
+######################################################################
+
+my $otherwin = open_window;
+is_num_children($third_ws, 2, 'two containers on the third workspace');
+
 ######################################################################
 # 3) test that the --no-startup-id flag for exec leads to no DESKTOP_STARTUP_ID
 # environment variable.