Review of "Wisp" version 1.0.6 (3)

Details Page Preview

A minimal, good-looking way to manage snapper snapshots, from the top bar. See every snapshot, take one, delete what you don't want. Open a snapshot to see what changed in it, put single files back, or roll the whole system over to it. Preferences hold the rest of snapper: retention limits, timers, btrfs maintenance and disk use. Wisp runs nothing as root: what needs root is shown as a command to copy and run in a terminal. The clipboard is touched only when asked: a copy button puts the command it shows there, and the text fields in the dialogs take copy and paste. Needs snapper and a filesystem it manages. GNOME 46 and later.

Extension Homepage
https://github.com/epogonii/wisp

No comments.

Diff Against

Files

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

Shexli (experimental) warning 2 manual_review 1

Shexli found 3 issues that may need reviewer attention.

EGO-A-005 manual_review

extensions should not access the clipboard directly

Direct clipboard access via `St.Clipboard.get_default()` requires reviewer scrutiny.

Review Guidelines

  • extension.js:374
    St.Clipboard.get_default()
  • lib/compare.js:320
    St.Clipboard.get_default()
  • lib/keys.js:89
    St.Clipboard.get_default()
  • lib/keys.js:96
    St.Clipboard.get_default()

EGO-L-003 warning

signals connected by extension should be disconnected in disable()

Signals assigned in `enable()` are missing matching disconnect calls in `disable()` or its helper methods.

Disconnect all signals

  • extension.js:420
                this._clickGesture?.connect('recognize', gesture =>
                    this._clicked(gesture.get_button?.() ?? Clutter.BUTTON_PRIMARY))
  • extension.js:443
            this.menu.connect('open-state-changed', (_menu, open) => {
                if (open) {
                    this._rebuild();
                } else {
                    this._shown.clear();
                    this._lock.relock();
                }
            })
  • extension.js:641
                row.connect('key-focus-in', () => {
                    if (row.hover)
                        return;
                    ensureActorVisibleInScrollView(scroll, row);
                })
  • lib/compare.js:157
    actor.connect('key-focus-in', reveal)
  • lib/compare.js:158
    actor.connect('key-focus-out', reveal)
  • lib/dialog.js:160
    this._aside.connect('destroy', () => (this._aside = null))

EGO-L-004 warning

main loop sources should be removed in disable()

Main loop sources assigned in `enable()` are missing matching removals in `disable()` or its helper methods.

Remove main loop sources

  • extension.js:167
            this._freshId = GLib.timeout_add_seconds(GLib.PRIORITY_DEFAULT,
                Math.ceil(FRESH - age), () => {
                    this._freshId = 0;
                    this.remove_style_class_name('wisp-fresh');
                    return GLib.SOURCE_REMOVE;
                })
  • lib/dialog.js:215
            this._pending = GLib.idle_add(GLib.PRIORITY_DEFAULT, () => {
                this._pending = 0;
                this._measure();
                return GLib.SOURCE_REMOVE;
            })
  • lib/dialog.js:162
                this._ticking = GLib.timeout_add_seconds(GLib.PRIORITY_DEFAULT, 1,
                    () => {
                        this._say();
                        return GLib.SOURCE_CONTINUE;
                    })

All Versions

Version Status
1.0.10 (6) Active
1.0.9 (5) Rejected
1.0.8 (4) Rejected
1.0.6 (3) Rejected
1.0.4 (2) Rejected
1.0.3 (1) Rejected

Previous Reviews on this Version

JustPerfection rejected
1. Timeout should be removed before creating a new one (line 167 `extension.js`): [EGO Review Guidelines: Timeout](https://gjs.guide/extensions/review-guidelines/review-guidelines.html#remove-main-loop-sources) 2. `this._destroyed` is a bad practice. Please remove that: - [Extensions Best Practices Guidelines](https://gjs.guide/extensions/review-guidelines/best-practices.html#lifecycle-and-destruction-state) - [EGO Review Guidelines: AI](https://gjs.guide/extensions/review-guidelines/review-guidelines.html#extensions-must-not-be-ai-generated) 3. `pkexec` commands here are not static. You shouldn't dynamically run privileged spawn commands like that. Most parts of this extension should be moved to an app. Shouldn't be inside the shell process. That app can be a dependency of this extension. Then they can communicate via [D-Bus](https://gjs.guide/guides/gio/dbus.html)