Merge pull request #3697 from orestisf1993/ewmh

Fix ewmh atom issues

include/ewmh.h

@@ -11,6 +11,12 @@
 
 #include <config.h>
 
+/**
+ * Updates all the EWMH desktop properties.
+ *
+ */
+void ewmh_update_desktop_properties(void);
+
 /**
  * Updates _NET_CURRENT_DESKTOP with the current desktop number.
  *
@@ -20,24 +26,6 @@
  */
 void ewmh_update_current_desktop(void);
 
-/**
- * Updates _NET_NUMBER_OF_DESKTOPS which we interpret as the number of
- * noninternal workspaces.
- */
-void ewmh_update_number_of_desktops(void);
-
-/**
- * Updates _NET_DESKTOP_NAMES: "The names of all virtual desktops. This is a
- * list of NULL-terminated strings in UTF-8 encoding"
- */
-void ewmh_update_desktop_names(void);
-
-/**
- * Updates _NET_DESKTOP_VIEWPORT, which is an array of pairs of cardinals that
- * define the top left corner of each desktop's viewport.
- */
-void ewmh_update_desktop_viewport(void);
-
 /**
  * Updates _NET_WM_DESKTOP for all windows.
  * A request will only be made if the cached value differs from the calculated value.

src/commands.c

@@ -2002,9 +2002,7 @@ void cmd_rename_workspace(I3_CMD, const char *old_name, const char *new_name) {
     cmd_output->needs_tree_render = true;
     ysuccess(true);
 
-    ewmh_update_desktop_names();
+    ewmh_update_desktop_properties();
-    ewmh_update_desktop_viewport();
-    ewmh_update_current_desktop();
 
     startup_sequence_rename_workspace(old_name_copy, new_name);
     free(old_name_copy);

src/ewmh.c

@@ -11,6 +11,11 @@
 
 xcb_window_t ewmh_window;
 
+#define FOREACH_NONINTERNAL                                             \
+    TAILQ_FOREACH(output, &(croot->nodes_head), nodes)                  \
+    TAILQ_FOREACH(ws, &(output_get_content(output)->nodes_head), nodes) \
+    if (!con_is_internal(ws))
+
 /*
  * Updates _NET_CURRENT_DESKTOP with the current desktop number.
  *
@@ -19,28 +24,34 @@ xcb_window_t ewmh_window;
  *
  */
 void ewmh_update_current_desktop(void) {
+    static uint32_t old_idx = NET_WM_DESKTOP_NONE;
     const uint32_t idx = ewmh_get_workspace_index(focused);
-    if (idx != NET_WM_DESKTOP_NONE) {
+
-        xcb_change_property(conn, XCB_PROP_MODE_REPLACE, root, A__NET_CURRENT_DESKTOP, XCB_ATOM_CARDINAL, 32, 1, &idx);
+    if (idx == old_idx || idx == NET_WM_DESKTOP_NONE) {
+        return;
     }
+    old_idx = idx;
+
+    xcb_change_property(conn, XCB_PROP_MODE_REPLACE, root, A__NET_CURRENT_DESKTOP, XCB_ATOM_CARDINAL, 32, 1, &idx);
 }
 
 /*
  * Updates _NET_NUMBER_OF_DESKTOPS which we interpret as the number of
  * noninternal workspaces.
  */
-void ewmh_update_number_of_desktops(void) {
+static void ewmh_update_number_of_desktops(void) {
-    Con *output;
+    Con *output, *ws;
+    static uint32_t old_idx = 0;
     uint32_t idx = 0;
 
-    TAILQ_FOREACH(output, &(croot->nodes_head), nodes) {
+    FOREACH_NONINTERNAL {
-        Con *ws;
+        idx++;
-        TAILQ_FOREACH(ws, &(output_get_content(output)->nodes_head), nodes) {
+    };
-            if (STARTS_WITH(ws->name, "__"))
+
-                continue;
+    if (idx == old_idx) {
-            ++idx;
+        return;
-        }
     }
+    old_idx = idx;
 
     xcb_change_property(conn, XCB_PROP_MODE_REPLACE, root,
                         A__NET_NUMBER_OF_DESKTOPS, XCB_ATOM_CARDINAL, 32, 1, &idx);
@@ -50,35 +61,24 @@ void ewmh_update_number_of_desktops(void) {
  * Updates _NET_DESKTOP_NAMES: "The names of all virtual desktops. This is a
  * list of NULL-terminated strings in UTF-8 encoding"
  */
-void ewmh_update_desktop_names(void) {
+static void ewmh_update_desktop_names(void) {
-    Con *output;
+    Con *output, *ws;
     int msg_length = 0;
 
     /* count the size of the property message to set */
-    TAILQ_FOREACH(output, &(croot->nodes_head), nodes) {
+    FOREACH_NONINTERNAL {
-        Con *ws;
-        TAILQ_FOREACH(ws, &(output_get_content(output)->nodes_head), nodes) {
-            if (STARTS_WITH(ws->name, "__"))
-                continue;
         msg_length += strlen(ws->name) + 1;
-        }
+    };
-    }
 
     char desktop_names[msg_length];
     int current_position = 0;
 
     /* fill the buffer with the names of the i3 workspaces */
-    TAILQ_FOREACH(output, &(croot->nodes_head), nodes) {
+    FOREACH_NONINTERNAL {
-        Con *ws;
-        TAILQ_FOREACH(ws, &(output_get_content(output)->nodes_head), nodes) {
-            if (STARTS_WITH(ws->name, "__"))
-                continue;
-
         for (size_t i = 0; i < strlen(ws->name) + 1; i++) {
             desktop_names[current_position++] = ws->name[i];
         }
     }
-    }
 
     xcb_change_property(conn, XCB_PROP_MODE_REPLACE, root,
                         A__NET_DESKTOP_NAMES, A_UTF8_STRING, 8, msg_length, desktop_names);
@@ -88,39 +88,39 @@ void ewmh_update_desktop_names(void) {
  * Updates _NET_DESKTOP_VIEWPORT, which is an array of pairs of cardinals that
  * define the top left corner of each desktop's viewport.
  */
-void ewmh_update_desktop_viewport(void) {
+static void ewmh_update_desktop_viewport(void) {
-    Con *output;
+    Con *output, *ws;
     int num_desktops = 0;
     /* count number of desktops */
-    TAILQ_FOREACH(output, &(croot->nodes_head), nodes) {
+    FOREACH_NONINTERNAL {
-        Con *ws;
-        TAILQ_FOREACH(ws, &(output_get_content(output)->nodes_head), nodes) {
-            if (STARTS_WITH(ws->name, "__"))
-                continue;
-
         num_desktops++;
     }
-    }
 
     uint32_t viewports[num_desktops * 2];
 
     int current_position = 0;
     /* fill the viewport buffer */
-    TAILQ_FOREACH(output, &(croot->nodes_head), nodes) {
+    FOREACH_NONINTERNAL {
-        Con *ws;
-        TAILQ_FOREACH(ws, &(output_get_content(output)->nodes_head), nodes) {
-            if (STARTS_WITH(ws->name, "__"))
-                continue;
-
         viewports[current_position++] = output->rect.x;
         viewports[current_position++] = output->rect.y;
     }
-    }
 
     xcb_change_property(conn, XCB_PROP_MODE_REPLACE, root,
                         A__NET_DESKTOP_VIEWPORT, XCB_ATOM_CARDINAL, 32, current_position, &viewports);
 }
 
+/*
+ * Updates all the EWMH desktop properties.
+ *
+ */
+void ewmh_update_desktop_properties(void) {
+    ewmh_update_number_of_desktops();
+    ewmh_update_desktop_viewport();
+    ewmh_update_current_desktop();
+    ewmh_update_desktop_names();
+    ewmh_update_wm_desktop();
+}
+
 static void ewmh_update_wm_desktop_recursively(Con *con, const uint32_t desktop) {
     Con *child;
 
@@ -354,18 +354,12 @@ Con *ewmh_get_workspace_by_index(uint32_t idx) {
 
     uint32_t current_index = 0;
 
-    Con *output;
+    Con *output, *ws;
-    TAILQ_FOREACH(output, &(croot->nodes_head), nodes) {
+    FOREACH_NONINTERNAL {
-        Con *workspace;
+        if (current_index == idx) {
-        TAILQ_FOREACH(workspace, &(output_get_content(output)->nodes_head), nodes) {
+            return ws;
-            if (con_is_internal(workspace))
-                continue;
-
-            if (current_index == idx)
-                return workspace;
-
-            ++current_index;
         }
+        current_index++;
     }
 
     return NULL;
@@ -381,19 +375,14 @@ Con *ewmh_get_workspace_by_index(uint32_t idx) {
 uint32_t ewmh_get_workspace_index(Con *con) {
     uint32_t index = 0;
 
-    Con *workspace = con_get_workspace(con);
+    Con *target_workspace = con_get_workspace(con);
-    Con *output;
+    Con *output, *ws;
-    TAILQ_FOREACH(output, &(croot->nodes_head), nodes) {
+    FOREACH_NONINTERNAL {
-        Con *current;
+        if (ws == target_workspace) {
-        TAILQ_FOREACH(current, &(output_get_content(output)->nodes_head), nodes) {
-            if (con_is_internal(current))
-                continue;
-
-            if (current == workspace)
             return index;
-
-            ++index;
         }
+
+        index++;
     }
 
     return NET_WM_DESKTOP_NONE;

src/main.c

@@ -852,10 +852,7 @@ int main(int argc, char *argv[]) {
     ewmh_update_workarea();
 
     /* Set the ewmh desktop properties. */
-    ewmh_update_current_desktop();
+    ewmh_update_desktop_properties();
-    ewmh_update_number_of_desktops();
-    ewmh_update_desktop_names();
-    ewmh_update_desktop_viewport();
 
     struct ev_io *xcb_watcher = scalloc(1, sizeof(struct ev_io));
     xcb_prepare = scalloc(1, sizeof(struct ev_prepare));

src/tree.c

@@ -300,10 +300,8 @@ bool tree_close_internal(Con *con, kill_window_t kill_window, bool dont_kill_par
     x_con_kill(con);
 
     if (ws == con) {
-        DLOG("Closing a workspace container, updating EWMH atoms\n");
+        DLOG("Closing workspace container %s, updating EWMH atoms\n", ws->name);
-        ewmh_update_number_of_desktops();
+        ewmh_update_desktop_properties();
-        ewmh_update_desktop_names();
-        ewmh_update_wm_desktop();
     }
 
     con_free(con);

src/workspace.c

@@ -161,10 +161,7 @@ Con *workspace_get(const char *num, bool *created) {
         con_attach(workspace, content, false);
 
         ipc_send_workspace_event("init", workspace, NULL);
-        ewmh_update_number_of_desktops();
+        ewmh_update_desktop_properties();
-        ewmh_update_desktop_names();
-        ewmh_update_desktop_viewport();
-        ewmh_update_wm_desktop();
         if (created != NULL)
             *created = true;
     } else if (created != NULL) {
@@ -520,10 +517,7 @@ void workspace_show(Con *workspace) {
                 old_focus = NULL;
             }
 
-            ewmh_update_number_of_desktops();
+            ewmh_update_desktop_properties();
-            ewmh_update_desktop_names();
-            ewmh_update_desktop_viewport();
-            ewmh_update_wm_desktop();
         }
     }
 

testcases/t/294-update-ewmh-atoms.t

@@ -101,11 +101,19 @@ is_deeply(\@actual_names, \@expected_names);
 # Kill first window to close a workspace.
 cmd '[id="' . $second->id . '"] kill';
 
-is(get_current_desktop, 2, '_NET_CURRENT_DESKTOP should be updated');
+is(get_current_desktop, 1, '_NET_CURRENT_DESKTOP should be updated');
 is(get_num_of_desktops, 2, '_NET_NUMBER_OF_DESKTOPS should be updated');
 my @actual_names = get_desktop_names;
 my @expected_names = ('0', '2');
 is_deeply(\@actual_names, \@expected_names, '_NET_DESKTOP_NAMES should be updated');
 
+# Rename workspace to reorder them.
+cmd 'rename workspace 0 to 5';
+
+is(get_current_desktop, 0, '_NET_CURRENT_DESKTOP should be updated');
+is(get_num_of_desktops, 2, '_NET_NUMBER_OF_DESKTOPS should remain the same');
+my @actual_names = get_desktop_names;
+my @expected_names = ('2', '5');
+is_deeply(\@actual_names, \@expected_names, '_NET_DESKTOP_NAMES should be updated');
 
 done_testing;