What's the difference between this one and the original?

Hey JustPerfection, I'm an admin and maintainer for PaperWM. So, this is the original PaperWM. We've been working on it for a while (including updated Gnome shell version compatibility and a bunch of new features). I've now (finally) aligned PaperWM with GJS guidelines (see https://github.com/paperwm/PaperWM/pull/556) and this it our first submission to extensions.gnome.org!

1. You mean this instead (line 347 kludges.js)? ```js if (version[0] >= 40) { // ^ ``` Same issue for line 443 (kludges.js). 2. Remove `session-modes` or mention the reason in the disable comment section: https://gjs.guide/extensions/review-guidelines/review-guidelines.html#session-modes 3. What's the reason for not just using `ExtensionUtils.getCurrentExtension()` instead of lines 6-10 (app.js)? I see that in other files too. 4. Use initTranslations() and getSettings() from ExtensionUtils instead of creating your own custom functions (remove convenience.js after that): https://gitlab.gnome.org/GNOME/gnome-shell/-/blob/main/js/misc/extensionUtils.js 5. Please remove line 3-6 (extension.js). That's unnecessary since you are using 42 and higher. 6. Remove the timeout in disable/destroy: - line 409 gestures.js - line 220, 289 navigator.js - line 54 settings.js - line 96, 233 stackoverlay.js - line 1686, 1752, 2787, 2798 tiling.js https://gjs.guide/extensions/review-guidelines/review-guidelines.html#remove-main-loop-sources 7. Import at top of the code instead of importing inside a function (for example, line 409 gestures.js). In GNOME Shell 45, you will have a hard time to maintain a code like that. 8. `Tweener` is a deprecated module. You can use `ease()` instead. For example: ```js widget.ease({ x: newX, y: 10, opacity: 100, duration: 2000, mode: Clutter.AnimationMode.EASE_OUT_BOUNCE, onComplete: () => { log('Animation is finished'); } }); ``` 9. Your extension is 42 and higher, you don't need to use/check/modify: - line 263 keybindings.js. - line 47-222, 428, 434, 399, 607 kludges.js btw, the way you are modifying the globals is simply bad for extensions. because you cannot revert back or remove them on disable (for example line 219 kludges.js). Instead of doing that you should check for GNOME Shell version and run the proper code accordingly. 10. Don't use `print()` in your extension (for example, line 439 keybindings.js). If you want to log errors, use `console.error()` instead. 11. You cannot create instances in global scope which is the same as init: - line 29, 67, 800 kludges.js - line 23 liveAltTab.js - line 17 prefs.js - line 49-54 tiling.js https://gjs.guide/extensions/review-guidelines/review-guidelines.html#only-use-init-for-initialization 12. You shouldn't import `Gtk` in GNOME Shell: - line 65 kludges.js. (We have `St.Settings` though. so, the code 64-79 is unnecessary.) Indirectly imports `Gtk` inside extension.js: - line 21 (liveAltTab.js) - line 19 (minimap.js) - line 28-29 (topbar.js) - line 44 (tiling.js) 13. How do you revert back to original after using `disableHotcorners()` (line 285 kludges.js)? 14. Please remove `Lang` import (line 11 minimap.js). 15. `destroy` should also call `dismissDispatcher()` (line 238 navigator.js). 16. Remove unnecessary logs (for example, line 842 pref.js): https://gjs.guide/extensions/review-guidelines/review-guidelines.html#no-excessive-logging 17. Bad caps on schema id (line 4, 263, 270, 406, 407 schemas/org.gnome.shell.extensions.paperwm.gschema.xml). 18. Too much for global scope (line 89 settings.js): https://gjs.guide/extensions/review-guidelines/review-guidelines.html#only-use-init-for-initialization 19. You forgot to null out `schemaSource`, `workspaceList`, `conflictSettings` in disable (line 114 settings.js). 20. You should untrack on destroy (line 82 stackoverlay.js): ```js Main.layoutManager.untrackChrome(enterMonitor); ``` 21. Cannot say if you call `removePreview()` on destroy (line 275 stackoverlay.js). 22. I understand that you call `removeBarrier()` in the destroy of another class (line 198 stackoverlay.js), but please remove the timeout in the same class, so we can track the timeout removal easier while reviewing (line 382 stackoverlay.js). 23. Please use semicolon since the other lines using it (line 8-105 virtTiling.js): https://gjs.guide/extensions/review-guidelines/review-guidelines.html#general-advice 24. Don't add or remove `style_class` like that to avoid removing class names that other extensions adding (line 736, 743). Use `add_style_class_name` and `remove_style_class_name` instead: https://gjs-docs.gnome.org/st12~12/st.widget#method-add_style_class_name https://gjs-docs.gnome.org/st12~12/st.widget#method-remove_style_class_name 25. Please remove `Gdk` import (line 29 tiling.js). 26. Remove current module import (line 8 keybindings.js). 27. It would be much better if you can find a way to drop `Gtk` and `Gdk` from lines 404, 413 and 447 (keybindings.js), so you can remove `Gtk` and `Gdk` import in that file. If you need any help with your extension you can ask us on: - [GNOME Matrix Channel](https://matrix.to/#/#extensions:gnome.org) - IRC Bridge: irc://irc.gimpnet.org/shell-extensions

btw, for `Gdk.Keymap` replacement read this: https://gitlab.gnome.org/GNOME/gnome-shell/-/issues/4706#note_1292344

Thanks JustPerfection! That's a pretty in-depth review! I appreciate the time and guidance here for sure. Yes, PaperWM has a long history (and sadly quite some legacy code and approaches). In any case, I'll work to address each of these for another (upcoming) submission. Appreciate the time you've put into this review (it's our first so I was expecting quite a few changes needed). Apologies for missing a few of these (like #2, which I actually remember reading but missed this change in the submission branch). Cheers, Jay.

No problem ;) Please let me know if you need any help with fixing the issues. btw, for #1 I meant line 437.

Evening! Just a heads-up that I've created a branch to tackle and track the issues raised here: https://github.com/paperwm/PaperWM/pull/557 2 down... will chip away at the rest.

Morning JustPerfection! Halfway there! https://github.com/paperwm/PaperWM/pull/557. I honestly think this is the best thing to happen to PaperWM - over 1746 lines changed (found a whole bunch of other issues too that have now been addressed). A lot of cleanup and removal of unneeded things. Unfortunately, I can't seem to get rid of Gtk entirely as we rely on `https://gjs-docs.gnome.org/gtk40~4.0/gtk.accelerator_parse` and `https://gjs-docs.gnome.org/gtk40~4.0/gtk.accelerator_parse_with_keycode` for the keybind system. Let me know if you have any ideas here. Thanks again!

Hi, that's great! Code cleanup is fun! For `accelerator_parse_with_keycode`, I don't think you should use that in extensions. as stated in the doc: > This is only useful for system-level components, applications should > use [func@Gtk.accelerator_parse] instead. For `keybindings.js`, I think that's too complicated for extension.js. You simply need to add and remove the key binding in extension.js where you have all the keys stored in gsettings. On the key press callback the first parameter is `Clutter.KeyEvent` which has all the info you need. btw, `bindkey()` and `unbindkey()` seems to be unused functions (line 334, 391 keybindings.js). So, maybe you don't need `Gtk.accelerator_parse` after removing those functions. If you still think you need `Gtk.accelerator_parse`, I highly recommend to ask that in GNOME Matrix channel or [GNOME Discourse](https://discourse.gnome.org/tag/extensions). We have GNOME Shell developers and extension developers there. Maybe they can help you with that.

Hmm - no, those two function are used through PaperWM (for example, in `layouts.js` etc. I must admit, I'm somewhat confused here. We're actually using it like the following extensions do (which are also on `extensions.gnome.org`): - https://extensions.gnome.org/extension/1160/dash-to-panel/ - https://extensions.gnome.org/extension/4481/forge/ - https://extensions.gnome.org/extension/3780/ddterm/ - https://extensions.gnome.org/extension/3433/fly-pie/ - https://extensions.gnome.org/extension/3612/wireguard-indicator/ These and many other extensions also use Gtk.accelerator_parse in a similar fashion. Hence, I'm not sure why we shouldn't be importing Gtk and using accelerator_parse etc. when other extensions do(?). Anyways, I've done 17 of the 26 now! Hoping to submit these changes shortly. For reference, I just did a search on: https://github.com/search?q=accelerator_parse%28+language%3AJavaScript&type=code&l=JavaScript In any case,

Ok, if it is possible, use `Gtk.accelerator_parse` instead of `accelerator_parse_with_keycode`. If there isn't any other way, use `accelerator_parse_with_keycode` since that's falling under "there is no other way". btw, you mean `layouts.js` in this package? because there isn't `layouts.js` here unless you are talking about `layouts.js` in GNOME Shell.

Ah, apologies! I missed this message. Yes, good point, `layouts.js` in an in example file that's not part of this package (well, it's part of PaperWM, but I didn't submit it since it's not needed...). I just saw a reference when I did a search and assumed it was then needed. Also, I misinterpreted [12] - I thought you were saying we shouldn't import `Gtk` in PaperWM (but I can now see you meant in `extensions.js`, not `prefs.js` etc.). Anyways, leave this with me - I'm going to look at this again and see how to improve the dependence on `Gtk` here (or at least try get rid of accelerator_parse_with_keycode`). Btw, only a few more to go (https://github.com/paperwm/PaperWM/pull/557). I'm mentioned it before, but I'm very appreciative of your guidance here (honestly) - I've wanted to clean-up and improve our codebase for some time now (but lacked the motivation to do so, since things were working okay). So glad I did, and so glad you've given us a really good review and highlighted lots of issues (which was sorely needed!). Looking forward to doing another submission soon (hopefully in the next few days). Might still be a few issues that crop up for us to look at, but it should drastically(!) improved from our initial submission. Jay.

Evening JustPerfection, Here's the notes re the changes for this new version submission, specifically addressing each of the items from the last submsision. You can see these and a bit more detail from https://github.com/paperwm/PaperWM/pull/556 : - [x] **1 DONE**. Good pickup and call here - was a bit of a mess. Including a very interesting hard-coding of the `version` var (was written back before Gnome 40). In any case, the first commit here was fixing that and removing old/deprecated code that was for gnome 3.xx. - [x] **2 DONE**. `sessionModes` removed from metadata.json (not needed in PaperWM). Also improved the `safeCall` logging to only log if called method exists in module. - [x] **3 DONE**. I'm not sure of the original reason for setting `Extension` like it was doing. Replaced with a much simpler `var Extension = imports.misc.extensionUtils.getCurrentExtension();`. - [x] **4 DONE**. Removed `convenience.js` as it duplicates current Gnome ExtensionUtils and refactored modules that used it . - [x] **5 DONE**. Removed unneeded check. - [x] **6 DONE.**. did full review of all `mainloop.timeout_add` instances - most were actually (intended) as once off calls (e.g. destroy after first handler execution) - changed these to destroy after handler function execution. - [x] **7 DONE**. Went through and cleaned up `imports.xxx` references (and moving them to top where possible). - [x] **8 DONE**. We were actually delegating the animation through `utils.tweener`... which was actually using `Widget.ease(...)`. This was basically because it was easier to do this when we switched to `ease`. However, this is quite confusing for users/reviews. I've taken the opportunity now to rename `Tweener.addTween(...)` to `Ease.addEase(...)` etc. - [x] **9 DONE** (finally!). This was a very good point in the review. Polyfilling in functions on global gnome objects isn't the best approach here. I've now replaced the polyfills with backwards-compatible utility functions which is a cleaner/safer/better approach and should make this much easier to maintain going forward. - [x] **10 DONE**. Replaced the use of `print(...)` across PaperWM. - [x] **11 DONE**. Moved global scope creation of instances to `enable` in `Kludges.sj`, `liveAltTab.js`, `prefs.js`, `tiling.js`. Also cleaned up and removed unneeded imports dependencies and improved formatting. - [x] **12 DONE**. Hmm - we don't see an issue with importing Gtk and using accelerator_parse etc. But we have cleaned up and removed references to Gtk where not needed. - [x] **13 DONE**. We've removed the hot corner overrding/disabling from PaperWM. This was largely superfluous since hot corner works fine in PaperWM, and if users want to disable it they can do so via the usual gnome setting. Having two functions that disable hot corner (one in gnome and one in PaperWM) seems superfluous. - so, on PaperWM `disable`, calling `layout._updateHotCorners()` resets hot-corners, effectively restoring the hot-corner user setting. - [x] **14 DONE**. Removed unneeded import in `minimap.js`. - [x] **15 DONE**. `destroy` should not call `dismissDispatcher` since `dismissDispatcher` calls `destroy`... leading to an infinite loop (recursion). - [x] **16 DONE**. All `log(...)` lines reviewed and remove superfluous logging; - [x] **17 DONE**. Lowercased PaperWM schema id values and recompiled schema. - [x] **18 DONE**. Refactored to NOT call `setSchemas()` on Settings `init` (is not called on `enable`). - [x] **19 DONE**. nulled out vars (that we missed). Good pickup! - [x] **20 DONE**. - [x] **21 DONE**. Refactored StackOverlay class - added proper destroy method and now removePreview on destroy. - [x] **22 DONE**. Timeouts are now oneshot (remove/destroy timeout after function callback). This should address this issue. - [x] **23 DONE**. Added missing semicolons and other cleanup in `virtTiling.js`. - [x] **24 DONE**. Now using `add_style_class_name` and `remove_style_class_name` to not run over other extension styles. - [x] **25 DONE**. Removed Gdk import from Tiling.js. - [x] **26 DONE**. Removed module self reference from `keybindings.js` and `tiling.js`. - [x] **27 DONE**. Replaced `Gdk.Keymap` (which isn't available in Gnome 40+) with `Clutter.get_default_backend().get_default_seat().get_keymap()` and hence removed `Gdk` import from `keybindings.js`. Haven't been able to remove `Gtk` since we need `accelerator_parse` from `Gtk`.

Oops, ignore this line: ``` - so, on PaperWM `disable`, calling `layout._updateHotCorners()` resets hot-corners, effectively restoring the hot-corner user setting. ``` We've actually just removed the hotcorner overriding code entirely from PaperWM.

Dang it, I blame my keyboard. This should read: ``` - [x] **18 DONE**. Refactored to NOT call `setSchemas()` on Settings `init` (is NOW called on `enable`). ```