Review of "OpenWeather" version 108

Details Page Preview

Display weather information for any location on Earth in the GNOME Shell

Extension Homepage
https://gitlab.com/skrewball/openweather

No comments.

Diff Against

Files

Note: Binary files aren't shown on the web site. To see all files, please download the extension zipfile.

All Versions

Previous Reviews on this Version

skrewball posted a review
This version removes the legacy Lang class and updates libsoup code to v3. Also removes all darksky code, lots of bug fixes including the icon issues some users were having on PopOS. The next version I am rebuilding prefs.js using libadwaita.
JustPerfection rejected
Great! Please fix these and send it again: 1. No need for spawn in line 931 (extension.js) since it is only supported in 40 and higher. 2. Remove all of the timeouts when the prefs window is getting closed: ```js prefswidget.connect('close-request', () => { // ... }); ```
skrewball posted a review
Removed the spawn on line 931, but just wondering about the timeouts when the prefs window is getting closed. Do you mean line 163 and 164 in prefs.js? If so I'm not sure what timeouts are linked to them? I added those 2 lines because when in prefs clicking edit location or add location opens sub windows, they were not closing when clicking the close button on the titlebar and that did it, those connects are not linked to the main window. If you're referring to the timeouts from line 318 I'm not exactly sure why Jens had those there, they appear to be linked to the spinners I'm not sure why, perhaps they were needed in older versions of gnome? Was planning to take care of those when rebuilding prefs.js for libadwaita but I'll investigate now.
skrewball posted a review
It seems the timeouts on the spinners and slider was used to prevent the value from continuously being saved while the user is changing the value. Removing those does not seem to hurt anything.
skrewball posted a review
So I just realized that you probably meant to use a close-request signal on the main prefs window to remove the timeouts when the window closes rather than completely remove them? Apologies, lack of sleep lately I should have caught that lol. I remember you asking about the timeouts on the last version. The main difference I've noticed without them is that there can be a little delay when adjusting the spinners and slider. I already uploaded a new version with the timeouts completely removed, would it be better to reject that version too and re-add the timeouts while connecting to a close-request to remove them? Or we could just go ahead with them completely removed.
JustPerfection posted a review
Basically, when a window is getting closed, still, the timeout callback can get triggered. 99.99% of the extensions don't use any timeouts in the prefs (there isn't so many use case for that) but if you have any timeout, it needs to be cleaned out in close. I think it's good that you sent the next version without any timeouts in prefs. Thatnks!
skrewball posted a review
Perfect, I'll move forward without those timeouts. Thanks! :)