I just implemented Google Mail notification based on DRAFT spec described in bug 13349. Web: http://git.collabora.co.uk/?p=user/nicolas/telepathy-gabble.git;a=shortlog;h=refs/heads/email-notification Git: git://git.collabora.co.uk/git/user/nicolas/telepathy-gabble.git (branch email-notification)
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.
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.