Skip to content

Fix extensions.gnome.org review rejection on GNOME 51 - #325

Closed
mohammaddimanpodu wants to merge 1 commit into
dsheeler:mainfrom
mohammaddimanpodu:fix/ego-review-gnome51
Closed

mohammaddimanpodu wants to merge 1 commit into
dsheeler:mainfrom
mohammaddimanpodu:fix/ego-review-gnome51

Conversation

@mohammaddimanpodu

Copy link
Copy Markdown

Reviewer JustPerfection rejected v90 (shell-version 51) on 2026-11-04: https://extensions.gnome.org/review/77811

Addresses all four findings plus the Shexli warnings:

  • Timeout (manager.js) the 1s unminimize fallback timeout is now also removed when the window actor is destroyed, and disconnected inside restore().
  • Selective disable (preview.js) removed the _cleanedUp early-return guard in cleanup(); teardown always runs fully. All _destroying || _cleanedUp guards reduced to _destroying.
  • Unnecessary try/catch (preview.js) dropped around this.source = null and in _unbindAll().
  • Best-practices sweep removed blanket try/catch in the D-Bus next/prev/select handlers (guarded on switcher existence) and around GSettings reads in platform.js.

Shexli warnings:

  • EGO-X-004 (manager.js) D-Bus interface XML loaded with load_contents_async instead of synchronous load_contents.
  • EGO-L-005 / EGO-L-003 (swipeTracker.js) scroll-event signal ids and the changed::natural-scroll setting signal are disconnected in destroy(); _touchpadSettings/_mouseSettings released with null. Bonus fix: TouchpadSwipeGesture.destroy() disconnected from global.stage although the signal was connected on the actor passed in.
  • switcher.js hidden-window notify::visible signals tracked with connectObject(..., this) (no manual ids + try/catch disconnect); also removed an accidental duplicate push of the same actor.

Syntax-checked all modules (node --check, ESM) and packed with make build. Only smoke-level verification; would appreciate a runtime test on a GNOME 51 session.

Reviewer (JustPerfection) rejected v90 with 4 findings, plus
Shexli warnings on the same submission. This addresses all of them:

- manager.js: remove the 1s unminimize-timeout when the window actor
  is destroyed, not only on map/timeout (EGO Review Guidelines: Timeout).
- manager.js: load the D-Bus interface XML asynchronously
  (load_contents_async) instead of Gio.File.load_contents (EGO-X-004).
- manager.js: drop blanket try/catch in the D-Bus next/previous/select
  handlers; guard on switcher existence instead.
- preview.js: remove the selective-disable guard in cleanup()
  (early return on _cleanedUp) so teardown always runs fully.
- preview.js: remove unnecessary try/catch around 'this.source = null'
  and _unbindAll() unbind calls.
- swipeTracker.js: disconnect scroll-event signals in destroy()
  (ScrollGesture, MouseScroll) and null the Gio.Settings refs; also fix
  TouchpadSwipeGesture.destroy() disconnecting from global.stage when
  the signal was connected on the actor passed to the constructor.
- platform.js: drop the blanket try/catch around GSettings reads.
- switcher.js: track hidden-window 'notify::visible' signals with
  connectObject(this) instead of manual ids + try/catch disconnect;
  remove an accidental duplicate push of the same actor.
@mohammaddimanpodu

Copy link
Copy Markdown
Author

Closing — will rework offline. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant