TopIcons-plus icon indicating copy to clipboard operation
TopIcons-plus copied to clipboard

Top icons stop displaying.

Open zelch opened this issue 7 years ago • 26 comments

Alright, if you have a single icon being displayed by TopIconPlus, and we get a tray-icon-removed signal for an icon that isn't on top, we do the following:

We destroy the icon. We call icons.splice(-1, 1), which appears to remove the first element of the array. If the array is then empty (length == 0, and it will be if we only had one), we proceed to set the iconsContainer.active.visible = false.

Except... We actually have icons left, but now we can't ever see them, and future operations are going to get confused because we have icons in the container that are not in the icons array.

If you look at the _onTrayIconRemoved function in gnome-shell's legacyTray.js, we see:

_onTrayIconRemoved: function(tm, icon) {
    if (!this.actor.contains(icon))
        return;

    icon.get_parent().destroy();
    this._sync();
},

The this.actor.contains check seems to be trying to catch this exact case.

I suggest adding the following check to the top of onTrayIconRemoved:

if (icons.indexOf(icon) == -1) {
    log("onTrayIconRemoved for invalid icon!");
    return;
}

(Or without the log statement.)

With that in place, so far my vanishing pidgin seems to no longer be vanishing, and I have seen log entries for the invalid icon case.

zelch avatar Jun 19 '17 20:06 zelch

Thank you for your research and detailed explanation ! Very appreciated. I will look at this in a few days, as soon as I have free time.

phocean avatar Jun 20 '17 04:06 phocean

I'm glad to help. :)

Looking at the logs (I added a fair bit of debugging statements while tracking this down, and have not removed them yet), I see another case which is causing things to disappear.

Somehow, moveToTop is getting called repeatedly, as best as I can tell there is a rapid sequence of disable/enable several times in a row. This gets moveToTray called repeatedly for each disable, which might cause problems with the tray signals getting connected repeatedly, but otherwise looks safe.

However 4 seconds later the idle handlers run, causing movetoTop to get called several times in a row.

The first one does what it is supposed to, however the following ones are another story. The repeated calls to disconnect cause warnings, but the bigger problem is that we create brand new iconsBoxLayout and iconsContainer objects, and since we have already removed the items from the tray iconBox, we don't add the icons to the new objects. (Nor do we destroy the old ones.)

At this point, things are broken and will remain broken until gnome-shell is restarted.

I am going to test with the following added to the top of moveToTop:

if (iconsBoxLayout) {
    log("moveToTop called while already moved to the top.");
    return;
} 

zelch avatar Jun 20 '17 21:06 zelch

I am still seeing some oddities that I am trying to track down, it is helping, but... Odd.

I am curious, why is moveToTop called via GLib.idle_add instead of being called directly?

zelch avatar Jun 20 '17 22:06 zelch

@zelch u a fixed it bug? i have it bag too. icons just invisible sometime. and i don't know similar apps for tray icons..

tihoho avatar Aug 09 '17 08:08 tihoho

My icons are stopped displaying as well. I see a chunk of space where they ought to be. I have around 4-5 icons. Tried changing all of the display settings/opacity/contrast etc. No luck.

Unshackle8078 avatar Aug 12 '17 01:08 Unshackle8078

I have the same bug as @Enigma0, screenshot attached: image I can click on the "icons" that are invisible in that area and that brings up the appropriate windows and context menus, but there's no icon visible still.

marsdeat avatar Aug 19 '17 21:08 marsdeat

Started working again for me somehow.

Unshackle8078 avatar Aug 27 '17 14:08 Unshackle8078

@Enigma0 did you update the extension or anything special?

phocean avatar Aug 27 '17 19:08 phocean

@zelch sorry for the long silence, I am coming back little by little. With all your testing, have you got some stable code that fix the issue?

About the Glib call, it is inherited from the original extension. I am not sure if the priority really makes sense or is respected in any way... Another thing to test.

phocean avatar Aug 27 '17 20:08 phocean

@phocean Nope. I installed the original TopIcons extension several times with a bunch of enabling/disabling of each one.

Unshackle8078 avatar Aug 27 '17 21:08 Unshackle8078

@phocean I have code that definitely fixes the issue at the top. From the description I'm pretty sure that @Enigma0 has a different bug.

(Though, it is possible that they are tightly related.)

Now, I can't swear about stable. I keep encountering a gnome-shell segfault, though not very frequently (definitely not frequently enough to say that just because I don't see it in a week that it's really gone). I don't have any reason to believe that it is related to this extension at all, let alone my changes, but I don't have any evidence pointing at anything else either.

My raw diff is largely debug lines, I'll attach the whole thing though, as it may be helpful. And I do suggest going forward with the actual code changes. (Which I'd be happy to isolate out into a separate diff.)

zelch avatar Aug 29 '17 04:08 zelch

TopIcons.diff.txt

Here is the diff with all the debugging.

zelch avatar Aug 29 '17 04:08 zelch

And here is the diff with just the code changes.

TopIcons.code_only.diff.txt

zelch avatar Aug 29 '17 04:08 zelch

I still occasionally see 1 or 2 icons disappear (they still exist and can be interacted with) but usually closing and re-opening that particular tray program fixes it.

Unshackle8078 avatar Aug 29 '17 05:08 Unshackle8078

@Enigma0 If you can easily reproduce this, can you try applying my diff and restarting gnome-shell?

Save TopIcons.diff.txt in /tmp/

cd ~/.local/share/gnome-shell/extensions/[email protected] cat /tmp/TopIcons.diff.txt | patch -s

Then log out and log back in.

If that fixes the problem, that would be interesting. If it doesn't fix the problem then getting the debug output from around the time that the icons disappeared with the following command would be helpful.

journalctl | grep gnome-session

(Note, you don't need to send the whole thing, just the timestamps right around when things started going wonky.)

Thanks.

zelch avatar Aug 29 '17 22:08 zelch

I get the same thing with an app called openFortiGUI. After some time it kills topicons-plus and I have to disable/enable a few times, sometimes kill the app and restart.

@zelch I'll add your patch and see if it resolves the issue for me.

matjam avatar Sep 03 '17 16:09 matjam

@zelch I cannot anymore - seems to be fixed for me.

Unshackle8078 avatar Sep 04 '17 15:09 Unshackle8078

@Enigma0 To confirm, applying the patch seems to have resolved the issue?

zelch avatar Sep 04 '17 20:09 zelch

Here is an additional fix on top of my non-logging version, which seems to have cleared up a crash due to race condition.

TopIcons_More.diff.txt

And here is an updated version of my full patch with all of the debug statements, sorry, no incremental patch here.

TopIcons_Debug.diff.txt

I will update if I see any additional crashes, but I'm pretty sure that this is pretty close to final.

@phocean How would you like everything for a release? The current diffs? A new diff with all the changes but none of the logging? A pull request?

zelch avatar Sep 05 '17 04:09 zelch

@zelch Thanks, good news! :-) The best would be a pull request, without the logging parts. But I can manage anyway.

phocean avatar Sep 05 '17 06:09 phocean

@zelch No I was saying that since it was already working for me again, I wouldn't be able to test the patch.

Unshackle8078 avatar Sep 05 '17 13:09 Unshackle8078

@zelch the first patch didn't resolve it for me. It seems to reliably break when the system sleeps and is woken up again.

Where are the logs written?

edit: nvm; on my system journalctl /usr/bin/gnome-shell retrieves them.

I've reinstalled and added the latest Debug patch.

matjam avatar Sep 05 '17 15:09 matjam

@matjam Please let me know if the most recent patch does it for you. There was a race condition where if the icon changed between us adding it and a time based trigger running things would crash. This looks like it was causing the crashes I have been able to see. (But I don't do a lot of suspend/resume on my system.)

zelch avatar Sep 05 '17 15:09 zelch

@phocean It looks like you have recently merged in some other changes. I will get everything merged, tested and commited and try and get you a pull request in the not too distant future.

zelch avatar Sep 05 '17 16:09 zelch

Alright, my first attempt at merging my stuff with the current git head is... Horrifically unstable.

Enough so that I may need to bite the bullet and make a VM for more development of this, because doing it on my work machine is becoming non-viable.

zelch avatar Sep 07 '17 10:09 zelch

@zelch Sorry for my long period of inactivity. If by any chance you are still using it, is it still an issue or can I close it?

I have to say that I have no issue at all in my environment.

phocean avatar Apr 28 '20 08:04 phocean