Review of "Automatic Theme Switcher" version 5

Details Page Preview

Automatically switches between light and dark themes based on sunrise/sunset times for your location. Supports multiple trigger options (golden hour, dawn, dusk, etc.), gradual brightness transitions, and syncs with Night Light. Uses ipinfo.io, sunrisesunset.io, and openstreetmap.org services. If you find any bugs/issues, feel free to create an issue at https://github.com/amritashan/gnome-shell-extension-auto-theme-switcher/issues

Extension Homepage
https://github.com/amritashan/gnome-shell-extension-auto-theme-switcher

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

Version Status
8 Active
7 Inactive
6 Rejected
5 Rejected
4 Rejected
3 Inactive
2 Rejected
1 Rejected

Previous Reviews on this Version

amritashan posted a review
i'm really sorry for the large diff. I only intended to fix the brightness issue, but I had to redesign the approach because it was too tight. Plus, the last review about the large class file size of extension.js made me realise that the entire thing was too monolithic. I have refactored to create individual controllers so that future reviews are easier. 1. Large Default Class in extension.js Issue: The exported class was too large (~1,195 lines), making it hard to review for cleanup. Resolution: Refactored the monolithic class into a modular architecture: Before: Single 1,195-line file Now: Main extension.js reduced to ~530 lines (55% reduction) New Module Structure: brightnessController.js - All brightness control logic (256 lines) themeController.js - Theme switching and Night Light management (86 lines) apiClient.js - API calls for location detection, sun times, and geocoding (128 lines) timeCalculator.js - Time parsing and calculation utilities (70 lines) Each module has a single responsibility and includes its own cleanup() method, making resource management easier to review. 2. Unnecessary try-catch Blocks Issue: Code was wrapped with excessive try-catch blocks. Resolution: Simplified the disable() function: Removed: try-catch around simple variable assignments (e.g., this._settings = null) Kept: try-catch only where actual failures can occur (signal disconnections, D-Bus operations) 3. Session Mode Justification in disable() Issue: Missing comment explaining why session mode signals need cleanup. Resolution: Added clear justification at the top of disable() function (lines 205-208): 4. Future Diff Management Issue: Large diffs make reviews harder and longer. Resolution: The new modular structure ensures future changes will be focused, smaller, easier to review and have better separation of concerns
JustPerfection rejected
1. Create the session instance once in the constructor instead (line 13, 76 `apiClient.js`). Then you should call `abort()` on disable or destroy: [`Soup.Session.abort()`](https://gjs-docs.gnome.org/soup30~3.0/soup.session#method-abort) Don't forget to call destroy on the instance of `APIClient`. 2. All the timeouts you are creating in `extension.js` are stored in `this._timeoutId` which is a bad practice. You have three options: - Store the ids in different properties. - Move all the timeouts into a function, then you can call that function when needed. This way only one place will be creating a timeout. - Remove the `this._timeoutId` before each one of those timeouts. 3. Try and catch blocks are still unnecessary. For example, line 118, 133, ... `extension.js`. If the goal is to be able to get the info from users when something wrong is happening, the log will have the backtrace. It will include the extension folder which is obvious where the error comes from. 4. The default class can be approved like this but you can make the entry point as small as possible. Maybe looking at my extension can give you the idea about how a large extension can still manage to have a very small entry point: - [extension.js](https://gitlab.gnome.org/jrahmatzadeh/just-perfection/-/blob/f43c7f7642bc838a3508bee423c4987a0636fbd7/src/extension.js#L44-160) - [prefs.js](https://gitlab.gnome.org/jrahmatzadeh/just-perfection/-/blob/f43c7f7642bc838a3508bee423c4987a0636fbd7/src/prefs.js#L21-56) Btw, My extension **only** imports module in the `extension.js` and `prefs.js`. You don't need to use that architecture in your extension. Without that, even the entry point can be smaller. If you still need help with the architecture or changes I'm open to talk in - [GNOME Extensions Matrix Channel](https://matrix.to/#/#extensions:gnome.org) - IRC Bridge: irc://irc.gimpnet.org/shell-extensions