GNOME Bugzilla – Bug 674913
gdbus-codegen does not honor "Property.EmitsChangedSignal" annotations
Last modified: 2018-05-24 14:11:47 UTC
I'm wrinting a D-Bus service using GDBus and gdbus-codegen to implement MPRIS2 in my application. It is required that one of the properties does not emit the org.freedesktop.DBus.Properties.PropertiesChanged signal when it is changed (http://specifications.freedesktop.org/mpris-spec/latest/Player_Node.html#Property:Position). So I put this in my XML file: <property name="Position" type="x" access="read"> <annotation name="org.freedesktop.DBus.Property.EmitsChangedSignal" value="false"/> </property> (all other properties have this set to "true"). However, when using the code generated by gdbus-codegen, the PropertiesChanged signal is still emited when I set this property in my code, and I can't find a way to disable that. This problem seems to have been acknowledged on the mailing-list (http://lists.freedesktop.org/archives/dbus/2012-March/015016.html), but I could not find a relevant bug here. I'm using glib 2.32.1 on Arch Linux. Could someone please add support for the EmitsChangedSignal annotation in gdbus-codegen? Or at least give me pointers on how to do it myself? (Disclaimer: I know very little about D-Bus and Glib internals. For example I have never written a new type deriving from GObject. In my code I'm using the generated interface with *_skeleton_new() and signal handlers...) Thanks!
Yea, it would be good to support this in gdbus-codegen(1)
Created attachment 227209 [details] [review] patch adding support for EmitsChangedSignal Here is a patch that adds support for the EmitsChangedSignal annotation to gdbus-codegen. If absent or set to "true", this doesn't change anything to the previous behaviour. If present and set to "false", changing the associated property won't trigger a call to IFACE_schedule_emit_changed. I've tested this patch with my little project that uses gdbus-codegen and it seems to work fine, but more tests on real-world applications would be much appreciated.
Review of attachment 227209 [details] [review]: Looks pretty good, thanks, but I would also like to see a test case for this in the test suite. The test should go into gio/tests/gdbus-test-codegen.c probably in the check_bar_proxy() function and use the same trick to check for the *absence* of the PropertiesChanged signal.... If you prefer, I can write the test case. ::: gio/gdbus-2.0/codegen/codegen.py @@ +103,3 @@ ' const gchar *hyphen_name;\n' ' gboolean use_gvariant;\n' + ' gboolean emits_changed_signal;\n' I would change this to guint use_gvariant : 1; guint emits_changed_signal : 1; to pack these two fields together and save some room in the struct - unfortunately it doesn't work with gboolean because it's a gint... ::: gio/gdbus-2.0/codegen/dbustypes.py @@ +354,3 @@ + if utils.lookup_annotation(self.annotations, 'org.freedesktop.DBus.Property.EmitsChangedSignal') == 'false': + self.emits_changed_signal = False According to http://dbus.freedesktop.org/doc/dbus-specification.html#introspection-format the rules are pretty complicated If set to false, the org.freedesktop.DBus.Properties.PropertiesChanged signal, see the section called “org.freedesktop.DBus.Properties” is not guaranteed to be emitted if the property changes. If set to invalidates the signal is emitted but the value is not included in the signal. If set to true the signal is emitted with the value included. The value for the annotation defaults to true if the enclosing interface element does not specify the annotation. Otherwise it defaults to the value specified in the enclosing interface element. I don't think we want to support all this (only support 'false' on annotations for the signal as you do is good enough) but I do think we need to state in a code comment referencing this bug that this is all we do for now. Suggest to just add # NOTE: for now we only support 'false' on the signal itself, see #674913 and # http://dbus.freedesktop.org/doc/dbus-specification.html#introspection-format # for details
Created attachment 247270 [details] [review] corrected patch adding support for EmitsChangedSignal David, Thanks a lot for your valuable advice, and sorry for the veeeeery long delay :) Here is an updated patch against master that addresses your suggestions and adds a test case. It works fine for my application as well. Please tell me if there's more I can do to help. This time I won't take 8 months to reply :)
I would also like to see this fixed, since Audacious is now using GDBus to implement MPRIS 2.
I too would also like to see this fixed, if possible, as I'm trying to add an MPRIS 2 interface to Shairport Sync.
-- 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/542.