Bug 25766

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

(In reply to comment #1)
> 586       g_value_set_boxed (value, mails);
> 
> You can use g_value_take_boxed() here instead. No need to free it then.

That would require to add a ref on every mail hash table. Also, I don't want to do that since mail hash table could be changed before the get operation completes. Note that the mail are copied since the boxed type is more then GPtrArray.

>  - I'm concerned it's not very obvious how support for other mail protocols
>    should be added

There is no XMPP spec for that, and that it's not clear what could be shared with those unkown protocol, I don't think it worth the effort. The result would have to be reworked anyway. (The Google specific part is update_unread_mails () plus the new-mail signal. Which is everything, except data structure, getter and subscription.)


Other Items has been fixed:

commit 8b70db3a5473259cf61c3f81e519ed0806b72229
    Mail Notification: Fix coding style

commit 3a4801b26c7c1e60380f83090696f9b4a82c1448
    Mail Notification: Replace g_string_append_printf by g_strdup_printf()

commit 43d7828b1fe70090b254123a56463a6c93c523f8
    Moved macro CHECK_STR_EMPTY to util and use it where possible

commit 74968d90295cd5bc487bd8ea2aa272d64487e44a
    Changed mails_subscribers into HashTable

(In reply to comment #2)
> (In reply to comment #1)
> > 586       g_value_set_boxed (value, mails);
> > 
> > You can use g_value_take_boxed() here instead. No need to free it then.
> 
> That would require to add a ref on every mail hash table. Also, I don't want to
> do that since mail hash table could be changed before the get operation
> completes. Note that the mail are copied since the boxed type is more then
> GPtrArray.

Ok. That's a valid point.

> >  - I'm concerned it's not very obvious how support for other mail protocols
> >    should be added
> 
> There is no XMPP spec for that, and that it's not clear what could be shared
> with those unkown protocol, I don't think it worth the effort. The result would
> have to be reworked anyway. (The Google specific part is update_unread_mails ()
> plus the new-mail signal. Which is everything, except data structure, getter
> and subscription.)

Ok. Perhaps we should document somewhere at the top that this currently only supports the Google mail notification spec. Possibly with a link to how that works?

Some more review:

125   if (g_hash_table_lookup_extended (conn->mail_subscribers, sender, NULL, NULL))
126     {
127       DEBUG ("Sender '%s' is already subscribed!", sender);
128       goto done;
129     }

Looks like it leaks 'sender'

529 static gboolean
530 foreach_cancel_watch (gpointer key,
531     gpointer value,
532     gpointer user_data)
533 {
534   GabbleConnection *conn = user_data;
535   const gchar *sender_name = user_data;

How can these both be 'user_data' ?

Additionally, when casting GObjects, use the cast macro to ensure validity (even if you don't have to because it's a void ptr), so it would be:

GabbleConnection *conn = GABBLE_CONNECTION (user_data);

There is a lot of implicit reference passing in this code, and it's very subtle. I'm wondering if perhaps some comments might be useful for clearing up who takes ownership of what references.

Some other style things:
 - Line 145, put the first parameter on the next line so that it fits
 - Line 183, comment wraps
 - Line 361, you're not meant to align arguments -- see style Example 5
 - Line 363, missing space
 - Line 372, misspelt "guarantee"
 - Line 510, missing return
 - Line 606, too much indenting
 - Line 646, "Unknown" is misspelt

(In reply to comment #3)

Fixed:

ommit 60349a138708a90c5e1b870ae74d537d15aa91e0
    Mail Notification: More style fix and introduction comment on top

commit e3b49c42aac5e324dee456900bc1ff528bba6913
    Mail Notification: Highlight ownership passing with comments

commit a2d393795945d5f71a728f1031feaa5249fa5101
    Mail Notification: Fix memory leak on subscribe while already subscribed

commit d75ca7712756733cf982cb358d32bd241ae86861
    Mail Notification: Fix wrong cast and added object cast macro

Added unit test for google mail notification in Gabble and fixed a crash found during test.

commit b4ad56f28f7ececf67ffddee44f9be43685fca21
    Adding unit test for Google mail notification

commit 70adaf177abd765c88eb310c087a6763001632fa
    Don't crash when attribute unread_mails HashTable is NULL

Updated code to latest Mail Notification spec. No new feature, thanks for reviewing.

Spec has been updated, here a commit that port to the new interface.

commit 8bd3e7536bb303066df9d58376eadf23ead11acd
Date:   Wed Jan 27 17:20:52 2010 -0500
    Bumped to latest MailNotification.DRAFT
    - Subscribe/Unsubscribe is now refcounted
    - Name or flags has been changed and flags exist for URL request
    - Mail "snippet" is now stored in "content" with "truncated set to TRUE
    - Mail "url_data" is now "url-data"
    - Timestamps are now 64 bit signed integer

Needs minor porting to the current draft of MailNotification: URL_Data is
now a variant, etc. Before merging, the XML should be updated from
the telepathy-spec 0.19.1 release (when I've made that), and any necessary
updates should be made at that point.

These review comments don't include the actual implementation in
conn-mail-notif.[ch], just the supporting code; I'll come back to the actual
implementation in a moment.

Notes for the future
====================

Using CHECK_STR_EMPTY() in irrelevant places was inappropriate for this branch:
I'd have preferred this to be added on a separate branch that was
"fast-tracked" in (to avoid interdependent changes). However, now that you've
done it, we might as well just include it in this branch.

(I also don't like the naming, once it leaks out of a single source file,
but I'll fix that later.)

The addition of add_query_node to make_result_iq should have been a separate
commit, on a trivia branch that was insta-reviewed and fast-tracked in.

The comment "Unforbids events" in the test adds no information; in future
please don't comment things that are already obvious from the code.

Trivial changes
===============

Patching connection_disco_cb adds trailing whitespace; don't do that (I think
git can be configured to warn you about wrong whitespace while committing?).
The same occurs in struct _GabbleConnection, in GoogleXmlStream in
gabbletest.py, and in test-mail-notification.py.

The TpDBusDaemon in struct _GabbleConnection isn't specific to
MailNotification (even though it was added for this branch) so I'd prefer
it not to be under the MailNotification heading. I'm thinking of adding
tp_base_connection_get_dbus_daemon()...

I don't like the abbreviation "mail notif"; we don't usually abbreviate.
In particular, the debug key in debug.[ch] is UI (of a sort) and should
just be called "mail", even if we keep "mail_notif" in function and file
names.

New test scripts shouldn't start with "test-", which just interferes with
tab completion.

In test-mail-notification.py, check_properties_empty doesn't have Python-style
whitespace:

    -def check_properties_empty (conn, capabilities = 0):
    +def check_properties_empty(conn, capabilities=0):

(yes, that whitespace convention differs from the one we use for C, sorry).

Typos in test-mail-notification.py: functionnality -> functionality,
notificagtion -> notification, lets use -> let's use, date are -> dates are,
thread3_subject should probably not be "subject2" (likewise for body),
gabble query mail data -> gabble queries mail data, unkown -> unknown
(multiple times), "is not support" -> is not supported (twice),
"that Gabble react correctly" -> reacts correctly, "Make sure method return"
-> method returns.

test-mail-notification.py should use cs.NOT_IMPLEMENTED rather than inventing
its own not_implemented constant.

OK, I'm done with reviewing; please put the patch keyword back when this is ready for further review. There's no need to reassign to me, I receive telepathy-bugs mail.

Review of conn-mail-notif.[ch]:

Notes for the future
====================

Pedantically, conn-mail-notif.h should include <glib-object.h> instead of or
in addition to <glib.h>, since it references GObject. (This isn't very
important, since connection.h necessarily includes <glib-object.h> anyway,
but it would become more important in library code.)

Trivial changes
===============

conn-mail-notif.[ch] are copyright 2009-2010 Collabora Ltd., not just 2009.

Why is namespaces.h included right at the beginning of conn-mail-notif.c?
I'd have put it with connection/debug/util. Likewise for
extensions/extensions.h, which is part of Gabble.

(You were right to include conn-mail-notif.h immediately after config.h
though, since this is an implicit assertion that it's self-contained.)

conn-mail-notif.c contains trailing whitespace; it shouldn't.

Coding style: many occurrences of [a-zA-Z]( should have a space before the
parenthesis (although #define IMPLEMENT(x) must not have a space there,
due to cpp syntax).

Coding style: we compare pointers to NULL with "pointer != NULL", not with
"!pointer", and compare integers to zero explicitly too. The only case
where there's no explicit operator should be gboolean.

Coding style: the outer "if" in unsubscribe() has {}, therefore the
corresponding "else" should have them too (even though it doesn't strictly
need them).

Coding style: we prefer blank lines around each if{}, if{}else{}, while{},
etc., like this:

>    node = wocky_xmpp_node_get_child (parent_node, "snippet");
> +
>    if (node)

conn-mail-notif.c should reference the API by its English URL :-) - remove
/intl/fr.

I don't like the assumption that a zero-filled GPtrArray is valid. Does GLib
explicitly say so? If not, then we shouldn't assume this. Since empty_array
is only used in dbus-glib method calls, which use deep-copying and a lot of
g_malloc'ing already, I'd just allocate a new empty GPtrArray in each function
that needs one and free it afterwards.

sender_name_owner_changed() should refer to the unique name as "client",
"subscriber" or "unique_name", not as "sender" (the message being processed
here wasn't sent by the client). It's up to you whether you rename the variable
"sender" in gabble_mail_notification_[un]subscribe() to be consistent with that
or not (in these contexts, the client *is* the sender).

In struct MailsHelperData, the hash tables and array should have a brief
comment explaining what's in them and the ownership (owned / weak ref /
borrowed / etc., and if it's borrowed, mention who owns the reference and
ensures that it remains valid). A hypothetical example:

  /* name (string) borrowed from the mushroom => GMushroom ref */
  GHashTable *mushrooms;

query_unread_mails_cb shouldn't debug-output the whole node - Wocky will
output all received nodes (if given appropriate debug flags) anyway.

I don't like the name of struct MailsHelperData. It appears to be a closure
for the call to mail_thread_info_each in store_unread_emails? If that's the
case, then it should be declared just above mail_thread_info_each,
and should be typedef'd so you don't have to keep saying "struct". something
like this:

    typedef struct {
    ...
    } MailThreadCollector;

(then replace all "struct MailsHelperData *" with "MailThreadCollector *").

> +  if (data.mails_added->len || mails_removed->len)

Coding style: this should be of the form if (x > 0 || y > 0) (i.e. compare
against 0 explicitly).

> +  DEBUG ("reached");

I suspect/hope this is no longer useful :-)

> +          g_hash_table_ref (mail);
> +          g_hash_table_remove (data->old_mails, tid);

You can use g_hash_table_steal to avoid it being freed prematurely?

> +      handle_senders (node, mail, &dirty);

I think I'd prefer three occurrences of:

    if (handle_foo (node, mail))
        dirty = TRUE;

The final "else" clause of conn_mail_notif_properties_getter should be in
{}, and should just be a g_assert_not_reached (it can only be reached if
someone adds a property to the GabbleConnection and fails to add it to the
getter).

Logic
=====

In gabble_mail_notification_request_inbox_url (also request_mail_url):
> +  /* TODO Make sure we don't have to authenticate again */

Either fix that TODO, declare it to be unimportant, or file a bug.

(One of my spec changes was to relax the pre-authenticated URL from
"pre-authenticated" to "pre-authenticated if possible", so I think it would
be OK to just delete these.)

In gabble_mail_notification_request_inbox_url (also r_m_u), how could
conn->inbox_url be NULL? Shouldn't you raise NotAvailable if that's the case?

In gabble_mail_notification_request_mail_url, why do you convert in_id
to an int64 and then back to a string? Can't you just incorporate it into
the URL with %s?

In conn_mail_notif_init, if conn->daemon is NULL, all future methods are
basically going to assert. I think it would be reasonable to use g_error
here, since Gabble connects to the session bus as the first thing it does.

In conn_mail_notif_properties_getter, is username@stream-server definitely
correct for all Google servers? In particular I'm thinking of: SRV records,
gmail.com vs. googlemail.com, Google Apps For Your Domain.

If stream-server is the "logical" name of the server (gmail.com/googlemail.com
rather than talk.google.com), you don't need to encode the JID, you could just
use the self-handle (tp_base_connection_get_self_handle()) and stringify it
with the appropriate TpHandleRepo.

If it's the "physical" name of the server (talk.google.com) then I think the
logic's wrong.

In sender_each:

> +          0, wocky_xmpp_node_get_attribute (node, "name") ?: "",

I'm pretty sure ?: isn't standard C, and it's certainly not clear.
I'd prefer the struct to be constructed with tp_value_array_build()
(which probably requires a telepathy-glib 0.10 dependency). However,
I don't think the sender should be added at all if they don't have an
email address, and the name should probably default to the email address
rather than the empty string?

ASSIGNED status, since this is actively being worked on.

(In reply to comment #8)
> Needs minor porting to the current draft of MailNotification: URL_Data is
> now a variant, etc. Before merging, the XML should be updated from
> the telepathy-spec 0.19.1 release (when I've made that), and any necessary
> updates should be made at that point.

Ported yesterday, but if we undraft we still need to update. For the unit test see:

commit d7b2d39df88760a26952662564935c0d52660a63
    Fixed mail-notification test for new spec

> 
> These review comments don't include the actual implementation in
> conn-mail-notif.[ch], just the supporting code; I'll come back to the actual
> implementation in a moment.

Ok.

> 
> Notes for the future
> ====================
> 
> Using CHECK_STR_EMPTY() in irrelevant places was inappropriate for this branch:
> I'd have preferred this to be added on a separate branch that was
> "fast-tracked" in (to avoid interdependent changes). However, now that you've
> done it, we might as well just include it in this branch.
> 
> (I also don't like the naming, once it leaks out of a single source file,
> but I'll fix that later.)
> 
> The addition of add_query_node to make_result_iq should have been a separate
> commit, on a trivia branch that was insta-reviewed and fast-tracked in.
> 
> The comment "Unforbids events" in the test adds no information; in future
> please don't comment things that are already obvious from the code.

removed in commit d7b2d39df88760a26952662564935c0d52660a63
    Fixed mail-notification test for new spec

> 
> Trivial changes
> ===============
> 
> Patching connection_disco_cb adds trailing whitespace; don't do that (I think
> git can be configured to warn you about wrong whitespace while committing?).
> The same occurs in struct _GabbleConnection, in GoogleXmlStream in
> gabbletest.py, and in test-mail-notification.py.

I really need to find how to fix my vim config for that. So I've grep trailing
spaces and fixed everyone I've introduced in:

commit d48baf98cd0f204ab3bd34d4a1ae542b3e80471f
    Removed trailing white space introduce by mail notification

> 
> The TpDBusDaemon in struct _GabbleConnection isn't specific to
> MailNotification (even though it was added for this branch) so I'd prefer
> it not to be under the MailNotification heading. I'm thinking of adding
> tp_base_connection_get_dbus_daemon()...

TODO After lunch.

> 
> I don't like the abbreviation "mail notif"; we don't usually abbreviate.
> In particular, the debug key in debug.[ch] is UI (of a sort) and should
> just be called "mail", even if we keep "mail_notif" in function and file
> names.

commit c140a312c89fc30fc1e1cd867854872f21c5cbf8
    Renamed "mail-notif" debug section into "mail"

> 
> New test scripts shouldn't start with "test-", which just interferes with
> tab completion.

commit adee9130493bfb03fb753b8f42a629b272059379
    Renamed test-mail-notification.py into mail-notification.py

> 
> In test-mail-notification.py, check_properties_empty doesn't have Python-style
> whitespace:
> 
>     -def check_properties_empty (conn, capabilities = 0):
>     +def check_properties_empty(conn, capabilities=0):
> 
> (yes, that whitespace convention differs from the one we use for C, sorry).

commit 3e7d62418c439d14b05dece4e85a3751448a9cb1
    Fix python coding style in mail-notifiation test

> 
> Typos in test-mail-notification.py: functionnality -> functionality,
> notificagtion -> notification, lets use -> let's use, date are -> dates are,
> thread3_subject should probably not be "subject2" (likewise for body),
> gabble query mail data -> gabble queries mail data, unkown -> unknown
> (multiple times), "is not support" -> is not supported (twice),
> "that Gabble react correctly" -> reacts correctly, "Make sure method return"
> -> method returns.

commit bc085f2f8fa78fcabd128c289f82ffe9827b6d81
   Fixed various typos in mail-notifiaction test

> 
> test-mail-notification.py should use cs.NOT_IMPLEMENTED rather than inventing
> its own not_implemented constant.
> 
commit d13f7490b499e4bd18ed940992a2a6305d54dbdf
    Use cs.NOT_IMPLEMENTED instead of custom string

(In reply to comment #9)
> OK, I'm done with reviewing; please put the patch keyword back when this is
> ready for further review. There's no need to reassign to me, I receive
> telepathy-bugs mail.
> 
> Review of conn-mail-notif.[ch]:
> 
> Notes for the future
> ====================
> 
> Pedantically, conn-mail-notif.h should include <glib-object.h> instead of or
> in addition to <glib.h>, since it references GObject. (This isn't very
> important, since connection.h necessarily includes <glib-object.h> anyway,
> but it would become more important in library code.)

ommit 3d68d43e9dbf72c3b1d2576327ac8b188c920415
    Replaced glib.h with glib-object.h in conn-mail-notif.h

> 
> Trivial changes
> ===============
> 
> conn-mail-notif.[ch] are copyright 2009-2010 Collabora Ltd., not just 2009.

I always got told that this was the right way to go (keep beginning year and add - <Current year> when you modify the file). I would like some references before doing anything.

> 
> Why is namespaces.h included right at the beginning of conn-mail-notif.c?
> I'd have put it with connection/debug/util. Likewise for
> extensions/extensions.h, which is part of Gabble.

Because I have not read the coding style carefully enough ;)

commit 2a20b335a18288f5ba778eede337a80701fe0948
    Fix conn-mail-notif.c headers ordering

> 
> (You were right to include conn-mail-notif.h immediately after config.h
> though, since this is an implicit assertion that it's self-contained.)
> 
> conn-mail-notif.c contains trailing whitespace; it shouldn't.

Fixed in one of the commit in previous comment.

> 
> Coding style: many occurrences of [a-zA-Z]( should have a space before the
> parenthesis (although #define IMPLEMENT(x) must not have a space there,
> due to cpp syntax).

commit 0aeb8506a19a8d3c92c3595870abc04498803c64
    Fixed missing space before parenthesis in conn-mail-notif.c

> 
> Coding style: we compare pointers to NULL with "pointer != NULL", not with
> "!pointer", and compare integers to zero explicitly too. The only case
> where there's no explicit operator should be gboolean.
"... I wonder why people are so frightened by the fact that 0 means false ..."

commit 0ed079f5001dc3b077c8d343892a728af0401ef0
    Add explicit comparison in conn-mail-notif.c

> 
> Coding style: the outer "if" in unsubscribe() has {}, therefore the
> corresponding "else" should have them too (even though it doesn't strictly
> need them).

commit ef618d9ee7362ff62be8362de1ff6fc828aba37c
    conn-mail-notif.c: Add curly braces to "else", following coding style

> 
> Coding style: we prefer blank lines around each if{}, if{}else{}, while{},
> etc., like this:
> 
> >    node = wocky_xmpp_node_get_child (parent_node, "snippet");
> > +
> >    if (node)

commit 837da697eba5f18f9dec75cb8b4c75d27c29bbc4
    Add blank lines around each if/while in conn-mail-notif.c

> 
> conn-mail-notif.c should reference the API by its English URL :-) - remove
> /intl/fr.

commit 20a41e06b7fdffb4e139d56e31f22b034d215bab
    Removed intl/fr from link to Google Mail extension doc

> 
> I don't like the assumption that a zero-filled GPtrArray is valid. Does GLib
> explicitly say so? If not, then we shouldn't assume this. Since empty_array
> is only used in dbus-glib method calls, which use deep-copying and a lot of
> g_malloc'ing already, I'd just allocate a new empty GPtrArray in each function
> that needs one and free it afterwards.

commit 842f65512e18d4b319ea66b0d1ad8d3b9722f3ee
    Remove static empty_array in conn-mail-notif.c

> 
> sender_name_owner_changed() should refer to the unique name as "client",
> "subscriber" or "unique_name", not as "sender" (the message being processed
> here wasn't sent by the client). It's up to you whether you rename the variable
> "sender" in gabble_mail_notification_[un]subscribe() to be consistent with that
> or not (in these contexts, the client *is* the sender).

My reflection didn't went that far. I call dbus_g_method_get_sender () which gives me a sender. But in our case it's confusing with the sender being an element of Mail. Let's uses subscriber with match well with mail_subscribers hash table.

commit d99254cb214519df1dfd70f1e37b6dd82b56890e
    User subscriber instead of sender for var name in conn-mail-notif.c

> 
> query_unread_mails_cb shouldn't debug-output the whole node - Wocky will
> output all received nodes (if given appropriate debug flags) anyway.
> 
> +  DEBUG ("reached");
> I suspect/hope this is no longer useful :-)

commit fc7cd40516a001831987a5e2a840979737d1718e
    Removed unneeded debug output in conn-mail-notif.c

> 
> In struct MailsHelperData, the hash tables and array should have a brief
> comment explaining what's in them and the ownership (owned / weak ref /
> borrowed / etc., and if it's borrowed, mention who owns the reference and
> ensures that it remains valid). A hypothetical example:
> 
>   /* name (string) borrowed from the mushroom => GMushroom ref */
>   GHashTable *mushrooms;
> 
> I don't like the name of struct MailsHelperData. It appears to be a closure
> for the call to mail_thread_info_each in store_unread_emails? If that's the
> case, then it should be declared just above mail_thread_info_each,
> and should be typedef'd so you don't have to keep saying "struct". something
> like this:
> 
>     typedef struct {
>     ...
>     } MailThreadCollector;
> 
> (then replace all "struct MailsHelperData *" with "MailThreadCollector *").

I agree that this name sucks, MailThreadCollector is correct, but not more. I'll try to find something, otherwise I'll take that. I don't agree with typdefing systematically all the structures. Unless it's a very special structure (like a gobject) or an opaque structure. I think it just hides the fact that it's a structure, and it's bad to hide things.

That fix will wait for tomorrow ... (along with moving TpDBusDaemon to a more connection wide locattion).

> 
> > +  if (data.mails_added->len || mails_removed->len)
> 
> Coding style: this should be of the form if (x > 0 || y > 0) (i.e. compare
> against 0 explicitly).

That's explicit comparison, see previous comments.

> 
> > +          g_hash_table_ref (mail);
> > +          g_hash_table_remove (data->old_mails, tid);
> 
> You can use g_hash_table_steal to avoid it being freed prematurely?

commit daa971aef811fb222934ffcd5dc4f70809f41e96
    Use g_hash_table_steal () in conn-mail-notif.c

> 
> > +      handle_senders (node, mail, &dirty);
> 
> I think I'd prefer three occurrences of:
> 
>     if (handle_foo (node, mail))
>         dirty = TRUE;

commit 82d47b2374dc3222b3da9b11552f92acac3f4e4a
    Use return value instead of in/out param in conn-mail-notif.c

> 
> The final "else" clause of conn_mail_notif_properties_getter should be in
> {}, and should just be a g_assert_not_reached (it can only be reached if
> someone adds a property to the GabbleConnection and fails to add it to the
> getter).

commit d4925f16609290059f00577420b6e10fbe1ecd0a
    Just use g_assert_not_reached() in conn-mail-notif.c property getter

> 
> Logic
> =====
> 
> In gabble_mail_notification_request_inbox_url (also request_mail_url):
> > +  /* TODO Make sure we don't have to authenticate again */
> 
> Either fix that TODO, declare it to be unimportant, or file a bug.
> 
> (One of my spec changes was to relax the pre-authenticated URL from
> "pre-authenticated" to "pre-authenticated if possible", so I think it would
> be OK to just delete these.)

The thing is that it is possible, the win32 client does it. It requires a bit of reverse engineering, which I never completed. For this reason we should file a but and refer it in the TODO comment, and maybe replace the TODO with a FIXME.

> 
> In gabble_mail_notification_request_inbox_url (also r_m_u), how could
> conn->inbox_url be NULL? Shouldn't you raise NotAvailable if that's the case?

We don't control the server side, so we must not make any assumption about the the XML attributes presence. One thing is clear to me, we must not crash. 

Indeed, we could do more check (NULL, empty "" or not starting with "http[s]://" and raise error in these cases, but that would mean the server is bugged.

I won't change it because the server could also return an URL that is not working, in all case the client will eventually discover that it's broken by trying it, it's not CMs fault. 

> 
> In gabble_mail_notification_request_mail_url, why do you convert in_id
> to an int64 and then back to a string? Can't you just incorporate it into
> the URL with %s?

The tid string is represented in decimal on XMPP side, but on the webmail side they use hexadecimal (%" G_GINT64_MODIFIER "x").

commit 67a2007d767332f6c0603e852fea7c4c72698712
    Explain why we convert Mail id from decimal to hexadecimal

> 
> In conn_mail_notif_init, if conn->daemon is NULL, all future methods are
> basically going to assert. I think it would be reasonable to use g_error
> here, since Gabble connects to the session bus as the first thing it does.

commit 72c97874236b07652d00039b8612afdf4da1c9a6
    Use g_error if we cannot allocate TpDBusDaemon

> 
> In conn_mail_notif_properties_getter, is username@stream-server definitely
> correct for all Google servers? In particular I'm thinking of: SRV records,
> gmail.com vs. googlemail.com, Google Apps For Your Domain.
> 
> If stream-server is the "logical" name of the server (gmail.com/googlemail.com
> rather than talk.google.com), you don't need to encode the JID, you could just
> use the self-handle (tp_base_connection_get_self_handle()) and stringify it
> with the appropriate TpHandleRepo.
> 
> If it's the "physical" name of the server (talk.google.com) then I think the
> logic's wrong.

user@googlemail.com will have googlemail.com has a stream server. Turns out that it's a DNS that points to the same pool of servers, not a redirection. The TpHandle thing seems to be very obscure to me and it's not clearly stated what it contains. I feel safer to keep what works atm, and it's not time critical.

> 
> In sender_each:
> 
> > +          0, wocky_xmpp_node_get_attribute (node, "name") ?: "",
> 
> I'm pretty sure ?: isn't standard C, and it's certainly not clear.

The ?: is indeed a GNU extension, I'll fix that, whether it's "not clear" is an opinion I don't share with you, sorry.

commit 8291e0c8e4e115ee03685d6a762d88b60f5f9a1e
    Don't use GNU extension operator ?: in conn-mail-notif.c

> I'd prefer the struct to be constructed with tp_value_array_build()
> (which probably requires a telepathy-glib 0.10 dependency).

I'm not sure if it's right to return a GPtrArray of generic GValueArray, because a GPtrArray of GABBLE_STRUCT_TYPE_MAIL_ADDRESS is expected.

> However,
> I don't think the sender should be added at all if they don't have an
> email address, and the name should probably default to the email address
> rather than the empty string?
> 

Has said before, CMs not responsible for server bugs, and google server always gives an e-mail address. Faking name with e-mail address is a very bad idea. It prevents the UI from detecting that Name is missing (empty string means that, no choice since dbus-glib crash with NULL) and would lead to ugly display like:
    nicolas.dufresne@collabora.co.uk <nicolas.dufresne@collabora.co.uk>

(In reply to comment #12)
> > conn-mail-notif.[ch] are copyright 2009-2010 Collabora Ltd., not just 2009.
> 
> I always got told that this was the right way to go (keep beginning year and
> add - <Current year> when you modify the file).

Sorry, yes, I phrased that ambiguously. What I meant is that their real copyright status is 2009-2010, but you wrote 2009 and haven't updated it since.

(So, your understanding of copyright as expressed here is right, but the branch isn't :-)

> I don't agree with
> typdefing systematically all the structures. Unless it's a very special
> structure (like a gobject) or an opaque structure. I think it just hides the
> fact that it's a structure, and it's bad to hide things.

I disagree: GPtrArray, GValue, ..., and basically every other opaque type are necessarily structs too (what else could they be?), so everyone's GLib coding style is full of structs already. Structs are the closest C has to classes, so everything that would be a (simple) class in C++/Java/... is a struct here; there's no point in repeating that every time you define a struct.

> > > +  /* TODO Make sure we don't have to authenticate again */
[...]
> The thing is that it is possible, the win32 client does it. It requires a bit
> of reverse engineering, which I never completed. For this reason we should file
> a but and refer it in the TODO comment, and maybe replace the TODO with a
> FIXME.

Yeah, sounds like a job for an enhancement/low bug.

> 
> > 
> > In gabble_mail_notification_request_inbox_url (also r_m_u), how could
> > conn->inbox_url be NULL? Shouldn't you raise NotAvailable if that's the case?
> 
> We don't control the server side, so we must not make any assumption about the
> the XML attributes presence. One thing is clear to me, we must not crash. 

Sure, we mustn't crash; this is why D-Bus has recoverable exceptions. Raising NotAvailable seems preferable to returning a known-wrong inbox_url, but if you insist on shifting complexity into the client, it seems mostly harmless to leave it as-is.

> user@googlemail.com will have googlemail.com has a stream server. Turns out
> that it's a DNS that points to the same pool of servers, not a redirection.

For XMPP, the indirection is done with SRV records, FWIW:

% host -t srv _xmpp-client._tcp.gmail.com
_xmpp-client._tcp.gmail.com has SRV record 5 0 5222 talk.l.google.com.
[... and some alternatives ...]
% host -t srv _xmpp-client._tcp.googlemail.com
_xmpp-client._tcp.googlemail.com has SRV record 5 0 5222 talk.l.google.com.
[... and some alternatives ...]

> The
> TpHandle thing seems to be very obscure to me and it's not clearly stated what
> it contains.

tp_base_connection_get_self_handle() returns the result of the GetSelfHandle() D-Bus method, which is (an integer that maps to) the user's own unique identifier in the IM protocol.

If you're not comfortable with the handle mechanism, please consult telepathy-spec for details (you'll need to be aware of it for most CM work). Handles are basically integer tokens representing normalized strings, much like a GQuark, but reference-counted rather than being "immortal" like a GQuark.

In the case of XMPP, users are identified by JIDs, so the self-handle is (an integer that maps to) the user's own JID.

> > I'd prefer the struct to be constructed with tp_value_array_build()
> > (which probably requires a telepathy-glib 0.10 dependency).
> 
> I'm not sure if it's right to return a GPtrArray of generic GValueArray,
> because a GPtrArray of GABBLE_STRUCT_TYPE_MAIL_ADDRESS is expected.

It is right: the GType-generating macros are just a way to generate boxed types for GLib representations of D-Bus types, and get those types registered with dbus-glib. There's no subclassing or anything like that going on (you can't subclass GValueArray).

The versions of these macros that appear in telepathy-glib do (somewhat) document what GLib types they represent, but since this particular one is a Gabble extension, it doesn't go into any gtkdoc.

> Has said before, CMs not responsible for server bugs, and google server always
> gives an e-mail address.

I still think omitting senders who don't have an email address, and making an effort to supply some sort of non-empty display name, is a better handling for this, to be nicer to UIs (UIs should be programmed defensively too, of course, but there are likely to be more of them than there are CMs and they won't all get the subtleties right).

(In reply to comment #13)
> I still think omitting senders who don't have an email address, and making an
> effort to supply some sort of non-empty display name, is a better handling for
> this, to be nicer to UIs (UIs should be programmed defensively too, of course,
> but there are likely to be more of them than there are CMs and they won't all
> get the subtleties right).
> 

Omitting senders who don't have a email address is one thing, but you may want to omit also the mail that does not have any senders. Again, I won't do that, it's a waste of time, the server is not bugged atm. The only thing that I care around this subject is that if it was bugged, I must not crash.

But you absolutely need to understand that faking an common name is very bad thing. The common name is an optional field, if you don't have it it's not an error, leave it empty, the UI will understand. The common name is an additional information that is used to better understand who is the sender when email address does not contains the real senders name. No standard make it mandatory and even Google with their huge search server cannot generate one that really make sense. All email client I know lives with that fact and displays the email address alone if common name is not provided. And that happens very often.

(In reply to comment #14)
> The common name is an optional field, if you don't have it it's not an
> error, leave it empty, the UI will understand.

OK, you've convinced me; please leave this bit as-is, and say in the spec that the display name can be empty.

(In reply to comment #11)
> Ported yesterday, but if we undraft we still need to update.

The spec isn't ready to be undrafted just yet (I'd prefer to get a draft implementation merged in Gabble, and at least nearly-ready in one of the other CMs, first), so please just make sure it's in sync with the draft in the 0.19.1 spec release, by copying in the one from the 0.19.1 spec, verbatim.

This will include non-functional changes, like the <tp:added> notation - that's desirable, it indicates more clearly which version we claim to implement.

As far as I can tell, it will also change the type of URL_Data from string to variant, which will require some code fixes. You'd have spotted this if you'd copied in the spec XML from 0.19.1 verbatim, rather than picking out individual changes.

Remaining merge blockers, which are all simple changes: use tp_value_array_build(), correct copyright years, typedef the struct, file an enhancement/low bug for pre-authenticated URLs, and either reference that bug by its fd.o bug number in the TODO comments or just remove them.

Getting our own JID via tp_base_connection_get_self_handle would be nice, but isn't a blocker; the spec clarification I requested in Comment #15 blocks undrafting (IMO), but doesn't block this branch.

(In reply to comment #13)
> In the case of XMPP, users are identified by JIDs, so the self-handle is (an
> integer that maps to) the user's own JID.

Ok, looking at the code I find it harder to understand, but it works:
commit 77b7ccb1562ff8d367048383ee921652e7f7024b
    Use SelfHandle instead of username@stream_server MailAddress

Also I found that MailAddress was not covered in the test:
commit b011ce16a3c61fea0234024080d67c71ef99de00
    Test MailNotification.MailAddress property

And the RequestMailURL was not testing the actual result:
commit 994946151237c04b45e9338a9445f2260f94e119
    Test the value returned by MailNotification.RequestMailUrl

(In reply to comment #16)
> (In reply to comment #11)
> > Ported yesterday, but if we undraft we still need to update.
> 
> The spec isn't ready to be undrafted just yet (I'd prefer to get a draft
> implementation merged in Gabble, and at least nearly-ready in one of the other
> CMs, first), so please just make sure it's in sync with the draft in the 0.19.1
> spec release, by copying in the one from the 0.19.1 spec, verbatim.
> 
> This will include non-functional changes, like the <tp:added> notation - that's
> desirable, it indicates more clearly which version we claim to implement.

Good idea, I didn't notice you had added a version.

> 
> As far as I can tell, it will also change the type of URL_Data from string to
> variant, which will require some code fixes. You'd have spotted this if you'd
> copied in the spec XML from 0.19.1 verbatim, rather than picking out individual
> changes.

Only the commit with the version number is (btw the file is from master verbatim, no picking of individual changes was ever done). The variant stuff is already in too, I accidentally included it in another commit, sorry, I'll rebase and split this:

commit 8ff0447245fab74dcebe23bfc5b5b8de06efb743
    Mail_Type has been replaced by a THREAD_BASED flag

Has we don't use url-data in this implementation, the change is very small. The relevant part of the patch was the following:

--- a/src/conn-mail-notif.c
+++ b/src/conn-mail-notif.c
@@ -244,7 +244,7 @@ static void
 gabble_mail_notification_request_mail_url (
     GabbleSvcConnectionInterfaceMailNotification *iface,
     const gchar *in_id,
-    const gchar *in_url_data,
+    const GValue *in_url_data,
     DBusGMethodInvocation *context)
 {
   GabbleConnection *conn = GABBLE_CONNECTION (iface);


> 
> Remaining merge blockers, which are all simple changes: use
> tp_value_array_build(), correct copyright years, typedef the struct, file an
> enhancement/low bug for pre-authenticated URLs, and either reference that bug
> by its fd.o bug number in the TODO comments or just remove them.
> 
> Getting our own JID via tp_base_connection_get_self_handle would be nice, but
> isn't a blocker; the spec clarification I requested in Comment #15 blocks
> undrafting (IMO), but doesn't block this branch.

JID stuff has been changed now. About the email common name clarification I'll introduce it in Spec review bug.

(Merging this doesn't depend on undrafting the spec, and undrafting the spec does depend on this being merged, so please stop reverting the dependency relationship. When it's ready, I'll undraft the interface in the spec and in Gabble more or less simultaneously, but there'll be at least one Gabble release between now and then.)

No need to split the variant stuff away from its commit; it should just have had a better commit message the first time :-)

I think this is ready for merge.

(In reply to comment #19)
> I think this is ready for merge.

Sorry, make that "ready for merge with trivial changes", assuming the commits you haven't pushed yet are good.

Done.

commit a88baea0dd8f3144678175cb871c84576a511c77
    Made TpDBusDaemon globally accessible by all Connection extension

commit 8917c1bf051e98ba31ac9ddc0d4e0e4f0f754299
    conn-mail-notif.c: Use tp_value_array_build to simplify the code

commit a77a14a07d31adc4307ea4e45de789d5c1170e58
    Renamed structure MailsHelperData into MailThreadCollector

commit 35909d9d4fbef4ac60752481efd6722eb6819326
    Fixed copyright year to include 2010

Merged to master, will be in 0.9.7. Please open separate bugs for any problems with this.