From a91c3aef7aa4168308e4fdb5e2bc7568c029a319 Mon Sep 17 00:00:00 2001 From: crumblingstatue Date: Tue, 13 May 2025 20:38:18 +0200 Subject: [PATCH 1/6] [WIP] status-area: Handle menuless items properly SNI protocol does not mandate the presence of a DBus menu. The `ItemIsMenu` method returns whether the item is a menu or not. But we should also handle items that don't provide a menu, even if they don't advertise this fact through `ItemIsMenu`. If the item isn't a menu (or doesn't have one), then: - Left click should call the `Activate` method on the item. - Right click should call the `ContextMenu` method on the item. This is a work-in-progress implementation to make the applet respect the above. --- .../src/components/app.rs | 6 +++ .../src/components/status_menu.rs | 48 ++++++++++++++++--- .../src/subscriptions/status_notifier_item.rs | 37 ++++++++++---- 3 files changed, 75 insertions(+), 16 deletions(-) diff --git a/cosmic-applet-status-area/src/components/app.rs b/cosmic-applet-status-area/src/components/app.rs index 105abbc1..3a80db58 100644 --- a/cosmic-applet-status-area/src/components/app.rs +++ b/cosmic-applet-status-area/src/components/app.rs @@ -26,6 +26,7 @@ pub enum Msg { TogglePopup(usize), Hovered(usize), Surface(surface::Action), + RightPressed(usize), } #[derive(Default)] @@ -188,6 +189,10 @@ impl cosmic::Application for App { } Task::none() } + Msg::RightPressed(id) => { + self.menus[&id].ctx_menu_activate(); + Task::none() + } Msg::Hovered(id) => { let mut cmds = Vec::new(); if let Some(old_id) = self.open_menu.take() { @@ -261,6 +266,7 @@ impl cosmic::Application for App { .on_press_down(Msg::TogglePopup(*id)), ) .on_enter(Msg::Hovered(*id)) + .on_right_press(Msg::RightPressed(*id)) .into() }); self.core diff --git a/cosmic-applet-status-area/src/components/status_menu.rs b/cosmic-applet-status-area/src/components/status_menu.rs index 0f41c149..2e30cc6d 100644 --- a/cosmic-applet-status-area/src/components/status_menu.rs +++ b/cosmic-applet-status-area/src/components/status_menu.rs @@ -71,8 +71,11 @@ impl State { iced::Task::none() } - Msg::Click(id, is_submenu) => { - let menu_proxy = self.item.menu_proxy().clone(); + Msg::Click(id, is_submenu) => 'block: { + let Some(menu_proxy) = self.item.menu_proxy().cloned() else { + tracing::error!("Msg::click on item without menu_proxy"); + break 'block iced::Task::none(); + }; tokio::spawn(async move { let _ = menu_proxy.event(id, "clicked", &0.into(), 0).await; }); @@ -118,15 +121,48 @@ impl State { } pub fn opened(&self) { - let menu_proxy = self.item.menu_proxy().clone(); + let Some(menu_proxy) = self.item.menu_proxy().cloned() else { + let item_proxy = self.item.item_proxy().clone(); + tokio::spawn(async move { + if let Err(e) = item_proxy.activate(0, 0).await { + tracing::error!("Error on Activate: {e}"); + } + }); + return; + }; + let item_proxy = self.item.item_proxy().clone(); + tokio::spawn(async move { + let is_menu = match item_proxy.item_is_menu().await { + Ok(is_menu) => is_menu, + Err(e) => { + tracing::error!("Error on ItemIsMenu: {e}"); + false + } + }; + if is_menu { + let _ = menu_proxy.event(0, "opened", &0i32.into(), 0).await; + let _ = menu_proxy.about_to_show(0).await; + } else { + if let Err(e) = item_proxy.activate(0, 0).await { + tracing::error!("Error on Activate: {e}"); + } + } + }); + } + + pub fn ctx_menu_activate(&self) { + let item_proxy = self.item.item_proxy().clone(); tokio::spawn(async move { - let _ = menu_proxy.event(0, "opened", &0i32.into(), 0).await; - let _ = menu_proxy.about_to_show(0).await; + if let Err(e) = item_proxy.context_menu(1920, 0).await { + tracing::error!("Error on ContextMenu: {e}"); + } }); } pub fn closed(&self) { - let menu_proxy = self.item.menu_proxy().clone(); + let Some(menu_proxy) = self.item.menu_proxy().cloned() else { + return; + }; tokio::spawn(async move { let _ = menu_proxy.event(0, "closed", &0i32.into(), 0).await; }); diff --git a/cosmic-applet-status-area/src/subscriptions/status_notifier_item.rs b/cosmic-applet-status-area/src/subscriptions/status_notifier_item.rs index 79686baf..ada7e32c 100644 --- a/cosmic-applet-status-area/src/subscriptions/status_notifier_item.rs +++ b/cosmic-applet-status-area/src/subscriptions/status_notifier_item.rs @@ -12,7 +12,7 @@ use zbus::zvariant::{self, OwnedValue}; pub struct StatusNotifierItem { name: String, item_proxy: StatusNotifierItemProxy<'static>, - menu_proxy: DBusMenuProxy<'static>, + menu_proxy: Option>, } #[derive(Clone, Debug, zvariant::Value)] @@ -44,12 +44,19 @@ impl StatusNotifierItem { .build() .await?; - let menu_path = item_proxy.menu().await?; - let menu_proxy = DBusMenuProxy::builder(connection) - .destination(dest.to_string())? - .path(menu_path)? - .build() - .await?; + let menu_proxy = match item_proxy.menu().await { + Ok(menu_path) => Some( + DBusMenuProxy::builder(connection) + .destination(dest.to_string())? + .path(menu_path)? + .build() + .await?, + ), + Err(e) => { + eprintln!("Error: {e}"); + None + } + }; Ok(Self { name, @@ -64,7 +71,9 @@ impl StatusNotifierItem { // TODO: Only fetch changed part of layout, if that's any faster pub fn layout_subscription(&self) -> iced::Subscription> { - let menu_proxy = self.menu_proxy.clone(); + let Some(menu_proxy) = self.menu_proxy.clone() else { + return iced::Subscription::none(); + }; Subscription::run_with_id( format!("status-notifier-item-layout-{}", &self.name), async move { @@ -109,8 +118,12 @@ impl StatusNotifierItem { ) } - pub fn menu_proxy(&self) -> &DBusMenuProxy<'static> { - &self.menu_proxy + pub fn menu_proxy(&self) -> Option<&DBusMenuProxy<'static>> { + self.menu_proxy.as_ref() + } + + pub fn item_proxy(&self) -> &StatusNotifierItemProxy<'static> { + &self.item_proxy } } @@ -135,6 +148,10 @@ trait StatusNotifierItem { #[zbus(signal)] fn new_icon(&self) -> zbus::Result<()>; + + fn activate(&self, x: i32, y: i32) -> zbus::Result<()>; + fn context_menu(&self, x: i32, y: i32) -> zbus::Result<()>; + fn item_is_menu(&self) -> zbus::Result; } #[derive(Clone, Debug)] From e03f5ce51a6f6e592d1271f59f999cc89ec81063 Mon Sep 17 00:00:00 2001 From: crumblingstatue Date: Wed, 14 May 2025 14:52:22 +0200 Subject: [PATCH 2/6] Improve error message when getting Menu property fails --- .../src/subscriptions/status_notifier_item.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/cosmic-applet-status-area/src/subscriptions/status_notifier_item.rs b/cosmic-applet-status-area/src/subscriptions/status_notifier_item.rs index ada7e32c..ce43375e 100644 --- a/cosmic-applet-status-area/src/subscriptions/status_notifier_item.rs +++ b/cosmic-applet-status-area/src/subscriptions/status_notifier_item.rs @@ -53,7 +53,10 @@ impl StatusNotifierItem { .await?, ), Err(e) => { - eprintln!("Error: {e}"); + let destination = item_proxy.inner().destination(); + tracing::error!( + "Error getting Menu property for {destination}: {e}. Treating as menuless item.", + ); None } }; From c0fc7bced35c60f34ec08f43130dacfc85cd4ebd Mon Sep 17 00:00:00 2001 From: crumblingstatue Date: Wed, 14 May 2025 14:56:11 +0200 Subject: [PATCH 3/6] Remove unused imports --- cosmic-applet-status-area/src/components/app.rs | 2 +- cosmic-applet-status-area/src/components/status_menu.rs | 2 +- .../src/subscriptions/status_notifier_item.rs | 5 +---- 3 files changed, 3 insertions(+), 6 deletions(-) diff --git a/cosmic-applet-status-area/src/components/app.rs b/cosmic-applet-status-area/src/components/app.rs index 3a80db58..b59341e9 100644 --- a/cosmic-applet-status-area/src/components/app.rs +++ b/cosmic-applet-status-area/src/components/app.rs @@ -7,7 +7,7 @@ use cosmic::{ iced::{ self, platform_specific::shell::commands::popup::{destroy_popup, get_popup}, - window, Limits, Padding, Subscription, + window, Subscription, }, surface, widget::{container, mouse_area}, diff --git a/cosmic-applet-status-area/src/components/status_menu.rs b/cosmic-applet-status-area/src/components/status_menu.rs index 2e30cc6d..ee69d06c 100644 --- a/cosmic-applet-status-area/src/components/status_menu.rs +++ b/cosmic-applet-status-area/src/components/status_menu.rs @@ -3,7 +3,7 @@ use cosmic::{ applet::menu_button, - iced::{self, Padding}, + iced::{self}, widget::icon, }; diff --git a/cosmic-applet-status-area/src/subscriptions/status_notifier_item.rs b/cosmic-applet-status-area/src/subscriptions/status_notifier_item.rs index ce43375e..cf74d924 100644 --- a/cosmic-applet-status-area/src/subscriptions/status_notifier_item.rs +++ b/cosmic-applet-status-area/src/subscriptions/status_notifier_item.rs @@ -1,10 +1,7 @@ // Copyright 2023 System76 // SPDX-License-Identifier: GPL-3.0-only -use cosmic::{ - iced::{self, Subscription}, - widget::icon, -}; +use cosmic::iced::{self, Subscription}; use futures::{FutureExt, StreamExt}; use zbus::zvariant::{self, OwnedValue}; From c1619a5e94f8a537bbdef094941431f663afb8c0 Mon Sep 17 00:00:00 2001 From: crumblingstatue Date: Wed, 14 May 2025 15:26:27 +0200 Subject: [PATCH 4/6] Add stub function for getting item screen position --- .../src/components/status_menu.rs | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/cosmic-applet-status-area/src/components/status_menu.rs b/cosmic-applet-status-area/src/components/status_menu.rs index ee69d06c..cf43a142 100644 --- a/cosmic-applet-status-area/src/components/status_menu.rs +++ b/cosmic-applet-status-area/src/components/status_menu.rs @@ -124,7 +124,8 @@ impl State { let Some(menu_proxy) = self.item.menu_proxy().cloned() else { let item_proxy = self.item.item_proxy().clone(); tokio::spawn(async move { - if let Err(e) = item_proxy.activate(0, 0).await { + let [x, y] = item_screen_pos_stub(); + if let Err(e) = item_proxy.activate(x, y).await { tracing::error!("Error on Activate: {e}"); } }); @@ -143,7 +144,8 @@ impl State { let _ = menu_proxy.event(0, "opened", &0i32.into(), 0).await; let _ = menu_proxy.about_to_show(0).await; } else { - if let Err(e) = item_proxy.activate(0, 0).await { + let [x, y] = item_screen_pos_stub(); + if let Err(e) = item_proxy.activate(x, y).await { tracing::error!("Error on Activate: {e}"); } } @@ -153,7 +155,8 @@ impl State { pub fn ctx_menu_activate(&self) { let item_proxy = self.item.item_proxy().clone(); tokio::spawn(async move { - if let Err(e) = item_proxy.context_menu(1920, 0).await { + let [x, y] = item_screen_pos_stub(); + if let Err(e) = item_proxy.context_menu(x, y).await { tracing::error!("Error on ContextMenu: {e}"); } }); @@ -169,6 +172,15 @@ impl State { } } +/// Stub: Get the screen coordinates of an item +/// +/// Used for passing coordinates for SNI `Activate` and `ContextMenu` methods. +/// +/// TODO: Figure out how to implement +fn item_screen_pos_stub() -> [i32; 2] { + [0, 0] +} + fn layout_view(layout: &Layout, expanded: Option) -> cosmic::Element { iced::widget::column(layout.children().iter().filter_map(|i| { if !i.visible() { From b39f95e7f9d3fbdef05edcae0c3cc7aee4ea16f9 Mon Sep 17 00:00:00 2001 From: crumblingstatue Date: Wed, 14 May 2025 17:58:51 +0200 Subject: [PATCH 5/6] Rename `Msg::RightPressed` to `OpenContextMenu` --- cosmic-applet-status-area/src/components/app.rs | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/cosmic-applet-status-area/src/components/app.rs b/cosmic-applet-status-area/src/components/app.rs index b59341e9..9961425a 100644 --- a/cosmic-applet-status-area/src/components/app.rs +++ b/cosmic-applet-status-area/src/components/app.rs @@ -26,7 +26,15 @@ pub enum Msg { TogglePopup(usize), Hovered(usize), Surface(surface::Action), - RightPressed(usize), + /// Open the item's context menu. + /// + /// Currently, this always calls `StatusNotifierItem.ContextMenu`. + /// + /// TODO: Decide if behavior should be different in cases like `ContextMenu` method missing, + /// but a menu being available. + OpenContextMenu { + id: usize, + }, } #[derive(Default)] @@ -189,7 +197,7 @@ impl cosmic::Application for App { } Task::none() } - Msg::RightPressed(id) => { + Msg::OpenContextMenu { id } => { self.menus[&id].ctx_menu_activate(); Task::none() } @@ -266,7 +274,7 @@ impl cosmic::Application for App { .on_press_down(Msg::TogglePopup(*id)), ) .on_enter(Msg::Hovered(*id)) - .on_right_press(Msg::RightPressed(*id)) + .on_right_press(Msg::OpenContextMenu { id: *id }) .into() }); self.core From 0547d9860e8b9fb9ddd43ad3fc49547c9b6fe16f Mon Sep 17 00:00:00 2001 From: crumblingstatue Date: Wed, 14 May 2025 19:59:39 +0200 Subject: [PATCH 6/6] Rename layout_subscription to menu_layout_subscription This helps clarify that the layout is used for menus, and is irrelevant when the item doesn't provide a menu. --- cosmic-applet-status-area/src/components/status_menu.rs | 2 +- .../src/subscriptions/status_notifier_item.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cosmic-applet-status-area/src/components/status_menu.rs b/cosmic-applet-status-area/src/components/status_menu.rs index cf43a142..3032eccf 100644 --- a/cosmic-applet-status-area/src/components/status_menu.rs +++ b/cosmic-applet-status-area/src/components/status_menu.rs @@ -115,7 +115,7 @@ impl State { pub fn subscription(&self) -> iced::Subscription { iced::Subscription::batch([ - self.item.layout_subscription().map(Msg::Layout), + self.item.menu_layout_subscription().map(Msg::Layout), self.item.icon_subscription().map(Msg::Icon), ]) } diff --git a/cosmic-applet-status-area/src/subscriptions/status_notifier_item.rs b/cosmic-applet-status-area/src/subscriptions/status_notifier_item.rs index cf74d924..f6fb5b59 100644 --- a/cosmic-applet-status-area/src/subscriptions/status_notifier_item.rs +++ b/cosmic-applet-status-area/src/subscriptions/status_notifier_item.rs @@ -70,7 +70,7 @@ impl StatusNotifierItem { } // TODO: Only fetch changed part of layout, if that's any faster - pub fn layout_subscription(&self) -> iced::Subscription> { + pub fn menu_layout_subscription(&self) -> iced::Subscription> { let Some(menu_proxy) = self.menu_proxy.clone() else { return iced::Subscription::none(); };