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.

> 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.

I've tried to address this issue.  There are a few new commits in my 'presence' branch that implement some of these ideas, but not in a completely satisfactory way, unfortunately.  The commit messages spell out some of the problems I ran into.  comments appreciated.

(In reply to comment #1)
> +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.

pushed a commit which removes this superfluous check

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

done

> +               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.

ok, done.

> +               } else if (presence == IDLE_PRESENCE_OFFLINE) {
> +                       /* FIXME: send quit command? */
> +               }
> 
> We said that offline isn't user-settable, so this should not happen.

OK, i just made this a g_warn_if_reached()

> +       /* 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.

I didn't address this at all yet.

> 
> +       if (presence != priv->presence ||
> +           away_msg != priv->status_msg) {
> 
> This is always true, because strings in C. :)

oh dear.  that was stupid.  fixed.

I pushed a new set of commits to my branch which address the comments above.

i pushed one additional commit to my branch that attempts to fix some of the issues I alluded to in comment #2

 	gboolean dispose_has_run;
+  guint presence_query_id;

Consistency: this should be a tab.

+	/* no need to continue querying member presence after we close the channel
+	 */

This is going to need updating to deal with respawning channels (which I
thought was in master, but maybe isn't).

+	 * FIXME: avoid this for large rooms?

Or make it even less frequent?

The commit message for fc8739324 repeats a couple of lines halfway
through a sentence.

update_contact_presence could use tp_asv_new ("message", G_TYPE_STRING,
message, NULL);

Before this is merged, we need to fix the bug where Idle sends at most
one message a second, because otherwise if you have >20 IM channels open
(or fewer and are sending messages for some other reason) the WHO/WHOIS
requests will back up.

I wonder if we should treat contacts whose away message is "Away" (case
insensitively) as if they had no away message. This would mean such
contacts would show up in Empathy with the localized version of "away".

I like the final patch to try to cache the most informative version of
the contact's presence!

The issue of figuring out that the whois reply doesn't contain an away
reply and inferring that they're available is kind of unfortunate.
Perhaps the contact manager could hook into the beginning of the WHOIS
reply, and then if it gets ENDOFWHOIS without getting AWAY decide that
the contacts is available?

(In reply to comment #5)
> +       /* no need to continue querying member presence after we close the
> channel
> +        */
> 
> This is going to need updating to deal with respawning channels (which I
> thought was in master, but maybe isn't).

Can you explain this a bit to me?  How does channel re-spawning work?

> +        * FIXME: avoid this for large rooms?
> 
> Or make it even less frequent?

ok, I lengthened this timeout to 120s for now

> Before this is merged, we need to fix the bug where Idle sends at most
> one message a second, because otherwise if you have >20 IM channels open
> (or fewer and are sending messages for some other reason) the WHO/WHOIS
> requests will back up.

Agreed.  I'll work on this next.

> I wonder if we should treat contacts whose away message is "Away" (case
> insensitively) as if they had no away message. This would mean such
> contacts would show up in Empathy with the localized version of "away".

good idea.  I've implemented this.  As a bonus, xchat-gnome (and I guess xchat proper) default to setting the message to "Away" as well (at least when run in the english locale), so we'll get a little interoperability there as well.

> I like the final patch to try to cache the most informative version of
> the contact's presence!

Yeah, unfortunately, this has some downsides as well (as most hacks do).  For instance, when implementing the localized Away message (above), I had to be careful not to just omit the status message, or otherwise if a contact had an away status of "foo" and then switched it to "Away", it would attempt to continue to use the cached "foo" presence status.

> The issue of figuring out that the whois reply doesn't contain an away
> reply and inferring that they're available is kind of unfortunate.
> Perhaps the contact manager could hook into the beginning of the WHOIS
> reply, and then if it gets ENDOFWHOIS without getting AWAY decide that
> the contacts is available?

This sounds a bit complex, so I've avoided it for now.  Perhaps I'll take a stab at it a little later.  I've pushed changes for most of the other issues to my branch on dhansak for now.

(In reply to comment #6)
> > Before this is merged, we need to fix the bug where Idle sends at most
> > one message a second, because otherwise if you have >20 IM channels open
> > (or fewer and are sending messages for some other reason) the WHO/WHOIS
> > requests will back up.
> 
> Agreed.  I'll work on this next.

Has anyone done this? Failing that, can we merge this with an explicit "on" switch for presence and/or presence polling (compile-time or runtime), and turn it off by default, so we have the code ready for the Great Reindent but it doesn't suffer from this flaw in normal use?

I discussed this with Sjoerd this morning, trying to work out what to do with this branch (I'd like to either merge it or abandon it before too long, so we can run Idle through a re-indenter and my eyes can stop bleeding).

Our conclusion was that SimplePresence in its current form isn't really suitable for IRC, because it tries to offer better guarantees that IRC can give.

One possibility would be to move the IRC whois information to ContactInfo, which is already suitable for poll-only use (it's like that on XMPP too). However, that doesn't provide a very nice way to set your own status to Away, and Sjoerd would like to be able to see (at least some) contacts' away messages in chatrooms.

Another possibility would be to have a flag on the SimplePresence interface indicating that this protocol can only poll presence, and a RequestPresence or RefreshPresence method which polls and (perhaps) waits; then everyone would have Unknown presence until polled, but UIs could poll interesting contacts as appropriate.

One possible heuristic would be to poll the presence of everyone you highlight (i.e. mention their nick in a message) and everyone you have a 1-1 conversation with, once per n seconds. This would multiply traffic in a somewhat controllable way (no more than one whois per message sent, say, rather than stacking up presence pings for everyone in #telepathy), and if you stop communicating with them, the UI could display their presence as Unknown after a while.

Sjoerd doesn't think it makes sense for CMs to set policy for the point at which presence on poll-only protocols should be considered stale, which is consistent with our normal requirement that CMs don't set policy. However, that could mean that different UIs consider presence to be stale at different times, which would be somewhat confusing; perhaps the CM could be used as a synchronization point by having a ForgetStalePresence() method?

We could have a look at how XChat does this?

If we're going to have this sort of "partial presence" on IRC, Empathy's chatroom UI will need to display contacts' presence in some new "unobtrusive mode", in which Unknown maps to an empty space, rather than a question mark.

Over on bug 34796, Debarshi's implementing ContactInfo, and including presence information in the values returned there.

We'll need to implement SimplePresence to support setting our own presence. I don't think it's really worth adding API to SimplePresence to support presence-that-must-be-polled: we already have API for that in the form of ContactInfo.

We could make presence information that happens to come in via explicit requests on ContactInfo show up on SimplePresence, and have a flag on the SimplePresence interface to say that this is how things go on this protocol. Downside: a naïve client would show stale info to the user with no indication. I don't think that's necessarily so bad. It would make the Empathy UI for contact information work better, too: rather than having to handle the x-presence-... fields, the PresencesChanged() signal would tell us what we need to know.

I had a discussion with Sjoerd on IRC about this.

What XChat-GNOME does is issue a WHO message (not WHOIS) every 9 minutes against each channel and the reply has a list of nicks and some elementary presence information. It uses "H" or "G" to indicate if a nick is here or gone, and "@" or "+" to indicate if the nick is an operator, etc..

So maybe we can do the same thing, and if we notice that a nick has gone from "H" to "G", then we can issue a WHOIS message against that particular nick, and pick up the away message.

This does lead to some overlap with ContactInfo in the sense that ContactInfo also returns some x-presence-XXXX vcard fields because we get that for free from the reply to a WHOIS message. But I guess that is alright because IRC users expect their clients to show the away message when they issue a /whois against a particular nick.

(In reply to comment #10)
> I had a discussion with Sjoerd on IRC about this.
> 
> What XChat-GNOME does is issue a WHO message (not WHOIS) every 9 minutes
> against each channel and the reply has a list of nicks and some elementary
> presence information. It uses "H" or "G" to indicate if a nick is here or gone,
> and "@" or "+" to indicate if the nick is an operator, etc..
> 
> So maybe we can do the same thing, and if we notice that a nick has gone from
> "H" to "G", then we can issue a WHOIS message against that particular nick, and
> pick up the away message.

This sounds good to me too. The only downside is that someone who changes their message while away will have a stale message displayed until you hit them up with ContactInfo — doesn't seem like too great a downside though. And when you do hit them up with ContactInfo, the SimplePresence information can be updated.

> This does lead to some overlap with ContactInfo in the sense that ContactInfo
> also returns some x-presence-XXXX vcard fields because we get that for free
> from the reply to a WHOIS message. But I guess that is alright because IRC
> users expect their clients to show the away message when they issue a /whois
> against a particular nick.

(In reply to comment #10)

> What XChat-GNOME does is issue a WHO message (not WHOIS) every 9 minutes
> against each channel and the reply has a list of nicks and some elementary
> presence information. It uses "H" or "G" to indicate if a nick is here or gone,
> and "@" or "+" to indicate if the nick is an operator, etc..

I have no idea how I arrived at that 9 minutes figure. Looking at the code and rechecking with Wireshark tells me that it is 1 minute by default. In any case XChat-GNOME offers it as a configuration item, probably we should do the same.

I rebased jonner's branch against Git master a few weeks ago:
http://rishi.fedorapeople.org/simple-presence/

There were a few conflicts, which I hope I have resolved correctly, but I am not sure. A bit of testing and review should catch any mistakes.

While I don't care that much about getting IRC contacts' presence, it would be good if Idle could at least implement setting and exposing our own presence (at least Online/Offline). 

The current situation confuses Empathy and the Shell (which are both using tp_account_manager_get_most_available_presence()) if only Idle accounts are connected. As they don't implement SimplePresence, there is no "most available presence" and so UIs claim we are offline.

See https://bugzilla.gnome.org/show_bug.cgi?id=668350

(In reply to comment #14)
> The current situation confuses Empathy and the Shell (which are both using
> tp_account_manager_get_most_available_presence()) if only Idle accounts are
> connected. As they don't implement SimplePresence, there is no "most available
> presence" and so UIs claim we are offline.

I don't think this is really Idle's fault: MC, telepathy-glib and clients should cope with this situation. Is get_most_available_presence actually returning OFFLINE, or is it really UNSET?

If it's UNSET, I believe MC uses UNSET to mean "it's online but does not report any specific presence" - as a most available presence, UIs should treat that as online.

Rationale for UNSET meaning that per-account: if we used AVAILABLE, then this situation:

    XMPP: BUSY
    IRC: online

would be reported by get_most_available_presence as AVAILABLE, which is clearly wrong: it should be BUSY. With the special handling of UNSET, and UNSET prioritized below all online presences, it's reported as BUSY as expected.

I see two possibilities to fix this:

1) Change get_most_available_presence to return AVAILABLE if it would have returned UNSET (implementation detail: you will want to store priv->most_available_presence == UNSET, and only do the translation in the API - otherwise certain state transitions will be wrong)

2) Change clients (Empathy, Shell) to treat UNSET as an online status

Some things to test if you do (1):

* XMPP offline, IRC online -> AVAILABLE (or UNSET for (2))
* XMPP BUSY, IRC online -> BUSY
* XMPP offline, IRC online, XMPP changes to BUSY -> AVAILABLE changes to BUSY

(That last one is an example of a transition that would break if you stored priv->most_available_presence == AVAILABLE for the only-IRC-online case.)

(In reply to comment #15)
> If it's UNSET, I believe MC uses UNSET to mean "it's online but does not
> report any specific presence" - as a most available presence, UIs should
> treat that as online.

"If the connection is online but does not support the SimplePresence interface, the type SHOULD be Connection_Presence_Type_Unset." --the spec

(In reply to comment #15)
> (In reply to comment #14)
> > The current situation confuses Empathy and the Shell (which are both using
> > tp_account_manager_get_most_available_presence()) if only Idle accounts are
> > connected. As they don't implement SimplePresence, there is no "most available
> > presence" and so UIs claim we are offline.
> 
> I don't think this is really Idle's fault: MC, telepathy-glib and clients
> should cope with this situation. Is get_most_available_presence actually
> returning OFFLINE, or is it really UNSET?

That's OFFLINE, I opened bug #45120 about this as it's out of the scope of this bug.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/telepathy/telepathy-idle/issues/6.