Review of "QuickNotes" version 5

Details Page Preview

A quick notes and tasks panel menu for GNOME Shell with markdown formatting. This extension uses the system clipboard for copy, cut and paste operations.

Extension Homepage
https://gitlab.gnome.org/josechu/quicknotes

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 4 manual_review 1

Shexli found 5 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:359
    St.Clipboard.get_default()
  • extension.js:368
    St.Clipboard.get_default()
  • extension.js:379
    St.Clipboard.get_default()

EGO-A-004 warning

extension files should not contain excessive ungated console logging

File contains 7 ungated console.log/warn/error calls (threshold: 5).

No excessive logging

  • extension.js:124
    console.error(`QuickNotes: Load failed — ${e}`)
  • extension.js:170
    console.error(`QuickNotes: Async save — ${e}`)
  • extension.js:173
    console.error(`QuickNotes: Save error — ${e}`)
  • extension.js:183
    console.error(`QuickNotes: Sync save — ${e}`)
  • extension.js:196
    console.error(`QuickNotes: Bad tab-definitions — ${e}`)
  • extension.js:753
    console.warn(`QuickNotes: Markdown error — ${e}`)
  • extension.js:795
    console.warn(`QuickNotes: Theme colors unavailable — ${e}`)

EGO-L-002 warning

objects created by extension should be destroyed in disable()

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

Destroy all objects

  • extension.js:246
            this._contentArea = new St.BoxLayout({
                orientation: Clutter.Orientation.VERTICAL,
                x_expand: true, y_expand: true,
            })
  • extension.js:676
    this._focusTarget = textActor
  • extension.js:229
    this._headerBar = new St.BoxLayout({ x_expand: true, style: 'padding-bottom: 6px;' })
  • extension.js:224
            this._mainBox = new St.BoxLayout({
                orientation: Clutter.Orientation.VERTICAL,
                style: 'width: 350px; min-height: 250px;',
            })
  • extension.js:677
    this._notesTextActor = textActor
  • extension.js:230
    this._tabBar = new St.BoxLayout({ x_expand: true })

EGO-L-005 warning

owned object references should be released in disable()

Owned references that are cleaned up in `disable()` should also be released with `null` or `undefined`.

Destroy all objects

  • extension.js:246
            this._contentArea = new St.BoxLayout({
                orientation: Clutter.Orientation.VERTICAL,
                x_expand: true, y_expand: true,
            })
  • extension.js:229
    this._headerBar = new St.BoxLayout({ x_expand: true, style: 'padding-bottom: 6px;' })
  • extension.js:224
            this._mainBox = new St.BoxLayout({
                orientation: Clutter.Orientation.VERTICAL,
                style: 'width: 350px; min-height: 250px;',
            })
  • extension.js:230
    this._tabBar = new St.BoxLayout({ x_expand: true })

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:840
                    entry.clutter_text.connect('activate', () => {
                        this._pushUndo();
                        content.tasks.splice(i + 1, 0, '');
                        this._queueSave();
                        renderTasks(i + 1);
                    })
  • extension.js:847
                    entry.clutter_text.connect('key-press-event', (_a, event) => {
                        if (event.get_key_symbol() === Clutter.KEY_BackSpace
                            && entry.get_text() === ''
                            && content.tasks.length > 1) {
                            this._pushUndo();
    
  • extension.js:833
                    entry.clutter_text.connect('text-changed', () => {
                        if (this._isMutating) return;
                        content.tasks[i] = entry.get_text();
                        this._queueSave();
                        this._scheduleUndoCapture();
                    })

All Versions

Version Status
6 Active
5 Rejected
4 Active
3 Rejected
2 Rejected
1 Rejected

Previous Reviews on this Version

GdH rejected
The St Widgets `orientation` property is available since GNOME 48
josechu posted a review
Thanks for your review, you mean that probably my metadata should support from gnome 48 onwards then only? Yes, i think it makes sense. Drop GNOME < 48 support. Set shell-version to ["48", "49", "50", "51"] and keep orientation: Clutter.Orientation.VERTICAL Should i re-submit extension again with changes? or can be done in this review the change?
GdH posted a review
Older GNOME Shell versions would crash because of this. You can either add code to support both properties or remove older versions from the metadata. Please fix this and re-upload the updated package.