System Tray (v3) #743
System Tray (v3) #743
Conversation
|
Not sure how to help debug this further, but eww crashes when I add systray to the config (I exported Full stacktrace here: https://gist.githubusercontent.com/eclairevoyant/d077cd0ecd0ff56d9f9f946da11e2f7d/raw/5fce16030bcee06390d0ac809a090917b4fa1c2b/eww%2520logs%2520stacktrace For reference I'm building this as so: https://gist.githubusercontent.com/eclairevoyant/d077cd0ecd0ff56d9f9f946da11e2f7d/raw/5fce16030bcee06390d0ac809a090917b4fa1c2b/PKGBUILD |
|
@eclairevoyant I've pushed some changes, can you have another try? |
|
eww starts now, but icons don't consistently show up (for example Steam was showing up at first, but after killing and restarting it no longer shows up, no matter what order I open steam vs eww). FWIW the icons do show correctly in swaybar (and waybar, which seems to reliably update on the fly as well). Also, running |
|
@eclairevoyant thanks for finding those! the latest commits should fix those |
|
yep, seems to have fixed it. Fcitx5 and Steam show up correctly now. I did find another crash with Electron apps that use tray icons. backtrace: I don't think it's webcord specific as I was able to repro the crash with other apps like signal and discord (all use electron). Another crash I found with |
|
Any attempt of any app to create a status icon results in a log message like this, with nothing showing: |
|
@bwachter i made some commits earlier today that should fix that, can make sure you're on the latest commit? if you still have that issue on the latest version can you run |
|
It doesn't work for me on NixOS and gives this error: full log: eww.log |
|
thanks for the work, managed to style a tiny bit, but opening zoom (installed with flatpak, can't test without) caused a crash: backtrace does not seem to be more helpful here, do you need anything else? migth have to do with my icon themes not available in the flatpak container, but it does seem to work with slack (also flatpak) without issues and anyhow it should not crash I guess |
|
@ettancos |
|
this might be a eww-tray-hyprland interaction kind of thing so not very sure where this goes i had to enable focusable in the eww window to be able to hit escape to close the menu (very used to this kind of workflow). problem is if I set the window stacking prop to foreground eww will take over the mouse focus even if the mouse is over the application window so clicks are not going to the app (I assume eww gets them), keyboard focus seem to work, but if I set it to background the dbusmenu window will be drawn behind the application |
|
@ralismark my build was at 132b18c - I've now pulled additional commits, and with 94309f9 it crashes on startup: |
|
Since the two main programs I would want to use a dock for are
Here is the |
|
I'm also experiencing an error, which is likely related to @eclairevoyant's report (and also to @VuiMuich's report, which afaik seem to be the same) about electron apps, though this is a different error. This error only occurs when the bar is already running, and I start discord, while eclairevoyant's original report is what happens when discord is already running, and eww is only started afterwards. Note: In the v2, discord tray icon worked properly. Short backtraceFull backtraceIn case it would matter, I'm on wayland, not X11. |
|
Oh, right, I just skimmed through previous reports as I supposed those where all related to the build issues, sorry. As @ItsDrike reported, in the v2 branch discord worked well, so this might be discord vs dbus related? |
That simply isn't how Nix works. A nix build is not like building on other distros. If complete dependencies aren't defined, the package will not build. Derivations are supposed to describe everything that is needed to build them -- dependencies and build steps. If the build is failing, it is because the flake's package derivation is inadequate. If the build fails on one supported system, it fails on all supported systems regardless of the packages users install. Obviously, changes that would cause the package to fail builds are relevant to the PR. |
|
@Lord-Valen Incorrectly specifying dependencies in your nix derivation has nothing to do with this PR, which is why I said this isn't the place to get support for how to use nix |
|
Apologies, I didn't realise this repo had a flake file in it. Nevertheless, this is still a change that needs to be made on the nixpkgs end, because that's where all the dependencies are specified. As a workaround if you want to test this on nix you can use the same dependencies that were added to the dev shell output, by adding this after line 38: # TODO remove after next release
buildInputs = old.buildInputs ++ (with final; [ glib librsvg libdbusmenu-gtk3 ]);and applying the fix from #748. I don't know if that's necessary to put in the PR since it'll end up being removed, I'll leave that up to the maintainer to decide |
|
I just want to add on to @VuiMuich here: It's not Electron specific, I'm getting the same issue with I can add full traces, but here are the errors on crash: Note that uncommenting the icon creation code on the given line "fixes" the crash and the context menus work correctly when clicking on the space which the empty icon occupies. So it's just the actual icon that's causing issues. I'll try to look into this more myself but not really knowing much Rust or GTK I'm not very hopeful that I'll make much progress. |
|
Sorry for all the crashes! Compared to v2, this has a completely different implementation of the dbus bits. I mainly wanted limitations with the implementation to be really obvious while I'm still ironing out all the bugs. When I'm happy with the state of things, i'll add in better error handling that does something sensible. However, I've been seriously busy with life & work the past few weeks and haven't really had time to go through and actually investigate these issues.... :( |
|
Fixed the crashes at least! See ralismark#2 if you're impatient and want to patch it yourself. Discord (and probably other Electron stuff) still needs pixmap support so it will show a default "Not found" icon for now, but at least it won't crash the bar and the menu is functional. Excuse my Rust if something I did is a no-no 😛 |
|
I tried @Onyx47 fixes and they seem to be very stable, however what occurred to me that applets like nm-applet are not shown nor throw any error. |
|
While the code needs some serious review (because, again, still fumbling in Rust), applications that don't have a system icon but they expose it in the DBus interface now also have an icon. For example, Discord now works:
Hopefully once @ralismark gets some time to do stuff again this will at least help on the research side of things. There are still edge cases about (found one application that still has a broken icon), but so far no crashes at least. @mauricekraus - took a peek, |
|
nm-applet has supported appindicator for years, works fine on my end? maybe you have an older version installed?
|
|
Ah, @mauricekraus might be making the same mistake I did when I tried and it didn't work - and the man page is horrible: it mentions options but doesn't list any. With |
|
I actually didn't need to pass that flag, since as of 1.32.0 it defaults to appindicator when xembed isn't available |
Hey @ralismark, just checking to see how you're doing with the remainder of the items. Please let us know if there's anything you need help with -- I'm sure some of us who are really looking forward to this feature would be willing to do whatever we can to help get this across the finish line. :) |
There was a problem hiding this comment.
I'm pretty much happy with how everything is now -- I've fixed up all the TODOs I wanted to fix myself. All the remaining TODOs should now have a review comment on them explaining what they mean and how important they are -- I think all of them are pretty minor.
Let's get this (finally) merged!
|
|
||
| /// Get the current icon. | ||
| pub async fn icon(&self, size: i32, scale: i32) -> Option<gtk::gdk_pixbuf::Pixbuf> { | ||
| // TODO explain what size and scale mean here |
There was a problem hiding this comment.
@moetayuko you were the original contributor of the HiDPI feature, can you explain what size and scale mean?
| let (addr, path) = { | ||
| // Based on <https://github.com/oknozor/stray/blob/main/stray/src/notifier_watcher/notifier_address.rs> | ||
| // | ||
| // TODO is the service name format actually documented anywhere? |
There was a problem hiding this comment.
@oknozor I took this parsing logic from your repo, do you know how you came up with it?
| fn remove_item(&mut self, id: &str); | ||
| } | ||
|
|
||
| // TODO We aren't really thinking about what happens when we shut down a host. Currently, we don't |
There was a problem hiding this comment.
This TODO is only for proper behaviour when you keep the DBus connection around but stop having a systray. I don't think fixing this is a blocker for merging -- it only really means that tray icons may think there's a tray when there isn't one, which isn't really a problem.
| Err(e) => log::warn!("failed to get menu: {}", e), | ||
| } | ||
|
|
||
| // TODO this is a lot of code duplication unfortunately, i'm not really sure how to |
There was a problem hiding this comment.
This TODO is just about making the code here a bit nicer. I can't think of how so if you do know please let me know.
| } | ||
|
|
||
| async fn dbus_session() -> zbus::Result<&'static DBusSession> { | ||
| // TODO make DBusSession reference counted so it's dropped when not in use? |
There was a problem hiding this comment.
This TODO is just for not taking up resources when we don't need it -- there's not much benefit beyond that, to be honest.
|
|
||
| /// Get the menu of this item. | ||
| pub async fn menu(&self) -> zbus::Result<gtk::Menu> { | ||
| // TODO document what this returns if there is no menu. |
There was a problem hiding this comment.
Documentation TODO. Nice to have but not necessary I guess.
| #[zbus(connection)] con: &zbus::Connection, | ||
| #[zbus(signal_context)] ctxt: zbus::SignalContext<'_>, | ||
| ) -> zbus::fdo::Result<()> { | ||
| // TODO right now, we convert everything to the unique bus name (something like :1.234). |
There was a problem hiding this comment.
This might be a correctness issue, but if no one has raised it in the ~year this PR has been around it might be fine.
|
Looking great! I'll be using this for the next few days, and let's give the people you pinged a bit more time to reply -- but other than that, I guess this is pretty much ready for merge! HYPE! |
|
@elkowar can you also review ralismark#6? while it's okay to relocate it here as a new PR after this PR gets merged, it contains breaking changes so simultaneously check-in both is better for UX |
please do this, it's half-usable without proper click handling |
|
im trying to install through the aur and when i add the systray widget my eww closes it says unknown widget when i try and run it with debug |
|
This feature isn't on main branch yet so it's also not in the aur. You have to compile manually off this PR if you want to use the systray. |
|
Weird I thought it was bc of this https://aur.archlinux.org/packages/eww-tray-wayland-git |
|
You need to have that installed and follow this, https://elkowar.github.io/eww. |
|
Let's merge this! Thanks a TON for your work, @ralismark, and huge thanks to all the others that have worked on this over the past year(s). Y'all are amazing! |
|
since this is such a major added feature could you consider releasing a 5.1 build ? |
Description
A system tray widget that supports the StatusNotifierItem protocol.
Supersedes #676. Currently very WIP. See below for more info.
Usage
The widget is called
systrayand can take the following properties:pack_direction: one of"ltr"/"right","rtl"/"left","ttb"/"down","btt"/"up"to specify the direction in which widgets are laid outicon-size: an integer specifying the size of icons (in pixels)Showcase
Demonstration of
(systray :pack-direction "ttb")with a menu open:Additional Notes
Closes #111.
This supersedes my previous attempt #676, which itself is a continuation of #448. The context behind this rewrite is this comment on #676 -- the main ones being:
Checklist
Please make sure you can check all the boxes that apply to this PR.
docs/content/maindirectory has been adjusted to reflect my changes.cargo fmtto automatically format all code before committing🚧 Remaining Work / Known Issues 🚧
Currently, this feature is functional enough to show icons, but needs much more testing and likely has numerous bugs and unsupported things. If you find issues, it would be really appreciated if you report them!