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: reimplement Tray with StatusIconLinuxDbus on Linux #36333

Merged
merged 4 commits into from Nov 28, 2022
Merged

Conversation

zcbenz
Copy link
Member

@zcbenz zcbenz commented Nov 14, 2022

Description of Change

This PR rewrites Tray's implementation on Linux with StatusIconLinuxDbus, which communicates with the desktop environment with dbus directly instead of relying on libappindicator.

Close #36283.
Close #36274.
Close #28956.
Close #28131.
Close #27527.
Close #14941.
Close #14635.

Release Notes

Notes: Fix click event and tooltip of Tray not working on Linux.

@zcbenz zcbenz added target/20-x-y 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. labels Nov 14, 2022
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Nov 14, 2022
@MarshallOfSound
Copy link
Member

@zcbenz Can you test and ensure that #22443 is not going to be an issue if we bring this implementation back. There were quite a few issues with this implementation the first time we tried to add it.

#23674

@zcbenz zcbenz added the wip ⚒ label Nov 14, 2022
@zcbenz
Copy link
Member Author

zcbenz commented Nov 14, 2022

@MarshallOfSound It is still there :(, I'll see if I can fix it.

@zcbenz zcbenz requested a review from a team as a code owner November 15, 2022 00:59
@zcbenz zcbenz added the semver/patch backwards-compatible bug fixes label Nov 15, 2022
@zcbenz
Copy link
Member Author

zcbenz commented Nov 15, 2022

I have submitted a patch to upstream Chromium to fix the tray icon disappearing problem:
https://chromium-review.googlesource.com/c/chromium/src/+/4022621.

@zcbenz zcbenz removed the wip ⚒ label Nov 15, 2022
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Nov 15, 2022
@VerteDinde
Copy link
Member

I saw we updated the Tray documentation - is there anything else that Linux developers/users would need to modify in their app for these Tray changes, or should they work out of the box?

@zcbenz
Copy link
Member Author

zcbenz commented Nov 21, 2022

is there anything else that Linux developers/users would need to modify in their app for these Tray changes, or should they work out of the box?

Existing apps will just work without any modification, this PR only fixes limitations of previous implementation without changing existing behaviors.

Copy link
Member

@nornagon nornagon left a comment

Choose a reason for hiding this comment

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

overall lgtm, hope we can get some movement on the chromium review upstream!

Comment on lines +32 to +33
* Tray icon requires support of [StatusNotifierItem](https://www.freedesktop.org/wiki/Specifications/StatusNotifierItem/)
in user's desktop environment.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Tray icon requires support of [StatusNotifierItem](https://www.freedesktop.org/wiki/Specifications/StatusNotifierItem/)
in user's desktop environment.
* The user's desktop environment must support [StatusNotifierItem](https://www.freedesktop.org/wiki/Specifications/StatusNotifierItem/)
in order to display a tray icon.

Comment on lines +34 to +37
* The `click` event is emitted when the tray icon receives activation from
user, however the StatusNotifierItem spec does not specify which action would
cause an activation, for some environments it is left mouse click, but for
some it might be double left mouse click.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* The `click` event is emitted when the tray icon receives activation from
user, however the StatusNotifierItem spec does not specify which action would
cause an activation, for some environments it is left mouse click, but for
some it might be double left mouse click.
* The `click` event is emitted when the tray icon receives activation from the
user. However, the StatusNotifierItem spec does not specify which interactions
cause an activation. For some environments, a left mouse click causes an
activation, but for others it might be double left mouse click, or some other
interaction.

Comment on lines +92 to +93
Note that on Linux this event is emitted when the tray icon receives an
activation, which might not necessarily be left mouse click.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Note that on Linux this event is emitted when the tray icon receives an
activation, which might not necessarily be left mouse click.
Note that on Linux this event is emitted when the tray icon receives an
activation, which might not necessarily be a left mouse click.

@nornagon nornagon merged commit 16a7bd7 into main Nov 28, 2022
@nornagon nornagon deleted the tray-dbus branch November 28, 2022 19:36
@release-clerk
Copy link

release-clerk bot commented Nov 28, 2022

Release Notes Persisted

Fix click event and tooltip of Tray not working on Linux.

@trop trop bot removed the target/21-x-y PR should also be added to the "21-x-y" branch. label Nov 28, 2022
@trop
Copy link
Contributor

trop bot commented Nov 28, 2022

I have automatically backported this PR to "22-x-y", please check out #36472

@trop trop bot added in-flight/22-x-y and removed target/22-x-y PR should also be added to the "22-x-y" branch. labels Nov 28, 2022
@trop trop bot added merged/22-x-y PR was merged to the "22-x-y" branch. and removed in-flight/22-x-y labels Nov 29, 2022
jkleinsc pushed a commit that referenced this pull request Nov 29, 2022
* fix: reimplement Tray with StatusIconLinuxDbus on Linux (#36333)

Co-authored-by: Cheng Zhao <zcbenz@gmail.com>

* chore: remove incorrectly added patches

Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com>
Co-authored-by: Cheng Zhao <zcbenz@gmail.com>
Co-authored-by: Keeley Hammond <vertedinde@electronjs.org>
@trop trop bot removed the in-flight/21-x-y label Nov 30, 2022
@trungutt trungutt mentioned this pull request Dec 7, 2022
3 tasks
khalwa pushed a commit to solarwindscloud/electron that referenced this pull request Feb 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged/22-x-y PR was merged to the "22-x-y" branch. semver/patch backwards-compatible bug fixes
Projects
None yet
7 participants