phocean / TopIcons-plus

An gnome-shell extension to put the icons back to the tray.
674 stars 98 forks source link

fix compatibility with gnome40 #156

Closed kofemann closed 3 years ago

audreytoskin commented 3 years ago

I built and installed TopIcons Plus commit 295026a in a virtual machine of Rawhide (development branch of Fedora) x86_64, running GNOME 40.rc. With your patches, I can successfully install and enable TopIcons Plus once again, and Looking Glass shows no errors... though actually adding any system tray icons to the Shell's top bar is a little spotty.

Those latter two applications did used to show status icons with TopIcons Plus v27 on GNOME 3.38 and Fedora 34. I don't know if this is remaining regression in the extension, though, or if it's something about those applications, or Qt, or whatever.

audreytoskin commented 3 years ago

...Oh, sorry, that was while running in GNOME in a Wayland session. Switching GNOME to Xorg lets all three applications' status icons to display normally again -- which might be how it was on GNOME 3.38 too. So, I guess the issue is just that there's still some remaining problems with Wayland, I guess?

jwrdegoede commented 3 years ago

I can confirm that this fixes GNOME40 compatibility, resolving issue #155. It would be great if this could be merged.

audreytoskin commented 3 years ago

With commit 295026a, TopIcons Plus does manage to still display status icons in the top bar on GNOME 3.38, however the settings dialog is broken. Looking Glass shows no errors, but when you launch GNOME Extensions, and click the settings button for TopIcons Plus, it only shows a white dialog that says "Something's gone wrong..." The technical details area is blank.

So, either there would need to be a little extra work done to make the same verison of TopIcons Plus compatible with both GNOME 3.x and GNOME 40... Or, the metadata.json should be updated to show that this version is only compatible with 40+.

kofemann commented 3 years ago

Actually, the gnome-shell-extension-appindicator, which is recommended by @phocean, works right away.

audreytoskin commented 3 years ago

libappindicator doesn't support everything, last I checked. My package users on Fedora still rely on TopIcons Plus to show status icons for the apps that libappindicator misses.

jwrdegoede commented 3 years ago

libappindicator doesn't support everything, last I checked. Some of my package users on Fedora still rely on TopIcons Plus to show status icons for the apps that libappindicator misses.

Correct, for example I use hexchat and that uses an old-fashioned tray-icon.

When libappindicator is supported, then it is better to use gnome-shell-extension-appindicator though.

I have both installed. When both are installed then libappindicator enabled apps use the appindicator API and other ones use the old tray-icon stuff.

Only installing TopIcons works too, then libappindicator fallsback to using the trayicon, but only if the app us running through Xwayland.

If the app has native Wayland support then libappindicator + the extension must be used to get an icon in gnome-shell's panel.

audreytoskin commented 3 years ago

To be clear, @kofemann, I still think either the metadata.json should be updated to list "40" as the only compatible version, or else this pull request will need additional work to make TopIcons Plus compatible with both GNOME 3.36+ and GNOME 40. Because of the regression at least on 3.38 I mentioned in comment 5 | https://github.com/phocean/TopIcons-plus/pull/156#issuecomment-805118949

...Probably just changing the compatible versions to just "40" would be fine, though, since previous TopIcons Plus releases worked on GNOME 3.36 and 3.38, and the extension hasn't changed all that much since then.

Then it would be ready to merge, IMHO.

kofemann commented 3 years ago

removed all otehr version of gnome except 40. Bumped version the number.

jugendhacker commented 3 years ago

I added this patch to the AUR package of TopIcons Plus because without it, it won't work on Arch anymore (yay bleeding edge). But so far it seems to work again under GNOME 40 for me.

yookoala commented 3 years ago

@kofemann This PR works for me on GNOME 40, too.

If needed (i.e. if TopIcon plus really decide to discontinue), would you please fork and create another extension?

kofemann commented 3 years ago

@yookoala I am ok with fixing things that broken for me, but I am not ready maintain the extention, at least for two reasons:

niklasravnsborg commented 3 years ago

Installing https://extensions.gnome.org/extension/615/appindicator-support/ works for me on Fedora 34 with Gnome 40.

VergeDX commented 3 years ago

@yookoala I am ok with fixing things that broken for me, but I am not ready maintain the extention, at least for two reasons:

  • this is not my domain of expertise
  • I have switch myself to gnome-shell-extension-appindicator

Well but gnome-shell-extension-appindicator cannot convert wine apps tray icon... we still need this awesome plugins.

adamboutcher commented 3 years ago

I found that these modifications didn't work with gnome under Wayland (Intel) but it worked fine under X11 (Nvidia).

3v1n0 commented 3 years ago

FYI, we've included support for the legacy tray icons to https://github.com/ubuntu/gnome-shell-extension-appindicator so this should work also in the hexchat and wine / java case.

On the wayland aspect, the legacy icons won't work under wayland when x11 is disabled, while appindicators will (but you may need libappindicator installed).

As per this PR, few more changes are needed on the preferences though (such as adapting the margins), plus this extension should probably move away from using System.gc() at disable (so each time you lock the screen) now that unmanage_screen is provided.

jwrdegoede commented 3 years ago

@3v1n0, very nice that you have added legacy tray icon support to gnome-shell-extension-appindicator.

I see that you have also just tagged a v40 release, which includes the legacy tray icon support, great!

In the Fedora TopIcons package we were planning on adding a Requires on gnome-shell-extension-appindicator since that is needed to make trayicon/appicon like functionality to work for most native Wayland apps.

But given that TopIcons is not really being actively maintained; and that gnome-shell-extension-appindicator now also supports the legacy icons, I think that it might be better to just make gnome-shell-extension-appindicator Obsolete the TopIcons package, so that it automatically gets replaced with gnome-shell-extension-appindicator on upgrades. Do you have any opinion on this ?

Also I guess that since gnome-shell-extension-appindicator now supports the legacy icons the 2 extensions will conflict with each other if both are installed? Which would be another good reason to make gnome-shell-extension-appindicator obsolete the TopIcons extension package.

3v1n0 commented 3 years ago

gnome-shell-extension-appindicator now also supports the legacy icons, I think that it might be better to just make gnome-shell-extension-appindicator Obsolete the TopIcons package, so that it automatically gets replaced with gnome-shell-extension-appindicator on upgrades. Do you have any opinion on this ?

We also use it by default in ubuntu, so indeed that's something we're maintaining and keeping it in the long run, so I guess would be a wise choice also from fedora side.

Also I guess that since gnome-shell-extension-appindicator now supports the legacy icons the 2 extensions will conflict with each other if both are installed?

There's not a "huge" conflict, in the sense that legacy tray icons can be only shown by one extension, so the first comes, the first it takes them.

I initially wanted to just make appindicators not to try even load the tray icons in case someone else is monitoring the Shell.Traymanager (unfortunately the only way is relying on the fact whether anyone else is connected to the tray-icon-added and tray-icon-removed signals), but I eventually decided to just try it as it's not a big deal, some extension will just work.

So, a conflict could be added but wouldn't cause any practical issue (the only one could be that icons are visible in different places and/or using different aspect) in case both extensions are loaded.

adamboutcher commented 3 years ago

We also use it by default in ubuntu, so indeed that's something we're maintaining and keeping it in the long run, so I guess would be a wise choice also from fedora side.

After trying the gnome-shell-extension-appindicator plugin on Fedora 34, I'd highly recommend making it a default and deprecating topicons.

phocean commented 3 years ago

If someone here would like to step up and maintain the extension, I would be happy to give all required accesses.

No need to create another extension...