GNOME Bugzilla – Bug 665692
Use constructors correctly
Last modified: 2011-12-10 18:36:28 UTC
If folks is used from Python through introspection, IndividualAggregators can't be constructed properly. This is because we're doing all the initialisation of private members of IndividualAggregator inside the public IndividualAggregator(){} function, rather than inside a construct{} block. In terms of the C generated, it means that the code is put inside folks_individual_aggregator_new() instead of the object_init GObject virtual method (which is where it should be). This can be demonstrated using the following Python program: from gi.repository import Folks def callback (source, result, user_data=None): pass agg = Folks.IndividualAggregator() agg.prepare(callback, None) # Boom! (Provided by Frederik Elwert on the Telepathy ML.) This gives the following stacktrace: $ gdb python GNU gdb (GDB) Fedora (7.3.50.20110722-10.fc16) Copyright (C) 2011 Free Software Foundation, Inc. License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html> This is free software: you are free to change and redistribute it. There is NO WARRANTY, to the extent permitted by law. Type "show copying" and "show warranty" for details. This GDB was configured as "x86_64-redhat-linux-gnu". For bug reporting instructions, please see: <http://www.gnu.org/software/gdb/bugs/>... Reading symbols from /usr/bin/python...(no debugging symbols found)...done. Missing separate debuginfos, use: debuginfo-install python-2.7.2-5.2.fc16.x86_64 (gdb) from gi.repository import Folks Undefined command: "from". Try "help". (gdb) run Starting program: /usr/bin/python [Thread debugging using libthread_db enabled] Using host libthread_db library "/lib64/libthread_db.so.1". Python 2.7.2 (default, Oct 27 2011, 01:40:22) [GCC 4.6.1 20111003 (Red Hat 4.6.1-10)] on linux2 Type "help", "copyright", "credits" or "license" for more information. >>> from gi.repository import Folks >>> def callback(source, result, user_data=None) File "<stdin>", line 1 def callback(source, result, user_data=None) ^ SyntaxError: invalid syntax >>> def callback(source, result, user_data=None): ... pass ... >>> agg = Folks.IndividualAggregator() >>> agg.prepare(callback, None) Program received signal SIGSEGV, Segmentation fault. 0x00007fffee3b2993 in _folks_backend_store_load_disabled_backend_names_co (_data_=0x91c000) at /opt/gnome2/source/folks/folks/backend-store.vala:699 699 this._config_file = file; (gdb) t a a bt
+ Trace 229224
Thread 1 (Thread 0x7ffff7fda700 (LWP 3395))
$6 = (FolksIndividualAggregatorPrivate *) 0x918870 (gdb) print *$6 $7 = {_backend_store = 0x0, _stores = 0x0, _primary_store = 0x0, _backends = 0x0, _link_map = 0x0, _linking_enabled = 1, _is_prepared = 0, __lock__is_prepared = {mutex = {mutex = 0x825d80, unused = {__data = {__lock = 0, __count = 0, __owner = 0, __nusers = 0, __kind = 0, __spins = 0, __list = {__prev = 0x0, __next = 0x0}}, __size = '\000' <repeats 39 times>, __align = 0}}, depth = 1, {owner = 0, dummy = 0}}, _prepare_pending = 1, _debug = 0x0, _configured_primary_store_type_id = 0x0, _configured_primary_store_id = 0x0, _non_quiescent_persona_store_count = 0, _non_quiescent_backend_count = 0, _is_quiescent = 0, _user_configured_primary_store = 0, _individuals = 0x0, _individuals_ro = 0x0, _user = 0x0} (gdb) print *self $8 = {parent_instance = {g_type_instance = {g_class = 0x8ace50}, ref_count = 3, qdata = 0x8a8d20}, priv = 0x918870}
Created attachment 202951 [details] [review] Use constructors correctly https://www.gitorious.org/folks/folks/commits/665692-constructors This should fix the problem. Everything should be explained in the commit message. This should be reviewed very carefully, as I got bored half-way through and probably got sloppy. As mentioned in the commit message, special notice should be taken of any Object() calls which list non-construct properties, such as “alias”.
Review of attachment 202951 [details] [review]: This mostly looks good - thanks for figuring this out and doing the tedious work. One main concern is that I hit this test failure: Starting program: /home/treitter/collabora/folks/tests/eds/.libs/lt-updating-contacts [Thread debugging using libthread_db enabled] /UpdatingContacts/updating contact: [New Thread 0x7ffff20a3700 (LWP 24743)] [New Thread 0x7fffebfff700 (LWP 24757)] Program received signal SIGABRT, Aborted. 0x00007ffff584c405 in raise () from /lib/x86_64-linux-gnu/libc.so.6 (gdb) bt
+ Trace 229230
Other than that, see the following comments: ========================== backends/eds/lib/edsf-persona-store.vala + * This must in GLib key file format. Critical lack of a verb. + Object (id: eds_uid, display_name: eds_uid, source: s); One per line, please. backends/key-file/kf-persona-store.vala - Persona persona = new Kf.Persona (this._key_file, persona_id, this); + Persona persona = new Kf.Persona (persona_id, this); I almost complained that this was an API break. Good thing we never exposed a library for the key-file backend. backends/libsocialweb/lib/swf-persona-store.vala Object (display_name: service.get_display_name(), - id: service.get_name ()); + id: service.get_name (), service: service); One per line, please. backends/telepathy/lib/tpf-persona-store-cache.vala + Object (type_id: "tpf-persona-stores", id: store.id, store: store); One per line, please. backends/telepathy/lib/tpf-persona.vala @@ -297,6 +295,7 @@ public class Tpf.Persona : Folks.Persona, { if (this._alias == alias) { + message ("bail"); return; } @@ -306,9 +305,11 @@ public class Tpf.Persona : Folks.Persona, } this._alias = alias; - this.notify_property ("alias"); + this.notify_property ("alias");message ("finish change_alias with %s", th } I think you forgot to cut these messages. + debug ("Creating new Tpf.Persona '%s' for service-specific UID '%s': %p", + uid, id, this); s/Creating/Created/ folks/individual.vala - private HashSet<PhoneFieldDetails> _phone_numbers; + private HashSet<PhoneFieldDetails> _phone_numbers = + new HashSet<PhoneFieldDetails> ( + (GLib.HashFunc) PhoneFieldDetails.hash, + (GLib.EqualFunc) PhoneFieldDetails.equal); You moved all the similar structures' instantiation from the constructor to the top-level for the class. This ends up putting this code in the instance_init() function, which gets run after the construct() function. And the construct() function has code like: this._phone_numbers_ro = this._phone_numbers.read_only_view; (ie, it ends up calling gee_abstract_set_get_read_only_view (NULL)). At least, that's how I read it - but I'm assuming this would cause a lot of test failures (which I didn't see) and I'm sure you would have hit it. Am I missing something? folks/note-details.vala + Object (value: value, id: uid, parameters: parameters); One per line, please. folks/object-cache.vala /** + * A string identifying the type of object being cached. + * + * This has to be suitable for use as a directory name; i.e. lower case, + * hyphen-separated. + * + * @since UNRELEASED + */ + public string type_id { get; construct; } Maybe s/hyphen-separated/hyphen-separated tokens/ ? folks/phone-details.vala + Object (value: value, parameters: parameters); One per line, please. folks/postal-address-details.vala + Object (value: value, parameters: parameters, id: value.uid); One per line, please. folks/potential-match.vala + public static Set<string> known_email_aliases = + new Gee.HashSet<string> (str_hash, str_equal); private static double _DIST_THRESHOLD = 0.70; private const string _SEPARATORS = "._-+"; - public PotentialMatch () + static construct { - if (this.known_email_aliases == null) - { - this.known_email_aliases = new Gee.HashSet<string> (str_hash, - str_equal); - this.known_email_aliases.add ("admin"); - this.known_email_aliases.add ("abuse"); - this.known_email_aliases.add ("webmaster"); - } + PotentialMatch.known_email_aliases.add ("admin"); + PotentialMatch.known_email_aliases.add ("abuse"); + PotentialMatch.known_email_aliases.add ("webmaster"); } Same construct()/instance_init() problem as above? folks/role-details.vala + Object (value: value, parameters: parameters, id: value.uid); One per line, please. folks/url-details.vala + Object (value: value, parameters: parameters); One per line, please. folks/web-service-details.vala + Object (value: value, parameters: parameters); One per line, please.
Created attachment 203045 [details] [review] Use constructors correctly (updated) https://www.gitorious.org/folks/folks/commits/665692-constructors Fixup commits pushed to the above branch. Updated squashed version of the branch in the attachment. Replies to comments below (thanks for the fast review!).
(In reply to comment #2) > Review of attachment 202951 [details] [review]: > > This mostly looks good - thanks for figuring this out and doing the tedious > work. > > One main concern is that I hit this test failure: Fixed. I didn't realise that the E.Contact in an Edsf.Persona gets updated to a new instance every time _update() is called, and consequently had removed that code. Whoops. > Critical lack of a verb. Fixed. > One per line, please. Fixed (all of them). > I almost complained that this was an API break. Good thing we never exposed a > library for the key-file backend. Yup. Thought I'd take the opportunity to tidy it up a little. > I think you forgot to cut these messages. Whoops. Now cut. > s/Creating/Created/ Fixed. > You moved all the similar structures' instantiation from the constructor to the > top-level for the class. This ends up putting this code in the instance_init() > function, which gets run after the construct() function. And the construct() > function has code like: > > this._phone_numbers_ro = this._phone_numbers.read_only_view; > > (ie, it ends up calling gee_abstract_set_get_read_only_view (NULL)). > > At least, that's how I read it - but I'm assuming this would cause a lot of > test failures (which I didn't see) and I'm sure you would have hit it. Am I > missing something? The constructor() function is required by GObject to chain up to its parent as the first thing it does. The base implementation of constructor() calls the instance_init() functions, meaning that the code in instance_init() should always be called before that in constructor(). This is what we want. I've verified this with a quick test. > Maybe s/hyphen-separated/hyphen-separated tokens/ ? Fixed.
Review of attachment 203045 [details] [review]: Looks good except for this different test failure (it doesn't look like it'd be related to this branch at first glance, but master doesn't hit this): /ChangePrimaryStoreTests/test primary store changes in the IndividualAggregator: folks-WARNING **: Error preparing persona store 'eds:1323387730.3778.1@ridley': Couldn't get view for address book ?1323387730.3778.1@ridley?: The name :1.6 was not provided by any .service files
+ Trace 229237
(In reply to comment #5) > Review of attachment 203045 [details] [review]: > > Looks good except for this different test failure (it doesn't look like it'd be > related to this branch at first glance, but master doesn't hit this): > > /ChangePrimaryStoreTests/test primary store changes in the > IndividualAggregator: > folks-WARNING **: Error preparing persona store 'eds:1323387730.3778.1@ridley': > Couldn't get view for address book ?1323387730.3778.1@ridley?: The name :1.6 > was not provided by any .service files > That (“The name :1.6 was not provided by any .service files”) looks like a D-Bus/jhbuild problem to me, which is only causing the test to fail because it's a g_warning(). The test passes fine for me.
(In reply to comment #6) > (In reply to comment #5) > > Review of attachment 203045 [details] [review] [details]: > > > > Looks good except for this different test failure (it doesn't look like it'd be > > related to this branch at first glance, but master doesn't hit this): > > > > /ChangePrimaryStoreTests/test primary store changes in the > > IndividualAggregator: > > folks-WARNING **: Error preparing persona store 'eds:1323387730.3778.1@ridley': > > Couldn't get view for address book ?1323387730.3778.1@ridley?: The name :1.6 > > was not provided by any .service files > > > > That (“The name :1.6 was not provided by any .service files”) looks like a > D-Bus/jhbuild problem to me, which is only causing the test to fail because > it's a g_warning(). The test passes fine for me. I suspected it might be. And I just re-tried without that failure, so please apply.
Review of attachment 203045 [details] [review]: False warning due to my environment; please apply
Comment on attachment 203045 [details] [review] Use constructors correctly (updated) commit 2247cdd5f71a65f330343da2438e68fb8a3ecfd3 Author: Philip Withnall <philip@tecnocode.co.uk> Date: Tue Dec 6 23:09:14 2011 +0000 Bug 665692 — Use constructors correctly In order to allow libfolks to be used from introspected languages (such as Python) properly, we need to correctly use the GObject construction process, rather than generating code which does all object initialisation inside a *_new() function. This involves moving lots of code into construct{} blocks. There are some complications; mostly the need for various private variables to now be exposed as construct-only properties. Most of them should've been anyway. Other complications arose from the fact that moving code to a construct{} block can subtly change the execution order of the code if the Object() call lists properties which are non-construct properties (e.g. the “alias” property of a Persona). The setters for these properties will now be called _after_ the construct{} code, whereas previously they would've been called beforehand. This rears its head in Tpf.Persona, but hopefully nowhere else. Closes: bgo#665692 NEWS | 18 +++ backends/eds/eds-backend.vala | 5 + backends/eds/lib/edsf-persona-store.vala | 28 ++++-- backends/eds/lib/edsf-persona.vala | 31 ++++-- backends/eds/lib/memory-icon.vala | 4 + backends/key-file/kf-backend.vala | 5 + backends/key-file/kf-persona-store.vala | 41 +++++--- backends/key-file/kf-persona.vala | 37 ++++--- backends/libsocialweb/lib/swf-persona-store.vala | 26 +++-- backends/libsocialweb/lib/swf-persona.vala | 20 ++-- backends/libsocialweb/sw-backend.vala | 5 + backends/telepathy/lib/tpf-logger.vala | 11 ++- .../telepathy/lib/tpf-persona-store-cache.vala | 13 ++- backends/telepathy/lib/tpf-persona-store.vala | 5 +- backends/telepathy/lib/tpf-persona.vala | 110 +++++++++++--------- backends/telepathy/tp-backend.vala | 5 + backends/tracker/lib/trf-persona-store.vala | 39 ++++--- backends/tracker/lib/trf-persona.vala | 47 +++++++-- backends/tracker/tr-backend.vala | 5 + folks/abstract-field-details.vala | 4 +- folks/avatar-cache.vala | 5 + folks/backend-store.vala | 5 + folks/debug.vala | 4 + folks/individual-aggregator.vala | 5 + folks/individual.vala | 107 +++++++++---------- folks/note-details.vala | 9 +- folks/object-cache.vala | 55 +++++++--- folks/phone-details.vala | 5 +- folks/postal-address-details.vala | 13 ++- folks/potential-match.vala | 18 ++-- folks/role-details.vala | 13 ++- folks/url-details.vala | 5 +- folks/web-service-details.vala | 5 +- 33 files changed, 452 insertions(+), 256 deletions(-)