Review of "Quick Settings Audio Devices Hider" version 1

Details Page Preview

A Gnome Shell extension that allows you to hide selected output/input devices from the Quick Settings audio devices panel. Thanks to it, your Quick Settings panel will list only those devices that you actually use making it easier to quickly switch between them. Check out the https://extensions.gnome.org/extension/6000/quick-settings-audio-devices-renamer/ if you'd rather want to rename some device than hide it.

Extension Homepage
https://github.com/marcinjahn/gnome-quicksettings-audio-devices-hider-extension

No comments.

FAQ

Files

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

All Versions

Version Status
15 Active
14 Active
13 Active
12 Active
11 Rejected
10 Active
9 Active
8 Active
7 Active
6 Active
5 Rejected
4 Active
3 Active
2 Rejected
1 Rejected

Previous Reviews on this Version

JustPerfection rejected
1. Timeouts should be removed on disable (line 6 extension.js): https://gjs.guide/extensions/review-guidelines/review-guidelines.html#remove-main-loop-sources 2. You cannot create objects in init (line 180-182 extension.js): https://gjs.guide/extensions/review-guidelines/review-guidelines.html#only-use-init-for-initialization 3. Also null out all `Extension` properties in disable: https://gjs.guide/extensions/review-guidelines/review-guidelines.html#destroy-all-objects 4. You're creating too much `gsettings` object in prefs.js. Create `ExtensionUtils.getSettings()` inside `fillPreferencesWindow` and pass that as dependency injection or create `SettingsUtils.init()` that can create the settings once. 5. What's the reason for doing line 3-13 in prefs.js?
marcinjahn posted a review
1. GJS's implementation of `setTimeout` uses `GLib.SOURCE_REMOVE` (https://gitlab.gnome.org/GNOME/gjs/-/blob/master/modules/esm/_timers.js#L63), so I believe I do not need to remove these explicitly. 2. Moved objects creation to `enable()` 3. Made them null in `disable()`, although I'm not sure if it's necessary. Isn't it enough that they go out of scope? 4. I didn't know that `ExtensionUtils.getSettings()` is expensive. I added a static private field to SettingsUtils so that settings are retrieved just once. 5. It's a leftover, thanks for noticing. Removed. I uploaded version 2, thanks for such a quick feedback.
JustPerfection posted a review
1. As mentioned in the review guidelines: > You MUST remove all active main loop sources in disable(), > even if the callback function will eventually return false or GLib.SOURCE_REMOVE. We want that because the timeout can get delayed and the callback get trigged after disable. That will be a security flaw since that can happen in unlock dialog. 3. It's necessary since they belong to the object that init returns, so they won't get garbage collected if the don't clean up.