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

Skype Integration legacy icon removal sometimes fail #45

Closed
ronjouch opened this issue Dec 25, 2016 · 11 comments
Closed

Skype Integration legacy icon removal sometimes fail #45

ronjouch opened this issue Dec 25, 2016 · 11 comments
Labels

Comments

@ronjouch
Copy link

I use TopIcons-plus as well as Skype Integration, which has a feature that was broken by TopIcons-plus commit b332127 "add center position". The now-broken feature is that Skype-Integration can hide Skype's legacy tray icon (because it adds a GNOME HIG-compliant indicator). So, being unable to hide the legacy tray icon and adding a new modern indicator nevertheless, I end up with two Skype "top-right icons": the legacy, and the modern one.

Looking at recent changes, it looks like this line of TopIcons-plus b3321279 is the culprit: it changes the icon wm_class with:

let wmClass = icon.wm_class ? icon.wm_class.toLowerCase() : '';

... so "Skype""skype". Unfortunately, Skype-Integration legacy-icon hiding function checks on the capitalized "Skype" string:

if(this._skypeHideOriginalTrayIcon && icon.wm_class == "Skype") {

→ I'd propose a patch removing this call to .toLowerCase(), but looking at b332127 I fail to understand why it's necessary in the first place. Is it / can we remove it? (Note: looking at the commit, I realize this wm_class de-capitalization code is not new, it was just extracted into a function. Anyway, maybe the class-changing was happening later / less frequently before b332127 or some changed logic now causes a race between TopIcons-plus and Skype-Integration; whatever: the end result is that I never had this problem until upgrading TopIcons-plus a few days ago, and now it's apparent)

Thanks for the continued maintenance of the extension :)

@phocean
Copy link
Owner

phocean commented Dec 25, 2016

Hello,

Thank you for the report.
I am using Skype version 4.3.0.37 and it works as expected. And I don't understand how the change I made would have some impact on this.

To clarify:

  • TopIcons does not change wmClass, it hides Skype icon when the Skype integration extension is enable. What you see in the code in just a local variable allocation, in no way it is changing anything on the Skype window.

  • It is normal that the code uncapitalises wmClass before it is compared with the blacklist. Not necessary but just to be safe.

  • Skype integration extension also has an option to hide the tray based on "Skype" string.
    So both features are partially redondant, but TopIcons covers more wmClass string cases.

I believe for some reason your wmClass is not exactly Skype.
What version of Skype are you using ?

@phocean
Copy link
Owner

phocean commented Dec 25, 2016

After starting Skype, could you give me the output of:

% xprop | grep WM_CLASS

You will have to select the Skype contact window when the cross cursor shows up.

@ronjouch
Copy link
Author

ronjouch commented Dec 25, 2016

@phocean

TopIcons does not change wmClass, it hides Skype icon when the Skype integration extension is enable. What you see in the code in just a local variable allocation, in no way it is changing anything on the Skype window.

True, sorry; I did a quick parse/blame of recent commits and stumbled on this line but I didn't read what was done with it. So let me "de-pinpoint" my incorrect conclusion to a broader regression statement:

  • Since I updated TopIcons-Plus on December 23, Skype-Integration fails to destroy the legacy icon (whose features are replaced/re-implemented with a proper Shell indicator), resulting into a duplicate Skype icon:
    topicons-skypeintegration-dup
  • Bug appears when starting Skype. If then I restart Shell (Alt+F2, r), then the legacy icon is properly destroyed:
    topicons-skypeintegration-afterrestart

What version of Skype are you using ?

~ skype --version              
Skype 4.3.0.37
© 2014 Skype and/or Microsoft

What does xprop | grep WM_CLASS return?

~ xprop | grep WM_CLASS
WM_CLASS(STRING) = "skype", "Skype"

@phocean
Copy link
Owner

phocean commented Dec 25, 2016

Ok, thank you for the detailed information. ;-)

I could reproduce the issue myself, but only once in a while. The cause is quite obscure to me, I have to investigate.

@ronjouch ronjouch changed the title b3321279 causes Skype Integration icon removal to fail due to changing tray icon wm_class Skype Integration legacy icon removal sometimes fail Dec 25, 2016
@ronjouch
Copy link
Author

ronjouch commented Jan 12, 2017

One more thing: in the same timeframe (so maybe due to the same changes?), other applications started to fail to be TopIcon-ified. For example, Transmission doesn't get its TopIcon, and I have to restart Shell (Alt+F2, r) to get it. I can create a new bug if you think these are two separate problems.

@phocean
Copy link
Owner

phocean commented Jan 12, 2017

EDIT: Ignore more or less my previous answer if you got it.

Again weirdness: on my 2 test machines, I can reproduce it only on Wayland and with either the legacy tray (Gnome on) or the topicons extension.
On Xorg, it works without any problem.

It is strange because, if you can reload, it means you are on Xorg.

Have you tried with the legacy tray only?

@phocean
Copy link
Owner

phocean commented Jan 12, 2017

It could be related to #42, by the way.

@ronjouch
Copy link
Author

@phocean

It could be related to #42, by the way.

Indeed. Thanks.

It is strange because, if you can reload, it means you are on Xorg.

Yes I am running Xorg, I blacklisted Wayland because I need X for a few X-only tools I use (wmctrl, xdotool, autokey) with no Wayland equivalent as far as I know :-/ .

Have you tried with the legacy tray only?

If that's what you mean, I just tried disabling TopIcons (i.e. leaving Shell do its bottom-left corner job), and I couldn't get the Transmission icon to disappear like it happens with TopIcons; everything seems okay: Transmission's icon is always present in the bottom-left tray.

@phocean
Copy link
Owner

phocean commented Jan 15, 2017

see issue #47 for Transmission, sorry but it is not something we can fix.

phocean pushed a commit that referenced this issue Jan 15, 2017
@phocean
Copy link
Owner

phocean commented Jan 15, 2017

I pushed a fix attempt on the dev branch. And, wow, that was a stupid and obvious mistake of mine.

So far it is working for me, could you please test as well?

@ronjouch
Copy link
Author

I pushed a fix attempt on the dev branch. And, wow, that was a stupid and obvious mistake of mine. So far it is working for me, could you please test as well?

@phocean seems to work fine here too; closing this bug, will re-open if I run into problems. Thanks for the fix!

phocean pushed a commit that referenced this issue Jan 15, 2017
@phocean phocean added the bug label May 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants