diff --git a/src/dbus/tray/tray_service.cpp b/src/dbus/tray/tray_service.cpp index afdc0d920..a821fa09a 100644 --- a/src/dbus/tray/tray_service.cpp +++ b/src/dbus/tray/tray_service.cpp @@ -191,6 +191,19 @@ namespace { return std::nullopt; } + bool hasInt32ChildrenInVariant(const sdbus::Variant& value) { + if (value.containsValueOfType>()) { + return !value.get>().empty(); + } + if (value.containsValueOfType>()) { + return !value.get>().empty(); + } + if (value.containsValueOfType>()) { + return !value.get>().empty(); + } + return false; + } + std::vector bytesFromVariant(const sdbus::Variant& value) { try { return value.get>(); @@ -282,6 +295,14 @@ namespace { resetMenuEntryProperty(out, "children-display"); } + // Some providers omit children-display but still populate children ids. + // Treat a non-empty children vector as submenu-capable to match qs behavior. + if (const auto it = props.find("children"); it != props.end()) { + if (hasInt32ChildrenInVariant(it->second)) { + out.hasSubmenu = true; + } + } + if (const auto it = props.find("toggle-type"); it != props.end()) { if (const auto value = stringFromVariant(it->second); value.has_value()) { out.checkmark = (*value == "checkmark"); @@ -303,7 +324,8 @@ namespace { TrayMenuEntry decodeMenuEntry(const DbusMenuLayout& entryLayout) { TrayMenuEntry out; out.id = std::get<0>(entryLayout); - applyMenuEntryProperties(out, std::get<1>(entryLayout), true); + const auto& props = std::get<1>(entryLayout); + applyMenuEntryProperties(out, props, true); return out; } @@ -319,6 +341,12 @@ namespace { return true; } + const std::vector& requestedMenuProperties() { + // Per dbusmenu protocol, an empty property list means "all available properties". + static const std::vector kRequestedMenuProperties = {}; + return kRequestedMenuProperties; + } + std::vector int32ListFromVariant(const sdbus::Variant& value) { try { return value.get>(); @@ -635,7 +663,7 @@ bool TrayService::fetchMenuProperties(const std::string& itemId, const std::vect cache.proxy->callMethod("GetGroupProperties") .onInterface(k_menu_interface) .withTimeout(std::chrono::milliseconds(1000)) - .withArguments(entryIds, std::vector{}) + .withArguments(entryIds, requestedMenuProperties()) .storeResultsTo(properties); std::unordered_map> propertiesById; @@ -678,9 +706,45 @@ void TrayService::requestMenuSubtree(const std::string& itemId, std::int32_t par return; } + const auto now = std::chrono::steady_clock::now(); + if (const auto retryIt = cache.nextRetryAt.find(parentId); + retryIt != cache.nextRetryAt.end() && now < retryIt->second) { + return; + } + cache.loadingParents.insert(parentId); const auto generation = cache.generation; + // Root menus for some indicators never reply to AboutToShow but still serve GetLayout. + // Skip AboutToShow at root to avoid NoReply stalls and request storms. + if (parentId == 0) { + requestMenuLayoutAfterAboutToShow(itemId, parentId, generation); + + // Run root AboutToShow only once per cache lifetime. Some providers emit + // repeated LayoutUpdated storms when this is called every open. + if (!cache.rootAboutToShowPrimed) { + cache.rootAboutToShowPrimed = true; + try { + cache.proxy->callMethodAsync("AboutToShow") + .onInterface(k_menu_interface) + .withTimeout(std::chrono::milliseconds(500)) + .withArguments(parentId) + .uponReplyInvoke( + [this, itemId, parentId, generation](std::optional error, bool /*needsUpdate*/) { + if (error.has_value()) { + kLog.debug("root AboutToShow failed id={} parent={} err={}", itemId, parentId, error->what()); + } else { + kLog.debug("root AboutToShow ok id={} parent={}", itemId, parentId); + requestMenuLayoutAfterAboutToShow(itemId, parentId, generation); + } + }); + } catch (const sdbus::Error& e) { + kLog.debug("root AboutToShow async setup failed id={} parentId={} err={}", itemId, parentId, e.what()); + } + } + return; + } + try { cache.proxy->callMethodAsync("AboutToShow") .onInterface(k_menu_interface) @@ -688,7 +752,7 @@ void TrayService::requestMenuSubtree(const std::string& itemId, std::int32_t par .withArguments(parentId) .uponReplyInvoke([this, itemId, parentId, generation](std::optional error, bool /*needsUpdate*/) { if (error.has_value()) { - kLog.debug("AboutToShow async failed id={} parentId={} err={}", itemId, parentId, error->what()); + kLog.debug("AboutToShow failed id={} parent={} err={}", itemId, parentId, error->what()); } requestMenuLayoutAfterAboutToShow(itemId, parentId, generation); }); @@ -713,7 +777,7 @@ void TrayService::requestMenuLayoutAfterAboutToShow(const std::string& itemId, s cache.proxy->callMethodAsync("GetLayout") .onInterface(k_menu_interface) .withTimeout(std::chrono::milliseconds(2000)) - .withArguments(parentId, static_cast(-1), std::vector{}) + .withArguments(parentId, static_cast(-1), requestedMenuProperties()) .uponReplyInvoke([this, itemId, parentId, generation](std::optional error, std::uint32_t revision, DbusMenuLayout layout) { auto replyCacheIt = m_menuCache.find(itemId); @@ -729,22 +793,39 @@ void TrayService::requestMenuLayoutAfterAboutToShow(const std::string& itemId, s replyCache.loadingParents.erase(parentId); if (error.has_value()) { - kLog.debug("GetLayout async failed id={} parentId={} err={}", itemId, parentId, error->what()); + std::uint8_t& streak = replyCache.failureStreak[parentId]; + streak = static_cast(std::min(4, static_cast(streak) + 1)); + const int exponent = std::min(4, static_cast(streak)); + const auto backoff = std::chrono::milliseconds(250 * (1 << exponent)); + replyCache.nextRetryAt[parentId] = std::chrono::steady_clock::now() + backoff; + kLog.debug("GetLayout failed id={} parent={} err={} streak={} backoffMs={}", itemId, parentId, + error->what(), streak, backoff.count()); return; } + replyCache.failureStreak.erase(parentId); + replyCache.nextRetryAt.erase(parentId); + replyCache.revision = revision; ingestLayoutNode(layout, replyCache.entriesById, replyCache.childrenByParent); + const auto layoutRootId = std::get<0>(layout); + if (layoutRootId != parentId) { + if (const auto rootChildrenIt = replyCache.childrenByParent.find(layoutRootId); + rootChildrenIt != replyCache.childrenByParent.end()) { + replyCache.childrenByParent[parentId] = rootChildrenIt->second; + } + } + auto after = entriesForParent(replyCache.entriesById, replyCache.childrenByParent, parentId); if (after.empty()) { - const auto childIds = childIdsFromLayoutProperties(layout); - if (!childIds.empty()) { - replyCache.childrenByParent[parentId] = childIds; + const auto layoutChildIds = childIdsFromLayoutProperties(layout); + if (!layoutChildIds.empty()) { + replyCache.childrenByParent[parentId] = layoutChildIds; std::vector propertyEntries; - if (fetchMenuProperties(itemId, childIds, propertyEntries)) { + if (fetchMenuProperties(itemId, layoutChildIds, propertyEntries)) { kLog.debug("dbusmenu children-property fallback id={} parentId={} children={} entries={}", itemId, - parentId, childIds.size(), propertyEntries.size()); + parentId, layoutChildIds.size(), propertyEntries.size()); after = entriesForParent(replyCache.entriesById, replyCache.childrenByParent, parentId); } } @@ -755,7 +836,7 @@ void TrayService::requestMenuLayoutAfterAboutToShow(const std::string& itemId, s replyCache.rootLoaded = true; } - if (before != after || !after.empty()) { + if (before != after) { emitChanged(); } }); @@ -787,11 +868,14 @@ std::vector TrayService::menuEntries(const std::string& itemId) { return {}; } - if (!cacheIt->second.rootLoaded) { + auto entries = entriesForParent(cacheIt->second.entriesById, cacheIt->second.childrenByParent, 0); + + // If we already have root entries, keep showing them even when a provider + // emits noisy root invalidations. + if (!cacheIt->second.rootLoaded && entries.empty()) { requestMenuSubtree(itemId, 0); } - auto entries = entriesForParent(cacheIt->second.entriesById, cacheIt->second.childrenByParent, 0); if (entries.empty() && !cacheIt->second.loadingParents.contains(0)) { requestMenuSubtree(itemId, 0, true); } @@ -838,19 +922,63 @@ void TrayService::ensureMenuCache(const std::string& itemId, const std::string& auto proxy = sdbus::createProxy(m_bus.connection(), sdbus::ServiceName{busName}, sdbus::ObjectPath{menuPath}); // LayoutUpdated(rev, parent): server is telling us the subtree rooted at - // `parent` changed. Invalidate aggressively — menus are small so re-fetch - // is cheap — and wake the UI so it refreshes if currently visible. + // `parent` changed. Invalidate incrementally to avoid feedback loops where + // providers emit many LayoutUpdated signals while we're already loading. proxy->uponSignal("LayoutUpdated") .onInterface(k_menu_interface) .call([this, itemId](std::uint32_t revision, std::int32_t parent) { if (auto it = m_menuCache.find(itemId); it != m_menuCache.end()) { - it->second.entriesById.clear(); - it->second.childrenByParent.clear(); - it->second.loadedParents.clear(); - it->second.loadingParents.clear(); - it->second.rootLoaded = false; - it->second.revision = revision; - ++it->second.generation; + auto& cache = it->second; + + if (parent <= 0 && cache.rootLoaded && cache.revision == revision) { + kLog.debug("LayoutUpdated root unchanged ignored id={} rev={} parent={}", itemId, revision, parent); + return; + } + + if (const auto revIt = cache.lastLayoutUpdatedRevisionByParent.find(parent); + revIt != cache.lastLayoutUpdatedRevisionByParent.end() && revIt->second == revision) { + kLog.debug("LayoutUpdated duplicate ignored id={} rev={} parent={}", itemId, revision, parent); + return; + } + cache.lastLayoutUpdatedRevisionByParent[parent] = revision; + cache.revision = revision; + + // While root is loading or not yet established, suppress all + // layout-updated churn. The in-flight root fetch will converge us. + if (cache.loadingParents.contains(0) || !cache.rootLoaded) { + kLog.debug("LayoutUpdated suppressed while root unstable id={} rev={} parent={}", itemId, revision, + parent); + return; + } + + if (parent <= 0) { + const bool hadVisibleRootEntries = + !entriesForParent(cache.entriesById, cache.childrenByParent, 0).empty(); + + // Soft-invalidate root: keep current snapshot visible and let the + // next normal menu pull refresh it. Avoid force-refresh here, + // which can cause redraw loops on noisy providers. + cache.loadedParents.erase(0); + cache.loadingParents.erase(0); + cache.nextRetryAt.erase(0); + cache.failureStreak.erase(0); + cache.rootLoaded = false; + // Allow a fresh root AboutToShow after provider-side resets. + cache.rootAboutToShowPrimed = false; + + if (hadVisibleRootEntries) { + kLog.debug("LayoutUpdated root soft-invalidated without emit id={} rev={} parent={}", itemId, revision, + parent); + return; + } + } else { + // Invalidate only the changed subtree parent so we don't discard + // an otherwise usable root snapshot. + cache.loadedParents.erase(parent); + cache.loadingParents.erase(parent); + cache.nextRetryAt.erase(parent); + cache.failureStreak.erase(parent); + } } kLog.debug("LayoutUpdated id={} rev={} parent={}", itemId, revision, parent); emitChanged(); @@ -897,6 +1025,11 @@ void TrayService::ensureMenuCache(const std::string& itemId, const std::string& } if (changed) { + if (it->second.loadingParents.contains(0) && !it->second.rootLoaded) { + // During initial root hydration, providers can emit many partial + // property updates; emitting here causes redraw storms. + return; + } emitChanged(); } }); @@ -921,17 +1054,39 @@ void TrayService::sendMenuEvent(const std::string& itemId, std::int32_t entryId, const auto timestamp = static_cast( std::chrono::duration_cast(std::chrono::system_clock::now().time_since_epoch()).count()); try { - it->second.proxy->callMethod("Event") + it->second.proxy->callMethodAsync("Event") .onInterface(k_menu_interface) .withTimeout(std::chrono::milliseconds(500)) - .withArguments(entryId, eventName, sdbus::Variant{std::int32_t{0}}, timestamp); + .withArguments(entryId, eventName, sdbus::Variant{std::int32_t{0}}, timestamp) + .uponReplyInvoke([itemId, entryId, eventName](std::optional error) { + if (error.has_value()) { + kLog.debug("dbusmenu Event failed id={} entryId={} event={} err={}", itemId, entryId, eventName, + error->what()); + } + }); } catch (const sdbus::Error& e) { - kLog.debug("dbusmenu Event failed id={} entryId={} event={} err={}", itemId, entryId, eventName, e.what()); + kLog.debug("dbusmenu Event dispatch failed id={} entryId={} event={} err={}", itemId, entryId, eventName, e.what()); } } void TrayService::notifyMenuOpened(const std::string& itemId, std::int32_t entryId) { sendMenuEvent(itemId, entryId, "opened"); + + // Some dbusmenu providers populate rows only after they observe "opened". + // Refresh conditionally so right-click on already-hydrated roots does not + // trigger repeated redraw loops. + if (const auto itemIt = m_items.find(itemId); itemIt != m_items.end()) { + ensureMenuCache(itemId, itemIt->second.busName, itemIt->second.menuObjectPath); + if (entryId == 0) { + const auto cacheIt = m_menuCache.find(itemId); + if (cacheIt == m_menuCache.end() || cacheIt->second.proxy == nullptr || !cacheIt->second.rootLoaded || + !cacheIt->second.loadedParents.contains(0)) { + requestMenuSubtree(itemId, 0, false); + } + } else { + requestMenuSubtree(itemId, entryId, true); + } + } } void TrayService::notifyMenuClosed(const std::string& itemId, std::int32_t entryId) { @@ -946,12 +1101,19 @@ bool TrayService::activateMenuEntry(const std::string& itemId, std::int32_t entr const auto timestamp = static_cast( std::chrono::duration_cast(std::chrono::system_clock::now().time_since_epoch()).count()); try { - it->second.proxy->callMethod("Event") + it->second.proxy->callMethodAsync("Event") .onInterface(k_menu_interface) - .withArguments(entryId, std::string("clicked"), sdbus::Variant{std::int32_t{0}}, timestamp); + .withTimeout(std::chrono::milliseconds(1000)) + .withArguments(entryId, std::string("clicked"), sdbus::Variant{std::int32_t{0}}, timestamp) + .uponReplyInvoke([itemId, entryId](std::optional error) { + if (error.has_value()) { + kLog.debug("dbusmenu clicked failed id={} entryId={} err={}", itemId, entryId, error->what()); + } + }); + // Async call: true means dispatch succeeded locally, not remote activation completion. return true; } catch (const sdbus::Error& e) { - kLog.debug("dbusmenu clicked failed id={} entryId={} err={}", itemId, entryId, e.what()); + kLog.debug("dbusmenu clicked dispatch failed id={} entryId={} err={}", itemId, entryId, e.what()); return false; } } @@ -976,10 +1138,18 @@ bool TrayService::activateItem(const std::string& itemId, std::int32_t x, std::i } try { - it->second->callMethod("Activate").onInterface(k_item_interface).withArguments(x, y); + it->second->callMethodAsync("Activate") + .onInterface(k_item_interface) + .withTimeout(std::chrono::milliseconds(1000)) + .withArguments(x, y) + .uponReplyInvoke([itemId](std::optional error) { + if (error.has_value()) { + kLog.debug("activate failed id={} err={}", itemId, error->what()); + } + }); return true; } catch (const sdbus::Error& e) { - kLog.debug("activate failed id={} err={}", itemId, e.what()); + kLog.debug("activate dispatch failed id={} err={}", itemId, e.what()); return false; } } @@ -994,10 +1164,18 @@ bool TrayService::openContextMenu(const std::string& itemId, std::int32_t x, std } try { - it->second->callMethod("ContextMenu").onInterface(k_item_interface).withArguments(x, y); + it->second->callMethodAsync("ContextMenu") + .onInterface(k_item_interface) + .withTimeout(std::chrono::milliseconds(1000)) + .withArguments(x, y) + .uponReplyInvoke([itemId](std::optional error) { + if (error.has_value()) { + kLog.debug("context menu failed id={} err={}", itemId, error->what()); + } + }); return true; } catch (const sdbus::Error& e) { - kLog.debug("context menu failed id={} err={}", itemId, e.what()); + kLog.debug("context menu dispatch failed id={} err={}", itemId, e.what()); return false; } } @@ -1344,7 +1522,18 @@ bool TrayService::ensureItemProxy(const std::string& itemId) { const auto hints = path_name_hints(itemIt->second.objectPath); + // Path-only fallback probing is synchronous; keep it tightly bounded to avoid + // long compositor stalls when many bus names are present. + constexpr std::size_t kMaxProbeAttempts = 16; + constexpr auto kProbeTimeout = std::chrono::milliseconds(80); + std::size_t probeAttempts = 0; + auto tryCandidate = [&](const std::string& candidate) -> bool { + if (probeAttempts >= kMaxProbeAttempts) { + return false; + } + ++probeAttempts; + if (!looks_like_dbus_name(candidate)) { return false; } @@ -1354,7 +1543,7 @@ bool TrayService::ensureItemProxy(const std::string& itemId) { std::map props; probe->callMethod("GetAll") .onInterface("org.freedesktop.DBus.Properties") - .withTimeout(std::chrono::milliseconds(500)) + .withTimeout(kProbeTimeout) .withArguments(k_item_interface) .storeResultsTo(props); @@ -1412,7 +1601,13 @@ bool TrayService::ensureItemProxy(const std::string& itemId) { }; for (const auto& hint : hints) { + if (probeAttempts >= kMaxProbeAttempts) { + break; + } for (const auto& candidate : names) { + if (probeAttempts >= kMaxProbeAttempts) { + break; + } if (StringUtils::toLower(candidate).find(hint) != std::string::npos && tryCandidate(candidate)) { return true; } @@ -1423,12 +1618,16 @@ bool TrayService::ensureItemProxy(const std::string& itemId) { // D-Bus service auto-activation which blocks for hundreds of ms per candidate. // Unique names represent currently-running processes and respond immediately. for (const auto& candidate : names) { + if (probeAttempts >= kMaxProbeAttempts) { + break; + } if (!candidate.empty() && candidate[0] == ':' && tryCandidate(candidate)) { return true; } } - kLog.debug("could not resolve bus name for path-only tray item path={}", itemIt->second.objectPath); + kLog.debug("could not resolve bus name for path-only tray item path={} probes={}", itemIt->second.objectPath, + probeAttempts); return false; } diff --git a/src/dbus/tray/tray_service.h b/src/dbus/tray/tray_service.h index cf4a211e4..f02ba9ee2 100644 --- a/src/dbus/tray/tray_service.h +++ b/src/dbus/tray/tray_service.h @@ -72,6 +72,8 @@ public: [[nodiscard]] std::vector items() const; [[nodiscard]] std::vector menuEntries(const std::string& itemId); [[nodiscard]] std::vector menuEntriesForParent(const std::string& itemId, std::int32_t parentId); + // Returns true if the click event was dispatched to DBus successfully. + // This does not imply the remote menu action completed successfully. [[nodiscard]] bool activateMenuEntry(const std::string& itemId, std::int32_t entryId); // Notify the dbusmenu server that a (sub)menu is being opened/closed. `entryId` // is the menu item id: 0 for the root menu, or a submenu parent id otherwise. @@ -90,11 +92,15 @@ private: std::unordered_map entriesById; // Decoded child ids per parent-id. parentId=0 is the root menu. std::unordered_map> childrenByParent; + std::unordered_map nextRetryAt; + std::unordered_map failureStreak; + std::unordered_map lastLayoutUpdatedRevisionByParent; std::unordered_set loadedParents; std::unordered_set loadingParents; std::uint32_t revision = 0; std::uint64_t generation = 0; bool rootLoaded = false; + bool rootAboutToShowPrimed = false; }; void onRegisterStatusNotifierItem(const std::string& serviceOrPath, const std::string& senderBusName); diff --git a/src/shell/tray/tray_menu.cpp b/src/shell/tray/tray_menu.cpp index 2e3ae91cb..5c6c0e70b 100644 --- a/src/shell/tray/tray_menu.cpp +++ b/src/shell/tray/tray_menu.cpp @@ -57,6 +57,30 @@ namespace { std::size_t visibleEntryLimit(std::size_t entryCount) { return std::max(1, entryCount); } + // Convert an icon name like "audio-input-microphone-symbolic" to a readable label like "Audio Input Microphone". + std::string iconNameToLabel(std::string_view iconName) { + // Strip trailing "-symbolic" + constexpr std::string_view kSymbolicSuffix = "-symbolic"; + if (iconName.ends_with(kSymbolicSuffix)) { + iconName.remove_suffix(kSymbolicSuffix.size()); + } + std::string out; + out.reserve(iconName.size()); + bool capitaliseNext = true; + for (char c : iconName) { + if (c == '-') { + out.push_back(' '); + capitaliseNext = true; + } else if (capitaliseNext) { + out.push_back(static_cast(std::toupper(static_cast(c)))); + capitaliseNext = false; + } else { + out.push_back(c); + } + } + return out; + } + std::string toLower(std::string_view value) { std::string out(value); std::transform(out.begin(), out.end(), out.begin(), @@ -252,11 +276,17 @@ void TrayMenu::onTrayChanged() { if (!m_visible) { return; } + + auto previousEntries = std::move(m_entries); refreshEntries(); if (m_entries.empty()) { close(); return; } + if (m_entries == previousEntries) { + return; + } + resizeMainSurfaceToEntries(); rebuildScenes(); @@ -292,6 +322,13 @@ void TrayMenu::toggleForItem(const std::string& itemId) { } m_activeItemId = itemId; + + // Some dbusmenu servers only materialize menu rows after receiving "opened". + // Emit this before the first fetch so we don't render a persistent empty menu. + if (m_tray != nullptr) { + m_tray->notifyMenuOpened(m_activeItemId); + } + refreshEntries(); m_visible = true; @@ -301,14 +338,6 @@ void TrayMenu::toggleForItem(const std::string& itemId) { return; } - // Notify the dbusmenu server the root menu is being opened. Well-behaved - // servers (including Electron) rely on paired opened/closed events to reset - // internal state — skipping them causes their handlers to desync after many - // open/close cycles, eventually returning errors on every GetLayout. - if (m_tray != nullptr) { - m_tray->notifyMenuOpened(m_activeItemId); - } - rebuildScenes(); } @@ -585,7 +614,7 @@ uint32_t TrayMenu::surfaceHeightPx() const { for (const auto& entry : m_entries) { entries.push_back(ContextMenuControlEntry{ .id = entry.id, - .label = entry.label, + .label = entry.label.empty() ? iconNameToLabel(entry.iconName) : entry.label, .enabled = entry.enabled, .separator = entry.separator, .hasSubmenu = entry.hasSubmenu, @@ -787,7 +816,7 @@ void TrayMenu::buildScene(MenuInstance& inst, uint32_t width, uint32_t height) { for (const auto& entry : m_entries) { entries.push_back(ContextMenuControlEntry{ .id = entry.id, - .label = entry.label, + .label = entry.label.empty() ? iconNameToLabel(entry.iconName) : entry.label, .enabled = entry.enabled, .separator = entry.separator, .hasSubmenu = entry.hasSubmenu,