Review of "Persian Calendar" version 93

Details Page Preview

Displays Persian (Iranian/Jalali) calendar in the top panel It offers: 1. Displays the Persian/Iranian/Jalali calendar 2. Holiday indicator 3. Day change notifications 4. Converts dates between the Persian, Gregorian, and Hijri (lunar) calendars 5. Event listings: 5.1. Official solar events 5.2. Official lunar events 5.3. Official international events 5.4. Traditional Persian events 5.5. Notable Persian figures Please “rate” the project here and give it a “star” on GitHub. If you encounter any issues or have suggestions, feel free to open an issue there on GitHub!

Extension Homepage
https://github.com/omid/Persian-Calendar-for-Gnome-Shell

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

omid posted a review
Hello :) If you remember, it's related to having multi-language separated from Gnome-shell and OS language. The file related to this is the `utils/gettext.js`. I also have a simple caching, but not sure if a `let` global variable is a good practice or not. (I checked and the cache system works generally)
omid posted a review
(I just saw "locale/fa/LC_MESSAGES/persian-calendar.po~" file, it's not a big deal, I will remove it for the next releases)
JustPerfection rejected
Hi, 1. You cannot create objects in global scope which is the same as init (line 4 utils/gettext.js): https://gjs.guide/extensions/review-guidelines/review-guidelines.html#only-use-init-for-initialization 2. I missed this one before. You cannot create gsettings objects in global scope (line 10 extension.js). Move that line to enable and null that out in disable. 3. Also move line 5 (pref.js) to `App.constructor`. 4. Remove `locale/fa/LC_MESSAGES/persian-calendar.po~`. 5. Please inactivate version 92 after next version approval.
omid posted a review
Thanks. 1. So what is the idiomatic way of doing this? Create a variable in init and pass it around? In this case, what about perf.js file? It's ugly. Maybe I can ignore caching the gettext result! I'll do all others, thanks :)
JustPerfection posted a review
You have many options. Dependency injection is one of the way. Or you can create a settings module like this: ```js let settings; function getSettings () { if (!settings) { settings = ExtensionUtils.getSettings(); } return settings; } function destroy() { settings = null; } ``` on `disable()` you should use the `settings.destroy()`.
omid posted a review
Oh, thanks. So it's legal then. So I'll do this for the gettext cache. I think there is no need to destroy it for prefs, am I right?
JustPerfection posted a review
For prefs, just do this in the `App.constructor()`: ```js this._settings = ExtensionUtils.getSettings(); ``` The settings module I was suggested is more suited for `settings.js`. If you want to use it in prefs, you MUST destroy it in window close: ```js window.connect('close-request', () => { // destroy here }); ``` If you don't know how to get the window object: [42](https://gjs.guide/extensions/upgrading/gnome-shell-42.html#resize-preferences-window) [40 and 41](https://gjs.guide/extensions/upgrading/gnome-shell-40.html#get-prefs-window-and-resize-it)
omid posted a review
Could you please check the changes here, if this way is valid, then I'll upload a new version: https://github.com/omid/Persian-Calendar-for-Gnome-Shell/compare/master...fix_ego_comments I'm not sure about prefs.js, I think it'll free the memory when user closes it.
JustPerfection posted a review
AFAICS, it's ok. Send it here and I'll check that again. and yes, `App` object should be garbage collected when the prefs window is getting closed.