Review of "ddterm" version 44

Details Page Preview

Another drop down terminal extension for GNOME Shell. With tabs. Works on Wayland natively

Extension Homepage
https://github.com/ddterm/gnome-shell-extension-ddterm

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

JustPerfection rejected
1. You cannot create objects in global scope: - line 40 ddterm/pref/animation.js - line 34 ddterm/pref/util.js https://gjs.guide/extensions/review-guidelines/review-guidelines.html#only-use-init-for-initialization 2. This is >=40 so no need for line 27 ddterm/pref/shortcuts.js. 3. Extensions cannot use `run_dispose()` (line 246 ddterm/shell/extension.js): > This function should only be called from object system implementations. https://gjs-docs.gnome.org/gobject20~2.0/gobject.object#method-run_dispose 4. Timeout should be removed on disable (line 264 ddterm/shell/extension.js): https://gjs.guide/extensions/review-guidelines/review-guidelines.html#remove-main-loop-sources
amezin posted a review
> 1. You cannot create objects in global scope: I thought it wasn't a problem because: - this code is there since v13 (just moved between different files). - prefs aren't loaded int main GNOME Shell process (if I'm not mistaken, a new process is even spawned for every preferences dialog). - it's not a GObject. Is `Me.dir.get_child()` allowed then? Because it also creates an object (and, in this case, it's a GObject). And if it's not allowed - how to use Gtk templates (i. e. how to build template URIs)? > 2. This is >=40 so no need for line 27 ddterm/pref/shortcuts.js. Preferences dialog (i. e. the code in `ddterm/pref`) is also imported by the Gtk 3 app (otherwise I can't show the dialog on top of main window, if the main window is already "above all windows"). You could find a few more instances of Gtk 3 compatibility checks in `ddterm/pref`, they just don't say "is Gtk 3" explicitly. > 3. Extensions cannot use `run_dispose()` (line 246 ddterm/shell/extension.js): But multiple extensions from `gnome-shell-extensions` repo (which I thought are examples of how to do things correctly) do it with Gio.Settings: `auto-move-windows`, `places-menu`, `user-theme`, `window-list`. For example: https://github.com/GNOME/gnome-shell-extensions/blob/main/extensions/auto-move-windows/extension.js#L63 > 4. Timeout should be removed on disable (line 264 ddterm/shell/extension.js): Not sure which timeout exactly you're talking about, but: - The only place where timeouts are added in the shell extension is `wait_timeout()` (if I haven't forgot something) - which removes the timeout source when `cancellable` is cancelled - All cancellables for `wait_timeout()` are connected to `disable_cancellable`, which is cancelled in `disable()`, on the first line.
JustPerfection posted a review
1. Recently we added some examples here for the global scope objects: https://gjs.guide/extensions/review-guidelines/review-guidelines.html#only-use-init-for-initialization and for `prefs.js`: to let the garbage collector do its job, you shouldn't leave any instances of objects in global scope. For templates, `Me.path` is ok and you can also use `resource://` which can make porting to 45 much easier. 2. It's ok. 3. Unless there isn't any good reason for using that, it shouldn't be used in extensions. 4. It's ok.
amezin posted a review
> you can also use `resource://` which can make porting to 45 much easier. Does it mean I can load and register resource bundles (not for code, I guess - but for GtkBuilder ui, D-Bus interface definitions, just random text/json data)? Is it allowed in extension itself (i. e. main shell process), not only prefs? > 3. Unless there isn't any good reason for using that, it shouldn't be used in extensions. Extensions in `gnome-shell-extensions` seem to use `settings.run_dispose()` to avoid disconnecting from `changed` signal manually - and I want to use it for the same reason. Just setting `settings` to `null` doesn't immediately destroy the object, and it continues emitting `changed` signals when the extension is already disabled. https://github.com/GNOME/gnome-shell-extensions/commit/631f88ff42c9fed152f483764f3295f95fbcdddd
JustPerfection posted a review
1. Yes, you can do that for ui and image files. You can register on enable and then unregister on disable (extension.js) or window close request (prefs.js). 3. Just nulling them should be enough and as I mentioned before, it should only be called from object system implementations.
amezin posted a review
> 1. Yes, you can do that for ui and image files. What about text, json, css (css loaded by the included application, not extension)? > 3. Just nulling them should be enough and as I mentioned before, But it's not. I removed `run_dispose()`, reloaded the shell. Disabled the extension, opened its preferences, toggled "Behavior -> Exclude from overview/task bar". journalctl: Sep 24 14:57:30 amezin-laptop.home.arpa gnome-shell[640152]: JS ERROR: TypeError: window_manager is null set_skip_taskbar@/home/amezin/.local/share/gnome-shell/extensions/ddterm@amezin.github.com/ddterm/shell/extension.js:365:17 Sep 24 14:57:31 amezin-laptop.home.arpa gnome-shell[640152]: JS ERROR: TypeError: window_manager is null set_skip_taskbar@/home/amezin/.local/share/gnome-shell/extensions/ddterm@amezin.github.com/ddterm/shell/extension.js:365:17 Currently it's the only option affected - but only because everything else is disconnected manually (and I'd like to get rid of that code if possible). https://discourse.gnome.org/t/gio-settings-run-dispose-in-extensions/17361
JustPerfection posted a review
1. Yes, anything other than code is allowed there. 3. Let me try to test it and be back with the result.
JustPerfection posted a review
I don't have any logs after nulling out settings. Please disconnect the signal manually if you have any logs after disable.
JustPerfection posted a review
btw, for GNOME extensions you are referencing: https://gitlab.gnome.org/GNOME/gnome-shell-extensions/-/merge_requests/275