8000 fix: restore original GTK/appindicator implementation of tray icons by MarshallOfSound · Pull Request #23674 · electron/electron · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix: restore original GTK/appindicator implementation of tray icons #23674

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

Merged
merged 1 commit into from
Jun 3, 2020

Conversation

MarshallOfSound
Copy link
Member

Description of Change

In https://chromium-review.googlesource.com/c/chromium/src/+/1820024 Chromium removed the implementation of Tray icons we were using on Linux (GTK + Appindicator backed) in favour of their new DBUS backed implementation.

However the new DBUS implementation is not production ready and has a decent number of bugs that would prevent folks shipping to prod.

This PR carefully restores the original code that Chromium deleted. Tested so far on Ubuntu and will be testing on other distros shortly 😄

Note: I know this looks like a lot of code, but 99% of it is literally just copy-pasting code from Chromium that we had already shipped with for years.

The intention is to remove this implementation was the DBUS implementation has achieved production ready-ness which I will be actively monitoring. In a follow up PR (as this PR is supposed to be backported) I'll add a build flag to toggle between the implementations.

Release Notes

Notes: Restored old implementation of Linux Tray icons to fix a collection of issues where the tray icon wouldn't appear, would be the wrong size or would randomly disappear

@electron-cation electron-cation bot added the new-pr 🌱 PR opened recently label May 20, 2020
@MarshallOfSound MarshallOfSound force-pushed the tray-icon-but-i-dont-know-what-im-doing branch from 59c34c3 to 0255595 Compare May 20, 2020 00:49
@tarruda
Copy link
Contributor
tarruda commented May 24, 2020

To build on 8-x-y branch, this patch is necessary:

diff --git a/shell/browser/ui/gtk/menu_util.cc b/shell/browser/ui/gtk/menu_util.cc
index 3549e71f5..c80be3349 100644
--- a/shell/browser/ui/gtk/menu_util.cc
+++ b/shell/browser/ui/gtk/menu_util.cc
@@ -18,7 +18,6 @@
 #include "third_party/skia/include/core/SkUnPreMultiply.h"
 #include "ui/base/accelerators/accelerator.h"
 #include "ui/base/accelerators/menu_label_accelerator_util_linux.h"
-#include "ui/base/models/image_model.h"
 #include "ui/base/models/menu_model.h"
 #include "ui/events/event_constants.h"
 #include "ui/events/keycodes/keyboard_code_conversion_x.h"
@@ -155,6 +154,7 @@ void BuildSubmenuFromModel(ui::MenuModel* model,
   std::map<int, GtkWidget*> radio_groups;
   GtkWidget* menu_item = nullptr;
   for (int i = 0; i < model->GetItemCount(); ++i) {
+    gfx::Image icon;
     std::string label = ui::ConvertAcceleratorsFromWindowsStyle(
         base::UTF16ToUTF8(model->GetLabelAt(i)));
 
@@ -188,9 +188,8 @@ void BuildSubmenuFromModel(ui::MenuModel* model,
       }
       case ui::MenuModel::TYPE_SUBMENU:
       case ui::MenuModel::TYPE_COMMAND: {
-        auto icon_model = model->GetIconAt(i);
-        if (!icon_model.IsEmpty())
-          menu_item = BuildMenuItemWithImage(label, icon_model.GetImage());
+        if (model->GetIconAt(i, &icon))
+          menu_item = BuildMenuItemWithImage(label, icon);
         else
           menu_ite
8000
m = BuildMenuItemWithLabel(label);
 #if !GTK_CHECK_VERSION(3, 90, 0)
@@ -287,10 +286,9 @@ void SetMenuItemInfo(GtkWidget* widget, void* block_activation_ptr) {
 #if !GTK_CHECK_VERSION(3, 90, 0)
         G_GNUC_BEGIN_IGNORE_DEPRECATIONS;
         if (GTK_IS_IMAGE_MENU_ITEM(widget)) {
-          auto icon_model = model->GetIconAt(id);
-          if (!icon_model.IsEmpty()) {
-            GdkPixbuf* pixbuf = gtk_util::GdkPixbufFromSkBitmap(
-                *icon_model.GetImage().ToSkBitmap());
+          gfx::Image icon;
+          if (model->GetIconAt(id, &icon)) {
+            GdkPixbuf* pixbuf = gtk_util::GdkPixbufFromSkBitmap(*icon.ToSkBitmap());
             gtk_image_menu_item_set_image(GTK_IMAGE_MENU_ITEM(widget),
                                           gtk_image_new_from_pixbuf(pixbuf));
             g_object_unref(pixbuf);

Copy link
Contributor
@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.

Looks good to me since the code is easy to maintain.

Copy link
Contributor
@tarruda tarruda left a comment

Choose a reason for hiding this comment

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

LGTM

Also built branch 8-x-y with the patch I posted above and it worked fine on Debian 9

@vladimiry
Copy link
vladimiry commented May 26, 2020

I wish this change happened in along with Electron v8 release since it would save tons of developers time and effort in working around the DBus-based tray icons/menus behavior on different systems. But better late than never 👍 @MarshallOfSound, thanks for taking an initiative.

The bad thing is that we will likely lose the proper left/primary mouse click behavior again (originally broken starting from the @electron v3, see #14941).

@hovancik
Copy link
hovancik commented Jun 5, 2020

I am actually sorry to hear the StatusIconLinuxDbus is getting ripped of. For me, it solved many tray related issues and my app was finally working well on Linux ;/

@ashpieboop
Copy link
ashpieboop commented Jun 5, 2020

@hovancik as of what I understand, it should be brought back when it's more widely spread (and stable?) across DEs. Probably years to wait for that though.

@antanasja
Copy link
antanasja commented Jun 10, 2020

@MarshallOfSound It caused a new issue at least on Ubuntu 20.04 #24045

@Kodehawa
Copy link
Kodehawa commented Nov 17, 2020

I know this is an already-merged PR, but why does this use Ubuntu's AppIndicators? They've been dead for years and the active fork is Ayatana Indicators (Explanation)

It should be compatible with the old AppIndicator logic, but I think it only makes sense to use what everyone else (see: LightDM, MATE, etc) uses now, instead of the old Unity AppIndicators which are as dead as everything Unity7 related, with many distros not even shipping Unity's AppIndicators anymore (Arch only ships Ayatana with libappindicator, for example)

(Commenting because I just saw this on the Electron 11 release notes)

ttys3 added a commit to ttys3/lark-for-linux that referenced this pull request Mar 29, 2021
see "Tray icon disappear from Topbar when system goes in stand by"
electron/electron#22443

and related fixup: electron/electron#23674
Ericwyn pushed a commit to Ericwyn/electron-lark that referenced this pull request Apr 26, 2021
see "Tray icon disappear from Topbar when system goes in stand by"
electron/electron#22443

and related fixup: electron/electron#23674
PedroHLC referenced this pull request in osdlyrics/osdlyrics May 20, 2021
Using the menu items to modify the delay is just not handy at all. To
make it easier to do it using the tray icon, add a scroll callback
acting just like in the main window (up to increase delay, down to
reduce it).
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.

9 participants
0