It'd be handy and nicely-centralized to have the Telepathy logger store and manage a set of favorite Telepathy contacts. I've created a branch of the logger which adds and implements a simple API for this. The upside to doing this in one place is that different Telepathy-based IM clients can use the same set of favorites. Danielle, could you please review this? You can browse my branch at the given URL (it's up-to-date with the mainline, as of this writing). The only change is the last commit (though it's several hundred lines; splitting it didn't seem useful). There are a few open questions (comments including the keyword "XXX"). One of the bigger ones (which may or may not still have a comment in my latest version) is whether the favorite contacts functionality should have its own namespace (vs. org.freedesktop.Telepathy.Logger[.DRAFT]). I also wish I could have avoided the FavouriteContactsIsReady property and FavouriteContactsReady signal altogether. But there doesn't seem to be any other way that doesn't either expose a race condition between the logger deserializing the favorite contacts file or adding some excessive infrastructure (like queues for method calls that arrive between the start and end of that deserialization). Beyond that, the first is gramatically awkward (and inappropriate for a property?) and they sound like synonyms to me anyhow. I'm looking forward to any and all critiques.
+ <!-- XXX: would it be better for this to be the type a(oas) - the object + path to the account and an array of contact IDs? --> I feel: yes. + <property name="FavouriteContactsIsReady" + tp:name-for-bindings="Favourite_Contacts_Is_Ready" type="b" access="read"> + <tp:docstring> + Whether the <tp:dbus-ref + namespace="org.freedesktop.Telepathy.Logger.DRAFT">FavouriteContacts</tp:dbus-ref> property is valid and may be manipulated. + </tp:docstring> + </property> I don't like this at all. If there is a delay in loading the data from file, this should probably be replaced by a GetFavouriteContacts method that can delay as long as it needs to prepare the data. + <signal name="FavouriteContactAdded" + tp:name-for-bindings="Favourite_Contact_Added"> + <tp:docstring> + The contact is now a favourite. + </tp:docstring> I think that this should be a signal FavouriteContactsChanged, in the same vein as MembersChanged, with parameters: account, added, removed (oasas) + if (filename) + return filename; Not Telepathy style. + dir = g_build_filename (g_get_user_data_dir (), TPL_DATA_DIR, NULL); + filename = g_build_filename (dir, FAVOURITE_CONTACTS_FILENAME, NULL); + g_free (dir); Why not use one single g_build_filename() ? +static gboolean +favourite_contacts_parse_string (TplDBusService *self, + const gchar *content) I know this file should never get super long, but I tend to not like loading files into memory to parse them when you can parse them on the fly, consider using g_data_input_stream_read_line_async() ? Is it worth using sqlite3 to store this information? + closure = g_new0 (FileSaveClosure, 1); + g_free (closure); Should use GSlice + GValueArray *value_array; + GValue value_a = {0}; + GValue value_b = {0}; + gchar *str_a; + gchar *str_b; Use tp_value_array_build() + you don't need to allocate those strings + you're leaking the GValues you initialised. + GPtrArray *array; Create in scope. Although I think you should replace these properties.
(In reply to comment #1) I've addressed all of these concerns in the two new commits to favorites-try2, except for changing the signals to FavouriteContactsChanged (which I just realized). Danielle, could you please re-review this? It's probably easiest to view all three changes together, since there's a lot of noise between the first and third commits (the second commit just extends the TplActionChain API).
(In reply to comment #2) > (In reply to comment #1) > I've addressed all of these concerns in the two new commits to favorites-try2, > except for changing the signals to FavouriteContactsChanged (which I just > realized). > > Danielle, could you please re-review this? > > It's probably easiest to view all three changes together, since there's a lot > of noise between the first and third commits (the second commit just extends > the TplActionChain API). > And, for what it's worth, I did a bit of careful manual testing of this new code - of course it'd be better with some real, automated tests, but I'm a bit cramped on time right now. Most of the race-condition-solving method handler queuing that made this last couple of commits so complicated won't happen in real life very often at all, but it seems pretty solid now.
+ link->action (self, link->user_data); + g_slice_free (TplActionLink, link); Minor: use link_free() ? I am happy with the TplActionChain changes though. Those can be merged immediately. + <signal name="FavouriteContactsReady" + tp:name-for-bindings="Favourite_Contacts_Ready"> + <tp:docstring> + The <tp:dbus-ref + namespace="org.freedesktop.Telepathy.Logger.DRAFT">FavouriteContacts</t + </tp:docstring> + </signal> You can remove this now, right? + g_hash_table_insert (contacts, g_strdup (contact_id), "placeholder"); 0x1 is more traditional. or GINT_TO_POINTER (TRUE). + contact_ids = g_malloc0 (sizeof (gchar*) * (g_hash_table_size (contacts)+1)); g_new0 -> less braces :) + contact_ids[i] = g_strdup ((const gchar*) l->data); + g_strfreev (contact_ids); Save yourself the strdup here and just g_free (contact_ids) when you're done? + g_hash_table_foreach (priv->accounts_contacts_map, + (GHFunc) append_favourite_contacts_account_and_contacts, packed); Would a GHashTableIter be more useful here? +#define TPL_DATA_DIR "TpLogger" Better to export $(datadir) from Makefile.am + FavouriteContactClosure *closure = (FavouriteContactClosure*) user_data; Isn't this cast redundant? + if (action_chain != NULL) + tpl_actionchain_continue (action_chain); Too much indenting.
> + <signal name="FavouriteContactsReady" > + tp:name-for-bindings="Favourite_Contacts_Ready"> > + <tp:docstring> > + The <tp:dbus-ref > + > namespace="org.freedesktop.Telepathy.Logger.DRAFT">FavouriteContacts</t > + </tp:docstring> > + </signal> > > You can remove this now, right? I think this shows how annoying the distinction between FavouriteContactsReady and FavouriteContactsIsReady would have been - I removed the latter and forgot about the former. > + contact_ids = g_malloc0 (sizeof (gchar*) * (g_hash_table_size > (contacts)+1)); > > g_new0 -> less braces :) Er, right. I mixed it up in my mind with some other function that doesn't let you specify the count. > + g_hash_table_foreach (priv->accounts_contacts_map, > + (GHFunc) append_favourite_contacts_account_and_contacts, packed); > > Would a GHashTableIter be more useful here? I find a foreach more compact and easier to read. > +#define TPL_DATA_DIR "TpLogger" > > Better to export $(datadir) from Makefile.am Isn't that always some absolute path that the user doesn't usually have write access to (eg, default /usr/local/share)? Do you mean something like $(PACKAGE_NAME) (so we end up with $(XDG_DATA_DIR)/$(PACKAGE_NAME))? I've made that change locally. "TpLogger" is somewhat historical, since the logger used to use that for something else, but apparently doesn't any longer. I've also made the other fixes you've suggested (and trivial style fixes to satisfy "make check"). Please review the last two commits on branch favorites-try3. It's re-based upon the latest mainline.
Squish and Merge!
(In reply to comment #6) > Squish and Merge! > Merged.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.