Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace libappindicator with ksni #201

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from
Open

Conversation

dfaust
Copy link

@dfaust dfaust commented Nov 1, 2024

Using ksni and the xdg standard for tray icons on Linux unlocks missing functionality and new features (#104 and more), as well as unblocks a GTK4 migration (tauri-apps/webkit2gtk-rs#94).

  • Predefined menu items are not handled, except separator and about.
  • Shortcuts are ignored.

Companion PR for muda:
Export more internals so muda can be used in combination with ksni: tauri-apps/muda#239

Tauri issue:
[feat] Use ksni crate for tray icons on Linux: tauri-apps/tauri#11293

Comment on lines +51 to +52
*control_flow = ControlFlow::Poll;
std::thread::sleep(std::time::Duration::from_millis(16));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason why this was changed to use thread sleep?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because otherwise the event loop won't wake up after the timeout, only when the user clicks the tray icon.

tauri-apps/tao#988

examples/tao.rs Outdated
@@ -95,3 +101,15 @@ fn load_icon(path: &std::path::Path) -> tray_icon::Icon {
};
tray_icon::Icon::from_rgba(icon_rgba, icon_width, icon_height).expect("Failed to open icon")
}

fn menu_icon(path: &std::path::Path) -> muda::Icon {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this function shares some logic with load_icon, let's reduce this duplication

pub fn new(id: TrayIconId, attrs: TrayIconAttributes) -> crate::Result<Self> {
let icon = attrs.icon.map(|icon| icon.inner.into());

let title = title_or_pkg_name(attrs.title.unwrap_or_default());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would leave this up to the user to specify, we should default to an empty string, also env!("CARGO_PKG_NAME") will always be tray-icon not the consuming crate

Ok(())
}

pub fn set_menu(&mut self, menu: Option<Box<dyn crate::menu::ContextMenu>>) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like the only way to change the tray menu is by calling this method, which is a breaking change.

Previously, changing the state on the submenu, whether adding/removing items or changing the title/icon/check/enabled state of the item would also be reflected on the tray menu without overriding the menu again.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you have to call set_menu explicitly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is kind of a huge blocker, and we won't be able to merge as it will break tauri usage.

Copy link
Member

@amrbashir amrbashir Nov 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we can provide a hook in muda that fires some event when a context menu changes its items including submenu items.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is, that the ksni::TrayService runs in a separate thread and the muda::MenuItemKind contains a inner value of type Rc<RefCell<MenuChild>>. So we can't send the menu items directly to the tray service thread. Ideally we would build the menu out of the latest version of the menu items whenever its displayed.
As far as I can see, a hook would need to be passed to the muda::MenuItem constructor, which would make the API very ugly.

I don't see a good solution here. So how about we add another field to the muda::MenuItemKind structs. The type would be Arc<ArcSwap<MenuItem>> with MenuItem being the enum that I added to tray-icon, which I would move to muda. Whenever the menu is changed, this value then needs to be updated as well. It also means that the data is duplicated. This is not great, but it should work relatively easily.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's experiment with that idea, maybe even put it behind a ksni feature flag in muda

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It didn't work as I hoped. ksni::Handle::update must be called in order to update the menu. So I added a channel and a thread to do the update. Please have a look and tell me what you think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants