-
Notifications
You must be signed in to change notification settings - Fork 16.2k
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
fix: restore original GTK/appindicator implementation of tray icons #23674
Conversation
59c34c3
to
0255595
Compare
0255595
to
dda720b
Compare
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); |
There was a problem hiding this 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.
There was a problem hiding this 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
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). |
I am actually sorry to hear the |
@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. |
@MarshallOfSound It caused a new issue at least on Ubuntu 20.04 #24045 |
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) |
see "Tray icon disappear from Topbar when system goes in stand by" electron/electron#22443 and related fixup: electron/electron#23674
see "Tray icon disappear from Topbar when system goes in stand by" electron/electron#22443 and related fixup: electron/electron#23674
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).
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