Review of "EasyEffects Preset Selector" version 9

Details Page Preview

Quickly show and load EasyEffects Presets

Extension Homepage
https://github.com/ulville/eepresetselector

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
24 Active
23 Active
22 Active
21 Active
20 Active
19 Active
18 Active
17 Active
16 Active
15 Active
14 Active
13 Active
12 Active
11 Active
10 Active
9 Rejected
8 Rejected
7 Active
6 Active
5 Active
4 Active
3 Active
2 Active
1 Rejected

Previous Reviews on this Version

ulville posted a review
This version uses Mainloop.timeout_add() instead of setTimeout() for older GNOME versions (<42)
JustPerfection rejected
Ok, but why don't you use `GLib.timeout_add()` and `GLib.Source.remove(timeoutId)` instead? and you should remove the timeout on disable: https://gjs.guide/extensions/review-guidelines/review-guidelines.html#remove-main-loop-sources
ulville posted a review
Ok, when I use `timeoutId = GLib.timeout_add(GLib.PRIORITY_DEFAULT, milliseconds, () => { resolve(); });` everytime the user refreshes the menu in less than 1 secs it's creating a new timeout source and overwriting timeoutId with it. On disable we can remove the last timeout source but previous ones stays untouched. So I think I should also remove the timeout right after the Promise which covers it gets resolved. But when I do that everything seems ok but it logs an error: "Source ID 9366 was not found when attempting to remove it". If I add `return GLib.SOURCE_REMOVE;`in the callback function it acts the same. Only If I do `return GLib.SOURCE_CONTINUE;` like in the example you sent, it doesn't error. But it feels awkward to use `return GLib.SOURCE_CONTINUE;` for a timeout we will only use once and remove immediately. Doesn't using `return GLib.SOURCE_REMOVE;` inside callback function already remove the timeout source? What would be the best approach in this situation?
ulville posted a review
https://docs.gtk.org/glib/const.SOURCE_REMOVE.html
JustPerfection posted a review
Because when you want to remove the timeout id in disable, you should do: ```js if (timeoutId) { GLib.Source.remove(timeoutId); } ``` Inside the callback you should null out the timeoutId: ```js timeoutId = GLib.timeout_add(GLib.PRIORITY_DEFAULT, 1000, () => { timeoutId = null; return GLib.SOURCE_REMOVE; }); ``` The reason we want you to do that is that the timeout can get delayed, so they can get triggered after disable.