Merge pull request #1960 from Airblader/bug-restore-mark

Fix duplicated marks on append_layout

include/con.h

@@ -146,6 +146,27 @@ Con *con_by_frame_id(xcb_window_t frame);
  */
 Con *con_by_mark(const char *mark);
 
+/**
+ * Toggles the mark on a container.
+ * If the container already has this mark, the mark is removed.
+ * Otherwise, the mark is assigned to the container.
+ *
+ */
+void con_mark_toggle(Con *con, const char *mark);
+
+/**
+ * Assigns a mark to the container.
+ *
+ */
+void con_mark(Con *con, const char *mark);
+
+/**
+ * If mark is NULL, this removes all existing marks.
+ * Otherwise, it will only remove the given mark (if it is present).
+ *
+ */
+void con_unmark(const char *mark);
+
 /**
  * Returns the first container below 'con' which wants to swallow this window
  * TODO: priority

src/commands.c

@@ -1120,27 +1120,10 @@ void cmd_mark(I3_CMD, char *mark, char *toggle) {
     }
 
     DLOG("matching: %p / %s\n", current->con, current->con->name);
-    current->con->mark_changed = true;
+    if (toggle != NULL) {
-    if (toggle != NULL && current->con->mark && strcmp(current->con->mark, mark) == 0) {
+        con_mark_toggle(current->con, mark);
-        DLOG("removing window mark %s\n", mark);
-        FREE(current->con->mark);
     } else {
-        DLOG("marking window with str %s\n", mark);
+        con_mark(current->con, mark);
-        FREE(current->con->mark);
-        current->con->mark = sstrdup(mark);
-    }
-
-    DLOG("Clearing all non-matched windows with this mark\n");
-    Con *con;
-    TAILQ_FOREACH(con, &all_cons, all_cons) {
-        /* Skip matched window, we took care of it already. */
-        if (current->con == con)
-            continue;
-
-        if (con->mark && strcmp(con->mark, mark) == 0) {
-            FREE(con->mark);
-            con->mark_changed = true;
-        }
     }
 
     cmd_output->needs_tree_render = true;
@@ -1153,24 +1136,7 @@ void cmd_mark(I3_CMD, char *mark, char *toggle) {
  *
  */
 void cmd_unmark(I3_CMD, char *mark) {
-    if (mark == NULL) {
+    con_unmark(mark);
-        Con *con;
-        TAILQ_FOREACH(con, &all_cons, all_cons) {
-            if (con->mark == NULL)
-                continue;
-
-            FREE(con->mark);
-            con->mark_changed = true;
-        }
-        DLOG("Removed all window marks.\n");
-    } else {
-        Con *con = con_by_mark(mark);
-        if (con != NULL) {
-            FREE(con->mark);
-            con->mark_changed = true;
-        }
-        DLOG("Removed window mark \"%s\".\n", mark);
-    }
 
     cmd_output->needs_tree_render = true;
     // XXX: default reply for now, make this a better reply

src/con.c

@@ -519,6 +519,79 @@ Con *con_by_mark(const char *mark) {
     return NULL;
 }
 
+/*
+ * Toggles the mark on a container.
+ * If the container already has this mark, the mark is removed.
+ * Otherwise, the mark is assigned to the container.
+ *
+ */
+void con_mark_toggle(Con *con, const char *mark) {
+    assert(con != NULL);
+    DLOG("Toggling mark \"%s\" on con = %p.\n", mark, con);
+
+    if (con->mark != NULL && strcmp(con->mark, mark) == 0) {
+        con_unmark(mark);
+    } else {
+        con_mark(con, mark);
+    }
+}
+
+/*
+ * Assigns a mark to the container.
+ *
+ */
+void con_mark(Con *con, const char *mark) {
+    assert(con != NULL);
+    DLOG("Setting mark \"%s\" on con = %p.\n", mark, con);
+
+    FREE(con->mark);
+    con->mark = sstrdup(mark);
+    con->mark_changed = true;
+
+    DLOG("Clearing the mark from all other windows.\n");
+    Con *other;
+    TAILQ_FOREACH(other, &all_cons, all_cons) {
+        /* Skip the window we actually handled since we took care of it already. */
+        if (con == other)
+            continue;
+
+        if (other->mark != NULL && strcmp(other->mark, mark) == 0) {
+            FREE(other->mark);
+            other->mark_changed = true;
+        }
+    }
+}
+
+/*
+ * If mark is NULL, this removes all existing marks.
+ * Otherwise, it will only remove the given mark (if it is present).
+ *
+ */
+void con_unmark(const char *mark) {
+    Con *con;
+    if (mark == NULL) {
+        DLOG("Unmarking all containers.\n");
+        TAILQ_FOREACH(con, &all_cons, all_cons) {
+            if (con->mark == NULL)
+                continue;
+
+            FREE(con->mark);
+            con->mark_changed = true;
+        }
+    } else {
+        DLOG("Removing mark \"%s\".\n", mark);
+        con = con_by_mark(mark);
+        if (con == NULL) {
+            DLOG("No container found with this mark, so there is nothing to do.\n");
+            return;
+        }
+
+        DLOG("Found mark on con = %p. Removing it now.\n", con);
+        FREE(con->mark);
+        con->mark_changed = true;
+    }
+}
+
 /*
  * Returns the first container below 'con' which wants to swallow this window
  * TODO: priority

src/load_layout.c

@@ -338,6 +338,10 @@ static int json_string(void *ctx, const unsigned char *val, size_t len) {
         } else if (strcasecmp(last_key, "mark") == 0) {
             char *buf = NULL;
             sasprintf(&buf, "%.*s", (int)len, val);
+
+            /* We unmark any containers using this mark to avoid duplicates. */
+            con_unmark(buf);
+
             json_node->mark = buf;
         } else if (strcasecmp(last_key, "floating") == 0) {
             char *buf = NULL;