ifl0w / RandomWallpaperGnome3

Random Wallpapers for Gnome 3
MIT License
178 stars 42 forks source link

Fix wrong libsoup version when already loaded #125

Closed pascallj closed 2 years ago

pascallj commented 2 years ago

If libsoup is already loaded, the import doesn't fail because it isn't executed. However because it is already loaded, we can check which version we are currently running.

This is just a theory however! It needs to be confirmed on both a libsoup3-only system and a combined system if it doesn't cause any regression on libsoup3 systems.

Fixes #123

ifl0w commented 2 years ago

Thank you again! :)

I did not know that the function get_major_version() exists. It should be possible to replace the check in the constructor of the Bowl class and drop the try-catch workaround completely. I think this would generally be a better solution than the try-catch thing. It is not important what version is loaded, and the Bowl class simply has to use the correct API. Alternatively, we could check if the function send_and_receive_async() exists on the Soup object. My initial workaround with the try-catch is pretty ugly anyway. ^^

I do not know whether your theory is correct; my first thought was that the try-catch block might be optimized away since the _s instance isn't used at all, but I'm not sure right if that would be a valid optimization at all. Both theories would require digging into the GJS code, I guess.

pascallj commented 2 years ago

Yep, there should also be minor and micro. The thing is that in some of the linked issues, there is a link to a commit in Gnome-Shell itself. And they use a similar try catch solution as well. So I'm not sure why that is; you might think they know the code better than we do. The function should be present in both versions.

At least the behavior I noticed was that in the settings window, the Try Catch block caught the exception and therefore set the versions variable correctly. However when enabling the extension or when using anything via the notification bar afterwards, there was no exception and therefore the variable was 'stuck' at 3.0.

ifl0w commented 2 years ago

I got the idea for the try-catch from the commit you mentioned. I think that their fix is loaded for the extension but not for the settings and you can also not load anything from the UI module (which contains the fix) in the settings app.

For me, it always uses version 2.4 in the extension and 3.0 in the preferences. I could imagine that this also depends on what other extensions are installed on the system. I do not think that extensions are properly sandboxed, so they could have an influence on each other. Also, I think that the preferences app starts its own GJS context.

I tested the fix and will merge it, but I'll also simplify the version check directly afterward.

pascallj commented 2 years ago

I got the idea for the try-catch from the commit you mentioned. I think that their fix is loaded for the extension but not for the settings and you can also not load anything from the UI module (which contains the fix) in the settings app.

But I mean there must be a reason why they are doing it this way instead via the version check function. If anything that function is made for situations like this. Which is why I didn't touch the try-catch it in the first place.

For me, it always uses version 2.4 in the extension and 3.0 in the preferences. I could imagine that this also depends on what other extensions are installed on the system. I do not think that extensions are properly sandboxed, so they could have an influence on each other. Also, I think that the preferences app starts its own GJS context.

I think you're on the money there. That would explain why the try-catch only works in the preferences app.

I tested the fix and will merge it, but I'll also simplify the version check directly afterward.

Thank you :+1:

ifl0w commented 2 years ago

Tbh, I have the feeling that the whole GJS thing is pretty messy, but it is also slowly getting better. Not sure either, but their fix with the try-catch might also be intended for internal use only.

In the GJS repository is a note somewhere that states that at some point the import system will be rewritten to match the standard ES6 import system. This is a good idea IMO but also means that the try-catch solution won't be applicable anymore at some point (due to the then likely removal of imports.gi.version).

The most important thing is: "For now it works! :tada:" :D

Thanks for the help!