Bug 26712

+    <!-- 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.