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

trayicon: migrate from libappindicator to org.kde.StatusNotifierItem #558

Merged

Conversation

deltragon
Copy link
Collaborator

@deltragon deltragon commented Dec 30, 2023

This is necessary for migrating to gtk4, as gtk4 does not support libappindicator (neither the gtk nor ayatana version, see AyatanaIndicators/libayatana-appindicator#22).

The org.kde.StatusNotifierItem is non-standard, however it seems more likely to be a way forward than libappindicator.
See also https://pagure.io/fedora-workstation/issue/246 for a discussion on this.
For example, this should also work on default ubuntu desktop installs, with GNOME using https://github.com/ubuntu/gnome-shell-extension-appindicator.

Unfortunately, since it is not standardized yet, there is no python library for using it, so this PR needs to create its own bespoke implementation of the DBus interactions.
Additionally, this also drops dbus-python in favor of Gio's dbus.

This is tested in KDE Plasma 5.27, Plasma 6, and GNOME 46 with the extension.

@archisman-panigrahi
Copy link
Collaborator

archisman-panigrahi commented Jul 6, 2024

I tested this in Linux Mint 22 (based on Ubuntu 24.04). The tray icon is there, but it opens an empty menu. There is no error message in terminal. Am I missing some dependencies?

Screenshot:
image

Note: I tested the branch https://github.com/deltragon/SafeEyes/tree/trayicon-statusnotifier, without merging the gtk4 branch into it.

@deltragon
Copy link
Collaborator Author

Oh, that is interesting. I'll have to look into that.
I'm not sure what extension Mint is using for the trayicons, I'm assuming it has to do with that? Or did you install the one I linked above?

@archisman-panigrahi
Copy link
Collaborator

I'm not sure what extension Mint is using for the trayicons, I'm assuming it has to do with that? Or did you install the one I linked above?

AFAIK, Cinnamon desktop does not use extensions to display tray icons. It does so natively for both GTK and Qt apps. GNOME extensions are not compatible with Cinnamon

@archisman-panigrahi
Copy link
Collaborator

archisman-panigrahi commented Jul 6, 2024

I tried after installing gir1.2-appindicator3-0.1, but it did not help

@archisman-panigrahi
Copy link
Collaborator

archisman-panigrahi commented Jul 6, 2024

Qt apps like KDE Connect sucessfully display their tray icon settings, but Safe Eyes does not.

image

@deltragon
Copy link
Collaborator Author

Oh, that's Cinnamon, makes sense. (I mistook it for GNOME)
Hmmm, maybe we'll have to make sure to support appindicator as well then... I'm gonna check what KDE Connect does there.
(Installing libappindicator won't make a difference here, since that's what this PR is dropping.)

@archisman-panigrahi
Copy link
Collaborator

Hmm. Ideally we would like SafeEyes to work everywhere (GNOME/KDE/XFCE/MATE/Cinnamon/LXDE....) out of the box, so we need more testers to let us know how it is working.

@deltragon
Copy link
Collaborator Author

I'm considering if I can add back support for libappindicator here, similar to how the smartpause plugin also supports xprintidle/swayidle/etc... that might be the safest option here.

@deltragon deltragon marked this pull request as draft July 6, 2024 21:10
@deltragon
Copy link
Collaborator Author

Marking this as draft until I have a chance to look into that.

@deltragon deltragon force-pushed the trayicon-statusnotifier branch 7 times, most recently from 2f723c9 to 0ade3bf Compare July 8, 2024 12:10
@archisman-panigrahi
Copy link
Collaborator

@deltragon Please ping me once you think it is ready.

@deltragon
Copy link
Collaborator Author

Okay, so coming back to this:
libappindicator, under the hood, also just uses org.kde.StatusNotifierItem as well.
(It has a fallback on XEmbed, but that does not seem to be necessary anymore on most modern desktops - I've tested LXQt, XFCE, Mate, Cinnamon and Budgie. In theory, some other setups/DEs might still need it, but it seems like SNI is a quite well supported "standard").
My idea to keep using libappindicator as a fallback won't work - the issue is that it depends on Gtk 3, but you can't have Gtk 3 and 4 in the same process. (There would be a theoretical hack of moving it to its own sub-process - however, that feels like an even bigger mess than manually implementing SNI.)

The issue on Cinnamon had been that, well, it's not actually a standard - while there is org.freedesktop.StatusNotifierItem, nobody actually uses that - everyone uses the KDE version, or specifically, the version that libappindicator/libayatana-appindicator implemented, which have actually diverged quite a bit. (Same story with org.canonical.dbusmenu.)
Originally, I was assuming that they all were the same as the freedesktop one - however, it appears that wasn't the case, and Cinnamon basically tailored its implementation to the libappindicator one.
I have now adapted the implementation to be closer to libappindicator, and successfully tested it on GNOME, KDE, LXQt, XFCE, Mate, Cinnamon and Budgie. (There is some slight variance in behaviour, eg. on LXQt left-click will not open the menu, only right-click will - but that seems to be an LXQt issue, that also happens with libappindicator.)

The only downside I see is that we have another bespoke implementation of the SNI "spec", with all the maintenance effort that brings. However, if we want a Gtk4 port with a working status icon, we will have to have that maintenance effort in some way or another, as there is no other implementation that fits our needs, nor a competing standard to SNI that is widely implemented.
(There is a Go or a Rust port of SNI - however, binding to these languages is probably just as complicated.)

In short, I have fixed the implementation, and tested it on all major DEs. In my opinion, this is ready to merge.

@deltragon deltragon marked this pull request as ready for review July 12, 2024 18:36
@archisman-panigrahi
Copy link
Collaborator

In short, I have fixed the implementation, and tested it on all major DEs. In my opinion, this is ready to merge.

Thank you very much! I will test it soon.

the issue is that it depends on Gtk 3 ....
However, if we want a Gtk4 port with a working status icon...

I don't want to argue about benefits of GTK3 vs GTK4, but, GTK3 is not going away anytime soon. Gimp3 depends on GTK3, and it is not even released yet. Also, the Linux Mint team is planning to continue working with GTK3 as it has better compatibility outside GNOME desktop. I am sure it will be supported for another 10 years or so...

However, since you have already implemented an alternative, sticking to GTK3 is not anymore necessary :)

The only downside I see is that we have another bespoke implementation of the SNI "spec", with all the maintenance effort that brings. However, if we want a Gtk4 port with a working status icon, we will have to have that maintenance effort in some way or another, as there is no other implementation that fits our needs, nor a competing standard to SNI that is widely implemented.

Given you have already implemented it, why do you think that it will require extra maintenance effort in future?

@deltragon
Copy link
Collaborator Author

@archisman-panigrahi Yeah, that's a fair argument. I was gonna say that I wanted Gtk4 for its better wayland support, and specifically for #576 - however, I just realized that that PR doesn't actually need Gtk4 anymore... so I've marked it ready for review :)

I don't see much maintenance effort in the future, assuming that things stay as they are, and the SNI implementations don't change much. It might be that we see a similar issue like with Cinnamon at some point, however, where maybe KDE decides that they now expect implementations to provide more methods.
Generally, the maintenance should be pretty self-contained on the one plugin.

@archisman-panigrahi
Copy link
Collaborator

archisman-panigrahi commented Jul 12, 2024

Works in Cinnamon in Linux Mint 22 beta. Also, both left and right click works (unlike what you found in LXQt). So far, so good.

I will test this in LXDE/Openbox (Raspberry Pi OS - which has millions of users), and merge it.

@archisman-panigrahi
Copy link
Collaborator

archisman-panigrahi commented Jul 16, 2024

Question: Suppose they disable the trayicon plugin, and run safeeyes from command line. How can they enable the plugin ever again? One cannot open settings without the tray icon.
What if they click on the "Disable plugin" button on first run by mistake?

I suggest this "Disable plugin" happens only in that session, and the error is again shown the next time safe eyes is run. If someone needs to stop showing the error, they will have to run safe eyes with a different argument (--no-warning or -nw or something like that).

(I hope you are not getting impatient by my comments, I respect your skills and patience to listen to my comments, I just want to ensure the best user experience of Safe Eyes before merging this).

@deltragon
Copy link
Collaborator Author

Okay.
I've thought about it some more, and I agree - I've changed it to only temporarily disable, until the next start.
(With accompanying wording changes).

For the commandline users, who don't care about the trayicon: I don't think we need to worry about them much, if I'm honest. There's a -s commandline option that they can use to open the settings, and if someone is commandline-only, they can already use that.
But I think the number of people who don't care about the trayicon, but also have the trayicon plugin enabled, is probably small.
(I did add a button to "Permanently disable" an errored plugin there, so they also have an option to disable the plugin and make the popup go away.)

So, to recap:
The users on LXDE that don't use the commandline will see:
image

And the commandline users who use safeeyes -s will see:
image

Conversely, I feel I should thank you for your patience! I think together we have much improved the situation. I really appreciate your feedback.

@archisman-panigrahi
Copy link
Collaborator

Great! I will test this in my LXDE setup in a day or two.

@archisman-panigrahi
Copy link
Collaborator

archisman-panigrahi commented Jul 17, 2024

Before merging, I want to test this in the following.

  1. Elementary OS-Pantheon desktop: Works after installing plugin for tray icon. See next comment for details. I found that tray icons are not supported in Pantheon desktop. It is not a safeeyes bug, and we won't fix this for now.

In Pantheon the current master branch of safe eyes runs without a tray icon, without any warning. https://github.com/deltragon/SafeEyes/tree/trayicon-statusnotifier shows the following error.
img

If I install snixembed, no icon is shown. But safeeyes runs in the background without any warning, and asks me to blink my eyes, etc.

After further checking I discovered that apps like telegram or ktorrent run without showing any tray icons.


  1. Customized LXDE-Raspberry Pi OS: Will test and update this comment
  2. LXDE-Debian 13: Verified it works (with my script to set up snixembed). However, trayicon only shows up with right click. No response on left click (I can live with that. That is probably the behavior of snixembed: "context menu on right mouse button", according to README). The eye blinking excercises work (however, there is no build-in notification support in Debian LXDE, it seems).
  3. Manjaro Sway: There is a bug - login does not work in the live image - let's forget it.
  4. Manjaro i3: Could not figure out how to use - let's forget it.
  5. Arch Linux KDE 6.1.3: Works!
  6. Debian 13 GNOME: works fine after installing the extension.

@deltragon
Copy link
Collaborator Author

The image for the error in Pantheon does not load for me.

@deltragon
Copy link
Collaborator Author

I did find these plugins for Pantheon:
https://github.com/donadigo/wingpanel-indicator-namarupa
https://github.com/MvBonin/wingpanel-community-indicators
But I think we can look into that outside of this PR, as it doesn't make the situation any worse imo.

@archisman-panigrahi
Copy link
Collaborator

archisman-panigrahi commented Jul 17, 2024

Works after installing Wingpanel Indicator. I updated the wiki for Pantheon.

BTW, in Elementary OS 7, I do get the GUI warning that tray icon is not working when Wingpanel indicator is not running (needs to be added to startup applications).

(I am considering moving to elementaryOS, it is so beautiful!).

image

I will just test in Raspberry Pi later today, and merge.

@archisman-panigrahi
Copy link
Collaborator

Thank you so much!

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