Merge lp:~lanoxx/indicator-applet/fix-duplicate-icons into lp:indicator-applet

[Sebastian Geiger]

Destroy the menuitem instead of hiding it when an entry is removed.

This fixes a bug that causes duplicate icons in the indicator-applet. To reproduce the bug, perform the following steps:

1. Start an application which registers a menu item in the indicator applet.
2. Close that application again. Internally the item will be hidden.
3. Move the panel around on the screen to a different edge of the screen.

The result is, that the item will be visible again.

This change fixes the problem by destroying the widget and removing it from the hash table.

[Alberts Muktupāvels (muktupavels)]

review: Approve

[Alberts Muktupāvels (muktupavels)]

Before merging we should check if this does not re-introduce problem - https://bazaar.launchpad.net/~indicator-applet-developers/indicator-applet/trunk.13.10/revision/403

[Dmitry Shachnev (mitya57)]

This reminds me that I had a very similar fix in https://bazaar.launchpad.net/~mitya57/indicator-applet/remove-old-menuitems/revision/436 that was fixing memory leak described in bug 1635577 (see comments #26 and #27), but I never created a merge proposal for it because it was fixed on owncloud-client side. (applet-main.c was renamed to indicator-applet.c since then, so it is the same code.)

If I understand correctly, gtk_widget_destroy also removes the widget from any containers, so gtk_container_remove call (like in my commit) is not needed, so this fix is better than mine.

Now, regarding the problem mentioned by Alberts — that commit is linked to bug 1045372, which describes how to trigger the issue. Sebastian, will you be able to test it?

review: Approve

[Sebastian Geiger (lanoxx)]

Hi Dmitry,

I looked at bug 1045372 and used the description from comment #5 to test
with my fix and I cannot reproduce the problem described there. I have
build a Deb package based on the package currently in Focal 20.04 which
includes my fix and I am currently running it. If I notice anything
strange, then I will let you know.

Thanks
Sebastian

On 6/9/20 7:48 pm, Dmitry Shachnev wrote:
> Review: Approve
>
> This reminds me that I had a very similar fix in https://bazaar.launchpad.net/~mitya57/indicator-applet/remove-old-menuitems/revision/436 that was fixing memory leak described in bug 1635577 (see comments #26 and #27), but I never created a merge proposal for it because it was fixed on owncloud-client side. (applet-main.c was renamed to indicator-applet.c since then, so it is the same code.)
>
> If I understand correctly, gtk_widget_destroy also removes the widget from any containers, so gtk_container_remove call (like in my commit) is not needed, so this fix is better than mine.
>
> Now, regarding the problem mentioned by Alberts — that commit is linked to bug 1045372, which describes how to trigger the issue. Sebastian, will you be able to test it?

[Dmitry Shachnev (mitya57)]

Hi Sebastian!

> If I notice anything strange, then I will let you know.

So did you notice anything strange? If no and you don't want to test for some more time, then I think we can land it.

[Sebastian Geiger (lanoxx)]

Hi Dmitry,

I did not noticed any regression so far. I have switched between monitors a few times this week which normally reproduced the problem and I confirm that my patch resolved that issue.

I also do not see any deactivated menus in Libre Office if I open any dialogs.

I think we can merge this.

[Sebastian Geiger (lanoxx)]

Hi Dmitry,

can we make an SRU for this for Ubuntu 20.04? I upgraded one of my VMs
this week and ran into the same problem again. On my main desktop I
still my custom build package installed and have not yet noticed any
problems. It would be great to have this on 20.04 in a couple of weeks.

Also I think it would be nice to move the code to a Gitlab based
platform (Salsa or GNOME) including a migration from Bazar to Git.

Thanks
Sebastian

On 13/9/20 7:36 pm, Dmitry Shachnev wrote:
> Hi Sebastian!
>
>> If I notice anything strange, then I will let you know.
> So did you notice anything strange? If no and you don't want to test for some more time, then I think we can land it.

[Dmitry Shachnev (mitya57)]

Hi Sebastian!

> can we make an SRU for this for Ubuntu 20.04?

Can you please file a bug and fill the description according to SRU template? (https://wiki.ubuntu.com/StableReleaseUpdates#SRU_Bug_Template) Then I will do an upload for Focal.

> Also I think it would be nice to move the code to a Gitlab based platform (Salsa or GNOME) including a migration from Bazar to Git.

This project is Ubuntu-specific, it is related to neither Debian nor GNOME. So I think Launchpad is the best platform for it. However I can migrate it to Git.

[Dmitry Shachnev (mitya57)]

> However I can migrate it to Git.

Looks like I don't have access to do that, so I asked for this project to be transferred to ~ubuntu-gnome-flashback team.

[Sebastian Geiger (lanoxx)]

> Can you please file a bug and fill the description according to SRU template? (https://wiki.ubuntu.com/StableReleaseUpdates#SRU_Bug_Template) Then I will do an upload for Focal.
>
Sure, I can do that. Will ping you when its done.

>> Also I think it would be nice to move the code to a Gitlab based platform (Salsa or GNOME) including a migration from Bazar to Git.
>
> This project is Ubuntu-specific, it is related to neither Debian nor GNOME. So I think Launchpad is the best platform for it. However I can migrate it to Git.

I am not familiar enough with the code to understand how difficult it would be to make it available in other distributions, but it would be nice if we did not constrain our selves to Ubuntu. If there is a chance this applet can be made compatible with Debian then I would vote to move it out of Launchpad. Also most flashback components are on the Gnome gitlab server.

I started to experiment on how to best migrate the code to git.

[Dmitry Shachnev (mitya57)]

> If there is a chance this applet can be made compatible with Debian then I would vote to move it out of Launchpad.

Unfortunately, Debian has switched from Ubuntu indicators to “Ayatana Indicators”: https://wiki.debian.org/Ayatana/IndicatorsTransition. So it will need to be patched to work on Debian.

Also Ubuntu indicators require a patch to GTK, so it will be hard to get them working on other distros: https://git.launchpad.net/ubuntu/+source/gtk+3.0/tree/debian/patches/ubuntu_gtk_custom_menu_items.patch.

> I started to experiment on how to best migrate the code to git.

I have done it locally with these two commands:

- bzr clone lp:indicator-applet indicator-applet-bzr
- git clone bzr::indicator-applet-bzr indicator-applet

=== modified file 'src/indicator-applet.c'
--- src/indicator-applet.c	2020-04-09 11:05:35 +0000
+++ src/indicator-applet.c	2020-09-05 22:24:55 +0000
@@ -457,7 +457,8 @@
                          NULL);
   }
 
-  gtk_widget_hide (menuitem);
+  g_hash_table_remove (menuitem_lookup, entry);
+  gtk_widget_destroy (menuitem);
 
   return;
 }