1275136 - Implement attention (XEP-0224)

Status: NEW

(Chat Core :: XMPP, defect)

Comment 1

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: v1 - attention

Comment 2

[aleth [:aleth]]

Comment on attachment 8755665 [details] [diff] [review]
v1 - attention

Review of attachment 8755665 [details] [diff] [review]:
-----------------------------------------------------------------

The expected behaviour should be documented in this bug first. Bugs without a bug description are bad because in the future they are hard to understand by anyone who encounters them.

Currently incoming messages in 1-1 conversations act like pings some ways: they get the blue tab colour and window.getAttention is called.

Iirc one proposal was to have messages in 1-1 conversations just act like incoming messages in MUCs (red tab, no window attention), and only act like pings (bold message, blue tab) in special circumstances (such as buzz). (That's not what this patch does, but let's get the expected behaviour clear first.)

However, then we need to ask: 

* What's the expected behaviour if the user's name is mentioned (in any protocol) in a 1-1 conv? Should that be a ping or is that just strange to anyone who isn't used to IRC?

* Do we still want to retain the window.getAttention call for normal messages in 1-1 convs which aren't pings, because we think 1-1 messages are sufficiently important?

If we agree to make changes here, these protocol-independent changes should happen first in a separate bug.

The alternative would be to just add a way for 1-1 messages (like the buzz here) to be printed in bold, like pings in MUCs, but leave the rest (tab colours etc) unchanged.

Comment 3

[aleth [:aleth]]

(In reply to aleth [:aleth] from comment #2)
> Currently incoming messages in 1-1 conversations act like pings some ways:
> they get the blue tab colour and window.getAttention is called.

No, the colour is red. I got confused looking at the code, sorry.

Anyway, we should clarify the expected behaviour here.

Comment 4

[Florian Quèze [:florian]]

(In reply to aleth [:aleth] from comment #2)

> * What's the expected behaviour if the user's name is mentioned (in any
> protocol) in a 1-1 conv? Should that be a ping or is that just strange to
> anyone who isn't used to IRC?

Mentioning the user's name in a 1:1 conversation is irrelevant.

> * Do we still want to retain the window.getAttention call for normal
> messages in 1-1 convs which aren't pings, because we think 1-1 messages are
> sufficiently important?

Yes.

Think about it as "directed message": we notify the user if a message is specifically directed to him. A mention in a MUC is just a way to specify that the user is the intended recipient.

Comment 5

[Florian Quèze [:florian]]

Comment on attachment 8755665 [details] [diff] [review]
v1 - attention

Review of attachment 8755665 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/locales/en-US/xmpp.properties
@@ +207,5 @@
>  #   a room because of a system shutdown.
>  conversation.message.mucShutdown=You have been removed from the room because of a system shutdown.
>  
> +#   %S is the name of the message recipient.
> +conversation.message.buzz=%S has sent buzz to you.

I don't understand this sentence.

@@ +250,5 @@
>  command.invite=%S <jid>[<message>]: Invite a user to join the current room with an optional message.
>  command.me=%S <action to perform>: Perform an action.
>  command.nick=%S <new nickname>: Change your nickname.
>  command.msg=%S <nick> <message>: Send a private message to a participant in the room.
> +command.buzz=%S [<message>]: Grap user's attention with an optional message if available, otherwise send normal message.

I don't understand the "if available", did you mean "if specified"?

In which case will a normal message be sent?

::: chat/protocols/xmpp/xmpp-commands.jsm
@@ +126,5 @@
> +      let params = aMsg.trim();
> +      let conv = getConv(aConv);
> +
> +      // We do not have user's resource, so we will send a normal message.
> +      if(!conv._targetResource && params) {

nit: space between "if" and "(".

If we aren't doing what the user asked for, we should let the user know with a system message.

::: chat/protocols/xmpp/xmpp.jsm
@@ +661,5 @@
>    },
>  
> +  // XEP-0224: Attention.
> +  // Used to grap user's attention with optional message.
> +  sendBuzz: function(aMsg = null) {

Shouldn't this be named "getAttention"? I don't see where the word "buzz" comes from, but it's not explicit to me.

::: im/content/conversation.xml
@@ +251,5 @@
>              return;
>            }
>  
> +          if (isUnreadMessage && (!aMsg.conversation.isChat ||
> +                                  aMsg.containsNick || aMsg.notification)) {

Do you expect aMsg.notification to ever be set for a MUC? If not, isn't the test you want really:
 aMsg.conversation.isChat ? aMsg.containsNick : aMsg.notification
?

::: im/themes/messages/bubbles/main.css
@@ +100,5 @@
>  p.nick {
>    font-weight: bold;
>  }
>  
> +p.notification {

merge this with the previous rule.

Note that if we create this new "notification" class, all the existing message themes won't take it into account.

Comment 6

[aleth [:aleth]]

Is "containsNick" currently ever set for normal conversations? Maybe we should just "abuse" it for buzz instead of adding "notification".

Comment 7

[aleth [:aleth]]

(In reply to aleth [:aleth] from comment #6)
> Is "containsNick" currently ever set for normal conversations? Maybe we
> should just "abuse" it for buzz instead of adding "notification".

OK, I took a quick look and it looks like for JS prpls, containsNick is only set for MUCs (in jsProtoHelper). For libpurple, prplxpcom just passes the flag on from whatever libpurple does.

So if everyone agrees, I'd suggest (instead of adding "notification" flag checks everywhere)
* Adding a comment to the idl for containsNick saying "This is to be interpreted as a ping, and doesn't necessarily imply the nick appears in the message text."
* Changing the getter in purpleMessage.cpp to ensure containsNick is never set when the conv is not a MUC (so we override whatever libpurple does here.)
* Going through all references to the containsNick flag and removing checks for conv.isChat where appropriate, making sure it does what you want for 1-1 convs.
This should be in a separate patch to the attention XEP stuff, but you can put it in this bug.

Comment 8

[Abdelrahman Ahmed [:abdelrahman]]

(In reply to aleth [:aleth] from comment #7)
> So if everyone agrees, I'd suggest (instead of adding "notification" flag
> checks everywhere)
> * Adding a comment to the idl for containsNick saying "This is to be
> interpreted as a ping, and doesn't necessarily imply the nick appears in the
> message text."
> * Changing the getter in purpleMessage.cpp to ensure containsNick is never
> set when the conv is not a MUC (so we override whatever libpurple does here.)
> * Going through all references to the containsNick flag and removing checks
> for conv.isChat where appropriate, making sure it does what you want for 1-1
> convs.
> This should be in a separate patch to the attention XEP stuff, but you can
> put it in this bug.

I'm OK with this suggestion, let's make sure if flo agrees with that.

Comment 9

[Florian Quèze [:florian]]

Seems reasonable to me.

libpurple may have some concept of get_attention too, that we may want to plug into this, but this can be a follow-up bug.

Comment 10

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: v1 - Never set containsNick flag when the conv is not a MUC (libpurple)

Comment 11

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: v1 - Remove checks for conv.isChat

Comment 12

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: v2 - Never set containsNick flag when the conv is not a MUC (libpurple)

Fixing a nit in the previous patch
> +    *aContainsNick =PR_FALSE;

Comment 13

[aleth [:aleth]]

Comment on attachment 8762596 [details] [diff] [review]
v1 - Remove checks for conv.isChat

Review of attachment 8762596 [details] [diff] [review]:
-----------------------------------------------------------------

Don't forget Thunderbird ;)

Looks good otherwise.

Comment 14

[Abdelrahman Ahmed [:abdelrahman]]

(In reply to aleth [:aleth] from comment #13)
> Don't forget Thunderbird ;)

I think it's already handled (https://dxr.mozilla.org/comm-central/search?q=.containsNick&redirect=false)

Comment 15

[aleth [:aleth]]

Comment on attachment 8762596 [details] [diff] [review]
v1 - Remove checks for conv.isChat

Review of attachment 8762596 [details] [diff] [review]:
-----------------------------------------------------------------

I thought there would be a hit in https://dxr.mozilla.org/comm-central/source/mail/components/im/modules/chatNotifications.jsm, but it looks like not, because it observes new-directed-incoming-message and not new-text. That's some more code that could be unforked sometime...

OK then :-)

Comment 16

[Abdelrahman Ahmed [:abdelrahman]]

(In reply to Florian Quèze [:florian] [:flo] from comment #5)
> > +#   %S is the name of the message recipient.
> > +conversation.message.buzz=%S has sent buzz to you.
> 
> I don't understand this sentence.

This happens when we receive attention message does not contain body.

> I don't understand the "if available", did you mean "if specified"?
> 
> In which case will a normal message be sent?

I think "if available" as we send send normal message in these cases
1. We do not have user's resource
2. User's client does not support attention
3. User does not allow to receive attention

> If we aren't doing what the user asked for, we should let the user know with
> a system message.

OK

> Shouldn't this be named "getAttention"? I don't see where the word "buzz"
> comes from, but it's not explicit to me.

According to XEP-0224
> This feature is known as ’nudge’ or ’buzz’ in some non-XMPP IM protocols
I used 'buzz' as I think users are familiar with it, so they expect the behavior quickly. But I'm OK if it's anything else.

> Do you expect aMsg.notification to ever be set for a MUC? If not, isn't the
> test you want really:
>  aMsg.conversation.isChat ? aMsg.containsNick : aMsg.notification
> ?

No, need for this according to previous comments

Comment 17

[aleth [:aleth]]

> > Shouldn't this be named "getAttention"? I don't see where the word "buzz"
> > comes from, but it's not explicit to me.
> 
> According to XEP-0224
> > This feature is known as ’nudge’ or ’buzz’ in some non-XMPP IM protocols
> I used 'buzz' as I think users are familiar with it, so they expect the
> behavior quickly. But I'm OK if it's anything else.

iirc /buzz is the name of the command in libpurple XMPP.

Comment 18

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: v1 - Implement attention (XEP-0224)

Comment 19

[aleth [:aleth]]

Comment on attachment 8763277 [details] [diff] [review]
v1 - Implement attention (XEP-0224)

Review of attachment 8763277 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/locales/en-US/xmpp.properties
@@ +85,5 @@
>  conversation.error.invalidJID=%S is an invalid jid (Jabber identifiers must be of the form user@domain).
>  conversation.error.commandFailedNotInRoom=You have to rejoin the room to be able to use this command.
> +#   %S is the name of the message recipient.
> +conversation.error.buzzNotAllowed=Could not buzz as %S's client does not support it or %S does not wish to receive buzz, so we will send normal message.
> +conversation.error.buzzNotAllowedWithoutMessage=Could not buzz as %S's client does not support it or %S does not wish to receive buzz.

I think we can use the same message for both here
conversation.error.buzzNotAllowed=%S's client does not allow this command.
Because the message we send (if we can) will appear as a normal message in the conversation view anyway, right?

@@ +86,5 @@
>  conversation.error.commandFailedNotInRoom=You have to rejoin the room to be able to use this command.
> +#   %S is the name of the message recipient.
> +conversation.error.buzzNotAllowed=Could not buzz as %S's client does not support it or %S does not wish to receive buzz, so we will send normal message.
> +conversation.error.buzzNotAllowedWithoutMessage=Could not buzz as %S's client does not support it or %S does not wish to receive buzz.
> +conversation.error.unableBuzz=Could not buzz %S, so we will send normal message.

If we don't have the target resource, I think we can just send a normal message without showing an accompanying system message. It's according to spec that you have to send a normal message before buzzing, and it's the best way to get the other user's attention available.

@@ +87,5 @@
> +#   %S is the name of the message recipient.
> +conversation.error.buzzNotAllowed=Could not buzz as %S's client does not support it or %S does not wish to receive buzz, so we will send normal message.
> +conversation.error.buzzNotAllowedWithoutMessage=Could not buzz as %S's client does not support it or %S does not wish to receive buzz.
> +conversation.error.unableBuzz=Could not buzz %S, so we will send normal message.
> +conversation.error.unableBuzzWithoutMessage=Could not buzz %S.

conversation.error.buzzWithoutResourceOrMessage=Try talking to %S first to get their attention.

@@ +213,5 @@
>  conversation.message.mucShutdown=You have been removed from the room because of a system shutdown.
>  
> +# LOCALIZATION NOTE (conversation.message.buzzWithoutMessage):
> +#	This is displayed as a system message when when an attention message is
> +#	received without body.

...without a message text.
(Don't assume localizers know what body is ;)

@@ +215,5 @@
> +# LOCALIZATION NOTE (conversation.message.buzzWithoutMessage):
> +#	This is displayed as a system message when when an attention message is
> +#	received without body.
> +#   %S is the name of the message sender.
> +conversation.message.buzzWithoutMessage=%S has requested your attention.

conversation.message.attentionRequestWithoutMessage

@@ +258,5 @@
>  command.invite=%S <jid>[<message>]: Invite a user to join the current room with an optional message.
>  command.me=%S <action to perform>: Perform an action.
>  command.nick=%S <new nickname>: Change your nickname.
>  command.msg=%S <nick> <message>: Send a private message to a participant in the room.
> +command.buzz=%S [<message>]: Grap user's attention with an optional message if available, otherwise send normal message.

Let's simplify this to
"Try to get the user's attention, together with an optional message."

::: chat/protocols/xmpp/xmpp.jsm
@@ +660,5 @@
>      delete this._typingState;
>    },
>  
> +  // XEP-0224: Attention.
> +  // Used to grap user's attention with optional message.

Grab the user's attention, with an optional message.

@@ +663,5 @@
> +  // XEP-0224: Attention.
> +  // Used to grap user's attention with optional message.
> +  getAttention: function(aMsg = null) {
> +    let iq = Stanza.iq("get", null, this.to,
> +                       Stanza.node("query", Stanza.NS.disco_info));

Add a TODO comment as we should really be using Entity Capabilities here (there's a bug open for that).

@@ +666,5 @@
> +    let iq = Stanza.iq("get", null, this.to,
> +                       Stanza.node("query", Stanza.NS.disco_info));
> +    this._account.sendStanza(iq, aStanza => {
> +      let features = this._account.onInfoDiscovery(aStanza);
> +      if (features.indexOf(Stanza.NS.attention) != -1) {

Array.includes has shipped, so let's use it ;)

@@ +751,5 @@
> +    let attention = aStanza.getElement(["attention"]);
> +    if (attention) {
> +      if (!aMsg) {
> +        aMsg = _("conversation.message.buzzWithoutMessage", this.shortName);
> +        flags.system = true;

This won't actually act as a ping (I guess we would need the incoming flag?), and a system message is easy to overlook. How about making an exception here and showing the replacement message we generate as a /me type message?

Comment 20

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: v2 - Implement attention (XEP-0224)

(In reply to aleth [:aleth] from comment #19)
> I think we can use the same message for both here
> conversation.error.buzzNotAllowed=%S's client does not allow this command.
> Because the message we send (if we can) will appear as a normal message in
> the conversation view anyway, right?

Yes, makes sense.

> This won't actually act as a ping (I guess we would need the incoming
> flag?), and a system message is easy to overlook. How about making an
> exception here and showing the replacement message we generate as a /me type
> message?

Done in this patch.

Comment 21

[aleth [:aleth]]

Comment on attachment 8763389 [details] [diff] [review]
v2 - Implement attention (XEP-0224)

Review of attachment 8763389 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/locales/en-US/xmpp.properties
@@ +210,5 @@
>  #   a room because of a system shutdown.
>  conversation.message.mucShutdown=You have been removed from the room because of a system shutdown.
>  
> +# LOCALIZATION NOTE (conversation.message.attentionRequestWithoutMessage):
> +#	This is displayed as an action message when when an attention message is

Nit: indentation

Better explain to localizers what action message means (i.e. that this will be the equivalent of "/me has ...")

Hmm, it's possible localizing this is nontrivial for RTL languages. I wonder what clokep thinks about this?

::: chat/protocols/xmpp/xmpp.jsm
@@ +667,5 @@
> +    let iq = Stanza.iq("get", null, this.to,
> +                       Stanza.node("query", Stanza.NS.disco_info));
> +    this._account.sendStanza(iq, aStanza => {
> +      let features = this._account.onInfoDiscovery(aStanza);
> +      if (features.includes(Stanza.NS.attention) != -1) {

includes returns a boolean

Comment 22

[Patrick Cloke [:clokep]]

Comment on attachment 8763389 [details] [diff] [review]
v2 - Implement attention (XEP-0224)

Review of attachment 8763389 [details] [diff] [review]:
-----------------------------------------------------------------

Are there any reasonable tests that we should be including here?

::: chat/locales/en-US/xmpp.properties
@@ +212,5 @@
>  
> +# LOCALIZATION NOTE (conversation.message.attentionRequestWithoutMessage):
> +#	This is displayed as an action message when when an attention message is
> +#	received without a message text.
> +conversation.message.attentionRequestWithoutMessage=has requested your attention.

Why is this sent as an action message? Shouldn't it be done as a system message that says "^s has requested your attention."?

@@ +255,5 @@
>  command.invite=%S <jid>[<message>]: Invite a user to join the current room with an optional message.
>  command.me=%S <action to perform>: Perform an action.
>  command.nick=%S <new nickname>: Change your nickname.
>  command.msg=%S <nick> <message>: Send a private message to a participant in the room.
> +command.buzz=%S [<message>]: Try to get the user's attention, together with an optional message.

This sentence is funny, I'd suggest "Try to get the user's attention, you can optionally include a message."

::: chat/protocols/xmpp/xmpp-commands.jsm
@@ +126,5 @@
> +      let params = aMsg.trim();
> +      let conv = getConv(aConv);
> +
> +      // We do not have user's resource, so we will send a normal message if
> +      // it's provided.

This sentence doesn't make sense to me. Which user's resource do we not have?

::: chat/protocols/xmpp/xmpp.jsm
@@ +674,5 @@
> +        this._account.sendStanza(s);
> +      }
> +      else {
> +        // User's client does not support it or user does not allow to receive
> +        // buzz, so we will send normal message.

Don't we only send a message if one was provided? What if I do "/buzz" (with no optional message) and the other user's client doesn't support it?

@@ +749,5 @@
> +    if (attention) {
> +      flags.containsNick = true;
> +      if (!aMsg) {
> +        // This is an exception as system messages will not act as a ping, so
> +        // we will show our message as an action message.

Is it a bug that system messages will not ping?

Comment 23

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: v3 - Implement attention (XEP-0224)

(In reply to Patrick Cloke [:clokep] from comment #22)
> Are there any reasonable tests that we should be including here?

I think no.

> Why is this sent as an action message? Shouldn't it be done as a system
> message that says "^s has requested your attention."?

> Is it a bug that system messages will not ping?

Systems messages do not act as ping, but we want to notify the user so we used action messages instead (comment 19)

> This sentence doesn't make sense to me. Which user's resource do we not have?

The resource part of jid (local@domain/resource) and it's optional for jid, but required for attention.

> Don't we only send a message if one was provided? What if I do "/buzz" (with
> no optional message) and the other user's client doesn't support it?

We will only show the system message |buzzNotAllowed|

Comment 24

[Patrick Cloke [:clokep]]

(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #23)
> Created attachment 8763588 [details] [diff] [review]
> v3 - Implement attention (XEP-0224)
> > Why is this sent as an action message? Shouldn't it be done as a system
> > message that says "^s has requested your attention."?
> 
> > Is it a bug that system messages will not ping?
> 
> Systems messages do not act as ping, but we want to notify the user so we
> used action messages instead (comment 19)

This doesn't answer my question, it just says what the current behavior is. :) I think making this an action message is weird.

> > This sentence doesn't make sense to me. Which user's resource do we not have?
> 
> The resource part of jid (local@domain/resource) and it's optional for jid,
> but required for attention.

This didn't answer my question. This is referring to the remote user?

> > Don't we only send a message if one was provided? What if I do "/buzz" (with
> > no optional message) and the other user's client doesn't support it?
> 
> We will only show the system message |buzzNotAllowed|

I think that means that this comment is wrong (and it's also weird user behavior to sometimes not try to get their attention, but maybe there's nothing we can do about that).

Comment 25

[Abdelrahman Ahmed [:abdelrahman]]

(In reply to Patrick Cloke [:clokep] from comment #24)
> This doesn't answer my question, it just says what the current behavior is.
> :) I think making this an action message is weird.

> Is it a bug that system messages will not ping?

Yes
 
> This didn't answer my question. This is referring to the remote user?

Yes

> I think that means that this comment is wrong (and it's also weird user
> behavior to sometimes not try to get their attention, but maybe there's
> nothing we can do about that).

You are right. I added to that comment "if it's provided" in the patch (v3)

Comment 26

[aleth [:aleth]]

(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #25)
> (In reply to Patrick Cloke [:clokep] from comment #24)
> > This doesn't answer my question, it just says what the current behavior is.
> > :) I think making this an action message is weird.
> 
> > Is it a bug that system messages will not ping?
> 
> Yes

I think this was intentional. System messages come from Instantbird, not some other person, and you don't want e.g. your own channel joins to ping you.

The problem here is that we have a message generated by Instantbird (so like a system message) but in response to direct action from a contact (/buzz without a message), so it's not clear what to do. I think we definitely want it to ping (however we display the message we generate) because otherwise it won't get anyone's attention.

Comment 27

[aleth [:aleth]]

(I suggested an action message not because I'm super happy with it, but because it was the best I could come up with - buzz is like a user action, after all)

Comment 28

[Patrick Cloke [:clokep]]

Comment on attachment 8763588 [details] [diff] [review]
v3 - Implement attention (XEP-0224)

Review of attachment 8763588 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/locales/en-US/xmpp.properties
@@ +210,5 @@
>  #   a room because of a system shutdown.
>  conversation.message.mucShutdown=You have been removed from the room because of a system shutdown.
>  
> +# LOCALIZATION NOTE (conversation.message.attentionRequestWithoutMessage):
> +#   This is displayed as an action message ("/me " is appended to the message)

The message is prefixed with "/me ", appended implies it's being added to the end of the message.

Comment 29

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: v4 - Implement attention (XEP-0224)

Comment 30

[Patrick Cloke [:clokep]]

Resetting assignee due to lack of activity.