GNOME Bugzilla – Bug 735243
Add attributes for allowing use of accumulators on signals
Last modified: 2018-05-22 15:17:43 UTC
GObject signals have a feature called accumulators that is currently not usable by vala. I would like to propose that we add CCode attributes to make it usable. My use case it that I would like to be able to use g_signal_accumulator_true_handled. My current workaround is to add > Signal.stop_emission_by_name (text_entry, "key-pressed"); to my signal handler, which is messy and easy to forget to do and prone to errors if you change the name of the signal. Instead, I would like to be able to write > [CCode (accumulator = "g_signal_accumulator_true_handled")] > public signal bool key_pressed (uint key_code); and have this automatically taken care of.
Created attachment 284250 [details] [review] patch-v1 Here is a patch to accomplish this.
Created attachment 284251 [details] test case And here is a test program to demonstrate how it works. If you remove the CCode atttribute, the output of the program is... > ** Message: test.vala:11: handler 1 > ** Message: test.vala:17: handler 2 > ** Message: test.vala:21: f.sig (0): false > ** Message: test.vala:11: handler 1 > ** Message: test.vala:17: handler 2 > ** Message: test.vala:23: f.sig (2): false However, with the patch applied and the CCode attribute in place, you get... > ** Message: test.vala:12: handler 1 > ** Message: test.vala:18: handler 2 > ** Message: test.vala:22: f.sig (0): false > ** Message: test.vala:12: handler 1 > ** Message: test.vala:24: f.sig (2): true
Awesome, thanks for your work. However, I'd like to find a solution to avoid C names for accumulators. A possibility would be to put a Vala symbol as string, like accumulator = "Some.accumulator". I don't like much that either, perhaps some kind of syntax which is not much obtrusive is better.
I thought about this as well, but it gets very tricky when you consider that you may need to pass accumulator_data as well (tricky for me anyway since I don't know anything about the code generator). CCode seems the simplest way to cover all use cases. I will think about it some more and see if I can come up with something else.
Created attachment 284258 [details] [review] Support signal accumulators
I created this patch recently. It introduces special 'accumulator blocks' and continue/break for emission continuation. Please let me know if this makes sense. Thank you :)
Created attachment 284297 [details] [review] Support signal accumulators I forgot the test case, here is the updated version.
That means redefining an accumulator for every signal. Doesn't seem feasible.
New idea: Add a new "signal contract keyword" called "accumulates" that works somewhat like "requires" or "throws" does for methods. Signal declarations would look like: > public virtual signal bool signal_1 (int arg1) accumulates (SignalAccumulator.true_handled); or > SignalAccumulatorFunc acc = (data) => { > var some_obj = (SomeType)data; > if (some_obj.has_state) > return true; > return false; > }; > > public virtual signal void signal_2 () accumulates (acc, some_obj); where accumulates is parsed like a method call with the following declaration: > void accumulates (SignalAccumulatorFunc func, void* data = null); This would let us use actual vala expressions to assign the values instead of strings. Of course, if you don't need an accumulator, you would leave out "accumulates ()" (which also means existing code won't break).
Yes that was the idea. I prefer the first one syntax. About the accumulator data. In theory, the scope where that data should be created is the same as when the signal is created. In practice, do we have any examples of accumulators that need userdata?
A quick grep through gtk+ and gio shows no userdata args whatsoever so I suggest leaving it at NULL or now. A later improvement might be to capture klass fields if thats really needed.
I should have said "and" instead of "or". Both are the same same syntax. The only practical use for the data parameter that I could think of is if you want to pass an object so that you can compare the individual signal handler return values against one of the object members to know if you should continue processing signal handlers or not. I have not been able to come up with a concrete example that would be useful in real life though.
Created attachment 284435 [details] [review] Partial implementation of accumulates keyword I have started implementation of my suggestion. I am stuck on the part about how to parse the expression in "accumulates (<expression>)". * Is expression the correct thing to use? * How do you check that the expression is of the type (delegate) SiganlAccumulatorFunc? * How do I add it to the generated c code?
I really think the data parameter should be omitted. Given that the signal is created in the class init function, the only things you could pass are constant, static or class members and there should be an easier way to access those.
Review of attachment 284435 [details] [review]: The accumulator expression has to be resolved and later checked in Signal.check(). You can then compare its statically computed type against the GSignalAccumulator delegate type using value_type.compatible(). I'm not sure whether we should allow anything fancier than a method name for accumulates(..), lambda expressions would be possible I guess.. ::: vala/valaparser.vala @@ +2889,3 @@ + } + expect (TokenType.CLOSE_PARENS); + } also add ACCUMULATES to skip_identifiers ::: vapi/gobject-2.0.vapi @@ +643,3 @@ } + namespace SignalAccumulator { maybe better to move to Signal.accumulator_*? @@ +652,3 @@ + [CCode (cname = "GSignalAccumulator", has_target = false)] + public delegate bool SignalAccumulatorFunc (SignalInvocationHint ihint, ref Value return_accu, Value handler_return, void *data); + maybe better 'SignalAccumulator' as with 'SignalInvocationHint'
Actually, instead of using value_type.compatible it is better to assign directly to accumulator.target_type and then perform the check.
Don't use an expression please, rather a Symbol to be later resolved. It should be enough for any use case.
Created attachment 284454 [details] [review] Partial implementation of accumulates keyword (2) > I really think the data parameter should be omitted. I hear you. However, the SignalAccumulator delegate has the data parameter because we are directly binding GSignalAccumulator. It seems like we should keep the data parameter in accumulates () or hide it in the delegate (if that is possible). > also add ACCUMULATES to skip_identifiers done. > maybe better to move to Signal.accumulator_*? Agreed. I doesn't make much sense to have a namespace with 2 members. > maybe better 'SignalAccumulator' as with 'SignalInvocationHint' Agreed. I changed it so it would not conflict with the namespace, which is no longer a problem. > Don't use an expression please, rather a Symbol to be later resolved. I have done this - hopefully correctly. ----- There are a few points I am stuck on now: * How to resolve the symbol? * How to get the c code for the symbol?
*** Bug 635665 has been marked as a duplicate of this bug. ***
*** Bug 711360 has been marked as a duplicate of this bug. ***
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/vala/issues/473.