[Bug 25766] Review Gabble Google Mail Notification

[bugzilla-daemon at freedesktop.org]

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


Nicolas Dufresne <nicolas.dufresne at collabora.co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Depends on|                            |13349
             Blocks|13349                       |




--- Comment #11 from Nicolas Dufresne <nicolas.dufresne at collabora.co.uk>  2010-02-24 09:14:52 PST ---
(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


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