[WIP] status-area: Handle menuless items properly #946
Conversation
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.
| @@ -26,6 +26,7 @@ pub enum Msg { | |||
| TogglePopup(usize), | |||
| Hovered(usize), | |||
| Surface(surface::Action), | |||
| RightPressed(usize), | |||
There was a problem hiding this comment.
I'm not sure what to name this.
If we go with this name, it might also make sense to rename TogglePopup to LeftPressed to better reflect the logic.
There was a problem hiding this comment.
It should be named based on what action the message will perform.
There was a problem hiding this comment.
Thank you, I renamed it to OpenContextMenu. Let me know if that's a suitable name.
| @@ -188,6 +189,10 @@ impl cosmic::Application for App { | |||
| } | |||
| Task::none() | |||
| } | |||
| Msg::RightPressed(id) => { | |||
| self.menus[&id].ctx_menu_activate(); | |||
| Task::none() | |||
There was a problem hiding this comment.
To be sure I'm not sure about what to return here.
I need to better familiarize myself with tasks (and async Rust in general)
| @@ -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<DBusMenuProxy<'static>>, | |||
There was a problem hiding this comment.
We make menu_proxy optional to reflect the fact that some clients don't provide a DBus menu.
cosmic-applet-status-area/src/subscriptions/status_notifier_item.rs
Outdated
Show resolved
Hide resolved
| @@ -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<Result<Layout, String>> { | |||
| let menu_proxy = self.menu_proxy.clone(); | |||
| let Some(menu_proxy) = self.menu_proxy.clone() else { | |||
| return iced::Subscription::none(); | |||
There was a problem hiding this comment.
Not sure what iced subscriptions are, or if it's okay to return none here. I'm gonna have to look into that as well.
There was a problem hiding this comment.
They are tasks that automatically spawn for the duration that they remain in the subscription state of the application. Typically kept active for the whole duration of the application.
There was a problem hiding this comment.
Thank you. It looks like Layout is used for menus only, so Subscription::none() can be safely returned here.
I also renamed layout_subscription to menu_layout_subscription to help clarify this.
This helps clarify that the layout is used for menus, and is irrelevant when the item doesn't provide a menu.
SNI protocol does not mandate the presence of a DBus menu. The
ItemIsMenumethod 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 throughItemIsMenu.If the item isn't a menu (or doesn't have one), then:
Activatemethod on the item.ContextMenumethod on the item.This is a work-in-progress implementation to make the applet respect the above.
Fixes #944
This is my first pull request to COSMIC, I would appreciate any mentoring!