From 7064e3b601261994667606c5aa843afb866100f2 Mon Sep 17 00:00:00 2001 From: Leandro Rodrigues Date: Sat, 8 Aug 2026 20:32:47 -0300 Subject: [PATCH] fix(review): address GNOME extension review findings Make main-loop source ownership explicit and keep disposal safe during reentrant cleanup. Move D-Bus interface metadata into runtime owners and make custom command argv flow directly traceable. Add lifecycle coverage for source replacement, bulk cleanup, and reentrant disposal. --- src/core/lifecycleScope.ts | 24 ++++++++++-- src/core/mainLoop.ts | 6 +++ src/desktop/trayIcons/dbusMenu.ts | 6 +-- src/desktop/trayIcons/sniHost.ts | 5 ++- src/panel/auroraMenu.ts | 51 +++++++++++++------------- tests/unit/core/lifecycleScope.test.ts | 29 +++++++++++++++ 6 files changed, 86 insertions(+), 35 deletions(-) diff --git a/src/core/lifecycleScope.ts b/src/core/lifecycleScope.ts index 9f5cde4..bdff0a7 100644 --- a/src/core/lifecycleScope.ts +++ b/src/core/lifecycleScope.ts @@ -50,6 +50,7 @@ class ManagedSourceImpl implements ManagedSource { export class LifecycleScope { private _teardowns: Teardown[] | null = []; + private _sources: ManagedSourceImpl[] | null = []; onDispose(teardown: Teardown): void { if (!this._teardowns) { @@ -71,15 +72,30 @@ export class LifecycleScope { manageSource(remove: SourceRemover): ManagedSource { const source = new ManagedSourceImpl(remove); - this.onDispose(() => source.dispose()); + if (!this._sources) { + source.dispose(); + return source; + } + + this._sources.push(source); return source; } dispose(): void { - if (!this._teardowns) return; - const teardowns = this._teardowns; + if (!this._teardowns || !this._sources) return; + + // Main-loop sources are owned explicitly by the scope. Every module/widget disposes + // its scope from disable()/destroy(), so no timeout or idle callback survives teardown. + const ownedSources = this._sources; + const ownedTeardowns = this._teardowns; + this._sources = null; this._teardowns = null; - for (const teardown of teardowns.reverse()) { + + for (const source of ownedSources.reverse()) { + source.dispose(); + } + + for (const teardown of ownedTeardowns.reverse()) { teardown(); } } diff --git a/src/core/mainLoop.ts b/src/core/mainLoop.ts index e2e11ef..a7a39e0 100644 --- a/src/core/mainLoop.ts +++ b/src/core/mainLoop.ts @@ -7,6 +7,12 @@ export function removeSource(sourceId: number): 0 { return 0; } +/** + * Registers one replaceable GLib source with an owner's LifecycleScope. + * + * ManagedSource.replace() removes the previous source before creating another one, + * and LifecycleScope.dispose() removes every source still active on disable/destroy. + */ export function createManagedSource(scope: LifecycleScope): ManagedSource { return scope.manageSource(removeSource); } diff --git a/src/desktop/trayIcons/dbusMenu.ts b/src/desktop/trayIcons/dbusMenu.ts index 431c42e..75366bd 100644 --- a/src/desktop/trayIcons/dbusMenu.ts +++ b/src/desktop/trayIcons/dbusMenu.ts @@ -31,8 +31,6 @@ const DBUS_MENU_XML = ` `; -const DBusMenuInterfaceInfo = Gio.DBusInterfaceInfo.new_for_xml(DBUS_MENU_XML); - // @ts-ignore — _promisify is a GJS extension not reflected in .d.ts Gio._promisify(Gio.DBusProxy.prototype, 'init_async'); // @ts-ignore @@ -48,6 +46,7 @@ type MenuNode = { }; export class DBusMenuClient { + private readonly _interfaceInfo: Gio.DBusInterfaceInfo; private _proxy: Gio.DBusProxy | null = null; private _busName: string; private _objectPath: string; @@ -55,6 +54,7 @@ export class DBusMenuClient { private _initialized = false; constructor(busName: string, objectPath: string) { + this._interfaceInfo = Gio.DBusInterfaceInfo.new_for_xml(DBUS_MENU_XML); this._busName = busName; this._objectPath = objectPath; this._cancellable = new Gio.Cancellable(); @@ -69,7 +69,7 @@ export class DBusMenuClient { g_name: this._busName, g_object_path: this._objectPath, g_interface_name: DBUS_MENU_IFACE, - g_interface_info: DBusMenuInterfaceInfo, + g_interface_info: this._interfaceInfo, g_flags: Gio.DBusProxyFlags.DO_NOT_LOAD_PROPERTIES, }); diff --git a/src/desktop/trayIcons/sniHost.ts b/src/desktop/trayIcons/sniHost.ts index 1bf934e..b1c5975 100644 --- a/src/desktop/trayIcons/sniHost.ts +++ b/src/desktop/trayIcons/sniHost.ts @@ -37,7 +37,6 @@ const SNI_ITEM_XML = ` `; -const SniItemInterfaceInfo = Gio.DBusInterfaceInfo.new_for_xml(SNI_ITEM_XML); const MIN_PIXMAP_SIZE = 8; const SYMBOLIC_CHANNEL_TOLERANCE = 18; const SYMBOLIC_REQUIRED_RATIO = 0.92; @@ -63,6 +62,7 @@ type SniHostOptions = { }; export class SniHost { + private readonly _itemInterfaceInfo: Gio.DBusInterfaceInfo; private _entries = new Map(); private _pendingRegistrations = new Map(); private _callbacks: HostCallbacks; @@ -71,6 +71,7 @@ export class SniHost { private _shouldRecolorSymbolicPixmaps: () => boolean; constructor(watcher: SniWatcher, callbacks: HostCallbacks, options: SniHostOptions = {}) { + this._itemInterfaceInfo = Gio.DBusInterfaceInfo.new_for_xml(SNI_ITEM_XML); this._watcher = watcher; this._callbacks = callbacks; this._getColorScheme = options.getColorScheme || (() => 'prefer-dark'); @@ -86,7 +87,7 @@ export class SniHost { const proxy = new Gio.DBusProxy({ g_connection: Gio.DBus.session, g_interface_name: 'org.kde.StatusNotifierItem', - g_interface_info: SniItemInterfaceInfo, + g_interface_info: this._itemInterfaceInfo, g_name: busName, g_object_path: objectPath, g_flags: Gio.DBusProxyFlags.GET_INVALIDATED_PROPERTIES, diff --git a/src/panel/auroraMenu.ts b/src/panel/auroraMenu.ts index 8855587..faaa87d 100644 --- a/src/panel/auroraMenu.ts +++ b/src/panel/auroraMenu.ts @@ -52,11 +52,10 @@ const MENU_ICONS = { type MenuIconKey = keyof typeof MENU_ICONS; -type MenuCommand = { +type MenuAction = { title: string; iconName: string; - argv?: string[]; - activate?: () => void; + activate: () => void; }; type RecentSubmenuItem = PopupMenu.PopupSubMenuMenuItem & { @@ -183,10 +182,10 @@ export class AuroraMenu extends Module { menu.removeAll(); this._lockMenuWidth(menu); - let hasItems = this._addCommandIfVisible(menu, SHOW_ABOUT_KEY, { + let hasItems = this._addActionIfVisible(menu, SHOW_ABOUT_KEY, { title: _('About This PC'), - argv: ['gnome-control-center', 'about'], iconName: 'help-about-symbolic', + activate: () => this._spawn(['gnome-control-center', 'about']), }); const filesAdded = await this._addSection( @@ -194,16 +193,17 @@ export class AuroraMenu extends Module { hasItems, [ () => - this._addCommandIfVisible(menu, SHOW_HOME_KEY, { + this._addActionIfVisible(menu, SHOW_HOME_KEY, { title: _('Home Folder'), - argv: ['xdg-open', GLib.get_home_dir()], iconName: 'user-home-symbolic', + activate: () => this._spawn(['xdg-open', GLib.get_home_dir()]), }), () => - this._addCommandIfVisible(menu, SHOW_DOWNLOADS_KEY, { + this._addActionIfVisible(menu, SHOW_DOWNLOADS_KEY, { title: _('Downloads'), - argv: ['xdg-open', this._getDownloadsDirectory() || GLib.get_home_dir()], iconName: 'folder-download-symbolic', + activate: () => + this._spawn(['xdg-open', this._getDownloadsDirectory() || GLib.get_home_dir()]), }), () => this._addRecentItems(menu, cancellable), ], @@ -218,19 +218,20 @@ export class AuroraMenu extends Module { hasItems, [ () => - this._addCommandIfVisible(menu, SHOW_SETTINGS_KEY, { + this._addActionIfVisible(menu, SHOW_SETTINGS_KEY, { title: _('System Settings'), - argv: ['gnome-control-center'], iconName: 'emblem-system-symbolic', + activate: () => this._spawn(['gnome-control-center']), }), () => - this._addCommandIfVisible(menu, SHOW_SOFTWARE_KEY, { + this._addActionIfVisible(menu, SHOW_SOFTWARE_KEY, { title: _('Software'), - argv: this._parseCommand(APP_STORE_COMMAND_KEY, ['gnome-software']), iconName: 'system-software-install-symbolic', + activate: () => + this._spawn(this._parseCommand(APP_STORE_COMMAND_KEY, ['gnome-software'])), }), () => - this._addCommandIfVisible(menu, SHOW_EXTENSIONS_KEY, { + this._addActionIfVisible(menu, SHOW_EXTENSIONS_KEY, { title: _('Extensions'), iconName: 'application-x-addon-symbolic', activate: () => this._openExtensionsManager(), @@ -247,24 +248,21 @@ export class AuroraMenu extends Module { ); } - private _addCommand(menu: PopupMenu.PopupMenu, command: MenuCommand): void { - const item = new PopupMenu.PopupMenuItem(command.title); - this._decorateItem(item, command.iconName); - item.connect('activate', () => { - if (command.activate) command.activate(); - else if (command.argv) this._spawn(command.argv); - }); + private _addAction(menu: PopupMenu.PopupMenu, action: MenuAction): void { + const item = new PopupMenu.PopupMenuItem(action.title); + this._decorateItem(item, action.iconName); + item.connect('activate', action.activate); menu.addMenuItem(item); } - private _addCommandIfVisible( + private _addActionIfVisible( menu: PopupMenu.PopupMenu, visibleKey: string, - command: MenuCommand, + action: MenuAction, ): boolean { if (!this.context.settings.getBoolean(visibleKey)) return false; - this._addCommand(menu, command); + this._addAction(menu, action); return true; } @@ -302,10 +300,11 @@ export class AuroraMenu extends Module { const argv = this._parseCommandLine(command.command, CUSTOM_ITEMS_KEY); if (argv.length === 0) continue; - this._addCommand(menu, { + this._addAction(menu, { title: command.label, - argv, iconName: 'application-x-executable-symbolic', + // The final argv is exactly the result of GLib.shell_parse_argv(); no shell is involved. + activate: () => this._spawn(argv), }); added = true; } diff --git a/tests/unit/core/lifecycleScope.test.ts b/tests/unit/core/lifecycleScope.test.ts index 23da190..aee2b67 100644 --- a/tests/unit/core/lifecycleScope.test.ts +++ b/tests/unit/core/lifecycleScope.test.ts @@ -85,6 +85,35 @@ test('LifecycleScope — disposes an active managed source', () => { assert.deepEqual(removed, [9]); }); +test('LifecycleScope — disposes every active source in reverse registration order', () => { + const removed: number[] = []; + const scope = new LifecycleScope(); + const first = scope.manageSource((id) => removed.push(id)); + const second = scope.manageSource((id) => removed.push(id)); + + first.replace(() => 1); + second.replace(() => 2); + scope.dispose(); + + assert.deepEqual(removed, [2, 1]); +}); + +test('LifecycleScope — remains idempotent when a source remover reenters dispose', () => { + const events: string[] = []; + const scope = new LifecycleScope(); + const source = scope.manageSource((id) => { + events.push(`remove:${id}`); + scope.dispose(); + }); + scope.onDispose(() => events.push('teardown')); + source.replace(() => 5); + + assert.doesNotThrow(() => scope.dispose()); + scope.dispose(); + + assert.deepEqual(events, ['remove:5', 'teardown']); +}); + test('LifecycleScope — does not create a managed source after disposal', () => { const removed: number[] = []; let created = false;