Please see https://github.com/paperwm/PaperWM/pull/556 for notes on changes implemented addressing each of the review issues (from review 1). Justification for session-mode `unlock-dialog`: PaperWM requires session mode `unlock-dialog` to maintain multi-monitor tiling layouts when locking/unlocking Gnome. Without this, tiled windows can move to different monitors on gnome unlock, which severely impacts/destroys user workflows and window placements. Regards, Jay.

Good but theses issues still need to get fixed: 1. Selective disable isn't allowed (line 122 and 126 extension.js): https://gjs.guide/extensions/review-guidelines/review-guidelines.html#session-modes 2. Move import lines to top of the file: - line 140-143 extension.js - line 219-221 scratch.js 3. Does `warnAboutGnomeShellVersionCompatibility()` function exist because of legacy code (line 147 extension.js)? 4. Too much for init (line 90-91 extension.js): https://gjs.guide/extensions/review-guidelines/review-guidelines.html#only-use-init-for-initialization 5. You should remove the timeout in disable: - line 405 gestures.js. - line 208 navigator.js - line 44 settings.js - line 89, 235 stackoverlay.js - line 1729, 2742, 2756 tiling.js https://gjs.guide/extensions/review-guidelines/review-guidelines.html#remove-main-loop-sources 6. For `session-modes` you should add the reason to the disable function but I don't think moving windows to another place is a good reason for that. because in lock, your extension will be disabled too, despite the fact you have `unlock-dialog` in session mode. after disable, it will be re-enable because you have `unlock-dialog` to `session-modes`. IMO you shouldn't rely on that to avoid windows getting out order. Maybe saving the window positions and re-arranging them on enable is the better solution. Read about session modes here: https://discourse.gnome.org/t/extension-disable-called-on-session-mode-change-but-only-for-the-first-time/15832 7. You cannot create instances in global scope which is the same as init: - line 12 extension.js https://gjs.guide/extensions/review-guidelines/review-guidelines.html#only-use-init-for-initialization 8. You forgot to untrack (line 253 stackoverlay.js). 9. You are still using many `log()`s. Use `console.log()` instead: https://gjs.guide/extensions/review-guidelines/review-guidelines.html#no-excessive-logging

> 3. Does `warnAboutGnomeShellVersionCompatibility()` function exist because of legacy code (line 147 extension.js)? Yes. But also, since PaperWM can be installed from source (which is how we've been doing it for the last 3-4 years) - often we'll enable a gnome-shell version (say like Gnome 45) so that our users can test and report issues - that notification is to remind users of this. > 4. Too much for init (line 90-91 extension.js) Fair enough for line 90, but line 91 does not create any instances etc - it basically calls `init' methods in other modules (since we spread and organise our code into various modules). Other PaperWM modules don't use init. PaperWM allows users to add a `user.js` file to provide some user specific customisation (which is a loved feature in PaperWM) - so line 91 calls a `init` in user.js (if a user has one). > 6. For `session-modes` you should add the reason to the disable function but I don't think moving windows to another place is a good reason for that. I was hoping you would say that :-). Hmmm - yes, I've seen the behaviour linked... what I've seen is that usually on the first lock, it seems to disable/enable PaperWM on unlock, but subsequent locks don't actually disable/enable it (have confirmed this through gnome logs too. The moving windows isssues I see only occur when using multiple monitors - and on enable individual windows seem to jump/move to another monitor. I'll look at that - but I had assumed "unlock-dialog" allows PaperWM to NOT disable/enable when the lockscreen is up. If it doesn't, what exactly does "unlock-dialog" do? > 7. You cannot create instances in global scope which is the same as init: - line 12 extension.js Can you confirm where you're seeing this? Line 12 in extension.js is part way through a comment. > 9 . You are still using many `log()`s. Use `console.log()` instead Can do. Do you see an issue with the number of log calls (or just the fact that we aren't using `console.log(...)`? Well, on the plus-side, 8 issues is much less than last time! (28). Thanks JustPerfection!

Re my comment for (6): * I was hoping you wouldn't say that * :-)

# 4 I understand but we don't want other than init translation there. # 6 Can still get disabled on lock because: - GNOME Shell disabling all extensions first. - Then enables extensions with `unlcok-dialog`. This can allow GNOME Shell to rebase properly. `session-modes` is more suited for extensions changing the lock screen look and for extensions want to work while the screen is locked. But your extension cannot use that for these reasons: - The disable enable process still happens (it can even happen multiple times with one lock). - You are using keybindings which is not allowed in lock screen for security reason. As we stated in ego review guidelines: > All signals related to keyboard events MUST be disconnected in unlock-dialog session mode. https://gjs.guide/extensions/review-guidelines/review-guidelines.html#session-modes # 7 Sorry about that. I meant line 12 `settings.js`. # 9 We generally prefer not using like that because log should be only available on debug mode. We have `console` since 42. That gives you the ability to still use log in different levels (`console.debug`, `console.log`, ...). You can read the debug with: ```bash journalctl -fo cat GNOME_SHELL_EXTENSION_UUID=paperwm@paperwm.github.com ```

> We have `console` since 42. That gives you the ability to still use log > in different levels (`console.debug`, `console.log`, ...). > > You can read the debug with: > > ```bash > journalctl -fo cat GNOME_SHELL_EXTENSION_UUID=paperwm@paperwm.github.com > ``` Okay, I've gone through and replaced log() with console.log(), console.debug(), and console.error(). Only console.log entries are showing in `journalctl -r /usr/bin/gnome-shell` and I'm not getting much `journalctl -fo cat GNOME_SHELL_EXTENSION_UUID=paperwm@paperwm.github.com`. How do you start the extension (or gnome) in debug mode to see the console.debug() messages?

I notice this (referring to `journalctl -fo cat GNOME_SHELL_EXTENSION_UUID=paperwm@paperwm.github.com`) at https://gjs.guide/extensions/development/debugging.html#filtering-by-extension: ``` Filtering by Extension WARNING This feature has limited usefulness, because it silences other messages from GNOME Shell and only works with the built-in log() function (not console.log()). ``` e.g. this doesn't actually work with `console.log()`... it only works with `log()` (which you've asked us not to use... I seem to be missing something here.

Oh! Sorry, I gave you wrong command. To enable debug mode, you can use this in your `.js` file: ```js GLib.log_set_debug_enabled(true); ``` or in nested session: ```bash G_MESSAGES_DEBUG=all dbus-run-session gnome-shell --nested --wayland ```

Hey @Javad, I'm putting some final touches/changes on PaperWM for the next submission. Question re `Mainloop` sources: So, I understand and agree with the intent of running a `remove_source` call for any Mainloop sources added in `disable`. Given that all our Mainloop sources return false, the and the sources get removed at the end of the callback function, removing these in `disable` does pollute the log with quite a few lines like ``` Source ID 41357 was not found when attempting to remove it ``` Since the source has actually already been removed from the Mainloop. Do we just accept those console lines as cleanup checks? (e.g. "oh, tried to remove it just in case but it's not there"). Alternatively, I was thinking of null the timeoutid just before a `return false` - then the check in `disable` won't attempt to remove a source that's already been removed (with a null check). Thoughts?

Hi, Because you are not cleaning up the source inside the *timeout callback*. ``` timeoutId = GLib.timeout_add( GLib.PRIORITY_IDLE, 500, () => { timeoutId = null; return GLib.SOURCE_REMOVE; } ); ``` Now you can remove it on disable: ```js if (timeoutId) { GLib.source_remove(timeoutId); timeoutId = null; } ``` btw, we want the timeout cleanup on disable because timeouts can get delayed. so the callback can get triggered on lock screen and that's a security issue. as we 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. https://gjs.guide/extensions/review-guidelines/review-guidelines.html#remove-main-loop-sources

Awesome, thanks Javad! Yep, I like the removal in disable, just wanted to remove those console lines and checking it's okay to do what you mentioned. Cheers.