GNOME Bugzilla – Bug 729288
Return values from default signal handlers should not be ignored
Last modified: 2015-10-27 22:57:02 UTC
There's a missing "return" in the GObject override which means that the return value of a class's default signal handler is ignored. This makes a signal with FIRST_WINS accumulation pretty useless. See the testcase in the attached patch for an example of what I mean.
Created attachment 275507 [details] [review] Patch with testcase and fix
Review of attachment 275507 [details] [review]: The fix is correct, but really, the on_* syntax was a mistake. One should use connect() or the vfunc, because on_* does not have a way to chain up and has weird behavior with custom marshallers (which breaks GtkListStore at least) Anyway, I guess that for compatibility we ought to have it working.
I don't know any users of on_, so I think we should just remove it.
Except one who filed this bug :-P Is there guaranteed to be a vfunc for every signal? I thought that was optional.
No. Just use this.connect('foo', ...);
That basically makes signals with accumulators unusable in GJS, as well as RUN_LAST and RUN_CLEANUP. Handlers connected with g_signal_connect() are executed in the order they are connected in (according to GObject docs), so connecting one with this.connect() in an object's constructor will mean that it is run _before_ any signals that an object's API consumers connect. So for example the FIRST_WINS accumulator that I mentioned in the original bug report would not be able to have a default value, because any signal handler providing that default value would execute before any user-connected handlers.
You could use this.connect_after. They're run in connect();, class_closure, connect_after(); order.
According to the docs the order is: 1. class closure for RUN_FIRST signals 2. connect() 3. class closure for RUN_LAST signals 4. connect_after() 5. class closure for RUN_CLEANUP signals No matter what, without default handlers, you lose some versatility with signals in GJS. You can imitate a RUN_FIRST class closure by connect()ing in the object's constructor, and you can imitate a RUN_LAST class closure by connect_after()ing in the constructor, but you lose RUN_CLEANUP.
Pushed to master