fix(sni): delay tray item insertion until proxies are ready

Only add tray widgets after the SNI proxy has finished initializing and the
item has a valid id/category pair.

This also removes invalid items through the host teardown path, refreshes the
tray when item status changes, and avoids calling DBus methods through a null
proxy during early clicks or scroll events.

Signed-off-by: Austin Horstman <khaneliman12@gmail.com>

include/modules/sni/host.hpp

@@ -16,7 +16,7 @@ class Host {
  public:
   Host(const std::size_t id, const Json::Value&, const Bar&,
        const std::function<void(std::unique_ptr<Item>&)>&,
-       const std::function<void(std::unique_ptr<Item>&)>&);
+       const std::function<void(std::unique_ptr<Item>&)>&, const std::function<void()>&);
   ~Host();
 
  private:
@@ -28,6 +28,10 @@ class Host {
   static void registerHost(GObject*, GAsyncResult*, gpointer);
   static void itemRegistered(SnWatcher*, const gchar*, gpointer);
   static void itemUnregistered(SnWatcher*, const gchar*, gpointer);
+  void itemReady(Item&);
+  void itemInvalidated(Item&);
+  void removeItem(std::vector<std::unique_ptr<Item>>::iterator);
+  void clearItems();
 
   std::tuple<std::string, std::string> getBusNameAndObjectPath(const std::string);
   void addRegisteredItem(const std::string& service);
@@ -43,6 +47,7 @@ class Host {
   const Bar& bar_;
   const std::function<void(std::unique_ptr<Item>&)> on_add_;
   const std::function<void(std::unique_ptr<Item>&)> on_remove_;
+  const std::function<void()> on_update_;
 };
 
 }  // namespace waybar::modules::SNI

include/modules/sni/item.hpp

@@ -11,6 +11,7 @@
 #include <libdbusmenu-gtk/dbusmenu-gtk.h>
 #include <sigc++/trackable.h>
 
+#include <functional>
 #include <set>
 #include <string_view>
 
@@ -25,9 +26,13 @@ struct ToolTip {
 
 class Item : public sigc::trackable {
  public:
-  Item(const std::string&, const std::string&, const Json::Value&, const Bar&);
+  Item(const std::string&, const std::string&, const Json::Value&, const Bar&,
+       const std::function<void(Item&)>&, const std::function<void(Item&)>&,
+       const std::function<void()>&);
   ~Item();
 
+  bool isReady() const;
+
   std::string bus_name;
   std::string object_path;
 
@@ -62,6 +67,8 @@ class Item : public sigc::trackable {
   void proxyReady(Glib::RefPtr<Gio::AsyncResult>& result);
   void setProperty(const Glib::ustring& name, Glib::VariantBase& value);
   void setStatus(const Glib::ustring& value);
+  void setReady();
+  void invalidate();
   void setCustomIcon(const std::string& id);
   void getUpdatedProperties();
   void processUpdatedProperties(Glib::RefPtr<Gio::AsyncResult>& result);
@@ -86,8 +93,13 @@ class Item : public sigc::trackable {
   gdouble distance_scrolled_y_ = 0;
   // visibility of items with Status == Passive
   bool show_passive_ = false;
+  bool ready_ = false;
+  Glib::ustring status_ = "active";
 
   const Bar& bar_;
+  const std::function<void(Item&)> on_ready_;
+  const std::function<void(Item&)> on_invalidate_;
+  const std::function<void()> on_updated_;
 
   Glib::RefPtr<Gio::DBus::Proxy> proxy_;
   Glib::RefPtr<Gio::Cancellable> cancellable_;

include/modules/sni/tray.hpp

@@ -19,6 +19,7 @@ class Tray : public AModule {
  private:
   void onAdd(std::unique_ptr<Item>& item);
   void onRemove(std::unique_ptr<Item>& item);
+  void queueUpdate();
 
   static inline std::size_t nb_hosts_ = 0;
   bool show_passive_ = false;

src/modules/sni/host.cpp

@@ -8,7 +8,8 @@ namespace waybar::modules::SNI {
 
 Host::Host(const std::size_t id, const Json::Value& config, const Bar& bar,
            const std::function<void(std::unique_ptr<Item>&)>& on_add,
-           const std::function<void(std::unique_ptr<Item>&)>& on_remove)
+           const std::function<void(std::unique_ptr<Item>&)>& on_remove,
+           const std::function<void()>& on_update)
     : bus_name_("org.kde.StatusNotifierHost-" + std::to_string(getpid()) + "-" +
                 std::to_string(id)),
       object_path_("/StatusNotifierHost/" + std::to_string(id)),
@@ -17,7 +18,8 @@ Host::Host(const std::size_t id, const Json::Value& config, const Bar& bar,
       config_(config),
       bar_(bar),
       on_add_(on_add),
-      on_remove_(on_remove) {}
+      on_remove_(on_remove),
+      on_update_(on_update) {}
 
 Host::~Host() {
   if (bus_name_id_ > 0) {
@@ -54,7 +56,7 @@ void Host::nameVanished(const Glib::RefPtr<Gio::DBus::Connection>& conn, const G
   g_cancellable_cancel(cancellable_);
   g_clear_object(&cancellable_);
   g_clear_object(&watcher_);
-  items_.clear();
+  clearItems();
 }
 
 void Host::proxyReady(GObject* src, GAsyncResult* res, gpointer data) {
@@ -117,13 +119,50 @@ void Host::itemUnregistered(SnWatcher* watcher, const gchar* service, gpointer d
   auto [bus_name, object_path] = host->getBusNameAndObjectPath(service);
   for (auto it = host->items_.begin(); it != host->items_.end(); ++it) {
     if ((*it)->bus_name == bus_name && (*it)->object_path == object_path) {
-      host->on_remove_(*it);
+      host->removeItem(it);
-      host->items_.erase(it);
       break;
     }
   }
 }
 
+void Host::itemReady(Item& item) {
+  auto it = std::find_if(items_.begin(), items_.end(),
+                         [&item](const auto& candidate) { return candidate.get() == &item; });
+  if (it != items_.end() && (*it)->isReady()) {
+    on_add_(*it);
+  }
+}
+
+void Host::itemInvalidated(Item& item) {
+  auto it = std::find_if(items_.begin(), items_.end(),
+                         [&item](const auto& candidate) { return candidate.get() == &item; });
+  if (it != items_.end()) {
+    removeItem(it);
+  }
+}
+
+void Host::removeItem(std::vector<std::unique_ptr<Item>>::iterator it) {
+  if ((*it)->isReady()) {
+    on_remove_(*it);
+  }
+  items_.erase(it);
+}
+
+void Host::clearItems() {
+  bool removed_ready_item = false;
+  for (auto& item : items_) {
+    if (item->isReady()) {
+      on_remove_(item);
+      removed_ready_item = true;
+    }
+  }
+  bool had_items = !items_.empty();
+  items_.clear();
+  if (had_items && !removed_ready_item) {
+    on_update_();
+  }
+}
+
 std::tuple<std::string, std::string> Host::getBusNameAndObjectPath(const std::string service) {
   auto it = service.find('/');
   if (it != std::string::npos) {
@@ -139,8 +178,9 @@ void Host::addRegisteredItem(const std::string& service) {
     return bus_name == item->bus_name && object_path == item->object_path;
   });
   if (it == items_.end()) {
-    items_.emplace_back(new Item(bus_name, object_path, config_, bar_));
+    items_.emplace_back(new Item(
-    on_add_(items_.back());
+        bus_name, object_path, config_, bar_, [this](Item& item) { itemReady(item); },
+        [this](Item& item) { itemInvalidated(item); }, on_update_));
   }
 }
 

src/modules/sni/item.cpp

@@ -37,13 +37,18 @@ namespace waybar::modules::SNI {
 static const Glib::ustring SNI_INTERFACE_NAME = sn_item_interface_info()->name;
 static const unsigned UPDATE_DEBOUNCE_TIME = 10;
 
-Item::Item(const std::string& bn, const std::string& op, const Json::Value& config, const Bar& bar)
+Item::Item(const std::string& bn, const std::string& op, const Json::Value& config, const Bar& bar,
+           const std::function<void(Item&)>& on_ready,
+           const std::function<void(Item&)>& on_invalidate, const std::function<void()>& on_updated)
     : bus_name(bn),
       object_path(op),
       icon_size(16),
       effective_icon_size(0),
       icon_theme(Gtk::IconTheme::create()),
-      bar_(bar) {
+      bar_(bar),
+      on_ready_(on_ready),
+      on_invalidate_(on_invalidate),
+      on_updated_(on_updated) {
   if (config["icon-size"].isUInt()) {
     icon_size = config["icon-size"].asUInt();
   }
@@ -85,6 +90,8 @@ Item::~Item() {
   }
 }
 
+bool Item::isReady() const { return ready_; }
+
 bool Item::handleMouseEnter(GdkEventCrossing* const& e) {
   event_box.set_state_flags(Gtk::StateFlags::STATE_FLAG_PRELIGHT);
   return false;
@@ -112,14 +119,18 @@ void Item::proxyReady(Glib::RefPtr<Gio::AsyncResult>& result) {
 
     if (this->id.empty() || this->category.empty()) {
       spdlog::error("Invalid Status Notifier Item: {}, {}", bus_name, object_path);
+      invalidate();
       return;
     }
     this->updateImage();
+    setReady();
 
   } catch (const Glib::Error& err) {
     spdlog::error("Failed to create DBus Proxy for {} {}: {}", bus_name, object_path, err.what());
+    invalidate();
   } catch (const std::exception& err) {
     spdlog::error("Failed to create DBus Proxy for {} {}: {}", bus_name, object_path, err.what());
+    invalidate();
   }
 }
 
@@ -217,18 +228,35 @@ void Item::setProperty(const Glib::ustring& name, Glib::VariantBase& value) {
 }
 
 void Item::setStatus(const Glib::ustring& value) {
-  Glib::ustring lower = value.lowercase();
+  status_ = value.lowercase();
-  event_box.set_visible(show_passive_ || lower.compare("passive") != 0);
+  event_box.set_visible(show_passive_ || status_.compare("passive") != 0);
 
   auto style = event_box.get_style_context();
   for (const auto& class_name : style->list_classes()) {
     style->remove_class(class_name);
   }
-  if (lower.compare("needsattention") == 0) {
+  auto css_class = status_;
+  if (css_class.compare("needsattention") == 0) {
     // convert status to dash-case for CSS
-    lower = "needs-attention";
+    css_class = "needs-attention";
   }
-  style->add_class(lower);
+  style->add_class(css_class);
+  on_updated_();
+}
+
+void Item::setReady() {
+  if (ready_) {
+    return;
+  }
+  ready_ = true;
+  on_ready_(*this);
+}
+
+void Item::invalidate() {
+  if (ready_) {
+    ready_ = false;
+  }
+  on_invalidate_(*this);
 }
 
 void Item::setCustomIcon(const std::string& id) {
@@ -464,6 +492,9 @@ void Item::makeMenu() {
 }
 
 bool Item::handleClick(GdkEventButton* const& ev) {
+  if (!proxy_) {
+    return false;
+  }
   auto parameters = Glib::VariantContainerBase::create_tuple(
       {Glib::Variant<int>::create(ev->x_root + bar_.x_global),
        Glib::Variant<int>::create(ev->y_root + bar_.y_global)});
@@ -491,6 +522,9 @@ bool Item::handleClick(GdkEventButton* const& ev) {
 }
 
 bool Item::handleScroll(GdkEventScroll* const& ev) {
+  if (!proxy_) {
+    return false;
+  }
   int dx = 0, dy = 0;
   switch (ev->direction) {
     case GDK_SCROLL_UP:

src/modules/sni/tray.cpp

@@ -13,7 +13,8 @@ Tray::Tray(const std::string& id, const Bar& bar, const Json::Value& config)
       box_(bar.orientation, 0),
       watcher_(SNI::Watcher::getInstance()),
       host_(nb_hosts_, config, bar, std::bind(&Tray::onAdd, this, std::placeholders::_1),
-            std::bind(&Tray::onRemove, this, std::placeholders::_1)) {
+            std::bind(&Tray::onRemove, this, std::placeholders::_1),
+            std::bind(&Tray::queueUpdate, this)) {
   box_.set_name("tray");
   event_box_.add(box_);
   if (!id.empty()) {
@@ -33,6 +34,8 @@ Tray::Tray(const std::string& id, const Bar& bar, const Json::Value& config)
   dp.emit();
 }
 
+void Tray::queueUpdate() { dp.emit(); }
+
 void Tray::onAdd(std::unique_ptr<Item>& item) {
   if (config_["reverse-direction"].isBool() && config_["reverse-direction"].asBool()) {
     box_.pack_end(item->event_box);