Bugfix: properly restore workspace containers (Thanks vals)

fixes #1306

include/load_layout.h

@@ -2,7 +2,7 @@
  * vim:ts=4:sw=4:expandtab
  *
  * i3 - an improved dynamic tiling window manager
- * © 2009-2011 Michael Stapelberg and contributors (see also: LICENSE)
+ * © 2009-2014 Michael Stapelberg and contributors (see also: LICENSE)
  *
  * load_layout.c: Restore (parts of) the layout, for example after an inplace
  *                restart.
@@ -10,4 +10,25 @@
  */
 #pragma once
 
+typedef enum {
+    // We could not determine the content of the JSON file. This typically
+    // means it’s unreadable or contains garbage.
+    JSON_CONTENT_UNKNOWN = 0,
+
+    // The JSON file contains a “normal” container, i.e. a container to be
+    // appended to an existing workspace (or split container!).
+    JSON_CONTENT_CON = 1,
+
+    // The JSON file contains a workspace container, which needs to be appended
+    // to the output (next to the other workspaces) with special care to avoid
+    // naming conflicts and ensuring that the workspace _has_ a name.
+    JSON_CONTENT_WORKSPACE = 2,
+} json_content_t;
+
+/* Parses the given JSON file until it encounters the first “type” property to
+ * determine whether the file contains workspaces or regular containers, which
+ * is important to know when deciding where (and how) to append the contents.
+ * */
+json_content_t json_determine_content(const char *filename);
+
 void tree_append_json(Con *con, const char *filename, char **errormsg);

src/commands.c

@@ -882,15 +882,27 @@ void cmd_nop(I3_CMD, char *comment) {
  */
 void cmd_append_layout(I3_CMD, char *path) {
     LOG("Appending layout \"%s\"\n", path);
+
+    json_content_t content = json_determine_content(path);
+    LOG("JSON content = %d\n", content);
+    if (content == JSON_CONTENT_UNKNOWN) {
+        ELOG("Could not determine the contents of \"%s\", not loading.\n", path);
+        ysuccess(false);
+        return;
+    }
+
     Con *parent = focused;
-    /* We need to append the layout to a split container, since a leaf
+    if (content == JSON_CONTENT_WORKSPACE) {
-     * container must not have any children (by definition).
+        parent = output_get_content(con_get_output(parent));
-     * Note that we explicitly check for workspaces, since they are okay for
+    } else {
-     * this purpose, but con_accepts_window() returns false for workspaces. */
+        /* We need to append the layout to a split container, since a leaf
-    while (parent->type != CT_WORKSPACE && !con_accepts_window(parent))
+         * container must not have any children (by definition).
-        parent = parent->parent;
+         * Note that we explicitly check for workspaces, since they are okay for
-    DLOG("Appending to parent=%p instead of focused=%p\n",
+         * this purpose, but con_accepts_window() returns false for workspaces. */
-         parent, focused);
+        while (parent->type != CT_WORKSPACE && !con_accepts_window(parent))
+            parent = parent->parent;
+    }
+    DLOG("Appending to parent=%p instead of focused=%p\n", parent, focused);
     char *errormsg = NULL;
     tree_append_json(parent, path, &errormsg);
     if (errormsg != NULL) {
@@ -914,6 +926,9 @@ void cmd_append_layout(I3_CMD, char *path) {
 
     restore_open_placeholder_windows(parent);
 
+    if (content == JSON_CONTENT_WORKSPACE)
+        ipc_send_event("workspace", I3_IPC_EVENT_WORKSPACE, "{\"change\":\"restored\"}");
+
     cmd_output->needs_tree_render = true;
 }
 

src/load_layout.c

@@ -52,11 +52,13 @@ static int json_start_map(void *ctx) {
                 DLOG("New floating_node\n");
                 Con *ws = con_get_workspace(json_node);
                 json_node = con_new_skeleton(NULL, NULL);
+                json_node->name = NULL;
                 json_node->parent = ws;
                 DLOG("Parent is workspace = %p\n", ws);
             } else {
                 Con *parent = json_node;
                 json_node = con_new_skeleton(NULL, NULL);
+                json_node->name = NULL;
                 json_node->parent = parent;
             }
         }
@@ -84,6 +86,40 @@ static int json_end_map(void *ctx) {
             }
         }
 
+        if (json_node->type == CT_WORKSPACE) {
+            /* Ensure the workspace has a name. */
+            DLOG("Attaching workspace. name = %s\n", json_node->name);
+            if (json_node->name == NULL || strcmp(json_node->name, "") == 0) {
+                json_node->name = sstrdup("unnamed");
+            }
+
+            /* Prevent name clashes when appending a workspace, e.g. when the
+             * user tries to restore a workspace called “1” but already has a
+             * workspace called “1”. */
+            Con *output;
+            Con *workspace = NULL;
+            TAILQ_FOREACH (output, &(croot->nodes_head), nodes)
+                GREP_FIRST(workspace, output_get_content(output), !strcasecmp(child->name, json_node->name));
+            char *base = sstrdup(json_node->name);
+            int cnt = 1;
+            while (workspace != NULL) {
+                FREE(json_node->name);
+                asprintf(&(json_node->name), "%s_%d", base, cnt++);
+                workspace = NULL;
+                TAILQ_FOREACH (output, &(croot->nodes_head), nodes)
+                    GREP_FIRST(workspace, output_get_content(output), !strcasecmp(child->name, json_node->name));
+            }
+            free(base);
+
+            /* Set num accordingly so that i3bar will properly sort it. */
+            json_node->num = ws_name_to_number(json_node->name);
+        } else {
+            // TODO: remove this in the “next” branch.
+            if (json_node->name == NULL || strcmp(json_node->name, "") == 0) {
+                json_node->name = sstrdup("#ff0000");
+            }
+        }
+
         LOG("attaching\n");
         con_attach(json_node, json_node->parent, true);
         LOG("Creating window\n");
@@ -390,26 +426,94 @@ static int json_double(void *ctx, double val) {
     return 1;
 }
 
+static json_content_t content_result;
+
+static int json_determine_content_string(void *ctx, const unsigned char *val, size_t len) {
+    if (strcasecmp(last_key, "type") != 0)
+        return 1;
+
+    DLOG("string = %.*s, last_key = %s\n", (int)len, val, last_key);
+    if (strncasecmp((const char *)val, "workspace", len) == 0)
+        content_result = JSON_CONTENT_WORKSPACE;
+    return 0;
+}
+
+/* Parses the given JSON file until it encounters the first “type” property to
+ * determine whether the file contains workspaces or regular containers, which
+ * is important to know when deciding where (and how) to append the contents.
+ * */
+json_content_t json_determine_content(const char *filename) {
+    FILE *f;
+    if ((f = fopen(filename, "r")) == NULL) {
+        ELOG("Cannot open file \"%s\"\n", filename);
+        return JSON_CONTENT_UNKNOWN;
+    }
+    struct stat stbuf;
+    if (fstat(fileno(f), &stbuf) != 0) {
+        ELOG("Cannot fstat() \"%s\"\n", filename);
+        fclose(f);
+        return JSON_CONTENT_UNKNOWN;
+    }
+    char *buf = smalloc(stbuf.st_size);
+    int n = fread(buf, 1, stbuf.st_size, f);
+    if (n != stbuf.st_size) {
+        ELOG("File \"%s\" could not be read entirely, not loading.\n", filename);
+        fclose(f);
+        return JSON_CONTENT_UNKNOWN;
+    }
+    DLOG("read %d bytes\n", n);
+    // We default to JSON_CONTENT_CON because it is legal to not include
+    // “"type": "con"” in the JSON files for better readability.
+    content_result = JSON_CONTENT_CON;
+    yajl_gen g;
+    yajl_handle hand;
+    static yajl_callbacks callbacks = {
+        .yajl_string = json_determine_content_string,
+        .yajl_map_key = json_key,
+    };
+    g = yajl_gen_alloc(NULL);
+    hand = yajl_alloc(&callbacks, NULL, (void *)g);
+    /* Allowing comments allows for more user-friendly layout files. */
+    yajl_config(hand, yajl_allow_comments, true);
+    /* Allow multiple values, i.e. multiple nodes to attach */
+    yajl_config(hand, yajl_allow_multiple_values, true);
+    yajl_status stat;
+    setlocale(LC_NUMERIC, "C");
+    stat = yajl_parse(hand, (const unsigned char *)buf, n);
+    if (stat != yajl_status_ok && stat != yajl_status_client_canceled) {
+        unsigned char *str = yajl_get_error(hand, 1, (const unsigned char *)buf, n);
+        ELOG("JSON parsing error: %s\n", str);
+        yajl_free_error(hand, str);
+    }
+
+    setlocale(LC_NUMERIC, "");
+    yajl_complete_parse(hand);
+
+    fclose(f);
+
+    return content_result;
+}
+
 void tree_append_json(Con *con, const char *filename, char **errormsg) {
     FILE *f;
     if ((f = fopen(filename, "r")) == NULL) {
-        LOG("Cannot open file \"%s\"\n", filename);
+        ELOG("Cannot open file \"%s\"\n", filename);
         return;
     }
     struct stat stbuf;
     if (fstat(fileno(f), &stbuf) != 0) {
-        LOG("Cannot fstat() the file\n");
+        ELOG("Cannot fstat() \"%s\"\n", filename);
         fclose(f);
         return;
     }
     char *buf = smalloc(stbuf.st_size);
     int n = fread(buf, 1, stbuf.st_size, f);
     if (n != stbuf.st_size) {
-        LOG("File \"%s\" could not be read entirely, not loading.\n", filename);
+        ELOG("File \"%s\" could not be read entirely, not loading.\n", filename);
         fclose(f);
         return;
     }
-    LOG("read %d bytes\n", n);
+    DLOG("read %d bytes\n", n);
     yajl_gen g;
     yajl_handle hand;
     static yajl_callbacks callbacks = {

testcases/t/234-layout-restore-output.t

@@ -0,0 +1,186 @@
+#!perl
+# vim:ts=4:sw=4:expandtab
+#
+# Please read the following documents before working on tests:
+# • http://build.i3wm.org/docs/testsuite.html
+#   (or docs/testsuite)
+#
+# • http://build.i3wm.org/docs/lib-i3test.html
+#   (alternatively: perldoc ./testcases/lib/i3test.pm)
+#
+# • http://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)
+#
+# Verifies that entire outputs can be saved and restored properly by i3.
+# Ticket: #1306
+# Bug still in: 4.8-26-gf96ec19
+use i3test;
+use File::Temp qw(tempfile);
+use List::MoreUtils qw(uniq);
+use IO::Handle;
+
+my $ws = fresh_workspace;
+
+################################################################################
+# Append a new workspace with a name.
+################################################################################
+
+ok(!workspace_exists('ws_new'), 'workspace "ws_new" does not exist yet');
+
+my ($fh, $filename) = tempfile(UNLINK => 1);
+print $fh <<'EOT';
+// vim:ts=4:sw=4:et
+{
+    // workspace with 1 children
+    "border": "pixel",
+    "floating": "auto_off",
+    "layout": "splith",
+    "percent": null,
+    "type": "workspace",
+    "name": "ws_new",
+    "nodes": [
+        {
+            "border": "pixel",
+            "floating": "auto_off",
+            "geometry": {
+               "height": 268,
+               "width": 484,
+               "x": 0,
+               "y": 0
+            },
+            "name": "vals@w00t: ~",
+            "percent": 1,
+            "swallows": [
+               {
+               // "class": "^URxvt$",
+               // "instance": "^urxvt$",
+               // "title": "^vals\\@w00t\\:\\ \\~$"
+               }
+            ],
+            "type": "con"
+        }
+    ]
+}
+EOT
+$fh->flush;
+cmd "append_layout $filename";
+
+ok(workspace_exists('ws_new'), 'workspace "ws_new" exists now');
+
+does_i3_live;
+
+close($fh);
+
+################################################################################
+# Append a new workspace with a name that clashes with an existing workspace.
+################################################################################
+
+my @old_workspaces = @{get_workspace_names()};
+
+cmd "append_layout $filename";
+
+my @new_workspaces = @{get_workspace_names()};
+cmp_ok(scalar @new_workspaces, '>', scalar @old_workspaces, 'more workspaces than before');
+
+my %created_workspaces = map { ($_, 1) } @new_workspaces;
+delete $created_workspaces{$_} for @old_workspaces;
+diag('created workspaces = ' . Dumper(keys %created_workspaces));
+cmp_ok(scalar keys %created_workspaces, '>', 0, 'new workspaces appeared');
+
+################################################################################
+# Append a new workspace without a name.
+################################################################################
+
+ok(!workspace_exists('unnamed'), 'workspace "unnamed" does not exist yet');
+
+($fh, $filename) = tempfile(UNLINK => 1);
+print $fh <<'EOT';
+// vim:ts=4:sw=4:et
+{
+    // workspace with 1 children
+    "border": "pixel",
+    "floating": "auto_off",
+    "layout": "splith",
+    "percent": null,
+    "type": "workspace",
+    "nodes": [
+        {
+            "border": "pixel",
+            "floating": "auto_off",
+            "geometry": {
+               "height": 268,
+               "width": 484,
+               "x": 0,
+               "y": 0
+            },
+            "name": "vals@w00t: ~",
+            "percent": 1,
+            "swallows": [
+               {
+               // "class": "^URxvt$",
+               // "instance": "^urxvt$",
+               // "title": "^vals\\@w00t\\:\\ \\~$"
+               }
+            ],
+            "type": "con"
+        }
+    ]
+}
+EOT
+$fh->flush;
+cmd "append_layout $filename";
+
+ok(workspace_exists('unnamed'), 'workspace "unnamed" exists now');
+
+################################################################################
+# Append a workspace with a numeric name, ensure it has ->num set.
+################################################################################
+
+ok(!workspace_exists('4'), 'workspace "4" does not exist yet');
+
+($fh, $filename) = tempfile(UNLINK => 1);
+print $fh <<'EOT';
+// vim:ts=4:sw=4:et
+{
+    // workspace with 1 children
+    "border": "pixel",
+    "floating": "auto_off",
+    "layout": "splith",
+    "percent": null,
+    "type": "workspace",
+    "name": "4",
+    "nodes": [
+        {
+            "border": "pixel",
+            "floating": "auto_off",
+            "geometry": {
+               "height": 268,
+               "width": 484,
+               "x": 0,
+               "y": 0
+            },
+            "name": "vals@w00t: ~",
+            "percent": 1,
+            "swallows": [
+               {
+               // "class": "^URxvt$",
+               // "instance": "^urxvt$",
+               // "title": "^vals\\@w00t\\:\\ \\~$"
+               }
+            ],
+            "type": "con"
+        }
+    ]
+}
+EOT
+$fh->flush;
+cmd "append_layout $filename";
+
+ok(workspace_exists('4'), 'workspace "4" exists now');
+my $ws = get_ws("4");
+is($ws->{num}, 4, 'workspace number is 4');
+
+done_testing;