Add indicator for recent icon IDs in prefs dialog

[Finii]

Back story:

When Mattermost updated the icon design recently someone decided there that a black-on-transparent tray icon would be a good idea. Unfortunately the vanilla Ubuntu Gnome TopBar is black, so the icon is black on black 😒

Then came the new ability to exchange icons. 🎉 This is really great and works as expected, also with icon paths given (as '/home/blah/fasel.png' for ex). Although the documentation for the feature is like nonexisting.

But then came up the question: What is the ID I need to put into the new dialog? How am I supposed to find that out?
The Mattermost icon ID is by the way Mattermost1 😒

Mattermost icon 2nd from left with Transparent Shell Top Bar to make it visible

[why]
When someone wants to exchange an icon with a custom one the icon name
is needed. That can be nontrivial to find it out.

[how]
Just collect all icon names that we encountered since the extension has
been started and list these names below the icon replacement table in
the settings dialog.

This is how it looks on my machine:

Implementation details:

• Recent IDs are stored in the settings object (maybe not a good choice)
• Are purged on extension (re)start
• Sometimes they are purged for other reasons :-(
• Are added whenever a new icon ID is encountered (with no locking)

Maybe it can be solved in a better way, this was just a Friday morning project. But I guess this improves the usability of the new custom icon functionality for ordinary users quite a bit.

I will try to improve on the spurious purges of the list. Fixed with force push.

[3v1n0]

Ok, looks intresting.

I'll see if I can define another way to get such icons though, without bothering gsettings

[Finii]

Just wanted to link @andia89's #287, as this is kind of a follow up.

[Faetu]

Hi Finii

Good idea! I tested it on my computer, but it doesn't update the list by itself. Do I have to restart Gnome when a new tray appears?

best regards

[Finii]

Do I have to restart Gnome when a new tray appears?

Well, you need to restart Gnome after you updated the extension (like alt-F2 r on X) (or 🙄 on Wayland)
After that it should start collecting icon IDs and add them to the settings dialog. You need to close and open the dialog to update the info, I believe.

[aunetx]

Ok, looks intresting.

I'll see if I can define another way to get such icons though, without bothering gsettings

I don't think there is any other (trivial) way, as the preferences cannot easily interfere with the shell or any running extension, even itself -- or at least I did not find how it could. Another option would be writing to a temporary file, but then gsettings really is cleaner, I guess?

By the way, this is a great addition -- I had a very hard time finding ids for appindicators some weeks ago, it easily took me more than and hour... I guess I'm not very good at it :)

I will try to update this by allowing the preferences to update automatically when a new icon is added, without needed to close the preferences.

[aunetx]

Here is a patch, which:

• update the list when an indicator is added while preferences are open
• fixes a bug where the icons are not added any more once the extension is started (the reason why _addToRecentIcons is called in _showIfReady)

diff --git a/indicatorStatusIcon.js b/indicatorStatusIcon.js
index 9eee93a..a245f0f 100644
--- a/indicatorStatusIcon.js
+++ b/indicatorStatusIcon.js
@@ -183,13 +183,6 @@ class AppIndicatorsIndicatorStatusIcon extends BaseStatusIcon {
             }
         });
 
-        const settings = SettingsManager.getDefaultGSettings();
-        const iconIDs = settings.get_value('recent-icons').deep_unpack();
-        if (this._indicator.id && !iconIDs.includes(this._indicator.id)) {
-            iconIDs.push(this._indicator.id);
-            settings.set_value('recent-icons', new GLib.Variant('as', iconIDs));
-        }
-
         this._showIfReady();
     }
 
@@ -243,6 +236,16 @@ class AppIndicatorsIndicatorStatusIcon extends BaseStatusIcon {
         }
     }
 
+    _addToRecentIcons() {
+        const settings = SettingsManager.getDefaultGSettings();
+        const iconIDs = settings.get_value('recent-icons').deep_unpack();
+
+        if (this._indicator.id && !iconIDs.includes(this._indicator.id)) {
+            iconIDs.push(this._indicator.id);
+            settings.set_value('recent-icons', new GLib.Variant('as', iconIDs));
+        }
+    }
+
     _showIfReady() {
         if (!this.isReady())
             return;
@@ -250,6 +253,7 @@ class AppIndicatorsIndicatorStatusIcon extends BaseStatusIcon {
         this._updateLabel();
         this._updateStatus();
         this._updateMenu();
+        this._addToRecentIcons();
 
         super._showIfReady();
     }
diff --git a/prefs.js b/prefs.js
index 80a94c3..919de10 100644
--- a/prefs.js
+++ b/prefs.js
@@ -290,16 +290,27 @@ class AppIndicatorPreferences extends Gtk.Box {
         iconIDListStore.set_column_types([
             GObject.TYPE_STRING,
         ]);
-        const iconIDs = this._settings.get_value('recent-icons').deep_unpack();
-        iconIDs.forEach(v => {
-            iconIDListStore.set(iconIDListStore.append(), [0], [v]);
-        });
+        const iconIDListTrack = [];
         const iconIDTreeView = new Gtk.TreeView({
-            model: iconIDListStore,
+            model: iconIDListStore
         });
         const iconIDTreeViewColumn = new Gtk.TreeViewColumn({
             title: 'Recent Indicator IDs',
         });
+
+        const update_recent_icons = _ => {
+            const iconIDs = this._settings.get_value('recent-icons').deep_unpack();
+            iconIDs.forEach(v => {
+                if (!iconIDListTrack.includes(v)) {
+                    iconIDListStore.set(iconIDListStore.append(), [0], [v]);
+                    iconIDListTrack.push(v);
+                }
+            });
+        }
+
+        this._settings.connect('changed::recent-icons', update_recent_icons);
+        update_recent_icons();
+
         const standardCellRenderer = new Gtk.CellRendererText();
         iconIDTreeViewColumn.pack_start(standardCellRenderer, true);
         iconIDTreeViewColumn.add_attribute(standardCellRenderer, 'text', 0);

I can make this a proper PR if you want, however this might just add noise for nothing.

[Finii]

You can add to my PR if you like - collaborator thing sent. In this way there will not be multiple PRs doing the same thing.
(Of course you could also create your own PR if you believe that is better.)

And then one can keep the PR rebased on master and wait ;-) 3v1n0 is rather busy with more important repos I believe ;)

[aunetx]

Ok thanks, I will commit it to your repo then!

I don't have much time to rebase it right now, but I guess this PR will wait here anyway so there is no hurry :)

[aunetx]

Sorry, I somehow pushed to the wrong branch, this is ok now

[Finii]

Rebase on master; force push.

Works beautifully on my machine! Thank you 👍

[Finii]

Ah there is some eslint stuff, will fix that. Force push again imminent.

[Finii]

@aunetx Had to remove your _, because there is a const one at global scope 😒

[aunetx]

@Finii sorry if that caused some problem, tell me if you need anything else :)

[Finii]

@aunetx All good, thank you very much. :-) Let's hope @3v1n0 finds time to review and decide on this; in the meanwhile I will keep this rebased on master (when I notice new commits on master).

[3v1n0]

As said in inline comment, I'd prefer to have another way to handle this.

One method would be using a GAction for example...

[AmirHosseinKarimi]

Is there any hope for this pull request to merge?

[a-plastic-bag]

Considering the ongoing problems with the dreaded three dots, I think it'd be a good idea to take a look at adding this again.

[aunetx]

I tried to merge the master branch, it works for me on GNOME 46 now, however while trying to merge it I think I changed the branch of this PR... @Finii sorry for this, honestly I am quite bad with git, tell me if I need to change something!

[Finii]

@aunetx I guess I do not know what I should do. The two commits you added are ... what? ;-D

[Finii]

Ah probably instead of merging the master INTO this branch (d6ca83e) you should have done a rebase of this branch ON master. I usually prefer that because then only the needed-for-this-feature changes are in the PR and nothing already in master.

Otoh rebasing can be a lot of work. But the result of a merge is often incomprehensible.

[aunetx]

Ho sorry for this :/ at least I did not touch to your master branch so you can go back to it if you want, but to be honest git always scares me a little bit, exactly because of this kind of things!

Tell me if I can help if I messed up somewhere irremediably

[aunetx]

Hello, this seems not to work with last versions, has I done something wrong? This seems weird as it worked some weeks ago...