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

fix: libappindicator left-click support #35319

Closed
wants to merge 1 commit into from

Conversation

janci007
Copy link

@janci007 janci007 commented Aug 12, 2022

Description of Change

There is a long-standing issue of left-click on tray icon not working on Linux when libappindicator is installed
#14941

This PR along with a libappindicator patch
(in progress here https://bugs.launchpad.net/ubuntu/+source/libappindicator/+bug/1910521 )
enables the Activate event of tray icon to be propagated into electron app.

This builds on already implemented solution of creating a replacement menu item in systray menu that will be triggered if the tray icon is clicked. This code was activated and adapted to create the replacement item as hidden.

In libappindicator, the relevant Activate method was implemented and will trigger activation of this replacement menu item.

Checklist

Release Notes

Notes: Added support for left-click app activation when using libappindicator based tray icon

@welcome
Copy link

welcome bot commented Aug 12, 2022

💖 Thanks for opening this pull request! 💖

We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix.

Examples of commit messages with semantic prefixes:

  • fix: don't overwrite prevent_default if default wasn't prevented
  • feat: add app.isPackaged() method
  • docs: app.isDefaultProtocolClient is now available on Linux

Things that will help get your PR across the finish line:

  • Follow the JavaScript, C++, and Python coding style.
  • Run npm run lint locally to catch formatting errors earlier.
  • Document any user-facing changes you've made following the documentation styleguide.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Aug 12, 2022
@janci007 janci007 changed the title Fix libappindicator support fix libappindicator left-click support Aug 12, 2022
@janci007 janci007 changed the title fix libappindicator left-click support fix: libappindicator left-click support Aug 12, 2022
Copy link
Member

@zcbenz zcbenz left a comment

Choose a reason for hiding this comment

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

While I'm good with this change, I think we should wait for maintainer's response on the upstream issue first.

@zcbenz zcbenz added semver/patch backwards-compatible bug fixes target/19-x-y target/21-x-y PR should also be added to the "21-x-y" branch. labels Aug 15, 2022
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Aug 15, 2022
@zcbenz zcbenz added the blocked/upstream ❌ Blocked on upstream; e.g. Chromium or Node label Aug 23, 2022
Copy link
Member

@ckerr ckerr left a comment

Choose a reason for hiding this comment

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

I'm fine with improving how Electron behaves on systems with libappindicator installed, but do want to point out that libappindicator is (IIUC) a dead legacy package. Does this patch change behavior on modern Linux desktops that don't ship with libappindicator?

@janci007
Copy link
Author

What modern Linux desktops do you mean and what is the replacement to libappindicator? In my experience many apps ship with libappindicator dependencies - many electron apps (slack, teams) and some native (remmina) use libappindicator.

If libappindicator is present on the system, electron will load it and use it and break the left-click behavior. It actually works fine when libappindicator is not present.

This change should not affect systems without libappindicator - changes were made in files belonging to libappindicator variant of tray icon implementation.

@ckerr
Copy link
Member

ckerr commented Sep 14, 2022

What modern Linux desktops do you mean and what is the replacement to libappindicator?

I should've been more careful with my wording; calling other desktops "modern" doesn't imply anything pejorative about desktops with libappindicator. I'm in favor of patches that help Electron to behave well on older systems as well.

To answer your question, I was thinking of Ubuntu. I don't believe that 22.04 ships with libappindicator.

@janci007
Copy link
Author

AFAIK libappindicator is in the ubuntu 22.04 repo but is not installed with the default desktop environment. It will be pulled in if any app with libappindicator dependency is installed.

Do you know what is a possible replacement to libappindicator?

(Please understand I would be more than happy if the libappindicator library would be wiped out of existence, it's just not something we can make happen.)

If there really is a replacement for libappindicator, Electron can use it in preference to libappindicator. It is not the case now - it will use libpappindicator whenever it is available in the system. That would be better solution to the left-click problem, as this fix depends on libappindicator accepting the fix mentioned before and it seems to be stalled.

@SpacingBat3
Copy link

SpacingBat3 commented Sep 20, 2022

While I'm good with this change, I think we should wait for maintainer's response on the upstream issue first.

To be honest, I've never thought this is an upstream issue seeing a lot of apps on XFCE actually doing custom actions on left-click. But even with the XFCE users have a choice to force showing right-click popup on left-click, so maybe it's still up to DE and their software to represent a libappindicator as an applet on the panel?

From what I understand by looking at this code on upstream GitLab repository, there seems that no matter of the method, it is always invoked, so if somehow the Activate method is emitted (I guess that is dependant on the DE), libappindicator seems to log a warning and call this as unknown method, but passes it further anyway.

From my perspective, it's been always that weird Electron limitation I did not understand when comparing Electron apps to native ones. This is why I'd love to see this PR merged even before upstream will define a logic for Activate (at least not call it unknown) if it doesn't break with the current libappindicator in any way.

Do you know what is a possible replacement to libappindicator?

Other than GtkStatusIcon that is already being supported and AFAIK is older and meant to be replaced by libappindicator, I don't see much alternatives. I don't see why libappindicator is a legacy package. Maybe the only alternative that comes to my mind is to develop a DE-specific code for custom applets, but that sounds crazy from the developers' perspective and is likely not worth it, considering libappindicator doing its job well.

@zcbenz
Copy link
Member

zcbenz commented Sep 28, 2022

libappindicator is just sending dbus messages under the hood, we can of course send raw messages in Electron instead of relying on libappindicator.

Chromium has actually removed uses of libappindicator and is sending raw dbus messages in their implementation:
https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/views/status_icons/status_icon_linux_dbus.cc?ss=chromium

@jkleinsc jkleinsc added the target/22-x-y PR should also be added to the "22-x-y" branch. label Sep 28, 2022
@jkleinsc
Copy link
Contributor

@jkleinsc jkleinsc closed this Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked/upstream ❌ Blocked on upstream; e.g. Chromium or Node semver/patch backwards-compatible bug fixes target/21-x-y PR should also be added to the "21-x-y" branch. target/22-x-y PR should also be added to the "22-x-y" branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants