From 9fb25c94d67393bfea99911e8c3927648f8c81f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Fri, 4 Jun 2021 17:08:13 +0200 Subject: [PATCH 01/16] test: Move unit tests into the unit suite It allows to run them easily --- tests/meson.build | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/meson.build b/tests/meson.build index 85339dd141..37b3edf99d 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -32,6 +32,7 @@ tests = [ foreach test : tests test(test, run_test, args: 'unit/@0@.js'.format(test), + suite: ['unit', 'js'], workdir: meson.current_source_dir()) endforeach -- GitLab From 725ddae3161962dd07570d2eb8f002242b5c68b7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Thu, 10 Nov 2022 11:03:52 +0100 Subject: [PATCH 02/16] tests/run-test.sh: Remove libtool usage in debug mode --- tests/run-test.sh.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/run-test.sh.in b/tests/run-test.sh.in index ea6d157261..71fd8c3ea4 100755 --- a/tests/run-test.sh.in +++ b/tests/run-test.sh.in @@ -11,7 +11,7 @@ debug= for arg in $@ ; do case $arg in -g|--debug) - debug="libtool --mode=execute gdb --args" + debug="gdb --args" ;; -v|--verbose) verbose=true -- GitLab From 0913da58119e0c932f458cc231619c644f871acb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Mon, 14 Nov 2022 18:05:47 +0100 Subject: [PATCH 03/16] tests/run-tests.sh: Use G_DEBUG=fatal-warnings by default While the JS suite fails on exceptions that are thrown by code that is directly called from javascript calls, it may not happen if that's called from C code, as it's in the case of native signal callbacks. In such cases in fact we do get a javascript error, but that's not enough to stop the tests execution. To ensure this, do not allow any GLib warnings or critical errors to happen in tests without being explicitly handled (e.g. GLib.test_expect_message). --- tests/run-test.sh.in | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/tests/run-test.sh.in b/tests/run-test.sh.in index 71fd8c3ea4..a28be68e9a 100755 --- a/tests/run-test.sh.in +++ b/tests/run-test.sh.in @@ -34,11 +34,20 @@ GI_TYPELIB_PATH="$GI_TYPELIB_PATH${GI_TYPELIB_PATH:+:}@MUTTER_TYPELIB_DIR@:$buil GJS_PATH="$srcdir:$srcdir/../js:$builddir/../js" GJS_DEBUG_OUTPUT=stderr $verbose || GJS_DEBUG_TOPICS="JS ERROR;JS LOG" +G_DEBUG=fatal-warnings GNOME_SHELL_TESTSDIR="$srcdir/" GNOME_SHELL_JS="$srcdir/../js" GNOME_SHELL_DATADIR="$builddir/../data" -export GI_TYPELIB_PATH GJS_PATH GJS_DEBUG_OUTPUT GJS_DEBUG_TOPICS GNOME_SHELL_TESTSDIR GNOME_SHELL_JS GNOME_SHELL_DATADIR LD_PRELOAD +export GI_TYPELIB_PATH \ + GJS_DEBUG_OUTPUT \ + GJS_DEBUG_TOPICS \ + GJS_PATH \ + GNOME_SHELL_DATADIR \ + GNOME_SHELL_JS \ + GNOME_SHELL_TESTSDIR \ + G_DEBUG \ + LD_PRELOAD for test in $tests ; do $debug $builddir/../src/run-js-test $test || exit $? -- GitLab From fc63811ce45e63f09b7829cd8bd764fe0cf6f8c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Mon, 14 Nov 2022 04:09:41 +0100 Subject: [PATCH 04/16] testUtils: Add test utility functions to improve JsUnit capabilities It may required at times to repeat in JS unit tests some repeated checks and that can be easily factorized in a tiny library. So add some utility functions to test some common code paths. --- tests/unit/testUtils.js | 70 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+) create mode 100644 tests/unit/testUtils.js diff --git a/tests/unit/testUtils.js b/tests/unit/testUtils.js new file mode 100644 index 0000000000..0e623b56ea --- /dev/null +++ b/tests/unit/testUtils.js @@ -0,0 +1,70 @@ +// -*- mode: js; js-indent-level: 4; indent-tabs-mode: nil -*- +/* exported testCase, assertNotReached, checkIsNotReachedError, + assertRaisesError, assertArrayEquals */ + +const {jsUnit: JsUnit} = imports; + +class NotReachedError extends Error {} + +/** + * Runs test cases + * + * @param {string} name - The name of the test case + * @param {Function} test - The test function to execute + */ +function testCase(name, test) { + print(`Running test ${name}`); + test(); +} + +/** + * Deeply compares two arrays for equality + * + * @param {Array} array1 - The array to compare + * @param {Array} array2 - The array to compare + */ +function assertArrayEquals(array1, array2) { + JsUnit.assertEquals(array1.length, array2.length); + for (let j = 0; j < array1.length; j++) + JsUnit.assertEquals(array1[j], array2[j]); +} + +/** + * Ensures this code is not reached, throwing an error in case. + */ +function assertNotReached() { + const error = new NotReachedError('This must not be reached'); + throw error; +} + +/** + * Verifies that an error is not thrown by assertNotReached() + * + * @param {Error} error - An error to check + */ +function checkIsNotReachedError(error) { + JsUnit.assertFalse(error instanceof NotReachedError); +} + +/** + * Check if the function call would raise an error, comparing the result + * + * @param {Function} func - The function to verify + * @param {(Error|string)} [error] - An error or an error message to compare + */ +function assertRaisesError(func, error = undefined) { + if (!(func instanceof Function)) + throw new Error('Argument must be a function'); + + try { + func(); + assertNotReached(); + } catch (e) { + checkIsNotReachedError(e); + + if (error instanceof Error) + JsUnit.assertEquals(error, e); + else if (typeof error === 'string') + JsUnit.assertTrue(e.message.includes(error)); + } +} -- GitLab From 34451fca1d0be82f7ac399c459cff3bf5db03931 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Mon, 14 Nov 2022 04:16:59 +0100 Subject: [PATCH 05/16] tests/signals: Add some basic signals tests Add some unit tests for the shell signals implementations, so ensuring all the main API expectations. --- tests/meson.build | 1 + tests/unit/signals.js | 73 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+) create mode 100644 tests/unit/signals.js diff --git a/tests/meson.build b/tests/meson.build index 37b3edf99d..38bd444ac7 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -25,6 +25,7 @@ tests = [ 'markup', 'params', 'signalTracker', + 'signals', 'url', 'versionCompare', ] diff --git a/tests/unit/signals.js b/tests/unit/signals.js new file mode 100644 index 0000000000..ceb48eb850 --- /dev/null +++ b/tests/unit/signals.js @@ -0,0 +1,73 @@ +// -*- mode: js; js-indent-level: 4; indent-tabs-mode: nil -*- + +const {jsUnit: JsUnit} = imports; +const {testUtils: TestUtils} = imports.unit; +const {testCase} = TestUtils; +const {signals: Signals} = imports.misc; + +const Environment = imports.ui.environment; + +Environment.init(); + +testCase('EventEmitter simple connections', () => { + let fooCalled = 0, barCalled = 0; + const emitter = new Signals.EventEmitter(); + const idFoo = emitter.connect('foo', () => ++fooCalled); + const idBar = emitter.connect('bar', () => ++barCalled); + + emitter.emit('foo'); + JsUnit.assertEquals(1, fooCalled); + JsUnit.assertEquals(0, barCalled); + + emitter.disconnect(idBar); + emitter.emit('bar'); + JsUnit.assertEquals(0, barCalled); + + emitter.disconnect(idFoo); +}); + +testCase('EventEmitter callbacks blocked', () => { + let fooCalled = false, foo2Called = false; + const emitter = new Signals.EventEmitter(); + + emitter.connect('foo', () => { + fooCalled = true; + return true; + }); + emitter.connect('foo', () => (foo2Called = true)); + + emitter.emit('foo'); + + JsUnit.assertTrue(fooCalled); + JsUnit.assertFalse(foo2Called); +}); + +testCase('EventEmitter connections', () => { + let fooCalled = 0, barCalled = 0; + const emitter = new Signals.EventEmitter(); + emitter.disconnect(emitter.connect('foo', () => TestUtils.assertNotReached())); + const idFoo = emitter.connect('foo', (self, ...args) => { + JsUnit.assertEquals(self, emitter); + TestUtils.assertArrayEquals(args, ['args', 5, null, emitter]); + fooCalled++; + }); + const idBar = emitter.connect('bar', () => ++barCalled); + + JsUnit.assertTrue(emitter.signalHandlerIsConnected(idFoo)); + JsUnit.assertTrue(emitter.signalHandlerIsConnected(idBar)); + + emitter.emit('foo', 'args', 5, null, emitter); + JsUnit.assertEquals(1, fooCalled); + JsUnit.assertEquals(0, barCalled); + + emitter.disconnect(idBar); + JsUnit.assertTrue(emitter.signalHandlerIsConnected(idFoo)); + JsUnit.assertFalse(emitter.signalHandlerIsConnected(idBar)); + + emitter.emit('bar'); + JsUnit.assertEquals(0, barCalled); + + emitter.disconnect(idFoo); + JsUnit.assertFalse(emitter.signalHandlerIsConnected(idFoo)); + JsUnit.assertFalse(emitter.signalHandlerIsConnected(idBar)); +}); -- GitLab From e42f4c64bc14cf8d8292474af4dda2b1f27b51b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Mon, 14 Nov 2022 04:29:39 +0100 Subject: [PATCH 06/16] tests/signalTracker: Move the main test into a testCase scope It will make things clearer and variables more scoped when we'll add more tests to this file. --- tests/unit/signalTracker.js | 125 +++++++++++++++++++----------------- 1 file changed, 65 insertions(+), 60 deletions(-) diff --git a/tests/unit/signalTracker.js b/tests/unit/signalTracker.js index 7943d0a20f..d017c41cf7 100644 --- a/tests/unit/signalTracker.js +++ b/tests/unit/signalTracker.js @@ -4,6 +4,9 @@ const { GObject } = imports.gi; +const {testUtils: TestUtils} = imports.unit; +const {testCase} = TestUtils; + const JsUnit = imports.jsUnit; const Signals = imports.misc.signals; @@ -21,95 +24,97 @@ const GObjectEmitter = GObject.registerClass({ Signals: { 'signal': {} }, }, class GObjectEmitter extends Destroyable {}); -const emitter1 = new Signals.EventEmitter(); -const emitter2 = new GObjectEmitter(); +testCase('Signal emissions can be tracked', () => { + const emitter1 = new Signals.EventEmitter(); + const emitter2 = new GObjectEmitter(); -const tracked1 = new Destroyable(); -const tracked2 = {}; + const tracked1 = new Destroyable(); + const tracked2 = {}; -let count = 0; -const handler = () => count++; + let count = 0; + const handler = () => count++; -emitter1.connectObject('signal', handler, tracked1); -emitter2.connectObject('signal', handler, tracked1); + emitter1.connectObject('signal', handler, tracked1); + emitter2.connectObject('signal', handler, tracked1); -emitter1.connectObject('signal', handler, tracked2); -emitter2.connectObject('signal', handler, tracked2); + emitter1.connectObject('signal', handler, tracked2); + emitter2.connectObject('signal', handler, tracked2); -JsUnit.assertEquals(count, 0); + JsUnit.assertEquals(count, 0); -emitter1.emit('signal'); -emitter2.emit('signal'); + emitter1.emit('signal'); + emitter2.emit('signal'); -JsUnit.assertEquals(count, 4); + JsUnit.assertEquals(count, 4); -tracked1.emit('destroy'); + tracked1.emit('destroy'); -emitter1.emit('signal'); -emitter2.emit('signal'); + emitter1.emit('signal'); + emitter2.emit('signal'); -JsUnit.assertEquals(count, 6); + JsUnit.assertEquals(count, 6); -emitter1.disconnectObject(tracked2); -emitter2.emit('destroy'); + emitter1.disconnectObject(tracked2); + emitter2.emit('destroy'); -emitter1.emit('signal'); -emitter2.emit('signal'); + emitter1.emit('signal'); + emitter2.emit('signal'); -JsUnit.assertEquals(count, 6); + JsUnit.assertEquals(count, 6); -emitter1.connectObject( - 'signal', handler, - 'signal', handler, GObject.ConnectFlags.AFTER, - tracked1); -emitter2.connectObject( - 'signal', handler, - 'signal', handler, GObject.ConnectFlags.AFTER, - tracked1); + emitter1.connectObject( + 'signal', handler, + 'signal', handler, GObject.ConnectFlags.AFTER, + tracked1); + emitter2.connectObject( + 'signal', handler, + 'signal', handler, GObject.ConnectFlags.AFTER, + tracked1); -emitter1.emit('signal'); -emitter2.emit('signal'); + emitter1.emit('signal'); + emitter2.emit('signal'); -JsUnit.assertEquals(count, 10); + JsUnit.assertEquals(count, 10); -tracked1.emit('destroy'); -emitter1.emit('signal'); -emitter2.emit('signal'); + tracked1.emit('destroy'); + emitter1.emit('signal'); + emitter2.emit('signal'); -JsUnit.assertEquals(count, 10); + JsUnit.assertEquals(count, 10); -emitter1.connectObject('signal', handler, tracked1); -emitter2.connectObject('signal', handler, tracked1); + emitter1.connectObject('signal', handler, tracked1); + emitter2.connectObject('signal', handler, tracked1); -transientHolder = new TransientSignalHolder(tracked1); + let transientHolder = new TransientSignalHolder(tracked1); -emitter1.connectObject('signal', handler, transientHolder); -emitter2.connectObject('signal', handler, transientHolder); + emitter1.connectObject('signal', handler, transientHolder); + emitter2.connectObject('signal', handler, transientHolder); -emitter1.emit('signal'); -emitter2.emit('signal'); + emitter1.emit('signal'); + emitter2.emit('signal'); -JsUnit.assertEquals(count, 14); + JsUnit.assertEquals(count, 14); -transientHolder.destroy(); + transientHolder.destroy(); -emitter1.emit('signal'); -emitter2.emit('signal'); + emitter1.emit('signal'); + emitter2.emit('signal'); -JsUnit.assertEquals(count, 16); + JsUnit.assertEquals(count, 16); -transientHolder = new TransientSignalHolder(tracked1); + transientHolder = new TransientSignalHolder(tracked1); -emitter1.connectObject('signal', handler, transientHolder); -emitter2.connectObject('signal', handler, transientHolder); + emitter1.connectObject('signal', handler, transientHolder); + emitter2.connectObject('signal', handler, transientHolder); -emitter1.emit('signal'); -emitter2.emit('signal'); + emitter1.emit('signal'); + emitter2.emit('signal'); -JsUnit.assertEquals(count, 20); + JsUnit.assertEquals(count, 20); -tracked1.emit('destroy'); -emitter1.emit('signal'); -emitter2.emit('signal'); + tracked1.emit('destroy'); + emitter1.emit('signal'); + emitter2.emit('signal'); -JsUnit.assertEquals(count, 20); + JsUnit.assertEquals(count, 20); +}); -- GitLab From c943ad71ab6797961637b678e755c9a6667abc20 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Mon, 14 Nov 2022 05:35:07 +0100 Subject: [PATCH 07/16] tests/signalTracker: Add more test cases for signal connections Verify that signal connections work as expected respecting both passed arguments and connections flags. --- tests/unit/signalTracker.js | 250 ++++++++++++++++++++++++++++++++++++ 1 file changed, 250 insertions(+) diff --git a/tests/unit/signalTracker.js b/tests/unit/signalTracker.js index d017c41cf7..52280c39e5 100644 --- a/tests/unit/signalTracker.js +++ b/tests/unit/signalTracker.js @@ -24,6 +24,103 @@ const GObjectEmitter = GObject.registerClass({ Signals: { 'signal': {} }, }, class GObjectEmitter extends Destroyable {}); +const GObjectEmitterInt = GObject.registerClass({ + Signals: {'signal-int': {param_types: [GObject.TYPE_INT]}}, +}, class GObjectEmitterInt extends GObjectEmitter {}); + + +function hasSignalHandler(object, signalId) { + if (!(object instanceof GObject.Object)) + throw new Error('Unsupported Object'); + + return !!GObject.signal_handler_find(object, {signalId}); +} + +testCase('Regular JS Objects cannot be registered as Destroyable types', () => { + JsUnit.assertRaises(() => registerDestroyableType({}.constructor.prototype)); + JsUnit.assertRaises(() => registerDestroyableType(class {})); +}); + +testCase('Signals.EventEmitter cannot be registered as Destroyable types', () => { + class BadJSDestroyable extends Signals.EventEmitter {} + JsUnit.assertRaises(() => registerDestroyableType(BadJSDestroyable)); +}); + +testCase('TransientSignalHolder monitors destruction of owned object', () => { + let ownedDestroyCalled = false; + const owner = new Destroyable(); + const owned = new TransientSignalHolder(owner); + + owned.connectObject('destroy', () => (ownedDestroyCalled = true), owner); + JsUnit.assertTrue(hasSignalHandler(owner, 'destroy')); + JsUnit.assertTrue(hasSignalHandler(owned, 'destroy')); + + owned.emit('destroy'); + JsUnit.assertTrue(ownedDestroyCalled); + + JsUnit.assertFalse(hasSignalHandler(owner, 'destroy')); + JsUnit.assertFalse(hasSignalHandler(owned, 'destroy')); +}); + +testCase('TransientSignalHolder monitors destruction of owned object without being destroyed', () => { + let ownedDestroyCalled = false; + let ownerDestroyCalled = false; + const owner = new Destroyable(); + const owned = new TransientSignalHolder(owner); + + owned.connectObject('destroy', () => (ownedDestroyCalled = true), owner); + JsUnit.assertTrue(hasSignalHandler(owner, 'destroy')); + JsUnit.assertTrue(hasSignalHandler(owned, 'destroy')); + + owner.connectObject('destroy', () => (ownerDestroyCalled = true)); + + owned.emit('destroy'); + JsUnit.assertTrue(ownedDestroyCalled); + JsUnit.assertFalse(ownerDestroyCalled); + + JsUnit.assertTrue(hasSignalHandler(owner, 'destroy')); + JsUnit.assertFalse(hasSignalHandler(owned, 'destroy')); +}); + +testCase('TransientSignalHolder is destroyed on owner destruction', () => { + let ownedDestroyCalled = false; + let ownerDestroyCalled = false; + const owner = new Destroyable(); + const owned = new TransientSignalHolder(owner); + + owned.connectObject('destroy', () => (ownedDestroyCalled = true), owner); + owner.connectObject('destroy', () => (ownerDestroyCalled = true)); + JsUnit.assertTrue(hasSignalHandler(owner, 'destroy')); + JsUnit.assertTrue(hasSignalHandler(owned, 'destroy')); + + owner.emit('destroy'); + JsUnit.assertTrue(ownedDestroyCalled); + JsUnit.assertTrue(ownerDestroyCalled); + + JsUnit.assertFalse(hasSignalHandler(owner, 'destroy')); + JsUnit.assertFalse(hasSignalHandler(owned, 'destroy')); +}); + +testCase('TransientSignalHolder owner destruction keeps early monitored destructions', () => { + let ownedDestroyCalled = false; + let ownerDestroyCalled = false; + const owner = new Destroyable(); + + owner.connectObject('destroy', () => (ownerDestroyCalled = true)); + + const owned = new TransientSignalHolder(owner); + owned.connectObject('destroy', () => (ownedDestroyCalled = true), owner); + JsUnit.assertTrue(hasSignalHandler(owner, 'destroy')); + JsUnit.assertTrue(hasSignalHandler(owned, 'destroy')); + + owner.emit('destroy'); + JsUnit.assertTrue(ownedDestroyCalled); + JsUnit.assertTrue(ownerDestroyCalled); + + JsUnit.assertFalse(hasSignalHandler(owner, 'destroy')); + JsUnit.assertFalse(hasSignalHandler(owned, 'destroy')); +}); + testCase('Signal emissions can be tracked', () => { const emitter1 = new Signals.EventEmitter(); const emitter2 = new GObjectEmitter(); @@ -118,3 +215,156 @@ testCase('Signal emissions can be tracked', () => { JsUnit.assertEquals(count, 20); }); + +testCase('Fails with unknown flags', () => { + const obj = new Signals.EventEmitter(); + TestUtils.assertRaisesError(() => obj.connectObject('signal', () => {}, 256, {}), + 'Invalid flag value 256'); +}); + +testCase('Emitter is same of tracker', () => { + const obj = new GObjectEmitter(); + let callbackCalled = false; + + obj.connectObject('signal', () => (callbackCalled = true), obj); + JsUnit.assertTrue(hasSignalHandler(obj, 'signal')); + JsUnit.assertTrue(hasSignalHandler(obj, 'destroy')); + + obj.emit('signal'); + JsUnit.assertTrue(callbackCalled); + JsUnit.assertTrue(hasSignalHandler(obj, 'signal')); + JsUnit.assertTrue(hasSignalHandler(obj, 'destroy')); + + obj.emit('destroy'); + JsUnit.assertFalse(hasSignalHandler(obj, 'signal')); + JsUnit.assertFalse(hasSignalHandler(obj, 'destroy')); +}); + +testCase('Emitter is same of tracker after', () => { + const obj = new GObjectEmitter(); + let callbackCalled = false; + + obj.connectObject('destroy', () => (callbackCalled = true), + GObject.ConnectFlags.AFTER, obj); + JsUnit.assertTrue(hasSignalHandler(obj, 'destroy')); + + obj.emit('destroy'); + JsUnit.assertTrue(callbackCalled); + JsUnit.assertFalse(hasSignalHandler(obj, 'destroy')); +}); + +testCase('Emitter is disconnected on tracker destruction', () => { + const obj = new GObjectEmitter(); + const tracker = new Destroyable(); + let callbackCalled = false; + + obj.connectObject('signal', () => (callbackCalled = true), tracker); + JsUnit.assertTrue(hasSignalHandler(obj, 'signal')); + JsUnit.assertTrue(hasSignalHandler(obj, 'destroy')); + JsUnit.assertTrue(hasSignalHandler(tracker, 'destroy')); + + obj.emit('signal'); + JsUnit.assertTrue(callbackCalled); + JsUnit.assertTrue(hasSignalHandler(obj, 'signal')); + JsUnit.assertTrue(hasSignalHandler(obj, 'destroy')); + JsUnit.assertTrue(hasSignalHandler(tracker, 'destroy')); + + tracker.emit('destroy'); + JsUnit.assertFalse(hasSignalHandler(obj, 'signal')); + JsUnit.assertFalse(hasSignalHandler(obj, 'destroy')); + JsUnit.assertFalse(hasSignalHandler(tracker, 'destroy')); +}); + +testCase('Emitter with no tracker, disconnects on destruction', () => { + const obj = new GObjectEmitter(); + let callbackCalled = false; + + obj.connectObject('signal', () => (callbackCalled = true)); + JsUnit.assertTrue(hasSignalHandler(obj, 'signal')); + JsUnit.assertTrue(hasSignalHandler(obj, 'destroy')); + + obj.emit('signal'); + JsUnit.assertTrue(callbackCalled); + + obj.emit('destroy'); + JsUnit.assertFalse(hasSignalHandler(obj, 'signal')); + JsUnit.assertFalse(hasSignalHandler(obj, 'destroy')); +}); + +testCase('Emitter with empty tracker, disconnects on disconnectObject', () => { + const obj = new GObjectEmitter(); + const tracker = {}; + + let callbackCalled = false; + obj.connectObject('signal', () => (callbackCalled = true), tracker); + JsUnit.assertTrue(hasSignalHandler(obj, 'signal')); + JsUnit.assertTrue(hasSignalHandler(obj, 'destroy')); + + obj.emit('signal'); + JsUnit.assertTrue(callbackCalled); + + obj.disconnectObject(tracker); + JsUnit.assertFalse(hasSignalHandler(obj, 'signal')); + JsUnit.assertFalse(hasSignalHandler(obj, 'destroy')); +}); + +testCase('Signal arguments are respected', () => { + const emitter = new Signals.EventEmitter(); + const tracked = new Destroyable(); + let cbCalled = false; + + emitter.connectObject('signal', (...args) => { + TestUtils.assertArrayEquals([emitter, 'add', 4, 'arguments', null], args); + cbCalled = true; + }, tracked); + + emitter.emit('signal', 'add', 4, 'arguments', null); + tracked.emit('destroy'); + + JsUnit.assertTrue(cbCalled); + emitter.emit('signal'); +}); + +testCase('Signal after connection is respected', () => { + let callbackCalled = false; + let callbackAfterCalled = false; + const emitter = new GObjectEmitter(); + + emitter.connectObject('signal', () => { + JsUnit.assertTrue(callbackCalled); + JsUnit.assertFalse(callbackAfterCalled); + callbackAfterCalled = true; + }, GObject.ConnectFlags.AFTER, {}); + + emitter.connectObject('signal', () => { + JsUnit.assertFalse(callbackAfterCalled); + JsUnit.assertFalse(callbackCalled); + callbackCalled = true; + }); + + emitter.emit('signal'); + JsUnit.assertTrue(callbackCalled); + JsUnit.assertTrue(callbackAfterCalled); +}); + +testCase('Signal after connection is respected in batch connections', () => { + let callbackCalled = false; + let callbackAfterCalled = false; + const emitter = new GObjectEmitter(); + + emitter.connectObject( + 'signal', () => { + JsUnit.assertTrue(callbackCalled); + JsUnit.assertFalse(callbackAfterCalled); + callbackAfterCalled = true; + }, GObject.ConnectFlags.AFTER, + 'signal', () => { + JsUnit.assertFalse(callbackAfterCalled); + JsUnit.assertFalse(callbackCalled); + callbackCalled = true; + }); + + emitter.emit('signal'); + JsUnit.assertTrue(callbackCalled); + JsUnit.assertTrue(callbackAfterCalled); +}); -- GitLab From 8e0c668eaba9d6015223377e3733aabfebb4966d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Mon, 14 Nov 2022 05:37:42 +0100 Subject: [PATCH 08/16] signalTracker: Do not throw when using GObject.ConnectFlags.DEFAULT Passing it or 0 is a valid flag value, so we should really not throw an error. --- js/misc/signalTracker.js | 2 +- tests/unit/signalTracker.js | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/js/misc/signalTracker.js b/js/misc/signalTracker.js index e4497e26aa..8f8c9af5e9 100644 --- a/js/misc/signalTracker.js +++ b/js/misc/signalTracker.js @@ -216,7 +216,7 @@ function connectObject(thisObj, ...args) { const flags = arg; let flagsMask = 0; Object.values(GObject.ConnectFlags).forEach(v => (flagsMask |= v)); - if (!(flags & flagsMask)) + if (flags && !(flags & flagsMask)) throw new Error(`Invalid flag value ${flags}`); if (flags & GObject.ConnectFlags.SWAPPED) throw new Error('Swapped signals are not supported'); diff --git a/tests/unit/signalTracker.js b/tests/unit/signalTracker.js index 52280c39e5..39b69c84e0 100644 --- a/tests/unit/signalTracker.js +++ b/tests/unit/signalTracker.js @@ -216,6 +216,11 @@ testCase('Signal emissions can be tracked', () => { JsUnit.assertEquals(count, 20); }); +testCase('Signal support default flags', () => { + const obj = new Signals.EventEmitter(); + obj.connectObject('signal', () => {}, GObject.ConnectFlags.DEFAULT ?? 0, {}); +}); + testCase('Fails with unknown flags', () => { const obj = new Signals.EventEmitter(); TestUtils.assertRaisesError(() => obj.connectObject('signal', () => {}, 256, {}), -- GitLab From f6b67638e7cf309a101b093f330fa0d8c8585c87 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Mon, 14 Nov 2022 06:59:30 +0100 Subject: [PATCH 09/16] signalTracker: Be stricter on checking connect flags We should only support the ones that are part of GObject.ConnectFlags, while any other value that is not included in the list should cause a failure. --- js/misc/signalTracker.js | 7 ++++--- tests/unit/signalTracker.js | 2 ++ 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/js/misc/signalTracker.js b/js/misc/signalTracker.js index 8f8c9af5e9..4a4233e41a 100644 --- a/js/misc/signalTracker.js +++ b/js/misc/signalTracker.js @@ -208,15 +208,16 @@ class SignalTracker { * @returns {void} */ function connectObject(thisObj, ...args) { + let flagsMask = 0; + Object.values(GObject.ConnectFlags).forEach(v => (flagsMask |= v)); + const getParams = argArray => { const [signalName, handler, arg, ...rest] = argArray; if (typeof arg !== 'number') return [signalName, handler, 0, arg, ...rest]; const flags = arg; - let flagsMask = 0; - Object.values(GObject.ConnectFlags).forEach(v => (flagsMask |= v)); - if (flags && !(flags & flagsMask)) + if (flags && (flags & flagsMask) !== flags) throw new Error(`Invalid flag value ${flags}`); if (flags & GObject.ConnectFlags.SWAPPED) throw new Error('Swapped signals are not supported'); diff --git a/tests/unit/signalTracker.js b/tests/unit/signalTracker.js index 39b69c84e0..db21ccc6ae 100644 --- a/tests/unit/signalTracker.js +++ b/tests/unit/signalTracker.js @@ -225,6 +225,8 @@ testCase('Fails with unknown flags', () => { const obj = new Signals.EventEmitter(); TestUtils.assertRaisesError(() => obj.connectObject('signal', () => {}, 256, {}), 'Invalid flag value 256'); + TestUtils.assertRaisesError(() => obj.connectObject('signal', () => {}, 234, {}), + 'Invalid flag value'); }); testCase('Emitter is same of tracker', () => { -- GitLab From 7092a0b2cee379a76837e43d9fd168e4174db7f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Mon, 14 Nov 2022 05:46:30 +0100 Subject: [PATCH 10/16] signalTracker: Add support for GObject.ConnectFlags.SWAPPED At times we want just ignore to receive the signal emitter argument, and implementing the native GObject.ConnectFlags.SWAPPED is simple enough in JS that when provided we can just expose the object as the last argument. --- js/misc/signalTracker.js | 7 ++++--- tests/unit/signalTracker.js | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 3 deletions(-) diff --git a/js/misc/signalTracker.js b/js/misc/signalTracker.js index 4a4233e41a..a99051294d 100644 --- a/js/misc/signalTracker.js +++ b/js/misc/signalTracker.js @@ -219,8 +219,6 @@ function connectObject(thisObj, ...args) { const flags = arg; if (flags && (flags & flagsMask) !== flags) throw new Error(`Invalid flag value ${flags}`); - if (flags & GObject.ConnectFlags.SWAPPED) - throw new Error('Swapped signals are not supported'); return [signalName, handler, flags, ...rest]; }; @@ -229,10 +227,13 @@ function connectObject(thisObj, ...args) { const func = (flags & GObject.ConnectFlags.AFTER) && isGObject ? 'connect_after' : 'connect'; + const orderedHandler = flags & GObject.ConnectFlags.SWAPPED + ? (instance, ...handlerArgs) => handler(...handlerArgs, instance) + : handler; const emitterProto = isGObject ? GObject.Object.prototype : Object.getPrototypeOf(emitter); - return emitterProto[func].call(emitter, signalName, handler); + return emitterProto[func].call(emitter, signalName, orderedHandler); }; const signalIds = []; diff --git a/tests/unit/signalTracker.js b/tests/unit/signalTracker.js index db21ccc6ae..5d72cf4ac1 100644 --- a/tests/unit/signalTracker.js +++ b/tests/unit/signalTracker.js @@ -332,6 +332,43 @@ testCase('Signal arguments are respected', () => { emitter.emit('signal'); }); +testCase('JSObject signal arguments can be swapped', () => { + const emitter = new Signals.EventEmitter(); + const tracked = new Destroyable(); + let cbCalled = false; + + emitter.connectObject('signal', (...args) => { + TestUtils.assertArrayEquals(['add', 4, 'arguments', null, emitter], args); + cbCalled = true; + }, GObject.ConnectFlags.SWAPPED, tracked); + + emitter.emit('signal', 'add', 4, 'arguments', null); + tracked.emit('destroy'); + + JsUnit.assertTrue(cbCalled); + emitter.emit('signal'); +}); + +testCase('GObject signal arguments can be swapped', () => { + const emitter = new GObjectEmitterInt(); + const tracked = new Destroyable(); + let cbCalled = false; + + emitter.connectObject('signal-int', (...args) => { + TestUtils.assertArrayEquals([5, emitter], args); + cbCalled = true; + }, GObject.ConnectFlags.SWAPPED, tracked); + + emitter.emit('signal-int', 5); + tracked.emit('destroy'); + + JsUnit.assertTrue(cbCalled); + + cbCalled = false; + emitter.emit('signal-int', 10); + JsUnit.assertFalse(cbCalled); +}); + testCase('Signal after connection is respected', () => { let callbackCalled = false; let callbackAfterCalled = false; -- GitLab From 96176e129edf2f4b74877b2b66fba18a46c2de25 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Mon, 14 Nov 2022 07:22:46 +0100 Subject: [PATCH 11/16] signalTracker: Fall-back to globalThis if no tracker is provided During connectObject we use globalThis as tracker if none is provided by the user, this is not happening on disconnection though, making a disconnectObject() call with no arguments to not behave as expected. Do this, and add a test to prove the case. --- js/misc/signalTracker.js | 3 ++- tests/unit/signalTracker.js | 15 +++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/js/misc/signalTracker.js b/js/misc/signalTracker.js index a99051294d..1ab7525eee 100644 --- a/js/misc/signalTracker.js +++ b/js/misc/signalTracker.js @@ -257,7 +257,8 @@ function connectObject(thisObj, ...args) { * @returns {void} */ function disconnectObject(thisObj, obj) { - SignalManager.getDefault().maybeGetSignalTracker(thisObj)?.untrack(obj); + SignalManager.getDefault().maybeGetSignalTracker(thisObj)?.untrack( + obj ?? globalThis); } /** diff --git a/tests/unit/signalTracker.js b/tests/unit/signalTracker.js index 5d72cf4ac1..52c728fe19 100644 --- a/tests/unit/signalTracker.js +++ b/tests/unit/signalTracker.js @@ -315,6 +315,21 @@ testCase('Emitter with empty tracker, disconnects on disconnectObject', () => { JsUnit.assertFalse(hasSignalHandler(obj, 'destroy')); }); +testCase('Emitter with no tracker, disconnects on disconnectObject', () => { + const obj = new GObjectEmitter(); + let callbackCalled = false; + obj.connectObject('signal', () => (callbackCalled = true)); + JsUnit.assertTrue(hasSignalHandler(obj, 'signal')); + JsUnit.assertTrue(hasSignalHandler(obj, 'destroy')); + + obj.emit('signal'); + JsUnit.assertTrue(callbackCalled); + + obj.disconnectObject(); + JsUnit.assertFalse(hasSignalHandler(obj, 'signal')); + JsUnit.assertFalse(hasSignalHandler(obj, 'destroy')); +}); + testCase('Signal arguments are respected', () => { const emitter = new Signals.EventEmitter(); const tracked = new Destroyable(); -- GitLab From cec1a0b53b849099febcf2e81c1d901cc5103386 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Mon, 14 Nov 2022 17:45:15 +0100 Subject: [PATCH 12/16] signalTracker: Add support for once-invoked signal handlers In various situations we need to connect to an object signal waiting for it to happen and immediately disconnect from it. This can be quite annoying to do using the signalTracker without using an extra transient signal holder to be used as tracker, while we can easily automatize this wrapping the callback function to also perform the cleanup operations once the signal callback has been received. Adding support for this, using a custom GObject.ConnectFlags flag for now, while some custom code can be removed in the GObject case when GLib will support this natively. This is also a pre-requisite work to have promise-based signal connections that automatically disconnect when the promises are resolved. Comes with tests. --- js/misc/signalTracker.js | 40 +++++++++- tests/unit/signalTracker.js | 151 ++++++++++++++++++++++++++++++++++++ 2 files changed, 188 insertions(+), 3 deletions(-) diff --git a/js/misc/signalTracker.js b/js/misc/signalTracker.js index 1ab7525eee..ff1e518ba7 100644 --- a/js/misc/signalTracker.js +++ b/js/misc/signalTracker.js @@ -3,6 +3,9 @@ const { GObject } = imports.gi; const destroyableTypes = []; +// Add custom shell connection flags, ensuring we don't override standard ones +GObject.ConnectFlags.SHELL_ONCE = 1 << 25; + /** * @private * @param {Object} obj - an object @@ -178,6 +181,24 @@ class SignalTracker { this._removeTracker(); } + /** + * @param {object} obj - tracked object instance + * @param {...number} handlerIds - tracked handler IDs to untrack + * @returns {void} + */ + untrackIds(obj, ...handlerIds) { + const {ownerSignals} = this._getSignalData(obj); + const ownerProto = this._getObjectProto(this._owner); + + handlerIds.forEach(id => { + this._disconnectSignalForProto(ownerProto, this._owner, id); + ownerSignals.splice(ownerSignals.indexOf(id), 1); + }); + + if (!ownerSignals.length) + this.untrack(obj); + } + /** * @returns {void} */ @@ -222,7 +243,11 @@ function connectObject(thisObj, ...args) { return [signalName, handler, flags, ...rest]; }; + const signalManager = SignalManager.getDefault(); + let obj; + const connectSignal = (emitter, signalName, handler, flags) => { + let connectionId; const isGObject = emitter instanceof GObject.Object; const func = (flags & GObject.ConnectFlags.AFTER) && isGObject ? 'connect_after' @@ -230,10 +255,19 @@ function connectObject(thisObj, ...args) { const orderedHandler = flags & GObject.ConnectFlags.SWAPPED ? (instance, ...handlerArgs) => handler(...handlerArgs, instance) : handler; + const realHandler = flags & GObject.ConnectFlags.SHELL_ONCE + ? (...handlerArgs) => { + const tracker = signalManager.getSignalTracker(emitter); + tracker.untrackIds(obj, connectionId); + return orderedHandler(...handlerArgs); + } + : orderedHandler; + const emitterProto = isGObject ? GObject.Object.prototype : Object.getPrototypeOf(emitter); - return emitterProto[func].call(emitter, signalName, orderedHandler); + connectionId = emitterProto[func].call(emitter, signalName, realHandler); + return connectionId; }; const signalIds = []; @@ -243,8 +277,8 @@ function connectObject(thisObj, ...args) { args = rest; } - const obj = args.at(0) ?? globalThis; - const tracker = SignalManager.getDefault().getSignalTracker(thisObj); + obj = args.at(0) ?? globalThis; + const tracker = signalManager.getSignalTracker(thisObj); tracker.track(obj, ...signalIds); } diff --git a/tests/unit/signalTracker.js b/tests/unit/signalTracker.js index 52c728fe19..23cfe6dc5f 100644 --- a/tests/unit/signalTracker.js +++ b/tests/unit/signalTracker.js @@ -427,3 +427,154 @@ testCase('Signal after connection is respected in batch connections', () => { JsUnit.assertTrue(callbackCalled); JsUnit.assertTrue(callbackAfterCalled); }); + +testCase('Signal connections once are automatically disconnected from GObject', () => { + let callback1Called = 0; + let callback2Called = 0; + const obj = new GObjectEmitter(); + + obj.connectObject( + 'signal', () => { + callback1Called++; + JsUnit.assertTrue(hasSignalHandler(obj, 'signal')); + }, GObject.ConnectFlags.SHELL_ONCE, + 'signal', () => { + callback2Called++; + JsUnit.assertFalse(hasSignalHandler(obj, 'signal')); + }, GObject.ConnectFlags.SHELL_ONCE); + + JsUnit.assertTrue(hasSignalHandler(obj, 'signal')); + JsUnit.assertTrue(hasSignalHandler(obj, 'destroy')); + + obj.emit('signal'); + JsUnit.assertFalse(hasSignalHandler(obj, 'signal')); + JsUnit.assertFalse(hasSignalHandler(obj, 'destroy')); + + JsUnit.assertEquals(1, callback1Called); + JsUnit.assertEquals(1, callback2Called); +}); + +testCase('Signal connections once are automatically disconnected from GObject keeping destroy monitor', () => { + let callback1Called = 0; + let callback2Called = 0; + + const obj = new GObjectEmitterInt(); + obj.connectObject( + 'signal-int', expectedCalls => { + JsUnit.assertEquals(callback1Called, expectedCalls); + callback1Called++; + JsUnit.assertFalse(hasSignalHandler(obj, 'signal-int')); + JsUnit.assertTrue(hasSignalHandler(obj, 'signal')); + obj.emit('signal-int', 10); + }, GObject.ConnectFlags.SHELL_ONCE | GObject.ConnectFlags.SWAPPED, + 'signal', () => { + callback2Called++; + JsUnit.assertFalse(hasSignalHandler(obj, 'signal-int')); + JsUnit.assertTrue(hasSignalHandler(obj, 'signal')); + }); + + JsUnit.assertTrue(hasSignalHandler(obj, 'signal-int')); + JsUnit.assertTrue(hasSignalHandler(obj, 'signal')); + JsUnit.assertTrue(hasSignalHandler(obj, 'destroy')); + + obj.emit('signal-int', 0); + JsUnit.assertFalse(hasSignalHandler(obj, 'signal-int')); + JsUnit.assertTrue(hasSignalHandler(obj, 'signal')); + JsUnit.assertTrue(hasSignalHandler(obj, 'destroy')); + + JsUnit.assertEquals(1, callback1Called); + JsUnit.assertEquals(0, callback2Called); + + obj.emit('signal'); + JsUnit.assertFalse(hasSignalHandler(obj, 'signal-int')); + JsUnit.assertTrue(hasSignalHandler(obj, 'signal')); + JsUnit.assertTrue(hasSignalHandler(obj, 'destroy')); + + JsUnit.assertEquals(1, callback1Called); + JsUnit.assertEquals(1, callback2Called); + + obj.emit('signal'); + JsUnit.assertEquals(2, callback2Called); + + obj.disconnectObject(); + + JsUnit.assertFalse(hasSignalHandler(obj, 'signal-int')); + JsUnit.assertFalse(hasSignalHandler(obj, 'signal')); + JsUnit.assertFalse(hasSignalHandler(obj, 'destroy')); +}); + +testCase('Signal connections once are automatically disconnected from JSObject', () => { + let callback1Called = 0; + let callback2Called = 0; + const obj = new Signals.EventEmitter(); + + obj.connectObject( + 'signal', (...args) => { + TestUtils.assertArrayEquals(args, ['arg1', 2, obj]); + callback1Called++; + }, GObject.ConnectFlags.SHELL_ONCE | GObject.ConnectFlags.SWAPPED, + 'signal', (...args) => { + TestUtils.assertArrayEquals(args, [obj, 'arg1', 2]); + callback2Called++; + }, GObject.ConnectFlags.SHELL_ONCE); + + obj.emit('destroy'); // It's ignored in such objects, must be no-op! + obj.emit('signal', 'arg1', 2); + + JsUnit.assertEquals(1, callback1Called); + JsUnit.assertEquals(1, callback2Called); + + obj.emit('signal'); + JsUnit.assertEquals(1, callback1Called); + JsUnit.assertEquals(1, callback2Called); + + obj.connectObject( + 'signal1', () => { + callback1Called++; + obj.emit('signal1'); + }, GObject.ConnectFlags.SHELL_ONCE, + 'signal2', () => { + callback2Called++; + }); + + obj.emit('signal1'); + JsUnit.assertEquals(2, callback1Called); + JsUnit.assertEquals(1, callback2Called); + + obj.emit('signal2'); + JsUnit.assertEquals(2, callback1Called); + JsUnit.assertEquals(2, callback2Called); + + obj.disconnectObject(); + obj.emit('signal1'); + obj.emit('signal2'); + + JsUnit.assertEquals(2, callback1Called); + JsUnit.assertEquals(2, callback2Called); +}); + +testCase('Signal connections once are disconnected on tracker destruction', () => { + let callback1Called = 0; + let callback2Called = 0; + const obj = new Signals.EventEmitter(); + const tracker = new Destroyable(); + + obj.connectObject( + 'signal-once', () => { + callback1Called++; + }, GObject.ConnectFlags.SHELL_ONCE | GObject.ConnectFlags.SWAPPED, + 'signal-once', () => { + callback2Called++; + }, GObject.ConnectFlags.SHELL_ONCE, + tracker); + + JsUnit.assertTrue(hasSignalHandler(tracker, 'destroy')); + + tracker.emit('destroy'); + + obj.emit('signal-once', 'arg1', 'arg2'); + JsUnit.assertEquals(0, callback2Called); + JsUnit.assertEquals(0, callback1Called); + + JsUnit.assertFalse(hasSignalHandler(tracker, 'destroy')); +}); -- GitLab From 64a85a5556fefd21df9faf4929122050cc5b6ac2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Mon, 14 Nov 2022 20:28:57 +0100 Subject: [PATCH 13/16] signalTracker: Avoid repeated signalData lookup on track When tracking ab object we're looking up the object signal data twice: one to try to connect to the destroy handler, and another one to push the handled signal IDs. We can avoid this by simply passing the signalData to the private _trackDestroy() function, and by doing that we can also avoid to perform an extra iteration on destroyableTypes if we've already a destroy handler. --- js/misc/signalTracker.js | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/js/misc/signalTracker.js b/js/misc/signalTracker.js index ff1e518ba7..9c59428cca 100644 --- a/js/misc/signalTracker.js +++ b/js/misc/signalTracker.js @@ -119,11 +119,11 @@ class SignalTracker { /** * @private * @param {GObject.Object} obj - tracked widget + * @param {object} signalData - object signal data, got via _getSignalData() */ - _trackDestroy(obj) { - const signalData = this._getSignalData(obj); + _trackDestroy(obj, signalData) { if (signalData.destroyId) - return; + throw new Error('Destroy already tracked'); signalData.destroyId = obj.connect_after('destroy', () => this.untrack(obj)); } @@ -157,10 +157,11 @@ class SignalTracker { * @returns {void} */ track(obj, ...handlerIds) { - if (_hasDestroySignal(obj)) - this._trackDestroy(obj); + const signalData = this._getSignalData(obj); + if (!signalData.destroyId && _hasDestroySignal(obj)) + this._trackDestroy(obj, signalData); - this._getSignalData(obj).ownerSignals.push(...handlerIds); + signalData.ownerSignals.push(...handlerIds); } /** -- GitLab From 3793736ecd4969ef2495692c5db448e1932de6f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Tue, 15 Nov 2022 09:28:50 +0100 Subject: [PATCH 14/16] signalTracker: Ensure that destroy-after signals happens last A SignalTracker is tracking the 'destroy' signal for its owner when possible and it does it using the "after" flag to ensure that we don't stop other handlers to be reached. However, if the owner is also a destroyable object, others can connect to its own 'destroy' signal and use the AFTER flag too, and in this case we wouldn't ever call such handler, because the SignalTracker would be cleared earlier than this. To ensure that this won't happen, when we're explicitly connecting to a 'destroy' signal, using the AFTER flag, force a SignalTracker re-connection to the 'destroy' signal, so that we can be sure that no other handler will be shadowed by it. --- js/misc/signalTracker.js | 33 ++++++++++++++++++++++++++++++--- tests/unit/signalTracker.js | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 3 deletions(-) diff --git a/js/misc/signalTracker.js b/js/misc/signalTracker.js index 9c59428cca..1142c9bd5d 100644 --- a/js/misc/signalTracker.js +++ b/js/misc/signalTracker.js @@ -89,11 +89,11 @@ class SignalTracker { * @param {Object=} owner - object that owns the tracker */ constructor(owner) { - if (_hasDestroySignal(owner)) - this._ownerDestroyId = owner.connect_after('destroy', () => this.clear()); - this._owner = owner; this._map = new Map(); + + if (_hasDestroySignal(owner)) + this._trackOwnerDestroy(); } /** @@ -116,6 +116,25 @@ class SignalTracker { return data; } + /** + * Reconnects to owner 'destroy' if any + */ + updateOwnerDestroyTracker() { + if (!this._ownerDestroyId) + return; + + this._disconnectSignal(this._owner, this._ownerDestroyId); + this._trackOwnerDestroy(); + } + + /** + * @private + */ + _trackOwnerDestroy() { + this._ownerDestroyId = this._owner.connect_after('destroy', + () => this.clear()); + } + /** * @private * @param {GObject.Object} obj - tracked widget @@ -158,6 +177,7 @@ class SignalTracker { */ track(obj, ...handlerIds) { const signalData = this._getSignalData(obj); + if (!signalData.destroyId && _hasDestroySignal(obj)) this._trackDestroy(obj, signalData); @@ -271,15 +291,22 @@ function connectObject(thisObj, ...args) { return connectionId; }; + let trackingAfterDestroy = false; const signalIds = []; while (args.length > 1) { const [signalName, handler, flags, ...rest] = getParams(args); signalIds.push(connectSignal(thisObj, signalName, handler, flags)); + if (signalName === 'destroy' && flags & GObject.ConnectFlags.AFTER) + trackingAfterDestroy = true; args = rest; } obj = args.at(0) ?? globalThis; const tracker = signalManager.getSignalTracker(thisObj); + + if (trackingAfterDestroy) + tracker.updateOwnerDestroyTracker(); + tracker.track(obj, ...signalIds); } diff --git a/tests/unit/signalTracker.js b/tests/unit/signalTracker.js index 23cfe6dc5f..f3b79ae7f7 100644 --- a/tests/unit/signalTracker.js +++ b/tests/unit/signalTracker.js @@ -121,6 +121,26 @@ testCase('TransientSignalHolder owner destruction keeps early monitored destruct JsUnit.assertFalse(hasSignalHandler(owned, 'destroy')); }); +testCase('TransientSignalHolder owner destruction always happens as last destruction event', () => { + let ownedDestroyCalled = false; + let ownerDestroyCalled = false; + const owner = new Destroyable(); + const owned = new TransientSignalHolder(owner); + + owned.connectObject('destroy', () => (ownedDestroyCalled = true), owner); + owner.connectObject('destroy', () => (ownerDestroyCalled = true), + GObject.ConnectFlags.AFTER); + JsUnit.assertTrue(hasSignalHandler(owner, 'destroy')); + JsUnit.assertTrue(hasSignalHandler(owned, 'destroy')); + + owner.emit('destroy'); + JsUnit.assertTrue(ownedDestroyCalled); + JsUnit.assertTrue(ownerDestroyCalled); + + JsUnit.assertFalse(hasSignalHandler(owner, 'destroy')); + JsUnit.assertFalse(hasSignalHandler(owned, 'destroy')); +}); + testCase('Signal emissions can be tracked', () => { const emitter1 = new Signals.EventEmitter(); const emitter2 = new GObjectEmitter(); @@ -260,6 +280,20 @@ testCase('Emitter is same of tracker after', () => { JsUnit.assertFalse(hasSignalHandler(obj, 'destroy')); }); +testCase('Emitter is same of tracker does not block after-destroy signals', () => { + let destroyCalled = false; + const obj = new Destroyable(); + + obj.connectObject('destroy', () => (destroyCalled = true), + GObject.ConnectFlags.AFTER, obj); + JsUnit.assertTrue(hasSignalHandler(obj, 'destroy')); + + obj.emit('destroy'); + JsUnit.assertTrue(destroyCalled); + + JsUnit.assertFalse(hasSignalHandler(obj, 'destroy')); +}); + testCase('Emitter is disconnected on tracker destruction', () => { const obj = new GObjectEmitter(); const tracker = new Destroyable(); -- GitLab From b72a2ff822f71df81d63c0391d42e6a78a5fd7cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Tue, 15 Nov 2022 19:19:13 +0100 Subject: [PATCH 15/16] TransientSignalHolder: Connect to after destroy owner signal Any object monitoring another one to perform destroy actions should try not to react too early, we do it in all the cases by explicitly using 'connect_after' but we don't in the TransientSignalHolder. This would have not previously worked properly in some scenarios (such as when something else was connected to the owner 'destroy' object before initializing the TransientSignalHolder), but as per previous commit, we can now safely do it. The 'TransientSignalHolder owner destruction keeps early monitored destructions' test is keep validating this behavior. --- js/misc/signalTracker.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/js/misc/signalTracker.js b/js/misc/signalTracker.js index 1142c9bd5d..e1fa251707 100644 --- a/js/misc/signalTracker.js +++ b/js/misc/signalTracker.js @@ -24,8 +24,10 @@ class TransientSignalHolder extends GObject.Object { constructor(owner) { super(); - if (_hasDestroySignal(owner)) - owner.connectObject('destroy', () => this.destroy(), this); + if (_hasDestroySignal(owner)) { + owner.connectObject('destroy', () => this.destroy(), + GObject.ConnectFlags.AFTER, this); + } } destroy() { -- GitLab From a85b5e8b45cd4547f2db3f951d5583e69973197e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Tue, 15 Nov 2022 19:25:59 +0100 Subject: [PATCH 16/16] signalTracker: Ignore double-destroy tracking In case an object is used both as emitter and a tracker, there's no need to connect to the owner 'destroy' signal twice. --- js/misc/signalTracker.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/js/misc/signalTracker.js b/js/misc/signalTracker.js index e1fa251707..2aa5f7e2c2 100644 --- a/js/misc/signalTracker.js +++ b/js/misc/signalTracker.js @@ -145,6 +145,8 @@ class SignalTracker { _trackDestroy(obj, signalData) { if (signalData.destroyId) throw new Error('Destroy already tracked'); + if (obj === this._owner) + return; signalData.destroyId = obj.connect_after('destroy', () => this.untrack(obj)); } -- GitLab