Use the track album art as icon if available

[abisammy]

This adds the ability to show the album art in the extension, with customisation settings:

[abisammy]

Could also be added to main menu of extension?

[abisammy]

For a branch with all my changes please see: https://github.com/abisammy/gnome-mpris-label/tree/abisammy-changes

[Batwam]

Hi,

This was discussed a while ago and it was decided not to implement it but I have to say that it looks nicer than I would have thought, even with a small panel height.

I can't comment on whether this will be merged or not but I have a couple of comments/suggestions:

• is this intended to replace the icon or be shown in addition to it? The screenshow above shows both the artwork and Shortwave icon but when I try, the artwork replaces the icon (which isn't a bad thing)
• I wonder if there is a "smart" way to set the icon size autimatically based on panel size. With the current implementation, the icon height is limited by the panel size anyway so when that limit is reached it end up increasing the padding. (edit: it can be obtained using Main.panel.height)
• might need to check error handling with sources which don't generate artwork like browsers:

For info, with these new options, on my system, the Settings window ends up getting a scroll bar on the right as the options no longer fit in the default window size.

[abisammy]

Hi,

1. This is intended to just be the label, the previous picture was not a good example as it included an emoji
2. I have just uploaded a commit to do it in a more "smart" way, by using the height, and a setting for the percentage of the height
3. I have also added an error checking fix, making sure the metadata exits

[abisammy]

Another thing just noticed, when using dash to panel, it doesnt always seem to work with the panel height, therefore icon size is sometimes too small, could be possible to add both percentage (currently implemented) as well as manual override

[Batwam]

Let's see what @Moon-0xff thinks.

Presumably you have to set the default size to 70% as the max icon size is smaller than the panel size due to icon padding? Ideally, you'll want to set the default so it maximises the icon size base on gnome's defaults for panel/padding. People with dash-to-panel or custom padding can adjust from there.

Like you suggested above, this type of implementation would allow to add additional field if needed for info like playbackStatus and Identity relatively easily for people who want to customise the label further.

[Batwam]

Another thing just noticed, when using dash to panel, it doesnt always seem to work with the panel height, therefore icon size is sometimes too small, could be possible to add both percentage (currently implemented) as well as manual override

Probably best to understand the cause of the issue before introducing a manual hack. I noticed this comment from JustPerfection when researching about panel height. Perhaps something to check? https://www.reddit.com/r/gnome/comments/t2hjyg/how_to_get_top_bar_height/hyn24xt

Alternatively, how does it look if you don't specify the size and let gnome shell work it out? Do you feel that there is actually a need to specify/reduce the size manually?

[abisammy]

Another thing just noticed, when using dash to panel, it doesnt always seem to work with the panel height, therefore icon size is sometimes too small, could be possible to add both percentage (currently implemented) as well as manual override

Probably best to understand the cause of the issue before introducing a manual hack. I noticed this comment from JustPerfection when researching about panel height. Perhaps something to check? https://www.reddit.com/r/gnome/comments/t2hjyg/how_to_get_top_bar_height/hyn24xt

Alternatively, how does it look if you don't specify the size and let gnome shell work it out? Do you feel that there is actually a need to specify/reduce the size manually?

Upon further investigation, its dash to panel thats causing the issues. Therefore I think whats best is that set the default to 70 (which works with the gnome default), and add a slider or input for the overide, which can adjust the percentage.

[Batwam]

you could also use this.box.get_height() with default size 100% which seems to maximise the icon size?

[abisammy]

you could also use this.box.get_height() with default size 100% which seems to maximise the icon size?

For some reason not working with dash to panel?

[Batwam]

I had a look at dash-to-panel and, for some reason, Main.panel.height and this.box.get_height() remain unchanged when the panel "thickness" is changed. The dash-to-panel height is stored independently so I'd allow scaling 50-250% perhaps so the user can adjust manually, especially as the panel would most likely be larger than gnome's normaly top panel.

[abisammy]

Screenshot for previous commit:

Will format code later...

[Moon-0xff]

As @Batwam pointed out we already discussed this a while ago, we didn't implemented it at the time but we didn't discard it either.

I will quote myself to save a few keystrokes:

Sounds real neat specially with shortwave, but we get less than 32px in a 1920x1080 screen, album covers are far too detailed to translate well into such a small amount of pixels.

As a "general" option i don't think is of too much use, as a "per-application" option sounds good, but we will need to implement "per-application options" first.

I haven't tested this branch yet, but it wouldn't surprised me if even in a 1080p screen the results look good.

As my previous comment says, I think this addition makes more sense as a per-application option, however there's no need to add a whole scheme to handle "per-application options" to do it, a simple entry widget to prefs should do the trick.

[abisammy]

Sounds real neat specially with shortwave, but we get less than 32px in a 1920x1080 screen, album covers are far too detailed to translate well into such a small amount of pixels.

I agree with this, may not look good on 1080p monitors, should be looked into. However, some people may choose to have wider panels.

As my previous comment says, I think this addition makes more sense as a per-application option, however there's no need to add a whole scheme to handle "per-application options" to do it, a simple entry widget to prefs should do the trick.

I understand, would it not be better to have the ability to enable and disable it for all applications (like it is now anyway) though? Additionally, if there is no album cover available, it will default to the application icon like previously.

[Moon-0xff]

would it not be better to have the ability to enable and disable it for all applications (like it is now anyway) though?

Yes, of course, but also add the ability of enabling it just for some players.

if there is no album cover available, it will default to the application icon like previously

That's a good default. It also depends from the 'show-icon' option, that might not be obvious.
I suggest to change the label of the option to: Use album art as icon when available:

[Moon-0xff]

One last thing I have in mind is performance.

Creating and displaying an image is hard work. _refresh is called synchronously and is tied to the shell's main loop, this means that a slow function there slows down the entire shell.

If this PR slows down the shell too much, or even creates micro-freezes under certain circumstances we will need to decouple _setIcon from _refresh first.

[abisammy]

1. Cleaned up the settings:

2. Will add the ability to filter album art per application (basically copying from filters)

3. So for I haven't noticed a performance decrease, your mileage may vary

[abisammy]

Settings now disabled when show source icon is off:

Or album art as icon is off:

[abisammy]

@Moon-0xff Added a whitelist for the apps to display the album art in:

Also refacted, removing repeated code!

[Batwam]

It might be best to make this whitelist optional (false by default) by having a toggle on the right (using addSwitch) so all apps display the album by default?

I have the feeling that people choosing to display the covers would expect it to be shown whenever available so it might be best if they don't have to manually list the apps they want to show it as default.

[Batwam]

if we are concerned about updating the icon on every refresh being suboptimal, we could include a check within _refresh() and only update if the player or label have changed:

let prevLabel = this.label.get_text();

[...]

if (this.player != prevPlayer || this.label.get_text() != prevLabel)
    this._setIcon();

you would also need to revert 52c5907 which removed the album-size binding

[Batwam]

the changes made to pref.js in 35decaf are messing up the formatting I'm afraid. As per the comment in the code you deleted, //using addSwitch messes up the layout for the other widgets in the page. My understanding is that this is due to the fact that there is no column 1 on this particular page which allows to have wider Entry fields which addSwitch will include the switch in column 1 by default.

If you want to avoid repeated code, you might want to implement an alternative addSwitch based on the original code which puts Entry and Switch in column 0 and could be used for both the existing "ignore all sources ..." and the toggle mentioned above for Album Art whitelist.

[abisammy]

Fixed!!

[abisammy]

It might be best to make this whitelist optional (false by default) by having a toggle on the right (using addSwitch) so all apps display the album by default?

I have the feeling that people choosing to display the covers would expect it to be shown whenever available so it might be best if they don't have to manually list the apps they want to show it as default.

Currently if the filter is empty, all apps are shown by default

[abisammy]

if we are concerned about updating the icon on every refresh being suboptimal, we could include a check within _refresh() and only update if the player or label have changed:

I believe we can use the 'g-properties-changed' event, would be a clearner solution.

[Batwam]

yeah, see what you can achieve.

Honestly, I always felt that we should be able to make this entire extension refreshless considering that we are currently updating the label every 300ms for something when we would probably only really need updating every few minutes (when a song changes) but we never quite got there... might need to be a separate PR though as it might require some extensive testing to make sure we don't break anything in the process...

[Batwam]

actually, we already have this.proxy.connect('g-properties-changed', this.update.bind(this)); as part of player.js here.

Rather than updating the icon at each cycle, wouldn't it makes more sense to set the album cover as this.player.album on 'g-properties-changed' event as part of this.update() here?

[Moon-0xff]

Yes, using 'g-properties-changed' in more places other than the Player class would be optimal. I've tried to do this but I wasn't able to pass the signal to other objects correctly.

In any case the heavy lifting is most likely decoupled from the main thread, testing is needed.

There's also the question if we need to tweak some parameters to not waste resources rendering the image, probably not but again, testing is needed.

[abisammy]

@Batwam @Moon-0xff I have made a system that is partially event based, still needs to be tested: https://github.com/Moon-0xff/gnome-mpris-label/pull/60

It uses events when a player is active, however relies on the old refreshing method until a player is open, maybe you know how to make this independent?

[abisammy]

What other features can be added to this system, or is it ready to be tested?

[Moon-0xff]

I don't have any more suggestions for this, I wonder what @Batwam thinks.

[Batwam]

feature-wise, I think that the code does what it needs to do. I would however have the following suggestions to make the code a little bit more efficient:

1. I don't have the number to quantify the impact but it feels like should try to avoid redrawing the icon on every refresh if we can avoid it. I understand that we will look to make changes to the way the label updates in a separate PR but in the meantime, we should try to implement a "smarter" way to update the icon (and bind album-size again if necessary as it was removed in 52c5907.
2. I think that the album icon could be set in player.js and stored under this.player.album so we don't have to regenerate it on each refresh. then we can set this.icon to this.player.icon or this.player.album depending on the setting
3. I believe that it would make more sense for the prefs.js list to be a blacklist rather than a whitelist for the following reasons:
   • it's more likely that people would want to show the album for most apps rather than only a few
   • default value blank meaning all apps are allowed isn't the normal behaviour for a white list. However, a blank blacklist means all apps are allowed which is the intended behaviour.

feel free to comment if you don't agree, these are only suggestions.

[abisammy]

feature-wise, I think that the code does what it needs to do. I would however have the following suggestions to make the code a little bit more efficient:

1. I don't have the number to quantify the impact but it feels like should try to avoid redrawing the icon on every refresh if we can avoid it. I understand that we will look to make changes to the way the label updates in a separate PR but in the meantime, we should try to implement a "smarter" way to update the icon (and bind album-size again if necessary as it was removed in 52c5907.
2. I think that the album icon could be set in player.js and stored under this.player.album so we don't have to regenerate it on each refresh. then we can set this.icon to this.player.icon or this.player.album depending on the setting

3. I believe that it would make more sense for the prefs.js list to be a blacklist rather than a whitelist for the following reasons:

   • it's more likely that people would want to show the album for most apps rather than only a few
   • default value blank meaning all apps are allowed isn't the normal behaviour for a white list. However, a blank blacklist means all apps are allowed which is the intended behaviour

feel free to comment if you don't agree, these are only suggestions.

Done! The icon is now set in the player update method. Therefore it will only change when the 'g-properties-changed' event is fired. Additionally the album art setting is now a blacklist!

[Batwam]

It could be a resolution thing but I still have an issue with the the 3 additional options making the Label page larger than the standard window size.

Would you mind splitting Behaviour and Appearance into 2 separate pages? The Label Page is getting quite busy so that would help keeping the pages more balanced and also avoid having Reset buttons changing too many settings at the same time.

Edit: I'm also getting the following showing up in journatcl which wasn't there before:

[Batwam]

Not direclty related to this PR but I'm getting this error in journactl which goes away by updating the lines to pref.js

        visibleFieldsBox.append(firstFieldComboBox);
        visibleFieldsBox.append(secondFieldComboBox);
        visibleFieldsBox.append(lastFieldComboBox);

pack_start had 4 arguments [pack_start](https://lazka.github.io/pgi-docs/Gtk-3.0/classes/Box.html#Gtk.Box.pack_start) (child, expand, fill, padding) but Gtk.Box.append only had one.

[abisammy]

Done!

Not too sure why they are showing in journalctl, I removed all the logs from debugging

[abisammy]

Not direclty related to this PR but I'm getting this error in journactl which goes away by updating the lines to pref.js

      visibleFieldsBox.append(firstFieldComboBox);
      visibleFieldsBox.append(secondFieldComboBox);
      visibleFieldsBox.append(lastFieldComboBox);

pack_start had 4 arguments [pack_start](https://lazka.github.io/pgi-docs/Gtk-3.0/classes/Box.html#Gtk.Box.pack_start) (child, expand, fill, padding) but Gtk.Box.append only had one.

That error doesn't necessarily have to be fixed if the new system is implemented. Btw, do you prefer the new or old notations?

[Batwam]

That error doesn't necessarily have to be fixed if the new system is implemented. Btw, do you prefer the new or old notations?

good point. I have commented on the other thread regarding the other PR.

[Moon-0xff]

Sourced prefs.js from main to merge cleanly.

I didn't take a good look to the changes in prefs but I wasn't satisfied with the look it had.

Things left to do is to add back the widgets for the other two album options and make a subcategory in panel for 'Icon'.
This to my opinion gives a cleaner look to the settings. I wasn't able to do it on time but it's the same layout as this screenshot: As the first subcategory of Panel

[Moon-0xff]

Minus the scale widget, for consistency I think a spin button would be better, at least for now.

[Batwam]

The reason I suggested splitting the contents of the Label page is because with the additional options, it was making the Label page taller than the default size, showing a scroll bar and hiding some of the options. Also, with so many options in a single page, the Reset button ends up changing too many settings which can be frustrating when only trying to reset some of the settings.

With shorter content, the page height will reduce to match the longest page, leaving a gap at the bottom of the window which looks a bit odd (to me). To fix this, you can update the following line in pref.js with vexpand: true so the widget can fill the window: let prefsWidget = new Gtk.Notebook({visible: true,vexpand: true});

before:

after:

[abisammy]

Minus the scale widget, for consistency I think a spin button would be better, at least for now.

I belive the scale is easier for the user to view. I also made the scale use the current modular system of adding inputs, therefore it can be applied to many other values for consistency.

[Moon-0xff]

I belive the scale is easier for the user to view.

I could agree with that but it looks out of place.

[Moon-0xff]

I moved the code of 8f70d19afb8b770afb55abc5253a0060d7858222 to a separate function because it was unclear where the album icon was resized. This denies the performance improvement, but it's negligible against the performance penalty of rebuilding the icon every cycle, which is a problem outside the scope of this PR.

[Batwam]

could we please have a version update and/or sync with main? This PR now has a lower rev than the version on EGO and gets updated in the background.

[Moon-0xff]

I think this branch it's ready to merge. I'm aware the prefs window is a bit longer than before so the GNOME 40+ window workaround might need some adjustment.

[Batwam]

Preferences window looks fine on my end now that the option has been moved to the Panel page.

For info, with the default panel size, I get max icon height (before it starts adding horizontal padding) at 65%. If I use the default 60%, the icon is smaller than its max possible size by 1-2px.Is the icon at it maximum height already when you use 60%?

also, it might be worth specifying in the tooltip that by %, we mean "% of the panel height" perhaps?

[Moon-0xff]

Max icon height is 65% or very close to it here too. I deliberately choose an smaller value so the icon aligns better with the rest of the panel. I would choose 56% but the 4% extra makes a big difference on a 1080p screen.

And I don't think there's need to explain it when the value can be seen and adjusted easily.

[Moon-0xff]

I tested the changes with the code we used in #12

On my end the values jump between 2ms and 3ms, sometimes 1ms.
With the option turned off the values jump between 1ms and 2ms, sometimes 3ms.

Last time I checked the refresh time it jumped around 0-2ms.

So it looks like there's no reason to worry about shell slow downs, at least on normal circumstances.

Here's the diff for testing:

diff --git a/extension.js b/extension.js
index 410016e..87c8cfb 100644
--- a/extension.js
+++ b/extension.js
@@ -356,6 +356,9 @@ class MprisLabel extends PanelMenu.Button {
    }

    _refresh() {
+       log("mpris-label ------------------------------------------------------------");
+       let start_time = new Date().getTime();
+
        const REFRESH_RATE = this.settings.get_int('refresh-rate');

        let prevPlayer = this.player;
@@ -377,6 +380,8 @@ class MprisLabel extends PanelMenu.Button {
        this._setIcon();
        this._removeTimeout();

+       let end_time = new Date().getTime(); let step = end_time - start_time; log("mpris-label - total cycle time: "+step+"ms");
+
        this._timeout = Mainloop.timeout_add(REFRESH_RATE, this._refresh.bind(this));
        return true;
    }

[Batwam]

When testing the Shortwave slowdowns, I was getting 5-6ms cycles on average, going up to 250ms for Shortwave so this is definitely within the margin of error.