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 compatibility with gnome40 #156

Merged
merged 1 commit into from
Aug 10, 2021
Merged

fix compatibility with gnome40 #156

merged 1 commit into from
Aug 10, 2021

Conversation

kofemann
Copy link
Contributor

No description provided.

@audreytoskin
Copy link
Contributor

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.

  • 👍 Latest Telegram client (the static executable binary from desktop.telegram.org) works as expected.
  • ⛔ ProtonMail Bridge 1.6.6 (RPM package downloaded from protonmail.com/bridge/) fails to display its status icon.
  • ⛔ KeePassXC 2.6.4 (revision 34a78f0, built with Qt 5.15.2) fails to display its status icon (after enabling in the app's menu bar > Tools > Settings > General > User Interface).

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

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

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

@audreytoskin
Copy link
Contributor

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
Copy link
Contributor Author

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

@audreytoskin
Copy link
Contributor

audreytoskin commented Mar 24, 2021

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

jwrdegoede commented Mar 24, 2021 via email

@audreytoskin
Copy link
Contributor

audreytoskin commented Apr 9, 2021

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 | #156 (comment)

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

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

@jugendhacker
Copy link

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
Copy link

yookoala commented Apr 16, 2021

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

@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

@niklasravnsborg
Copy link

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

@VergeDX
Copy link

VergeDX commented May 10, 2021

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

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

@3v1n0
Copy link

3v1n0 commented Jun 11, 2021

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

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

3v1n0 commented Jun 14, 2021

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
Copy link

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 phocean merged commit 0f1c932 into phocean:master Aug 10, 2021
@phocean
Copy link
Owner

phocean commented Aug 10, 2021

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...

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.

10 participants