Todo 1 - Icons alongside the label

[Batwam]

Here you go, first proof of concept including an app icon next to the text. I'd a rough test and spotify is loaded statically but it can also load the firefox and google-chrome icons. This works on startup only for now, I haven't worked out how this can be refreshed yet.

There will be more work needed to:

• make this an option (that's the easy part)
• apply the padding differently depending on whether there is an icon it now (or simply skip the image and keep icon/label)
• filter the app name from the address. There may be some work needed to extract the source name from the adress (browsers include an instance number) and interpretation work needed. For instance, I suspect that google-chrome and chromium both report as chromium in dbus.
• check that it works with various installs (normal install, flatpak, snaps...) and various distros
• recognise the original project which inspired the code (sp-tray)
• formatting clean up. there might be some rogue white spaces here and there from the original code, sorry... I've also included some temporary comments to discuss what I tested.

[Moon-0xff]

Wow you are fast. But there's a lot of questions and problems regarding that code

We need to understand what we are adding to the repo if we want to maintain this extension independently. Also, as you can read in the link, we should thank @koolskateguy89 for the code i suppose.

[Moon-0xff]

Sorry for reverting your commits. Added icon.js as the place where to dump our changes. That way our commits are easier to follow. I didn't added back all the koolskateguy stuff, only the bare minimum.

I have been trying to get the icons the same way AppMenuButton does. AppMenuButton is the label for the focused window. With default everything it's at the left of our extension. Inspecting the code it looks like an extension, but it's async all the way down and really hard to follow.

If we can implement this as GNOME does it, then we shouldn't worry too much about missing icons as GNOME seems to handle icons just fine (right?).

[Batwam]

So we can load up an icon when the extension loads which is nice and it looks like the snap special case wasn't required after all.

Do you have any idea how to change to another icon after the initial allocation? I tried a few different ways and we can obviously change the label the label but I wasn't able to get it to work for the icon...

Once we have that worked out, we can look to find a good location for this icon update which doesn't require loading the icon on every refresh... preferably only when the source is being changed which shouldn't require to much change in the code.

[Moon-0xff]

We can change icons by first removing it from box, redefining it, then adding it back. Try this:

                this.box.remove_child(this.icon);
                this.icon = new IconHandler().getIcon("rhythmbox");
                this.box.add_child(this.icon);

The source changes when this.player is redefined, this only happens in _pickPlayers and _cyclePlayers. With the auto-switch option disabled _pickPlayers only redefines this.player if the current player no longer exists. With auto-switch enabled, _pickPlayers redefines it every cycle.

[Moon-0xff]

Today wasn't too hard to follow AppMenuButton. Take a look at the last commit, i managed to copy AppMenuButton behaviour.

[Moon-0xff]

OK, I'm doing some real progress now. If all goes well I'm going to push it tomorrow.

[Batwam]

Yep, that's looking good, although focus_app if of less interest to us so we'd need to find a way to get the icon for the app associated to a given mpris source address. Regarding multiple browsers having the same "chromium" source name, perhaps there is a way to get a list of active apps and pick the most likely (or actual app name from link to mpris source dbus info but I haven't seem it in the dbus properties yet).

One thing I was thinking also when implemented the option in Settings is rather than have the option as "Show source icon (Experimental)" true/false, perhaps we could consider none/left/right which would allow not only to turn on/off but also decide left/right? I tend to prefer left btw so as long as I can get it on the left I'm happy, it's just that I see that it's on the right lately which feels a bit odd to me. If you want to have it on the left, then true/false switch is fine as it would be unlikely people would have both chromium and google-chrome open at the same time.

Edit: actually, may be can make use of this dbus properties of the source? I'm just not sure if every source reliably provides this info.

[Moon-0xff]

We can get the DesktopEntry directly from DBus? That's a real game changer.

[Moon-0xff]

perhaps there is a way to get a list of active apps and pick the most likely

Yes that's how i was implementing it.

We can get the list of active apps using this snippet:

                let appSystem = Shell.AppSystem.get_default();
                let runningApps = appSystem.get_running();

this returns a list of Shell App Objects. We can get the desktop entry from this using this. How do we know what App belongs to this.player.address? I haven't found a form of knowing for sure but we can guess it fairly accurately (i think).

https://specifications.freedesktop.org/desktop-entry-spec/desktop-entry-spec-latest.html#file-naming https://dbus.freedesktop.org/doc/dbus-specification.html#message-protocol-names

[Moon-0xff]

But if we have the desktop entry along with the DBus then we have the icon too! And even if we don't, we can match the desktop entry with the one we get from the shell app objects and get the icon the way AppMenu does, without having to guess.

[Batwam]

Indeed. Let's hope that we can go straight from dbus info to icon but I have the feeling that not all sources will provide the info. Let's have a look after your next commit when we get a better feel for what the latest code looks like. We can extract the DesktopEntry info and check to make sure all apps provide it.

I'm thinking that the most reliable would be using the dbus DesktopEntry info to match first and the best guess based on source address cross checked against active apps as a fallback if we find sources which don't provide the DesktopEntry info. If nothing works, leave blank.

[Moon-0xff]

After trying various ways of getting the icon, i settled on this way. It's based around this method. It returns a ThemedIcon. And that's probably better.

This last commit barely works, it's complicated, and it looks like the extension is broken in other places too. But at least demonstrates the idea well enough.

[Batwam]

No worries. Yes, I can see that it loads the rhythmbox icon but not chrome or spotify (flatpak). It also no longer switch sources or remove the text when the source is gone.

Anyway, it's running and an icon is loaded so it's a good start! Thanks for including comments in the code, that should help work out what needs to be done.

[Moon-0xff]

I believe an implementation like https://github.com/Moon-0xff/gnome-mpris-label/pull/5/commits/7f134f030c329d37a0c69249ae0d51cd4c57b625 could work too, but if we fail to guess the icon name, instead of throwing an error, it loads an "error icon", that's not good. edit: maybe there's a way of checking if the name is valid

[Batwam]

Yeah, I saw the error icon in the original iteration but not so much lately. It's useful for debugging to be honest. Once the rest is implemented, we should do a final check and try to intercept it and return a blank instead but as long as we pick an icon from the running apps, I suspect that the risk of running into this will be minimal.

I'm working my way through the issues as the extension is pretty broken right now. I'll push small fixes mostly for error handling to avoid pushing large chunks of code for while you are potentially working on the same parts. Not planning to do any major refactoring.

[Moon-0xff]

No, I'm not working on the same part for this exact reason. So feel free to push your commits.

[Moon-0xff]

It looks like i need to scramble getIcon.

[Batwam]

No worries, I won't be touching the code for a while so go for it!

[Moon-0xff]

Ok. This will be destructive, but only for the time being

[Moon-0xff]

Don't bother with testing it. So getIcon is a three part function

1. Get the variables
2. Try to guess
3. Get the icon

Our initial guessing game looks misguided, maybe not. But i feel like this part:

       // If both returned matches, compare them
       else if ( (DBusAddressMatches != null) && (DBusDesktopEntryMatches != null || undefined) ){
               //how?
               bestMatch = DBusDesktopEntryMatches[0][0];
       }

should be a function

[Moon-0xff]

OK, so why all of this? Because the function i added uses the same variables, gets a match, then returns an icon.

But really all that it should do is return the most likely match. or a sorted list of likely matches. That's how DesktopAppInfo.search() works.

And guessingGame(renamed compareMatches) should compare both sorted lists and return the most likely match or a better sorted list.

[Moon-0xff]

We can compare both results or prefer one. Hopefully this makes some sense (and works)

[Batwam]

ok, couple of thoughts:

• i tried to fix up the fact that the rhythmbox "fallback" icon wasn't going away when a player was started. I think that this is fixed now.
• firefox (snap) and spotify (flatak) are ok. google-chrome (deb) isn't. it worked with the original implementatation though. I tried to do a manual swap of suspectAppName from chromium to google-chrome but Gio.DesktopAppInfo.search() isn't finding it... perhaps Gio.DesktopAppInfo.search() won't be able to find it? Is there a way to get the full list of Desktop apps to check what's in the list?
• what do we want to do if the user doesn't have rhythmbox instead. Right now it's returning the error icon.
• Could we also hide the icon when paused? I had this working earlier today but the move of function removeTextPausedIsActive to label.js has broken this somehow. I don't really want to build a duplicate function to implement the delay as it seems a bit wasteful.
• is Gio.DesktopAppInfo.search searching through all the apps installed? if so, is there a way to only get apps which are actually launched?

[Moon-0xff]

i tried to fix up the fact that the rhythmbox "fallback" icon wasn't going away when a player was started. I think that this is fixed now.

Now completely fixed (hopefully) https://github.com/Moon-0xff/gnome-mpris-label/pull/5/commits/cc724d971663f283577ae10024afacaab79d7085

Is there a way to get the full list of Desktop apps to check what's in the list?

Try with this

what do we want to do if the user doesn't have rhythmbox instead. Right now it's returning the error icon.

Now the fallback is an icon included with GNOME https://github.com/Moon-0xff/gnome-mpris-label/pull/5/commits/bb9c18bc34c6a5dff0bdbd91233719eab30295a7

Could we also hide the icon when paused? I had this working earlier today but the move of function removeTextPausedIsActive to label.js has broken this somehow.

Fixed it https://github.com/Moon-0xff/gnome-mpris-label/pull/5/commits/cc724d971663f283577ae10024afacaab79d7085

if so, is there a way to only get apps which are actually launched?

a convenient way no, but surely is possible

[Batwam]

I made an update to the mechanism as I was getting some issues with Gio.DesktopAppInfo.search returning multiple matches. it still worked because of the compareMatches fallback but I wanted to make sure both method return a valid result. Based on this, firefox (snap), rhythmbox (deb) and spotify (flatpak) all work. google-chrome (deb) is returning the firefox icon for some reason but that's a story for another day...

[Batwam]

ok, current version works pretty well, I just fixed up an issue where the icon wasn't swapped when changing the source manually.

I have the feeling that the fallback icon isn't getting loaded. Perhaps it should be shown when nothing is running but this would be somewhat inconsistent with the fact that both icons and label get hidded when the source is paused (if the option is enabled of course). I guess the alternative would be to put the icon back so there is always an icon shown (if the show-icon option is enabled) and have either the fallback icon if nothing is running, or the source icon without label if the source is pause (as you had it originally i believe, sorry...). Let me know what seems logical to you.

Also, doesn't it look like there is some excessing padding on the right with this icon? it looks much wider than on the left

[Moon-0xff]

Yes padding is behaving badly, sometimes is applied in the middle, sometimes is applied twice in the left or right. maybe box isn't a good choice.

The fallback icon should appear only if the application icon wasn't found. I interpret the remove-text option as hide-everything, and i believe it makes more sense that way, but that option was your contribution, so you tell me.

[Batwam]

yeah, the intent was to hide everything as if the source wasn't active which is also why I updated the code to make sure the icon was also hidden. I was just having second thoughts but I think that it works fine as it is now.

[Batwam]

ok, I think that it looks better now, just needed to define (set as zero) icon padding:

even with 0 padding, icons seem to have ~5pt padding when compared to text which can be completely flush against the side so I have included a basic mechanism to correct this and make both sides look consistent. I also use this._onPaddingChanged() in _init() to avoid writing the same code twice.

Please have a look and let me know what you think. I think that it's working pretty well now so I'm going to let things settle a bit now and see how it all works over the next few days so feel free to review/update/correct as needed. I might also try on the Gnome 3.36 laptop as there is a chance this may not be compatible.

[Batwam]

ok, here is what I'm getting having tried all the mpris sources I could find on my PC:

Basically the mpris address is the most reliable source of info and works every time (subject to the google-chrome hack...). Unfortunately, not all sources provide DesktopEntry. Those which do provide something don't necessarily provide us with the full address so we end up with the same level of info as what we get from the address. With Rhythmbox and Totem where we have a portion of the result, the search doesn't work either so we'd need to do further processing on the DesktopEntry to strip out the org.gnome at the front just to help getting the same result we already have with the address.

With chrome where that DesktopEntry info would have been useful to work out which browser is actually used, the app doesn't even provide DesktopEntry so it's useless again... Have you found a case where DesktopEntry was providing a more reliable result than the mpris address info? if not, my suggestion would be to keep things simple, clean up the code and drop the DesktopEntry approach altogether since the info is unreliable and, at best, gives us the same result as the mpris address method.

[Moon-0xff]

drop the DesktopEntry approach altogether since the info is unreliable and, at best, gives us the same result as the mpris address method

Yes, i agree. Please test and review this latest commit. I left two log statements intentionally, you can view them using $ journalctl --follow | grep mpris-label

[Batwam]

ok, this works on Gnome3.36 too which is great. The only slight issue seems to be the padding which is different as the icon is closer to the text:

[Batwam]

not a big fan of version specific hacks but it looks better now with gnome 3.* specific padding (this is with left and right padding set to 0):

[Moon-0xff]

Honestly i don't like either of the padding fixes. But i can't really complain as they solve the issues. I believe the current approach to building the layout has some fundamental problems. But that's work for another day.

[Batwam]

My impression is that Gtk is including some default padding ~5pts on icons so I think that it's ok for us to manually adjust if we don't want it for our particular application.

On the Gnome3 specific stuff, I blame inconsistencies between the GTK versions. It looks like gnome3.* doesn't have this extra padding which was forcing me to manually remove 5 pts of padding on the right. It seems as if gtk4 includes at least 5 pt of padding around icons when the previous gtk didn't.

[Moon-0xff]

There's probably a way of having the same layout (in both 4x and 3.3x) with less problems.

I'm looking at Clutter.BoxLayout as it has a method to set the spacing. I haven't tested anything but it looks promising.

[Moon-0xff]

Also, the main class inherits from PanelMenu.Button and that probably has it's own rules regarding layout. I just glanced a bit at the code and it looks like it has some rules regarding layout but probably they don't matter to us. It also looks like it handles a menu so maybe implementing to-do number 3 will not be hard.

[Batwam]

Yeah, I think that down the track, the next logical step would be to add some sort of menu to include things like selecting player, shortcut to Settings,... it all depends on whether you want to keep this extension minimalistic or look to keep adding functionalities.

For the time being, my view is that the current implementation does work for the intended purpose of including icons but I'll happily test if you want to rewrite some of the code.

[Moon-0xff]

Yes, I'm pretty sure a rewrite will be necessary soon. Regarding the scope of the extension I'm more inclined to do a featureful extension rather than a minimalistic one.

[Moon-0xff]

Can you tell me which application icons are still failing to load on your end?

[Batwam]

Every app I tried (basically the 4 listed in the previous screenshot) works fines. My only concern would be with other browsers (gnome-web, brave, opera,...) as I'm not sure how they identify. Have you tried any others?

[Moon-0xff]

Yes, VLC(snap) and chromium(snap). the VLC icon loads just fine but the chromium icon fails to load.

[Moon-0xff]

Maybe we are done now with the getIcon function. I don't know if it's a good idea to add more "chromium workarounds". As the one we have will return the wrong icon if the user has both chromium and google-chrome installed.

[Batwam]

yeah, that's why I was wondering if there was a way to cross check Vs running apps, either using gtk built-in function direclty somehow or against something equivalent to ps -u $USER | grep $suspectAppName in bash. I can see that both Brave and Opera are also based on chromium so the current workaround is probably not good enough if they both report chromium as an address...

[Moon-0xff]

Shell.AppSystem.get_default().get_running() returns a list of running apps. Then maybe you will want to get the ids of each one and compare them with the guess. I have not tested this, but seems reasonable enough. edit: assuming the ids are desktop ids edit2: also, this response

[Moon-0xff]

Have you experienced any slow downs, freezes or crashes with the extension enabled?

[Batwam]

Have you experienced any slow downs, freezes or crashes with the extension enabled? no, I haven't

[Moon-0xff]

Woah. I installed the app and just by opening it it slow downs the system quite a bit. And yes, the lag goes away by turning off the extension.

[Batwam]

Ok, we need to find a way to make this asynchronous and/or be more careful with the number of dbus calls we make. Does it improve much if you ping every 1000ms instead of 300ms default?

I was actually just looking at something similar on the icon as we are currently resetting the icon on every refresh so I was trying to find a way to keep a record of the previous address/icon and only apply setIcon() if they changes.

There is another issue currently where if I launch a second source on top of the first with autoSwitch, the label will change but not the icon...

[Batwam]

Woah. I installed the app and just by opening it it slow downs the system quite a bit. And yes, the lag goes away by turning off the extension.

Do you want to open this as a new issue so this can be investigated separately? I see that some apps seem to struggle with Mpris: https://github.com/solus-project/budgie-desktop/issues/794