GNOME Bugzilla – Bug 380581
allow object classes to override signal connect/disconnect
Last modified: 2018-05-24 10:54:49 UTC
as a gobject subclass i might be interested in handling signal connect/disconnect for myself. i may, for example want to block a connect from occuring. or (and this is the immediate intended use) i may want to know when connects/disconnects happen. in this case i'd override the default implementation and chain my parent implementation from my handler. my handler would also be able to do other things. this is useful because there might be quite some hard work involved in generating signals. consider the rationale behind g_signal_has_handler_pending. this exists because sometimes it is hard to calculate arguments to a signal and therefore it would be better to just not emit it at all. sometimes, though, it is difficult to even know when a signal should be sent at all since you have to setup some sort of a poll/watch which allows you to emit the signal. if you know that nobody is watching you can avoid doing this, but you need to be told when someone does start watching. the most immediate example is the GtkRecentManager thing. it sends this "changed" signal when the recent list has been updated. in order to do this, however, it has to monitor the list (currently by polling it with stat). if nobody is watching "changed" then this is wasteful.
Created attachment 77353 [details] [review] a patch. this patch allows the described functionality in the most minimally intrusive way that i can think of.
concrete motivation: i'm on a crusade to kill all timer abuse. applications that wake up every so often to do polls drive me crazy. currently the GtkRecentManager wakes up every so often to stat the recent list to see if it has changed. this is bad. even if GtkRecentManager had access to some sort of file notification mechanism, it will still wake up periodically (the inotify code in gnome-vfs still has timers which run every 4 seconds or so), so having file notification support that gtk can use is only a partial solution. the real solution that i am proposing is thus: 1) deprecate the act of connecting to the "changed" signal on GtkRecentManager. document this deprecation. 2) add an API to GtkRecentManager to manually check if a change has occured. this can be done when the user pulls down the file menu (or whatever). this will use stat to check if the file is the same so the usual case will be quite fast. it definitely will never be more work than the way it is currently done. 3) add the patch in this bug to glib 4) take advantage of this new glib ability to allow GtkRecentManager to disable polling per default. polling would only be enabled when applications connect to the deprecated "changed" signal. if you connect to the deprecated signal you suffer the consequences. 5) applications smarten up and do things as described in (3). step (3) is the one that needs to happen first, since the others depend on it quite directly. this is why i file this bug. i told mclasen about this plan and he told me something like "it sounds like you have a plan... now make it happen". so i am trying to make it happen now :)
point 5 should read "...as described in (2)".
(In reply to comment #0) > as a gobject subclass i might be interested in handling signal > connect/disconnect for myself. the recommended way to cope with this is to introduce parallel _connect/_disconnect APIs for your subclass. > i may, for example want to block a connect from occuring. the way blocking is supported for GSignal is via _block/_unblock operations on signal handler IDs. not by avoiding connections intransparently to the user up front. > or (and this is the immediate intended use) i may want to know when > connects/disconnects happen. in this case i'd override the default > implementation and chain my parent implementation from my handler. my handler > would also be able to do other things. _connect/_disconnect are *not* GObject methods, which has certain implications: - currently, a GObject can live in one thread and be connected/disconnected to in another thread. if signal connections were to emit notifications or execute class methods, this threading scenario will break. - GSignal is a mechanism for *all* instantiatable types, instances with signals need not be GObject derived instances. > this is useful because there might be quite some hard work involved in > generating signals. consider the rationale behind > g_signal_has_handler_pending. this exists because sometimes it is hard to > calculate arguments to a signal and therefore it would be better to just not > emit it at all. sometimes, though, it is difficult to even know when a signal > should be sent at all since you have to setup some sort of a poll/watch which > allows you to emit the signal. if you know that nobody is watching you can > avoid doing this, but you need to be told when someone does start watching. you have a good point here. i don't see a clean way to provide you with the notification you're asking for though. that is, given the current design of GType/GObject/GSignal and they way they may interact in threaded scenarios. if GObjects were tied to a certain main loop (which GtkObjects are), notifying objects in the correct thread would at least theoretically be possible, but it'd still have other problems like reentrancy in the notifiers, alive-keeping for notification and certainly have performance implications. > the most immediate example is the GtkRecentManager thing. it sends this > "changed" signal when the recent list has been updated. in order to do this, > however, it has to monitor the list (currently by polling it with stat). if > nobody is watching "changed" then this is wasteful. the only way i see to help with your scenario is if you introduce extra API to request the notification you're asking for, like: void (*foo_notify) (GObject*, ...); void request_foo_notify(); /* need_foo_notify++; */ void unrequest_foo_notify(); /* need_foo_notify--; */
(In reply to comment #2) > currently the GtkRecentManager wakes up every so often to stat the recent list > to see if it has changed. this is bad. i understand your motivation here and think the solution you came up with sounds elegant, but unfortunately can't be made to work that way with the current GSignal system. giving this some further thought, delivering proper notification for changes in signal connections (basically, notifications about when the state of signal_hanmdler_pending will change) isn't entirely impossible with the current signal system, but it'd require significantly new programming concepts (strict binding of GType instances or GObjects to main loops) and major implementation effort in libgobject (virtualization of signal connection notification at the level of instantiatable fundamental types). and it could possibly badly affect the common case where no notification is required, because GSignal has *many* more cases (particularly with regards to disconnects) which would need to emit notification than what is covered by your patch, and for memory and speed optimization reasons, those don't necessarily have access to the originating instance anymore, or might operate after finalization of instances. > even if GtkRecentManager had access to some sort of file notification > mechanism, it will still wake up periodically (the inotify code in gnome-vfs > still has timers which run every 4 seconds or so), so having file notification > support that gtk can use is only a partial solution. > > the real solution that i am proposing is thus: [...] i'd say you need to essentially deprecate the current API and introduce either API like request_foo_notify/unrequest_foo_notify, or API like parallel _connect/_disconnect APIs for your subclass. one thing i'd like to add to my previous comment about this is: - if you use request_foo_notify/unrequest_foo_notify, you put the burden on the user, it's uneasy to request/unrequest notification along side signal connections, considering that signals may be automatically disconnected, e.g. because one argument is an object and g_signal_connect_object() was used for setting up the connection. - if you use parallel _connect/_disconnect APIs, you can make sure to get notification even about automated disconnects by attaching invalidation notifiers to all connected closures. just keep in mind that those may be invoked by any thread. i'm sorry, but for the moment, i'm going to close this bug report as rejected-feature-request, because implementing it would be considerably hard in multiple regards (implementation complexity and in terms of complicated new API for users), and because it'd provide comparatively small gains (over roughly 10 years, i've seen maybe 2 or 3 requests for connect/disconnect notification, all of which were scenrios that could possibly be solved reasonably with alternative measures).
We're hitting up against so many cases of objects that really really want to know (at the very least) when their user has disconnected the last handler for a particular signal. We should reconsider this.
Created attachment 291045 [details] [review] Add GObject::signal_handlers_changed vfunc This virtual function will be called when a signal handler is connected or disconnected on the object. The main use for this is for avoiding watching for notifications from external sources (filesystem, D-Bus, etc.) when nobody actually cares about the changes. If we can find out when someone connects a signal handler then we can hold off on monitoring until that point.
Created attachment 291046 [details] [review] tests: test GObject::signal_handlers_changed
Created attachment 291051 [details] [review] Add GObject::signal_handlers_changed vfunc This is a slightly tweaked version of the patch above. Instead of calling the notify function on the vtable of the class that registered the signal it calls it on the vtable of the object that the signal is being emitted on. This is slightly less efficient but it means that subclasses and interface implementations can watch for signal connection.
Review of attachment 291051 [details] [review]: ::: gobject/gtype.c @@ +4870,3 @@ +g_type_do_nothing (void) +{ +} I guess this ends up in gtype.c just because we already have a gtype-private.h ? Why have this anyway - comparing with NULL not sufficient ?
Review of attachment 291051 [details] [review]: To facilitate unconditional chain-up. I hate having to check before NULL in every handler before chaining up.
I'm not really convinced that the chaining up is very useful, though. Doesn't that mean I have to carefully filter out all my subclasses' signals ? Would be good to see an example of the expected usage for this.
(In reply to comment #12) > I'm not really convinced that the chaining up is very useful, though. Doesn't > that mean I have to carefully filter out all my subclasses' signals ? > > Would be good to see an example of the expected usage for this. I expect that people doing this would do something like: changed (signal_id, ...) { if (signal_id == MY_SIGNAL_ID) { ... stuff ...; } chainup; } so indeed they would end up filtering out everything else. I will try to write a patch for GSettings based on this.
to make my opinion a bit clearer: I prefer the first patch for its efficiency and its better API and not needing to chain up. I prefer the second patch because it means that objects can watch for signals that they themselves did not define. This is particularly important for interfaces.
(In reply to comment #13) > > changed (signal_id, ...) { > if (signal_id == MY_SIGNAL_ID) > { > ... stuff ...; > } > > chainup; > } > Missing a return there :-) So easy to get this wrong...
return intentionally excluded. Assuming this was some GSettings subclass that wanted to know when someone connected to the changed signal, it would definitely not want to block GSettings itself from discovering that. that said, I did say *MY*_SIGNAL_ID... :)
assuming that signal ids are globally unique (otherwise this whole thing wouldn't work), then letting MY_SIGNAL_ID chain up is pointless - it will never match a signal of the base class. Therefore, you _should_ return after ...stuff...
*** Bug 635054 has been marked as a duplicate of this bug. ***
Bug #635054 has some relevant patches too.
-- 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/glib/issues/70.