Review of "Workspace Matrix" version 41

Details Page Preview

Arrange workspaces in a two dimensional grid with workspace thumbnails. If you appreciate this extension please consider to donate $1.

Extension Homepage
https://github.com/mzur/gnome-shell-wsmatrix

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

mzur posted a review
The diff is also on GitHub which may be easier to review: https://github.com/mzur/gnome-shell-wsmatrix/pull/263/files
JustPerfection rejected
1. You cannot create instances in the class and leave it there after prefs window close (line 8 prefs.js). Make it local to `fillPreferencesWindow()` and attach it to `window` to avoid garbage collecting before window close: ```js let settings = this.getSettings(); window._settings = settings; ``` You can pass it to the methods needing that. https://gjs.guide/extensions/development/preferences.html#prefs-js 2. Timeout should be removed on destroy (line 371 workspacePopup/workspaceManagerOverride.js): https://gjs.guide/extensions/review-guidelines/review-guidelines.html#remove-main-loop-sources 3. Don't use the same method name for different timeout ids (line 102 and 137 workspacePopup/workspaceSwitcherPopup.js).
mzur posted a review
Thanks for taking the time to review this! I have questions/comments for 2. and 3.: 2. This method is a modified copy of js/ui/windowManager.js@handleWorkspaceScroll (line 1805 in 45.0). I try to stick to the original implementation as closely as possible. Should I still update this in my extension? 3. I'm afraid I don't understand. You mean I should use different variables for the IDs because both timeouts could be active at the same time?
JustPerfection posted a review
2. Yeah! you should fix it when you use it in the extensions. 3. Yes! I understand line 102 timeout is getting removed right before creating the new one but If you want to use the same method name for timeout ids, create a method that removes the old timeout and create the timeout. In that case you just call that method without creating two timeouts. That's a better practice and easy for future reviews since we only review the diff.