[Bug 25766] Review Gabble Google Mail Notification

[bugzilla-daemon at freedesktop.org]

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





--- Comment #1 from Danielle Madeley <danielle.madeley at collabora.co.uk>  2009-12-23 17:53:19 PST ---
conn-mail-notify.c

70  if (new_owner == NULL || new_owner[0] == '\0')

We should move CHECK_STR_EMPTY to utils and use it.

175  GString *url = g_string_new (base_url);
176  g_string_append_printf (url, "/#inbox/%" G_GINT64_MODIFIER "x", tid);

Could use g_strdup_printf here, surely.

387  query = wocky_xmpp_stanza_build ( WOCKY_STANZA_TYPE_IQ,

Superfluous space.

396   DEBUG("%s", result_str);

Missing space.

429 static gboolean
430 new_mail_handler (WockyPorter *porter, WockyXmppStanza *stanza, 
431     gpointer user_data)

Not in Telepathy style. See Example 4 in
http://telepathy.freedesktop.org/wiki/Style. Next function is the same.
conn_mail_notif_properties_getter also wrong, but differently so.

532 static GPtrArray*
533 get_unread_mails (GabbleConnection *conn)
534 {
535   GPtrArray *mails = g_ptr_array_new ();
536   GHashTableIter iter;
537   gpointer value;
538   g_hash_table_iter_init (&iter, conn->unread_mails);

Also in style: one line of space between type declarations and code. Also a
space before * -> "static GPtrArray *"

586       g_value_set_boxed (value, mails);

You can use g_value_take_boxed() here instead. No need to free it then.

Other notes:
 - as discussed, GData is the wrong data structure. You will leak a Quark for
   every sender that is subscribed. GData should only really be used when you
   have a  fixed number of strings (that is, you could look the Quarks up in
   advance)
 - lots of trailing spaces
 - I'm concerned it's not very obvious how support for other mail protocols
   should be added


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