tray: Properly handle missing _XEMBED_INFO

The icon not supporting XEMBED is not fatal, we just treat is as always
mapped.

Fixes #2479
Fixes #2442

CHANGELOG.md

@@ -9,6 +9,11 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 ## [Unreleased]
+### Fixed
+- The tray mistakenly removed tray icons that did not support XEMBED
+  ([`#2479`](https://github.com/polybar/polybar/issues/2479),
+  [`#2442`](https://github.com/polybar/polybar/issues/2442))
+
 ## [3.5.6] - 2021-05-24
 ### Build
 - Support building documentation on sphinx 4.0 ([`#2424`](https://github.com/polybar/polybar/issues/2424))

include/x11/tray_client.hpp

@@ -4,12 +4,12 @@
 
 #include "common.hpp"
 #include "utils/concurrency.hpp"
+#include "x11/xembed.hpp"
 
 POLYBAR_NS
 
 // fwd declarations
 class connection;
-struct xembed_data;
 
 class tray_client {
  public:
@@ -28,7 +28,10 @@ class tray_client {
   void mapped(bool state);
 
   xcb_window_t window() const;
-  xembed_data* xembed() const;
+
+  void query_xembed();
+  bool is_xembed_supported() const;
+  const xembed::info& get_xembed() const;
 
   void ensure_state() const;
   void reconfigure(int x, int y) const;
@@ -38,7 +41,18 @@ class tray_client {
   connection& m_connection;
   xcb_window_t m_window{0};
 
-  shared_ptr<xembed_data> m_xembed;
+  /**
+   * Whether the client window supports XEMBED.
+   *
+   * A tray client can still work when it doesn't support XEMBED.
+   */
+  bool m_xembed_supported{false};
+
+  /**
+   * _XEMBED_INFO of the client window
+   */
+  xembed::info m_xembed;
+
   bool m_mapped{false};
 
   unsigned int m_width;

include/x11/tray_manager.hpp

@@ -34,7 +34,6 @@ using namespace std::chrono_literals;
 
 // fwd declarations
 class connection;
-struct xembed_data;
 class background_manager;
 class bg_slice;
 

include/x11/xembed.hpp

@@ -21,24 +21,41 @@ POLYBAR_NS
 #define XEMBED_FOCUS_FIRST 1
 #define XEMBED_FOCUS_LAST 1
 
-struct xembed_data {
+/**
-  unsigned long version;
+ * Max XEMBED version supported.
-  unsigned long flags;
+ */
-  xcb_timestamp_t time;
+#define XEMBED_MAX_VERSION UINT32_C(0)
-  xcb_atom_t xembed;
-  xcb_atom_t xembed_info;
-};
 
+/**
+ * Implementation of parts of the XEMBED spec (as much as needed to get the tray working).
+ *
+ * Ref: https://specifications.freedesktop.org/xembed-spec/xembed-spec-latest.html
+ */
 namespace xembed {
-  xembed_data* query(connection& conn, xcb_window_t win, xembed_data* data);
+
+  class info {
+    public:
+      void set(uint32_t* data);
+
+      uint32_t get_version() const;
+      uint32_t get_flags() const;
+
+      bool is_mapped() const;
+
+    protected:
+      uint32_t version;
+      uint32_t flags;
+  };
+
+  bool query(connection& conn, xcb_window_t win, info& data);
   void send_message(connection& conn, xcb_window_t target, long message, long d1, long d2, long d3);
   void send_focus_event(connection& conn, xcb_window_t target);
-  void notify_embedded(connection& conn, xcb_window_t win, xcb_window_t embedder, long version);
+  void notify_embedded(connection& conn, xcb_window_t win, xcb_window_t embedder, uint32_t version);
   void notify_activated(connection& conn, xcb_window_t win);
   void notify_deactivated(connection& conn, xcb_window_t win);
   void notify_focused(connection& conn, xcb_window_t win, long focus_type);
   void notify_unfocused(connection& conn, xcb_window_t win);
   void unembed(connection& conn, xcb_window_t win, xcb_window_t root);
-}
+}  // namespace xembed
 
 POLYBAR_NS_END

src/x11/tray_client.cpp

@@ -5,16 +5,11 @@
 
 #include "utils/memory.hpp"
 #include "x11/connection.hpp"
-#include "x11/xembed.hpp"
 
 POLYBAR_NS
 
 tray_client::tray_client(connection& conn, xcb_window_t win, unsigned int w, unsigned int h)
-    : m_connection(conn), m_window(win), m_width(w), m_height(h) {
+    : m_connection(conn), m_window(win), m_width(w), m_height(h) {}
-  m_xembed = memory_util::make_malloc_ptr<xembed_data>();
-  m_xembed->version = XEMBED_VERSION;
-  m_xembed->flags = XEMBED_MAPPED;
-}
 
 tray_client::~tray_client() {
   xembed::unembed(m_connection, window(), m_connection.root());
@@ -60,20 +55,31 @@ xcb_window_t tray_client::window() const {
   return m_window;
 }
 
-/**
+void tray_client::query_xembed() {
- * Get xembed data pointer
+  m_xembed_supported = xembed::query(m_connection, m_window, m_xembed);
- */
+}
-xembed_data* tray_client::xembed() const {
+
-  return m_xembed.get();
+bool tray_client::is_xembed_supported() const {
+  return m_xembed_supported;
+}
+
+const xembed::info& tray_client::get_xembed() const {
+  return m_xembed;
 }
 
 /**
  * Make sure that the window mapping state is correct
  */
 void tray_client::ensure_state() const {
-  if (!mapped() && ((xembed()->flags & XEMBED_MAPPED) == XEMBED_MAPPED)) {
+  bool should_be_mapped = true;
+
+  if (is_xembed_supported()) {
+    should_be_mapped = m_xembed.is_mapped();
+  }
+
+  if (!mapped() && should_be_mapped) {
     m_connection.map_window_checked(window());
-  } else if (mapped() && ((xembed()->flags & XEMBED_MAPPED) != XEMBED_MAPPED)) {
+  } else if (mapped() && !should_be_mapped) {
     m_connection.unmap_window_checked(window());
   }
 }

src/x11/tray_manager.cpp

@@ -21,6 +21,12 @@
 #include "x11/winspec.hpp"
 #include "x11/xembed.hpp"
 
+/*
+ * Tray implementation according to the System Tray Protocol.
+ *
+ * Ref: https://specifications.freedesktop.org/systemtray-spec/systemtray-spec-latest.html
+ */
+
 // ====================================================================================================
 //
 // TODO: 32-bit visual
@@ -740,14 +746,20 @@ void tray_manager::process_docking_request(xcb_window_t win) {
   auto& client = m_clients.back();
 
   try {
-    m_log.trace("tray: Get client _XEMBED_INFO");
+    client->query_xembed();
-    xembed::query(m_connection, win, client->xembed());
+  } catch (const xpp::x::error::window& err) {
-  } catch (const std::exception& err) {
     m_log.err("Failed to query _XEMBED_INFO, removing client... (%s)", err.what());
     remove_client(win, true);
     return;
   }
 
+  m_log.trace("tray: xembed = %s", client->is_xembed_supported() ? "true" : "false");
+  if (client->is_xembed_supported()) {
+    m_log.trace("tray: version = 0x%x, flags = 0x%x, XEMBED_MAPPED = %s", client->get_xembed().get_version(),
+        client->get_xembed().get_flags(), client->get_xembed().is_mapped() ? "true" : "false");
+  }
+
+
   try {
     const unsigned int mask = XCB_CW_EVENT_MASK;
     const unsigned int values[]{XCB_EVENT_MASK_PROPERTY_CHANGE | XCB_EVENT_MASK_STRUCTURE_NOTIFY};
@@ -765,10 +777,12 @@ void tray_manager::process_docking_request(xcb_window_t win) {
     m_connection.reparent_window_checked(
         client->window(), m_tray, calculate_client_x(client->window()), calculate_client_y());
 
-    m_log.trace("tray: Send embbeded notification to client");
+    if (client->is_xembed_supported()) {
-    xembed::notify_embedded(m_connection, client->window(), m_tray, client->xembed()->version);
+      m_log.trace("tray: Send embbeded notification to client");
+      xembed::notify_embedded(m_connection, client->window(), m_tray, client->get_xembed().get_version());
+    }
 
-    if (client->xembed()->flags & XEMBED_MAPPED) {
+    if (!client->is_xembed_supported() || client->get_xembed().is_mapped()) {
       m_log.trace("tray: Map client");
       m_connection.map_window_checked(client->window());
     }
@@ -1019,7 +1033,6 @@ void tray_manager::handle(const evt::property_notify& evt) {
 
   m_log.trace("tray: _XEMBED_INFO: %s", m_connection.id(evt->window));
 
-  auto xd = client->xembed();
   auto win = client->window();
 
   if (evt->state == XCB_PROPERTY_NEW_VALUE) {
@@ -1027,20 +1040,17 @@ void tray_manager::handle(const evt::property_notify& evt) {
   }
 
   try {
-    m_log.trace("tray: Get client _XEMBED_INFO");
+    client->query_xembed();
-    xembed::query(m_connection, win, xd);
-  } catch (const application_error& err) {
-    m_log.err(err.what());
-    return;
   } catch (const xpp::x::error::window& err) {
     m_log.err("Failed to query _XEMBED_INFO, removing client... (%s)", err.what());
     remove_client(win, true);
     return;
   }
 
-  m_log.trace("tray: _XEMBED_INFO[0]=%u _XEMBED_INFO[1]=%u", xd->version, xd->flags);
+  m_log.trace("tray: version = 0x%x, flags = 0x%x, XEMBED_MAPPED = %s", client->get_xembed().get_version(),
+      client->get_xembed().get_flags(), client->get_xembed().is_mapped() ? "true" : "false");
 
-  if ((client->xembed()->flags & XEMBED_MAPPED) & XEMBED_MAPPED) {
+  if (client->get_xembed().is_mapped()) {
     reconfigure();
   }
 }

src/x11/xembed.cpp

@@ -5,26 +5,40 @@
 POLYBAR_NS
 
 namespace xembed {
+
+  void info::set(uint32_t* data) {
+    version = data[0];
+    flags = data[1];
+  }
+
+  uint32_t info::get_version() const {
+    return version;
+  }
+
+  uint32_t info::get_flags() const {
+    return flags;
+  }
+
+  bool info::is_mapped() const {
+    return (flags & XEMBED_MAPPED) == XEMBED_MAPPED;
+  }
+
   /**
    * Query _XEMBED_INFO for the given window
+   *
+   * \return Whether valid XEMBED info was found
    */
-  xembed_data* query(connection& conn, xcb_window_t win, xembed_data* data) {
+  bool query(connection& conn, xcb_window_t win, info& data) {
-    auto info = conn.get_property(false, win, _XEMBED_INFO, XCB_GET_PROPERTY_TYPE_ANY, 0L, 2);
+    auto info = conn.get_property(false, win, _XEMBED_INFO, _XEMBED_INFO, 0L, 2);
 
     if (info->value_len == 0) {
-      throw application_error("Invalid _XEMBED_INFO for window " + conn.id(win));
+      return false;
     }
 
-    std::vector<unsigned int> xembed_data{info.value<unsigned int>().begin(), info.value<unsigned int>().end()};
+    std::vector<uint32_t> xembed_data{info.value<uint32_t>().begin(), info.value<uint32_t>().end()};
+    data.set(xembed_data.data());
 
-    data->xembed = _XEMBED;
+    return true;
-    data->xembed_info = _XEMBED_INFO;
-
-    data->time = XCB_CURRENT_TIME;
-    data->flags = xembed_data[1];
-    data->version = xembed_data[0];
-
-    return data;
   }
 
   /**
@@ -53,8 +67,8 @@ namespace xembed {
   /**
    * Acknowledge window embedding
    */
-  void notify_embedded(connection& conn, xcb_window_t win, xcb_window_t embedder, long version) {
+  void notify_embedded(connection& conn, xcb_window_t win, xcb_window_t embedder, uint32_t version) {
-    send_message(conn, win, XEMBED_EMBEDDED_NOTIFY, 0, embedder, version);
+    send_message(conn, win, XEMBED_EMBEDDED_NOTIFY, 0, embedder, std::min(version, XEMBED_MAX_VERSION));
   }
 
   /**