[Bug 26712] Telepathy-logger should support storing and retrieving favorite Telepathy contacts

[bugzilla-daemon at freedesktop.org]

http://bugs.freedesktop.org/show_bug.cgi?id=26712





--- Comment #1 from Danielle Madeley <danielle.madeley at collabora.co.uk>  2010-02-23 15:32:32 PST ---
+    <!-- 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.


-- 
Configure bugmail: http://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.