Moon-0xff / gnome-mpris-label

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

Code cleanup for future compatibility #82

Closed Batwam closed 1 year ago

Batwam commented 1 year ago

Hi, I would like to propose some "transparent" updates to the code which don't impact the way it works or looks but will fix some upcoming issues ahead of the transition to gnome 45 based on some the errors I had to fix to get the gnome 45 version.

In the order of the commits, this would be:

The idea would be to merge this and only then introduce the gnome45 PR based on this cleaned up code.

Moon-0xff commented 1 year ago

I would separate 1fe61d4053dacd06bf1fa5e3c735892dc437e3cf and try to merge it first, is almost granted that ego will reject the next version until settings is deglobalized.

Batwam commented 1 year ago

yep, that's the main one (even in the context of the gnome45 port) and likely to be a must-have like you said. You can cherry-pick it if you want it in priority? However, the other ones are very minor if you want to have a look at them also. I split them so they are easier to follow but most are 1-2 lines commits.

Moon-0xff commented 1 year ago

Well, I did. It also merged c885407f41e6a05f3fc356167ba0962e144e46c7 for some reason, but that's fine.

Batwam commented 1 year ago

Ok, that's good. I've synced the branch so there is very little left in term of diff, mostly basic coding mistakes with missing variable definitions. Feel free to review, cherry-pick and merge at your own pace. I can reimplement the gnome45 port and resubmit a cleaned up PR which will hopefully be relatively short too.

Batwam commented 1 year ago

I am now pushing some updates to replace the use of ExtensionUtils as dependancy injection. This was recommended in my chat Just Perfection and will also help limit the code differences between gnome42 and gnome45 versions. Again, no change on the look and feel.

Batwam commented 1 year ago

ok, to provide some background on the latest commits, I was asking some questions on Matrix in relation to the gnome45 port. Just Perfection found the branch and started commenting on the code we currently have in main. I was told to use dependency injection where possible: image This greatly helps reducing the difference between the Gnome42 and Gnome45 versions as there will be very little difference in the code outside of the import section at the top.

also remove the use Global in label.js so I have updated the code accordingly: image

Moon-0xff commented 1 year ago

Looks great, I don't have a problem with merging this as it stands. Though I'm worried use strict could silently break stuff.

Batwam commented 1 year ago

Yeah, I did notice that with 'use strict', unless the issue is in the main code, it wasn't giving me any error to work with, it just failed silently and I had to go dig to find where the issue came from. It looks like gnome45 will flag these errors or refuse to run anyway so I'm ok if you want to revert that commit.