GNOME Bugzilla – Bug 126213
maybe signals-with-return-types should connect_before
Last modified: 2017-04-25 06:58:51 UTC
I think that all signals connect after (so the signal handlers are called after existing signal handlers). signals with return types (such as the bool *_event signals) also have connect_notify() which connect before, but ignore the return value. A normal connect is often impossible because the return value has stopped any further processing. Maybe these signals should be connected before by default. We should look at what people ususally do with the and Python APIs. I could be confused about some of this.
Hm, I don't think that this policy is correct. First things first: The connect() function of SignalProxy# always connects with G_CONNECT_AFTER to the signal. This seems not right to me, because Gtk specifically likes to connect with (GConnectFlags)0 as there is an extra #define for this (namely: g_signal_connect). To connect with G_CONNECT_AFTER you have to use g_signal_connect_after(). So it seems to me the GObject library prefers to connect BEFORE existing connections Eventually Gtkmm established "appending" to be common practice, so I won't argue here. As far as I can see connect_notify() does not more than to append the connection to the signal. This is possible with the connect method too (setting the second parameter "after = false"). There is no need for the connect_notify function if it was documented properly where the default AFTER connection won't have any effect. But this is a Gtk issue. I did an example in plain C/Gtk which emulates the behavior of example 19697 (attached in bug 125969). There connecting with AFTER does exactly the same as in gtkmm: nothing. (So it is a documentation problem.) This from my point of view justifies Gtk's behavior of prepending signals by default. This leaves only the speciality of a void return type. But: What is wrong with returning a signal-interrupting value from a function connected via connect_notify()? This seems to me is perfectly valid Gtk code and thus should be possible with Gtkmm too.
> The connect() function of SignalProxy# always connects with G_CONNECT_AFTER to the signal. Yes, because it seems normal that already-existing signal handlers should deal with the signal first. I normally want to add behaviour instead of changing behaviour. It's not always that clear, but I think that we will get strange behaviour if the user has to think about signal handlers that he doesn't even know exist. How would he know that the strange incomplete widget state during his signal handler is due to a signal handler that has not run yet. This has worked fine with zero problems since gtkmm 2.0. > This seems not right to me, because Gtk specifically likes to connect with (GConnectFlags)0 as there is an extra #define for this (namely: g_signal_connect) Yes, so g_signal_connect() does connect before. Still, again I think that's a problem, and gtkmm has never had a problem with it. Sometimes gtkmm makes things easier. > But: What is wrong with returning a signal-interrupting value from a function connected via connect_notify()? If connect_notify() (with no return value) connects before, then there is no return value for the signal. If it connects after then the return value has no effect, because there's nothing to stop. I am unlikely to change the behaviour of connect(). This bug is about the behaviour of connect_notify().
> I am unlikely to change the behaviour of connect(). This bug is about > the behaviour of connect_notify(). Sorry, that should be "I am unlikely to change the behaviour of connect() for non-X-event signals. This bug is about signals whose return type specifies whether further signal handlers will be called."
Here's an example of where the gtkmm connect-after-by-default behaviour is desirable (as well as intuitively expected) for a non-X-event-signal example: In the signal handler for the GtkNotebook::switch-page signal, gtk_notebook_get_current_page() will return the incorrect value when using g_signal_connect(), but will return the correct value (the same as the handler's page_num parameter) when using g_signal_connect_after(). Presumably the default signal handler does important work, which we want the benefit of in our own signal handler.
Created attachment 231920 [details] [review] patch: SignalProxy: Fix the documentation, especially of connect_notify(). Here is a patch that fixes the documentation of SignalProxyNormal and SignalProxy[0-6], especially the documentation of SignalProxy[0-6]:: connect_notify(). With the present documentation it's difficult to understand what it does, and how it differs from connect(). This patch only improves the documentation. It does not break API or ABI. It can be committed right away, unless someone objects.
Created attachment 231921 [details] [review] patch: SignalProxy: Connect signal handlers with return value before by default I agree that it would be a good idea to change the default behaviour of connect(), and connect before the default signal handler to signals with return values. This patch shows how it can be done. It breaks API. (In reply to comment #3) > I am unlikely to change the behaviour of > connect() for non-X-event signals. This bug is about signals whose > return type specifies whether further signal handlers will be called. My patch changes the behaviour of connect() for all signals with a return value, not just X-event signals. It would be possible to restrict the changed behaviour to signals that return bool, but that would also include non-X-event signals. I think that other signals with a return value can also benefit from a connect-before default behaviour. An example is Gio::Application:: signal_command_line(). The signal handler returns int. Only one signal handler is called. connect_notify() is not always suitable, because the application program may want to choose the value to return at run-time.
I have pushed the patch in comment 5 (the one that only changes the documentation). The patch in comment 6 (or a similar one) must wait until we can break API.
What's connect_notify() good for? It's not used in glibmm, pangomm, gtkmm or any of the example programs in gtkmm-documentation. > I agree that it would be a good idea to change the default behaviour of > connect(), and connect before the default signal handler to signals with > return values. I've changed my mind. Now I think it would be best to keep connect() as it is, always by default connecting after the default handler. It's true that this behaviour sometimes confuses people. I think it would also be confusing to let it by default connect before for some signals, and connect after for other signals. However the default value is determined, it would probably be unsuitable for some signals.
I suggest that I remove Glib::SignalProxy::connect_notify() and Glib::SignalProxyDetailed::connect_notify() in glibmm 2.54 and close this bug with Wontfix. (See comment 8)
Is it OK if I do as I suggest in comment 9? I can also add a sentence or two about the 'after' parameter to the documentation of connect(). Now connect_after() is better documented than connect().
s/connect_after()/connect_notify()/
I use connect_notify() in Glom. For instance: https://git.gnome.org/browse/glom/tree/glom/utility_widgets/adddel/adddel.cc#n93 https://git.gnome.org/browse/glom/tree/glom/mode_data/box_data_calendar_related.cc#n57 I think we added it to glibmm because it was slightly more obvious than passing a mysterious bool and then having to remember that the return value was irrelevant, though the API still forces you to return something. That still seems useful.
OK, then I won't remove connect_notify(). What shall we do with this bug? I suggest that I just improve the documentation of connect() and close the bug. Do you still think it would be better if connect() by default connected either before or after the default signal handler, depending on the return type of the signal handlers? Wouldn't that cause even more confusion than always by default connecting after?
Thanks. In general, I think these bool return types for signal handlers are horrible API. They need the application developer to know what the existing signal handlers do, and whether they even allow a connect "after". I wish we could avoid them altogether somehow. We might choose to just wrap them as vfuncs and not as signals, requiring people to derive new classes. Then the problem would be reduced to deciding whether to call the base class's signal handler. That's difficult enough, but it's not a problem that other APIs (Qt, Android, etc) have solved either. I wonder how disruptive that would be.
I have added to the documentation of SignalProxy::connect() and SignalProxyDetailed::connect(). https://git.gnome.org/browse/glibmm/commit/?id=7bd0fa6fa6404ff73d338c26543e114dc1031fc7 Wrapping as vfuncs instead of signals? It would force application developers to decide what to do: Add their code before or after the default handler, and whether to call the default handler (now becoming the base class vfunc). No one could "connect after" without being aware of doing so. But it's a rather drastic change. I wonder if there is some trick with C++ templates that would make the default value of the "after" parameter illegal for signals that return a value. That would be less drastic, and would force callers of connect() to explicitly assign a value to "after" for such signals.
> But it's a rather drastic change. And unfortunately it won't work when the application can't say that it's a derived class that should be instantiated. For instance in this code in glibmm/tests/giomm_tls_client/main.cc: auto tls_connection = Gio::TlsClientConnection::create(conn, address); tls_connection->signal_accept_certificate().connect(sigc::ptr_fun(&on_accept_certificate)); That signal handler must return a bool to indicate whether subsequent signal handlers should be called.
Created attachment 350208 [details] [review] patch: Glib::SignalProxy: Make a specialization for void signal handlers This is one way not to have a default value of 'after' for signal handlers that return a value: Make template specializations of SignalProxy<> and SignalProxyDetailed<>. I've tried std::enable_if, but the compiler disliked everything that I tried. With template specializations it would be possible to remove connect_notify() for signal handlers that return void, and still keep it for those that return a value. Would that be an improvement? Here's another reason not to wrap signals as vfuncs: It's impossible to wrap Gio::DBus::Server::signal_new_connection() as a vfunc, because the class struct GDBusServerClass is defined in gdbusserver.c, hidden from us. This signal returns bool.
(In reply to Kjell Ahlstedt from comment #17) > With template specializations it would be possible to remove connect_notify() > for signal handlers that return void, and still keep it for those that > return a > value. Would that be an improvement? I think so, yes, and that might be the first, simpler thing to do.
Created attachment 350266 [details] [review] patch: Glib::SignalProxy: Make a specialization for void signal handlers Is this patch OK then? Of course it would be possible to give connect()'s "after" parameter a default value = false in the primary template, but would it be better? It's probably fine to force application developers to make a conscious decision.
Yes, please, and thanks. And I agree that developers need to think about whether to connect before or after when connecting these "event" signals. The API is unpleasant, but there is apparently no good default. Maybe add a TODO for us to simplify this with "if constexpr" (static if) in C++17.
I've pushed the patch, but I've not added a TODO. I don't understand how "if constexpr" can be used here. Isn't it like an ordinary "if", except that it's evaluated during compilation, and the compiler discards one of its code branches? In SignalProxy we want to select a set of method definitions depending on the type of a template parameter. https://git.gnome.org/browse/glibmm/commit/?id=7ab03b5e8714286e9a8bb58467328c65485b1b72