Moon-0xff / gnome-mpris-label

A music related GNOME extension.
GNU General Public License v3.0
51 stars 10 forks source link

update to icon padding mechanism #16

Closed Batwam closed 1 year ago

Batwam commented 1 year ago

Hi, minor issue but I noticed that the padding didn't quite work when the icon is on the left. I took the opportunity to rewrite the adjustment to keep things concise as it was basically coding the Max function.

Also, the icon location wasn't loaded properly when opening the Settings so I fixed that too.

Moon-0xff commented 1 year ago

Hello @Batwam, good work catching these problems but have you tried another approach for this? The icon padding hack works well but it's very fiddly, breaks easily and I'm not very comfortable with it.

Moon-0xff commented 1 year ago

I have tried to use Clutter.BoxLayout before with no luck. Now I'm looking at the GNOME CSS documentation and perhaps we can set the style in some other, more reliable way. The interactive example of border-spacing looks like it does what we want.

Batwam commented 1 year ago

Yeah, it's a funny one as it really looks like the padding is applied inconsistently between icons Vs text and gtk3 Vs 4. It would be interesting to see if other method lead to different results. Having looked at the level of padding in other extensions, a few had similar issues with either excessive and/or inconsistent right/left padding. It just looks like most people don't really care. To be honest, it only really noticeable for people with low padding and a bad case of OCD 😄

I did also consider setting the min padding to 5 which would eliminate the need for the Max formula adjustment but it didn't seem worth reducing choice for people using text only just to remove a fraction of a line of code.

Moon-0xff commented 1 year ago

Surely there's a way!

Moon-0xff commented 1 year ago

I'm still not convinced with this approach, but it works and i can't come up with a better solution so i will merge it.

Moon-0xff commented 1 year ago

I reverted the file mode changes. I don't know why they were committed in the first place... I suppose it was a mistake?

Batwam commented 1 year ago

Yeah, I was going to look at that as I didn't recognise these changes. No idea how they go committed as they don't reflect the changes I made.

Batwam commented 1 year ago

Ok, i get it now, the changes to pref.js were due to the gap with the current main. What was the problem you found with the other commits?

Moon-0xff commented 1 year ago

the file mode bits of extension.js and icons.js were changed from 644 to 755 this is the same as executing this command: chmod +x extension.js icons.js

If you don't recall something like this maybe you should check your IDE configurations?

Batwam commented 1 year ago

ok, that's pretty odd and no, it wasn't intentional. Looking through the commits, it looks like this was changed with the first commit 1cb0f89 image

I have updated the permissions for these two files and pushed the update in https://github.com/Moon-0xff/gnome-mpris-label/commit/ff47d23491378ebbcf834b381747d30d012a2cd3

It looks like this pull request was already merged so let me know if I need to create a new pull request for this. Otherwise, it's just a matter of running chmod -x on the files.

Moon-0xff commented 1 year ago

Don't worry i reverted the file mode already. Thanks for the PR @Batwam

Batwam commented 1 year ago

Ok, no worries