Add source icon customization

[Cry4775]

So, I wasn't happy with the source icon that was colored green and didn't fit the theme of my desktop and I also wasn't happy of the absence of padding (there were the lines of code that added it but they weren't updated for gnome >40.

So I went ahead and added those customizations. There are 3 new settings:

• Left padding (0-50)
• Right padding (0-50)
• Use symbolic source icon

Left padding adds padding if the source icon is on the right, right padding does the opposite. Use symbolic source icon, changes the source icon to a monochromatic icon that matches the theme's color.

Padding works with the album art setting too.

Some screenshots: Original icon with padding:

Symbolic icon:

Art icon padding:

Some higher px padding just to test:

[Moon-0xff]

Looks very nice! I will review these changes soon enough.

While this is not required, I would appreciate if you split the PR in two, for a clearer commit log.

[Cry4775]

I'm afraid I can't do it because I unfortunately made only one commit for everything :( If there's a simple way that I can't think of let me know!

EDIT: Anyway, all the modifications fall into the "category" of source icon customization, so imo one PR is enough, but if you prefer 2 PR (I guess one for the symbolic icon and one for padding (?)) tell me if there's a way of doing it (besides rewriting the code and doing the PR one by one) because I can't think of any other ways (it's my first ever contribution)

[Moon-0xff]

None I can think of. As mentioned, this is not a problem. Don't worry.

[Moon-0xff]

Heads up, I will push to your branch.

[Moon-0xff]

Here you go, please review the comments and push a fix. If you have the time! Otherwise I will probably do it tomorrow.

I recognize you aren't familiar collaborating with git? (I apologize if isn't the case!) I will drop a few tips, I will probably add them to HACKING.md later:

• Create a different branch with your changes, preferably with a unique name than the other working branches, this greatly simplifies the git workflow (of course you don't need to make a new branch for this PR!)
• Review your changes with git diff when creating commits and when requesting a merge with git diff upstream/main
• If you don't have the upstream remote, add it with: git remote add upstream git@github.com:Moon-0xff/gnome-mpris-label.git
• Connect with ssh, or replace the remote with https
• More commits better, commits should ideally be explained with less than 50 characters, smaller commits help to relay the intent of your changes more easily
• Consult the git man pages, they are truly remarkable! $ man git

[Cry4775]

I think it should be ok now, let me know!

[Moon-0xff]

Changes look well, although it looks silly that we have 7 settings for _setIcon now!

On that note it looks possible to merge ICON_LEFT_PADDING with ICON_RIGHT_PADDING, and use USE_ICON to know which one to apply.

[Cry4775]

On that note it looks possible to merge ICON_LEFT_PADDING with ICON_RIGHT_PADDING, and use USE_ICON to know which one to apply.

I guess you mean ICON_PLACE instead of USE_ICON, if that's the case then I'll go and merge the padding into one.

Changes look well, although it looks silly that we have 7 settings for _setIcon now!

I guess 6 is the most we can do, since we need them all.

I'll go and change the padding thing now, let me know if there is something more

[Cry4775]

How's it now?

[Moon-0xff]

I will test and inspect more closely this PR tomorrow. I don't think it requires more changes, but I wonder what @Batwam thinks.

[Moon-0xff]

Not necessary, default refresh rate is 300 milliseconds, maximum refresh rate is 3 seconds.

[Cry4775]

Not necessary, default refresh rate is 300 milliseconds, maximum refresh rate is 3 seconds.

Alright, totally missed that

[Batwam]

I like the option for symbolic icon, this looks cool 👍

I haven't tested the padding change yet (only read the code) but my impression is that we are duplicating a lot of the code when setting the padding at player.js level. Since it's the same padding and logic for icons and album covers, could we look to include this directly within extension.js when adding the icon/cover?

I added a comment as a placeholder to explain where I would expect it to go. Based on that, the code change should be very minimal and we already have the if left/right there.

Regarding unbinding, shouldn't this stay if we go refreshless as part of the other PR?

[Batwam]

see proposed diff here: https://github.com/Batwam/Dropbox/blob/main/Cry4775_batwam.diff

I have combined the padding code for icon/album and removed the shell version check. Essentially, player.js should remain pretty much untouched as the padding logic is the same for icon & album. This should result in a reduction of the number of line changes overall.

[Cry4775]

I haven't tested the padding change yet (only read the code) but my impression is that we are duplicating a lot of the code when setting the padding at player.js level. Since it's the same padding and logic for icons and album covers, could we look to include this directly within extension.js when adding the icon/cover?

I've had the same thought yesterday but I hadn't thought of a good way of doing it. I've seen the diff and it looks pretty good, the only things I'm not sure about are:

• the SYMBOLIC_ICON in _setIcon() as in your commit or in getIcon(SYMBOLIC_ICON) because to me it belongs there as it's strictly related to how the icon is drawn, instead of a secondary styling thing like the padding.
• the shell version check for padding: it was there before I even started modifying the code but it wasn't working because it was checking for gnome version "3." and I have 42. I have no idea why there's a shell version check and if it is really relevant to us, but I'd delete it only if we are sure to do so. EDIT: I've read the issue where it's stated that you're going to target gnome version >40 so I guess it's ok.

Other than that, that's pretty good as I said.

[Moon-0xff]

Regarding unbinding, shouldn't this stay if we go refreshless as part of the other PR?

When we go refreshless, for now we still to worry about that. I don't expect that #60 will be merged before the next update.

EDIT: I've read the issue where it's stated that you're going to target gnome version >40 so I guess it's ok.

In the next month or two, for now we still target 3.36 and 3.38, so the workarounds need to stay.

[Cry4775]

I guess @Batwam can push his changes through then

[Batwam]

I'm fine either way with getIcon(SYMBOLIC_ICON) or getIcon(). The reason I removed it was to minimise the changes Vs main but since we are calling getIcon on every cycle anyway and using it to resize, then it's ok to apply the symbolic change there I guess.

if it was me, I would have getIcon() loading the app icon as this.player.icon only once when the player is loaded up rather than on every refresh. Then any set_style change (resize, padding, symbolic style) would be handled separately as it doesn't alter the underlying this.player.icon.

[Moon-0xff]

@Cry4775 Can't, but you can create a commit on behalf of someone else with the --author option. Matter of fact I already did.

[Moon-0xff]

@Batwam I initially didn't like the diff, but looking it closely it actually makes more sense than pushing the options to players.js

[Cry4775]

@Cry4775 Can't, but you can create a commit on behalf of someone else with the --author option. Matter of fact I already did.

Oh alright thanks for explaining! We're good then

[Moon-0xff]

current HEAD looks good, I will test it for a while and then probably merge it. Anything else to add?

[Batwam]

Could we adjust window.default_height = 775; to avoid the scroll bar on the right? (or make the Icon section its own page)

[Moon-0xff]

Could we adjust window.default_height = 775; to avoid the scroll bar on the right? (or make the Icon section its own page)

To what value? I'm using 3.38

[Batwam]

775 works for me. It's currently 700. 750 is too small too.

[Moon-0xff]

Alright, as far as I tested the branch behaves well, I will merge this now!

Thanks @Cry4775 for the PR!
And @Batwam for the diff.