[Bug 21957] Doesn't implement SimplePresence

[bugzilla-daemon at freedesktop.org]

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


Will Thompson <will.thompson at collabora.co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |will.thompson at collabora.co.u
                   |                            |k
                URL|                            |http://git.collabora.co.uk/?
                   |                            |p=user/wjt/telepathy-idle-
                   |                            |wjt.git;a=shortlog;h=refs/he
                   |                            |ads/presence




--- Comment #1 from Will Thompson <will.thompson at collabora.co.uk>  2009-08-29 08:47:30 PST ---
Jonathon has a branch implementing SimplePresence, which I've updated to
current master. Issues I've found from a cursory review which I need to fix
before this can be merged (or maybe someone would like to volunteer? :)):

+get_contact_statuses(GObject *obj, const GArray *contacts, GError **error)
...
+       if (!tp_handles_are_valid (handle_repo, contacts, FALSE, error))
+       {
+               return NULL;
+       }

get_contact_statuses doesn't strictly need to check that, tp-glib should do so
for us.

+       /* FIXME: this should really be sent with lowest priority so it doesn't
+        * block other messages */

Yes it probably should.

+               if (status->optional_arguments) {
+                       g_assert(status->index == IDLE_PRESENCE_AWAY);  /*
optional args only for AWAY */

tp-glib does currently pass a NULL HT if there are no optional arguments in the
status's spec, but that's a bit surprising. I don't think this assertion should
be here; the index check can go into the if's condition.

+                       value = g_hash_table_lookup(status->optional_arguments,
"message");
+                       if (value) {
+                               if (!G_VALUE_HOLDS_STRING (value)) {
+                                       g_set_error (error, TP_ERRORS,
TP_ERROR_INVALID_ARGUMENT,
+                                                    "Status argument 'message'
requires a string");
+                                       return FALSE;
+

The mixin should really check this for us. It knows what type the optional
argument should have. Sadly, it does not.

+                               /* But, the IRC protocol requires some status
for the
+                                * away command or otherwise it would be
interpreted as
+                                * 'not away' so we tack on a default message
here */
+                               away_msg = "Away";

:( IRC.

+               } else if (presence == IDLE_PRESENCE_OFFLINE) {
+                       /* FIXME: send quit command? */
+               }

We said that offline isn't user-settable, so this should not happen.

+       /* FIXME: do we really want to return TRUE here?  ideally, we'd check
to see
+        * if sending the AWAY command was successful... for now just pretend
+        * everything succeeded */
+       return TRUE;

In an ideal world we'd quietly retry if AWAY failed, and after a few attempts
give up and set our locally-cached status back to what it was before.

+       if (presence != priv->presence ||
+           away_msg != priv->status_msg) {

This is always true, because strings in C. :)

+       /* IRC doesn't give us a way to know the remote contacts 'away message'
+        * unless we actually try to send a PRIVMSG to them, so the message
will
+        * always be NULL for remote contacts */

We could WHOIS them?

---

More generally: this doesn't re-check someone's status unless you explicitly
re-request their presence. UIs basically will never do that, so you'll get
people's presence when you first join a channel with them (oh, and it doesn't
WHOIS people you're PRIVMSGing afaict) and then never again.

I'd be inclined to say that Idle should re-WHO rooms sporadically, and re-WHOIS
people you're PRIVMSGing. Maybe only for rooms with <n members? I think it's
probably worse to show out-of-date presence than no presence.


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