Bug 26146

commit 1086fcd5aee0108705fcc4b777cdc5414f0bc026
Author: Nicolas Dufresne <nicolas.dufresne@collabora.co.uk>
Date:   Wed Jan 27 17:37:40 2010 -0500

    Bumped to new version of MailNotification.DRAFT
    
    Name of capabilities flags has changed and "url_data" is now "url-data".
    Also fix a compilation issue and added external type Unix_Timestamp64.

Review comments:
What is xep.xsl used for? it sounds XMPP and/or Gabble specific.

Typically telepathy style is 2 space indentation.

A few lines in mail-notification.c are longer than 80 chars.

Should purple_account_set_check_mail be set/removed on subscribe/unsubscribe or doesn't that work right?

In haze_connection_mail_notify_emails, should g_utf8_strchr be used instead of strchr? If so you wouldn't need string.h included.

+                    mail = tp_asv_new (
+                            "id", G_TYPE_STRING, "none",
+                            "url_data", G_TYPE_STRING, url,
+                            "senders", addr_list_type, senders,
+                            "to-address", addr_list_type, recipients,
+                            "subject", G_TYPE_STRING, subject,
+                            NULL);
Should "id" even be passed here?
Are refs being leaked for senders and recipients here (in the enclosing function)?

+        g_value_set_uint (value,
+            HAZE_MAIL_NOTIFICATION_FLAG_EMITS_MAILS_RECEIVED);
It should broadcast Supports_Request_Mail_URL support.

Thanks for the review, see my comments below:

(In reply to comment #2)
> Review comments:
> What is xep.xsl used for? it sounds XMPP and/or Gabble specific.

Right, I blindly copied over the generation tools from Gabble. I'll try to remove that one, and look for other things in tools that make no sense.

> 
> Typically telepathy style is 2 space indentation.

The first thing I noticed when I started in haze was that all the code was 4 spaces indented instead of 2. It would be inconsistent to change that for a single file. Let me know if you still want the new code to be 2 spaces indented.

> 
> A few lines in mail-notification.c are longer than 80 chars.

I'll fix that.

> 
> Should purple_account_set_check_mail be set/removed on subscribe/unsubscribe or
> doesn't that work right?

I'll have to dig again to find out why I did it this way. It is possible that some protocols do not react when this parameter change, which would lead to never get any notification. If that is the case, I'll write a comment to explain, if not I'll implement the subscribe/unsubscribe mechanic.

> 
> In haze_connection_mail_notify_emails, should g_utf8_strchr be used instead of
> strchr? If so you wouldn't need string.h included.

Make sense, and will get rid of string.h.

> 
> +                    mail = tp_asv_new (
> +                            "id", G_TYPE_STRING, "none",
> +                            "url_data", G_TYPE_STRING, url,
> +                            "senders", addr_list_type, senders,
> +                            "to-address", addr_list_type, recipients,
> +                            "subject", G_TYPE_STRING, subject,
> +                            NULL);
> Should "id" even be passed here?

No it should not be passed since the spec now states that the id must be unique otherwise unset, thanks.

> Are refs being leaked for senders and recipients here (in the enclosing
> function)?

Right, good catch.

> 
> +        g_value_set_uint (value,
> +            HAZE_MAIL_NOTIFICATION_FLAG_EMITS_MAILS_RECEIVED);
> It should broadcast Supports_Request_Mail_URL support.

Good catch.

(In reply to comment #3)
> 
> Should purple_account_set_check_mail be set/removed on subscribe/unsubscribe or
> doesn't that work right?

It does not work for two protocols, myspace and google. MySpace needs the flag to be set before connection to enable a poll timer, while Google needs it to do initial query that enables further notifications. Also, protocols does not get notified when the configuration changes.

We need to choose between the right way of doing it, or the way that works for all of them. Any preferences ?

Other items are fixed :

commit c87c7cc85466a4a3d0c0985cc5d439878b3f54a8
    Add _SUPPORTS_REQUEST_MAIL_URL to MailNotification

commit e97711a470e861710720c5e0f155ca56057d0d10
    Fixed leak of senders and recipient in connection-mail.c

commit ea2d2f7324cb86f8e1032492b79517ac2c69b17f
    MailNotification spec says that mail['id'] should be unique or unset

commit 80ec29e30ff07999419e681046e8c89e93c9b1b2
    connection-mail.c: replace strchr() by g_utf8_strchr()

commit f1cbee8640322b7e4e75f5cd650c6f31450d66f7
    connection-mail.c: Fixed line larger the 80 character
    It was not possible on line 102, because the generated function name is
    already more the 80 characters.

commit f01d03a82a469899530d326455e335b781ee8c70
    Removed unused tools    
    Unused tools where imported along with the code generator for Telepathy
    spec.

(In reply to comment #3)
> The first thing I noticed when I started in haze was that all the code was 4
> spaces indented instead of 2. It would be inconsistent to change that for a
> single file. Let me know if you still want the new code to be 2 spaces
> indented.

That's fine. As wjt said, he'll re-indent it sometime. I just wanted to mention it. :)

(In reply to comment #4)
> It does not work for two protocols, myspace and google. MySpace needs the flag
> to be set before connection to enable a poll timer, while Google needs it to 
> do initial query that enables further notifications. Also, protocols does not
> get notified when the configuration changes.
>
> We need to choose between the right way of doing it, or the way that works for
> all of them. Any preferences ?

I'm fine with the way that works for all of them. Haze has plenty of hacks in it ;). My understanding is that "check-mail" is supposed to be more of a preference for when configuring an account anyway. A comment with the MySpace/Google reasoning wouldn't hurt though for posterity.


e97711a470e861710720c5e0f155ca56057d0d10
+                    g_ptr_array_free (senders, TRUE);
+                    g_ptr_array_free (recipients, TRUE);

I *think* that they are only reffed in tp_asv_new, so you probably want to g_ptr_array_unref them here instead of freeing them.

(In reply to comment #6)
> I'm fine with the way that works for all of them. Haze has plenty of hacks in
> it ;). My understanding is that "check-mail" is supposed to be more of a
> preference for when configuring an account anyway. A comment with the
> MySpace/Google reasoning wouldn't hurt though for posterity.

I'll add that comment then, thanks.

> 
> 
> e97711a470e861710720c5e0f155ca56057d0d10
> +                    g_ptr_array_free (senders, TRUE);
> +                    g_ptr_array_free (recipients, TRUE);
> 
> I *think* that they are only reffed in tp_asv_new, so you probably want to
> g_ptr_array_unref them here instead of freeing them.
> 

I've read the doc about G_VALUE_COLLECT/collect_value() and it's stated that object may be copied or referenced unless static. Which I think mean that doing an "unref" is a safer implementation. I'll change that.

And the two final items in list, I hopes it's good for merge now !

commit 4d7805685ae73bc891261d934ebc807f3d2a856e
    Explain why we enable mail notification before connecting

commit 687aa162e5b01ccb63cb7b8fedc5e9fe90be8336
    Use _unref instead of _free on GPtrArray passed to tp_asv_new
    
    tp_asv_new () uses G_VALUE_COLLECT() that refers to GTypeValueTable method
    collect_value (). In the doc of that method, they state that elements may
    be copied or referenced. Since we don't know, the safest way to prevent
    leak is to use unref.

Ok. Looks good to me :). Probably want to check with wjt just to make sure.

(In reply to comment #9)
> Ok. Looks good to me :). Probably want to check with wjt just to make sure.

If you both think it's good to go, ship it!

Merged. Thanks :)