From 63c6b13cbffeebe179be62687000ba548e4de2d2 Mon Sep 17 00:00:00 2001
From: patrick96 <p.ziegler96@gmail.com>
Date: Wed, 14 Sep 2022 22:12:13 +0200
Subject: [PATCH] tray: Cleanup client state handling

---
 include/x11/tray_client.hpp  |  9 ++++++-
 include/x11/tray_manager.hpp | 13 ++++-----
 src/x11/tray_client.cpp      | 32 +++++++++++++---------
 src/x11/tray_manager.cpp     | 51 ++++++++++++++++++------------------
 4 files changed, 61 insertions(+), 44 deletions(-)

diff --git a/include/x11/tray_client.hpp b/include/x11/tray_client.hpp
index 84fa44f8..ec9d4109 100644
--- a/include/x11/tray_client.hpp
+++ b/include/x11/tray_client.hpp
@@ -40,6 +40,8 @@ class tray_client : public non_copyable_mixin {
 
   void hidden(bool state);
 
+  bool should_be_mapped() const;
+
   xcb_window_t embedder() const;
   xcb_window_t client() const;
 
@@ -97,9 +99,14 @@ class tray_client : public non_copyable_mixin {
    */
   xembed::info m_xembed;
 
-  // TODO
+  /**
+   * Whether the wrapper window is currently mapped.
+   */
   bool m_mapped{false};
 
+  /**
+   * Whether the
+   */
   bool m_hidden{false};
 
   size m_size;
diff --git a/include/x11/tray_manager.hpp b/include/x11/tray_manager.hpp
index 9b7c1cff..391390f7 100644
--- a/include/x11/tray_manager.hpp
+++ b/include/x11/tray_manager.hpp
@@ -58,9 +58,9 @@ struct tray_settings {
 
 using on_update = std::function<void(void)>;
 
-class tray_manager : public xpp::event::sink<evt::expose, evt::visibility_notify, evt::client_message,
-                         evt::configure_request, evt::resize_request, evt::selection_clear, evt::property_notify,
-                         evt::reparent_notify, evt::destroy_notify, evt::map_notify, evt::unmap_notify>,
+class tray_manager : public xpp::event::sink<evt::expose, evt::client_message, evt::configure_request,
+                         evt::resize_request, evt::selection_clear, evt::property_notify, evt::reparent_notify,
+                         evt::destroy_notify, evt::map_notify, evt::unmap_notify>,
                      public signal_receiver<SIGN_PRIORITY_TRAY, signals::ui::update_background,
                          signals::ui_tray::tray_pos_change, signals::ui_tray::tray_visibility> {
  public:
@@ -81,6 +81,8 @@ class tray_manager : public xpp::event::sink<evt::expose, evt::visibility_notify
   bool is_inactive() const;
   bool is_waiting() const;
 
+  bool is_visible() const;
+
  protected:
   void reconfigure_window();
   void reconfigure_clients();
@@ -115,12 +117,11 @@ class tray_manager : public xpp::event::sink<evt::expose, evt::visibility_notify
 
   bool is_embedded(const xcb_window_t& win);
   tray_client* find_client(const xcb_window_t& win);
-  void remove_client(const tray_client& client, bool reconfigure = true);
-  void remove_client(xcb_window_t win, bool reconfigure = true);
+  void remove_client(const tray_client& client);
+  void remove_client(xcb_window_t win);
   bool change_visibility(bool visible);
 
   void handle(const evt::expose& evt) override;
-  void handle(const evt::visibility_notify& evt) override;
   void handle(const evt::client_message& evt) override;
   void handle(const evt::configure_request& evt) override;
   void handle(const evt::resize_request& evt) override;
diff --git a/src/x11/tray_client.cpp b/src/x11/tray_client.cpp
index f13fcd51..758527ab 100644
--- a/src/x11/tray_client.cpp
+++ b/src/x11/tray_client.cpp
@@ -167,6 +167,21 @@ void tray_client::hidden(bool state) {
   m_hidden = state;
 }
 
+/**
+ * Whether the current state indicates the client should be mapped.
+ */
+bool tray_client::should_be_mapped() const {
+  if (m_hidden) {
+    return false;
+  }
+
+  if (is_xembed_supported()) {
+    return m_xembed.is_mapped();
+  }
+
+  return true;
+}
+
 xcb_window_t tray_client::embedder() const {
   return m_wrapper;
 }
@@ -209,22 +224,15 @@ void tray_client::add_to_save_set() const {
  * Make sure that the window mapping state is correct
  */
 void tray_client::ensure_state() const {
-  bool should_be_mapped = true;
+  bool new_state = should_be_mapped();
 
-  if (is_xembed_supported()) {
-    should_be_mapped = m_xembed.is_mapped();
+  if (new_state == m_mapped) {
+    return;
   }
 
-  if (m_hidden) {
-    should_be_mapped = false;
-  }
+  m_log.trace("%s: ensure_state (hidden=%i, mapped=%i, should_be_mapped=%i)", name(), m_hidden, m_mapped, new_state);
 
-  // TODO can we stop here if should_be_mapped == m_mapped?
-
-  m_log.trace(
-      "%s: ensure_state (hidden=%i, mapped=%i, should_be_mapped=%i)", name(), m_hidden, m_mapped, should_be_mapped);
-
-  if (should_be_mapped) {
+  if (new_state) {
     m_log.trace("%s: Map client", name());
     m_connection.map_window_checked(embedder());
     m_connection.map_window_checked(client());
diff --git a/src/x11/tray_manager.cpp b/src/x11/tray_manager.cpp
index e58fe819..1f4bf802 100644
--- a/src/x11/tray_manager.cpp
+++ b/src/x11/tray_manager.cpp
@@ -115,6 +115,10 @@ bool tray_manager::is_waiting() const {
   return m_state == state::WAITING;
 }
 
+bool tray_manager::is_visible() const {
+  return is_active() && !m_hidden;
+}
+
 /**
  * Activate systray management
  */
@@ -268,13 +272,13 @@ void tray_manager::reconfigure_clients() {
   for (auto it = m_clients.rbegin(); it != m_clients.rend(); it++) {
     try {
       it->ensure_state();
+      // TODO skip if the client isn't mapped
       it->reconfigure(x, calculate_client_y());
 
       x += m_opts.client_size.w + m_opts.spacing;
     } catch (const xpp::x::error::window& err) {
-      // TODO print error
       m_log.err("Failed to reconfigure %s, removing ... (%s)", it->name(), err.what());
-      remove_client(*it, false);
+      remove_client(*it);
     }
   }
 }
@@ -283,8 +287,7 @@ void tray_manager::reconfigure_clients() {
  * Refresh the bar window by clearing it along with each client window
  */
 void tray_manager::refresh_window() {
-  // TODO create method that checks is_active and !m_hidden
-  if (!is_active() || m_hidden) {
+  if (!is_visible()) {
     return;
   }
 
@@ -502,19 +505,19 @@ tray_client* tray_manager::find_client(const xcb_window_t& win) {
 /**
  * Remove tray client
  */
-void tray_manager::remove_client(const tray_client& client, bool reconfigure) {
-  remove_client(client.client(), reconfigure);
+void tray_manager::remove_client(const tray_client& client) {
+  remove_client(client.client());
 }
 
 /**
  * Remove tray client by window
  */
-void tray_manager::remove_client(xcb_window_t win, bool reconfigure) {
+void tray_manager::remove_client(xcb_window_t win) {
+  auto old_size = m_clients.size();
   m_clients.erase(
       std::remove_if(m_clients.begin(), m_clients.end(), [win](const auto& client) { return client.match(win); }));
 
-  // TODO remove param. Reconfigure if clients were deleted
-  if (reconfigure) {
+  if (old_size != m_clients.size()) {
     tray_manager::reconfigure();
   }
 }
@@ -551,17 +554,6 @@ void tray_manager::handle(const evt::expose& evt) {
   }
 }
 
-/**
- * Event callback : XCB_VISIBILITY_NOTIFY
- */
-void tray_manager::handle(const evt::visibility_notify& evt) {
-  // TODO for which windows is this important?
-  if (is_active() && !m_clients.empty()) {
-    m_log.trace("tray: Received visibility_notify for %s", m_connection.id(evt->window));
-    reconfigure_window();
-  }
-}
-
 /**
  * Event callback : XCB_CLIENT_MESSAGE
  */
@@ -669,14 +661,11 @@ void tray_manager::handle(const evt::property_notify& evt) {
     client->query_xembed();
   } catch (const xpp::x::error::window& err) {
     m_log.err("Failed to query _XEMBED_INFO, removing %s ... (%s)", client->name(), err.what());
-    remove_client(*client, true);
+    remove_client(*client);
     return;
   }
 
-  // TODO only reconfigure if should_be_mapped changed
-  if (client->get_xembed().is_mapped()) {
-    reconfigure();
-  }
+  client->ensure_state();
 }
 
 /**
@@ -723,6 +712,12 @@ void tray_manager::handle(const evt::map_notify& evt) {
     redraw_window();
   } else if (is_embedded(evt->window)) {
     auto client = find_client(evt->window);
+
+    // If we received a notification on the wrapped window, we don't want to do anything.
+    if (client->embedder() != evt->window) {
+      return;
+    }
+
     m_log.trace("%s: Received map_notify", client->name());
 
     if (!client->mapped()) {
@@ -738,6 +733,12 @@ void tray_manager::handle(const evt::map_notify& evt) {
 void tray_manager::handle(const evt::unmap_notify& evt) {
   if (is_active() && is_embedded(evt->window)) {
     auto client = find_client(evt->window);
+
+    // If we received a notification on the wrapped window, we don't want to do anything.
+    if (client->embedder() != evt->window) {
+      return;
+    }
+
     m_log.trace("%s: Received unmap_notify", client->name());
 
     if (client->mapped()) {