Add /part and /join commands for XMPP MUCs

RESOLVED FIXED in 1.6

Status

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: sawrubh, Assigned: abdelrahman)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 8 obsolete attachments)

Comment hidden (empty)
Summary: Implement the left getter → Implement the left getter for XMPP MUCs

Comment 1

5 years ago
The left getter is in fact inherited from jsProtoHelper ;)

I'm mutating this bug to support leaving a MUC, by adding /part command, which is what is really missing. A /join command is also needed (/join without parameters should rejoin a left channel).
OS: Linux → All
Hardware: x86_64 → All
Summary: Implement the left getter for XMPP MUCs → Add /part and /join commands for XMPP MUCs
(Reporter)

Comment 2

5 years ago
I will not be able to take a look at this before December so unassigning myself.
Assignee: saurabhanandiit → nobody
(Assignee)

Comment 3

4 years ago
Assignee: nobody → a.ahmed1026
Status: NEW → ASSIGNED
Attachment #8605379 - Flags: review?(aleth)

Comment 4

4 years ago
Comment on attachment 8605379 [details] [diff] [review]
rev 1 - implement part and join commands

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

::: chat/protocols/xmpp/xmpp-commands.jsm
@@ +6,5 @@
> +
> +const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
> +
> +Cu.import("resource:///modules/imXPCOMUtils.jsm");
> +Cu.import("resource:///modules/jsProtoHelper.jsm");

Why do you need to import this?

@@ +7,5 @@
> +const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
> +
> +Cu.import("resource:///modules/imXPCOMUtils.jsm");
> +Cu.import("resource:///modules/jsProtoHelper.jsm");
> +Cu.import("resource:///modules/xmpp.jsm");

Don't import this here.

@@ +30,5 @@
> +      let conv;
> +      if (!params[0]) {
> +        conv = getConv(aConv);
> +        if (!conv.left)
> +          return false;

Only return false if the current tab is not a MUC. If it's a MUC and it's not left, then there is nothing to do and you can return true.

@@ +38,5 @@
> +        if (conv.chatRoomFields) {
> +          account.joinChat(conv.chatRoomFields);
> +          return true;
> +        }
> +        return false;

Instead of returning false, set params[0] appropriately if the current conv is a MUC.

@@ +42,5 @@
> +        return false;
> +      }
> +      let chatRoomFields = account.getChatRoomDefaultFieldValues(params[0]);
> +      if (params[1])
> +        chatRoomFields.setValue("password", params[1]);

I'm pretty sure getChatRoomDefaultFieldValues is smart enough to be able to handle passwords, i.e.
let chatRoomFields = account.getChatRoomDefaultFieldValues("roomname password");

@@ +53,5 @@
> +  {
> +    name: "part",
> +    get helpString() _("command.part"),
> +    usageContext: Ci.imICommand.CMD_CONTEXT_CHAT,
> +    run: function (aMsg, aConv) {

nit: no space after function

@@ +56,5 @@
> +    usageContext: Ci.imICommand.CMD_CONTEXT_CHAT,
> +    run: function (aMsg, aConv) {
> +      let conv = getConv(aConv);
> +      if (conv.left)
> +        return false;

It's probably better to return true here. After all, it's OK if /part does nothing if the room is already parted. (Returning false means the help string will be printed.)
Attachment #8605379 - Flags: review?(aleth) → review-
Comment on attachment 8605379 [details] [diff] [review]
rev 1 - implement part and join commands

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

Thanks for taking a look at this! I've included some comments to add to aleth's.

::: chat/protocols/xmpp/xmpp-commands.jsm
@@ +23,5 @@
> +var commands = [
> +  {
> +    name: "join",
> +    get helpString() _("command.join"),
> +    run: function(aMsg, aConv, aReturnedConv) {

Formatting nit: Please use some empty lines here to split this method in logical parts.

::: chat/protocols/xmpp/xmpp.js
@@ +19,5 @@
>  XMPPAccount.prototype = XMPPAccountPrototype;
>  
>  function XMPPProtocol() {
> +  Cu.import("resource:///modules/xmpp-commands.jsm", this);
> +  this.registerCommands();

This should also be added to GTalk, I assume?

::: chat/protocols/xmpp/xmpp.jsm
@@ +254,5 @@
>  
> +  // Leaves MUC conversation.
> +  part: function(aMsg = null) {
> +    let s = Stanza.presence({to: this.name + "/" + this._nick,
> +                            type: "unavailable"},

Nit: Line up "to" and "type".

@@ +257,5 @@
> +    let s = Stanza.presence({to: this.name + "/" + this._nick,
> +                            type: "unavailable"},
> +                            aMsg ? Stanza.node("status", null, null, aMsg) : null);
> +    let account = this._account;
> +    this._account.sendStanza(s, aStanza => {

If you're going to store account into a separate variable, please use it on this line too!
(Assignee)

Comment 6

4 years ago
(In reply to aleth [:aleth] from comment #4)
> Why do you need to import this?
I used them for testing and I forgot to remove them.

> I'm pretty sure getChatRoomDefaultFieldValues is smart enough to be able to
> handle passwords, i.e.
> let chatRoomFields = account.getChatRoomDefaultFieldValues("roomname
> password");
I'm not sure for that, because this does not apply on XMPP but it's right for IRC.
Check the sequence of calling functions in XMPP and it will end up with _parseJID method
(https://mxr.mozilla.org/comm-central/source/chat/protocols/xmpp/xmpp.jsm#1309)

(In reply to Patrick Cloke [:clokep] from comment #5)
> This should also be added to GTalk, I assume?
yes, you are right.
Attachment #8605379 - Attachment is obsolete: true
Attachment #8606017 - Flags: review?(aleth)

Comment 7

4 years ago
(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #6)
> > I'm pretty sure getChatRoomDefaultFieldValues is smart enough to be able to
> > handle passwords, i.e.
> > let chatRoomFields = account.getChatRoomDefaultFieldValues("roomname
> > password");
> I'm not sure for that, because this does not apply on XMPP but it's right
> for IRC.
> Check the sequence of calling functions in XMPP and it will end up with
> _parseJID method
> (https://mxr.mozilla.org/comm-central/source/chat/protocols/xmpp/xmpp.
> jsm#1309)

Yes, but you could extend parseDefaultChatName to handle passwords if JIDs can't contain spaces.
 
> (In reply to Patrick Cloke [:clokep] from comment #5)
> > This should also be added to GTalk, I assume?
> yes, you are right.

Has anyone tried if it's possible to join MUCs from gtalk 1) hosted by google 2) on other servers, given they don't federate?

Comment 8

4 years ago
Comment on attachment 8606017 [details] [diff] [review]
rev 2 - implement part and join commands

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

::: chat/protocols/xmpp/xmpp-commands.jsm
@@ +28,5 @@
> +      let conv;
> +
> +      if (!params[0]) {
> +        conv = getConv(aConv);
> +        if (!account._mucs.has(conv.name))

Use conv.isChat.

@@ +61,5 @@
> +    run: function(aMsg, aConv) {
> +      let conv = getConv(aConv);
> +      if (conv.left)
> +        return true;
> +      conv.part(aMsg.trim());

if (!conv.left) will save one "return true".
Attachment #8606017 - Flags: review?(aleth) → review-
(In reply to aleth [:aleth] from comment #7)
> > (In reply to Patrick Cloke [:clokep] from comment #5)
> > > This should also be added to GTalk, I assume?
> > yes, you are right.
> 
> Has anyone tried if it's possible to join MUCs from gtalk 1) hosted by
> google 2) on other servers, given they don't federate?

I had done this a while ago, but have not checked recently.
(Assignee)

Comment 10

4 years ago
Attachment #8606017 - Attachment is obsolete: true
Attachment #8606513 - Flags: review?(aleth)

Comment 11

4 years ago
Comment on attachment 8606513 [details] [diff] [review]
rev 3 - implement part and join commands

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

::: chat/protocols/xmpp/xmpp.jsm
@@ +769,5 @@
>    parseDefaultChatName: function(aDefaultChatName) {
>      if (!aDefaultChatName)
>        return {nick: this._jid.node};
>  
> +    let params = aDefaultChatName.trim().split(" ");

...split(/\s+/); would be better. Just in case the user typed two spaces etc.

@@ +775,5 @@
>      return {
>        room: jid.node,
>        server: jid.domain,
> +      nick: jid.resource || this._jid.node,
> +      password: params[1]

I'd prefer if we didn't set a password attribute if there is no params[1]. It's safer/cleaner that way because e.g. ("password" in chatRoomFields)==true for chatRoomFields.password = undefined;
Attachment #8606513 - Flags: review?(aleth) → review-
(Assignee)

Comment 12

4 years ago
Attachment #8606513 - Attachment is obsolete: true
Attachment #8606537 - Flags: review?(aleth)

Comment 13

4 years ago
Comment on attachment 8606537 [details] [diff] [review]
rev 4 - implement part and join commands

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

Some issues discovered during testing:
- After /part, the participant list still shows the user as present.
- Trying to send a message in a parted room makes a new tab open, with the same title, containing a system message "This message could not be delivered: Unknown error". Looks like an item-not-found error stanza isn't being handled well - we should instead put a better system message in the right tab.
- There is currently no system message when someone joins or leaves a room (and so the part message isn't shown either). This can be a followup bug.
- Didn't you add support for receiving invitations? It would be nice if we could send them too (with an /invite command). This is also a followup.
- It would be nice if there was a way to find out what the correct server to use "by default" is (i.e. find out the MUC server name for the server the user has an account on). There's probably a XEP for that ;) Then we could support "/join roomname".

Otherwise it works well!

::: chat/locales/en-US/xmpp.properties
@@ +115,5 @@
>  odnoklassniki.usernameHint=Profile ID
> +
> +# LOCALZIATION NOTE (command.*):
> +#  These are the help messages for each command.
> +command.join=join [<room@server>] [<password>]: Join a room, optionally providing a password of the room if needed.

You don't need the "of the room" here.

@@ +116,5 @@
> +
> +# LOCALZIATION NOTE (command.*):
> +#  These are the help messages for each command.
> +command.join=join [<room@server>] [<password>]: Join a room, optionally providing a password of the room if needed.
> +command.part=part [<reason>]: Leave a room.

For clarity let's use
command.part=part [<message>]: Leave the current room with an optional message.
Attachment #8606537 - Flags: review?(aleth) → review-

Comment 14

4 years ago
Comment on attachment 8606537 [details] [diff] [review]
rev 4 - implement part and join commands

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

::: chat/locales/en-US/xmpp.properties
@@ +115,5 @@
>  odnoklassniki.usernameHint=Profile ID
> +
> +# LOCALZIATION NOTE (command.*):
> +#  These are the help messages for each command.
> +command.join=join [<room@server>] [<password>]: Join a room, optionally providing a password of the room if needed.

Actually it's room@server/nick. XMPP MUCs have this feature that the nick you use in the chat doesn't have to match the name of the account.

I wonder what happens if we try to join the same room twice, with two different nicks? Does the spec say anything about that?

Comment 15

4 years ago
(In reply to aleth [:aleth] from comment #14)
> I wonder what happens if we try to join the same room twice, with two
> different nicks? Does the spec say anything about that?

I don't mean to suggest we should support this even if it is possible ;) It's just important that nothing breaks if someone tries.
(Assignee)

Comment 16

4 years ago
(In reply to aleth [:aleth] from comment #13)
> Some issues discovered during testing:
> - After /part, the participant list still shows the user as present.
I have handled this and also I have handled removing all participants to keep list updated before joining.

> - Trying to send a message in a parted room makes a new tab open, with the
> same title, containing a system message "This message could not be
> delivered: Unknown error". Looks like an item-not-found error stanza isn't
> being handled well - we should instead put a better system message in the
> right tab.
The error was not-acceptable and that was because of sending message to a parted room. I handled that with a system message in the right tab.

There was a problem to put message in correct conversation because error messages come to the muc room tab and the chat tab between two participants in this room. I used the callback to solve this issue.

Also there was an issue while sending message in chat tab in attribute to. The resource
is appended to jid even it exists e.g.(test23@conference.xmpp.jp/test20/test20) and this issue comes from normalization of jid when you try to send a private message to a participant in muc and the case when a participant initiates to send a private message to you (you will find the problem in the name/title of conversation), we can solve this also in a follow up bug.

> - It would be nice if there was a way to find out what the correct server to
> use "by default" is (i.e. find out the MUC server name for the server the
> user has an account on). There's probably a XEP for that ;) Then we could
> support "/join roomname".
Let this also be a follow up bug.

(In reply to aleth [:aleth] from comment #14)
> I wonder what happens if we try to join the same room twice, with two
> different nicks? Does the spec say anything about that?
I have not checked that in the spec, but our behavior will not do anything because joinChat method will return the current conversation of muc.
(https://dxr.mozilla.org/comm-central/source/chat/protocols/xmpp/xmpp.jsm?from=xmpp.jsm#784)
Attachment #8606537 - Attachment is obsolete: true
Attachment #8609791 - Flags: review?(clokep)
Attachment #8609791 - Flags: review?(aleth)
Comment on attachment 8609791 [details] [diff] [review]
rev 5 - implement part and join commands

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

How hard would it be to write tests for some of the stanza creation functions or for the commands?

Overall it looks good, my comments are all fairly minor / asking for more information. Thanks for taking another look at this. :)

::: chat/locales/en-US/xmpp.properties
@@ +52,5 @@
>  conversation.error.joinFailedRemoteServerNotFound=Could not join the room %S as the server the room is hosted on could not be reached.
> +#   This is displayed in a conversation as an error message when sending to the
> +#   room is not acceptable.
> +#   %S is the name of MUC room.
> +conversation.error.notAcceptable=Could not send to the room %S as sending is not acceptable.

This sentence is pretty awkward, what does "not acceptable" mean in this context?

@@ +119,5 @@
>  odnoklassniki.usernameHint=Profile ID
> +
> +# LOCALZIATION NOTE (command.*):
> +#  These are the help messages for each command.
> +command.join=join [<room@server>][/<nick>] [<password>]: Join a room, optionally providing a password if needed.

You can also optionally provide a nick. Please reword this to account for that.

::: chat/protocols/xmpp/xmpp.jsm
@@ +95,5 @@
>    // By default users are not in a MUC.
>     _left: true,
>  
>    _init: function(aAccount, aJID, aNick) {
> +    this._messageID = [];

Make this a set for better performance. Please name this "_messageId". (I know, ID is a weird one.)

@@ +268,5 @@
> +    this.notifyObservers(new nsSimpleEnumerator([nickString]),
> +                         "chat-buddy-remove");
> +  },
> +
> +  // Removes all praticipants in MUC conversation.

Typo: "participants"

@@ +383,5 @@
>    _targetResource: "",
>    get to() {
> +    if (!this._targetResource)
> +      return this.userName;
> +    return this._account.normalize(this.userName) + "/" + this._targetResource;

I'm not confident about these changes. Someone else will need to verify them.

@@ +393,5 @@
>      let cs = this.shouldSendTypingNotifications ? "active" : null;
>      let s = Stanza.message(this.to, aMsg, cs);
> +    this._account.sendStanza(s, aStanza => {
> +      this.incomingMessage(aMsg, aStanza, undefined);
> +    });

What does this change do? Please add a comment.
Attachment #8609791 - Flags: review?(clokep) → review-
(Assignee)

Comment 18

4 years ago
(In reply to Patrick Cloke [:clokep] from comment #17)
> How hard would it be to write tests for some of the stanza creation
> functions or for the commands?
Currently, XMPP has no tests and we can do that in a follow up bug, but I'm not sure if we can test on real service.

> This sentence is pretty awkward, what does "not acceptable" mean in this
> context?
When user leaves MUC room and tries to send a message to this room in its conversation, the service sends an error message informs you that sending to this room is not acceptable.

> You can also optionally provide a nick. Please reword this to account for
> that.
can this be better?
>"Join a room, optionally providing a password or a nick if needed."

> I'm not confident about these changes. Someone else will need to verify them.
> What does this change do? Please add a comment.
Check comment #16 and for the next patch I will add comment in code clarifying this section.
(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #18)
> (In reply to Patrick Cloke [:clokep] from comment #17)
> > How hard would it be to write tests for some of the stanza creation
> > functions or for the commands?
> Currently, XMPP has no tests and we can do that in a follow up bug, but I'm
> not sure if we can test on real service.

Not testing with real services, tests MUST be run locally. I was thinking more of testing that when you call a method (e.g. partRoom), testing whether the XML that gets generated is as expected. I'd like them to get added soon.

> > This sentence is pretty awkward, what does "not acceptable" mean in this
> > context?
> When user leaves MUC room and tries to send a message to this room in its
> conversation, the service sends an error message informs you that sending to
> this room is not acceptable.

So how about: "Could not send messages to %S as you are no longer in the room."?

> > You can also optionally provide a nick. Please reword this to account for
> > that.
> can this be better?
> >"Join a room, optionally providing a password or a nick if needed."

I'd rather them be in the order of the optional arguments: "Join a room, optionally providing a different nickname, or the room password."
(Assignee)

Comment 20

4 years ago
(In reply to Patrick Cloke [:clokep] from comment #19)
> Not testing with real services, tests MUST be run locally. I was thinking
> more of testing that when you call a method (e.g. partRoom), testing whether
> the XML that gets generated is as expected. I'd like them to get added soon.
Currently, I'm busy due to exams. I'll work on it, but I need sometime for that.
Also I think we need to break current methods because they generate XML and send it, so we can't test it.

> So how about: "Could not send messages to %S as you are no longer in the
> room."?
That's good one, I just modified it to contain also the message.

For this patch,
I reverted the changes of private messages to MUC as there is a separate bug for that, but added the handling that error on MUC conversation.
I used callback method because I found out in XEP-0045 spec. that "not-acceptable" can convey different meanings and every meaning depends on message/request you send and it's sent back with the same id.
Attachment #8609791 - Attachment is obsolete: true
Attachment #8609791 - Flags: review?(aleth)
Attachment #8613334 - Flags: review?(clokep)

Comment 21

4 years ago
(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #20)
> > Not testing with real services, tests MUST be run locally. I was thinking
> > more of testing that when you call a method (e.g. partRoom), testing whether
> > the XML that gets generated is as expected. I'd like them to get added soon.
> Currently, I'm busy due to exams. I'll work on it, but I need sometime for
> that.
> Also I think we need to break current methods because they generate XML and
> send it, so we can't test it.

+1 for XMPP tests! A good place to start would be tests for xmpp-xml.jsm, as everything else builds on that. There's already a separate bug for adding tests though.
 
> > Also there was an issue while sending message in chat tab in attribute to. The resource
> > is appended to jid even it exists e.g.(test23@conference.xmpp.jp/test20/test20) and this issue comes from normalization of
> > jid when you try to send a private message to a participant in muc and the case when a participant initiates to send a 
> > private message to you (you will find the problem in the name/title of conversation), we can solve this also in a follow up bug.
> I reverted the changes of private messages to MUC as there is a separate bug
> for that, but added the handling that error on MUC conversation.

You're absolutely right, dealing with the different ways jids are interpreted in MUCs (resources vs nicks) is probably the hardest item on the MUC todo list.

Comment 22

4 years ago
Comment on attachment 8613334 [details] [diff] [review]
rev 6 - implement part and join commands

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

This has improved a lot!

::: chat/locales/en-US/xmpp.properties
@@ +53,5 @@
> +#   This is displayed in a conversation as an error message when the user sends
> +#   a message to a room that is not in.
> +#   %1$S is the name of MUC room.
> +#   %2$S is the text of the message that wasn't delivered.
> +conversation.error.sendingMessageNotAcceptable=This message Could not be sent to %1$S as you are no longer in the room: %2$S

How about conversation.error.sendFailedAsNotInRoom ? Also, there's a typo in the string: Could -> could, and just "Message" instead of "This message" is probably better.

::: chat/protocols/xmpp/xmpp-commands.jsm
@@ +40,5 @@
> +          account.joinChat(conv.chatRoomFields);
> +          return true;
> +        }
> +
> +        aMsg = conv.name;

Coding style: It's generally not a good idea to overwrite arguments, use an auxiliary variable for this.

::: chat/protocols/xmpp/xmpp.jsm
@@ +95,5 @@
>    // By default users are not in a MUC.
>     _left: true,
>  
>    _init: function(aAccount, aJID, aNick) {
> +    this._messageId = new Set();

nit: _messageIds (plural)

@@ +108,2 @@
>      let s = Stanza.message(this.name, aMsg, null, {type: "groupchat"});
> +    this._account.sendStanza(s, aStanza => {

You can use handleErrors here, to avoid having to write your own error handler function.

@@ +110,5 @@
> +      let from = aStanza.attributes["from"];
> +      let error = this._account.parseError(aStanza);
> +      if (!error)
> +        return false;
> +      if (error.condition == "not-acceptable") {

"item-not-found" can also happen, if you were the only person in the room.
(Assignee)

Comment 23

4 years ago
Attachment #8613334 - Attachment is obsolete: true
Attachment #8613334 - Flags: review?(clokep)
Attachment #8615374 - Flags: review?(aleth)

Comment 24

4 years ago
Comment on attachment 8615374 [details] [diff] [review]
rev 7 - implement part and join commands

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

::: chat/locales/en-US/xmpp.properties
@@ +50,5 @@
>  #   is not found.
>  #   %S is the name of MUC room.
>  conversation.error.joinFailedRemoteServerNotFound=Could not join the room %S as the server the room is hosted on could not be reached.
> +#   This is displayed in a conversation as an error message when the user sends
> +#   a message to a room that is not in.

nit: ...that he is not in.

::: chat/protocols/xmpp/xmpp-commands.jsm
@@ +23,5 @@
> +    name: "join",
> +    get helpString() _("command.join"),
> +    run: function(aMsg, aConv, aReturnedConv) {
> +      let account = getAccount(aConv);
> +      let message = aMsg.trim();

It's not really a message in this case - let's call it params.

::: chat/protocols/xmpp/xmpp.jsm
@@ +95,5 @@
>    // By default users are not in a MUC.
>     _left: true,
>  
>    _init: function(aAccount, aJID, aNick) {
> +    this._messageIds = new Set();

Coding style: also add this property to the prototype itself, i.e.
_messageIds: new Set(),
with a comment above it saying what it's for.

@@ +117,3 @@
>      let s = Stanza.message(this.name, aMsg, null, {type: "groupchat"});
> +    let errorHandler = this._account.handleErrors({itemNotFound: notInRoom,
> +                                                  notAcceptable: notInRoom});

Move this under the let notInRoom =... block.
Nit: indentation of notAcceptable looks like it's missing one space?
Attachment #8615374 - Flags: review?(aleth) → review-
(Assignee)

Comment 25

4 years ago
Attachment #8615374 - Attachment is obsolete: true
Attachment #8615397 - Flags: review?(aleth)

Comment 26

4 years ago
Comment on attachment 8615397 [details] [diff] [review]
rev 8 - implement part and join commands

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

Works very well, thanks! Just some nits left.

::: chat/protocols/xmpp/xmpp.jsm
@@ +92,5 @@
>  // MUC (Multi-User Chat)
>  const XMPPMUCConversationPrototype = {
>    __proto__: GenericConvChatPrototype,
>    // By default users are not in a MUC.
>     _left: true,

The indentation is wrong here, let's fix that.

@@ +94,5 @@
>    __proto__: GenericConvChatPrototype,
>    // By default users are not in a MUC.
>     _left: true,
>  
> +  // Track received messages to avoid duplication in UI.

Let's mention why we may get duplication:
"Tracks all received messages to avoid possible duplication if the server sends us the last few messages again when we rejoin a room."

@@ +95,5 @@
>    // By default users are not in a MUC.
>     _left: true,
>  
> +  // Track received messages to avoid duplication in UI.
> +   _messageIds: new Set(),

and here too.

Updated

4 years ago
Attachment #8615397 - Flags: review?(aleth) → review-
(Assignee)

Comment 27

4 years ago
Attachment #8615397 - Attachment is obsolete: true
Attachment #8615508 - Flags: review?(aleth)

Comment 28

4 years ago
Comment on attachment 8615508 [details] [diff] [review]
rev 9 - implement part and join commands

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

Thanks!

Please file the followup bugs discussed in the comments here if they don't already exist, so they don't get forgotten!
Attachment #8615508 - Flags: review?(aleth) → review+

Updated

4 years ago
Keywords: checkin-needed

Updated

4 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 1.6

Comment 30

4 years ago
I noticed when parting jdev@conference.jabber.org that that server unfortunately does not include the id when responding to our part stanza, so the callback doesn't fire. But as onPresenceStanza already turns out to handle the same stanza as the callback, it turns out that doesn't matter much in this case, and in fact it looks to me like we can simply remove the duplicated code. Or am I missing something?
Attachment #8622648 - Flags: review?(a.ahmed1026)
(Assignee)

Comment 31

4 years ago
Comment on attachment 8622648 [details] [diff] [review]
partfollowup.diff

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

This a good notice as spec (XEP-0045) does not say that the service must response with the same id.

::: chat/protocols/xmpp/xmpp.jsm
@@ +328,5 @@
>      let s = Stanza.presence({to: this.name + "/" + this._nick, type: "unavailable"},
>                              aMsg ? Stanza.node("status", null, null, aMsg.trim()) : null);
>      let account = this._account;
> +    account.sendStanza(s);
> +    delete this.chatRoomFields;

I'd suggest moving this delete to the block of handling cases [1]
1. To be confirmed that user has parted the room.
2. To use it in other cases like (kicking, banning, etc.).

[1] https://dxr.mozilla.org/comm-central/source/chat/protocols/xmpp/xmpp.jsm#209
Attachment #8622648 - Flags: feedback+
(Assignee)

Updated

4 years ago
Attachment #8622648 - Flags: review?(a.ahmed1026)

Comment 32

4 years ago
(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #31)
> Comment on attachment 8622648 [details] [diff] [review]
> > +    delete this.chatRoomFields;
> 
> I'd suggest moving this delete to the block of handling cases [1]
> 1. To be confirmed that user has parted the room.
> 2. To use it in other cases like (kicking, banning, etc.).

The effect of delete this.chatRoomFields is to prevent future automatic reconnections, because the user has chosen to leave the room (rather than us leaving accidentally for some reason). I don't want to assume that whenever we leave the room (and [1] is called) that it was intentional. Instead, we should handle it case by case where we are sure about the user's intentions.
(Assignee)

Comment 33

4 years ago
(In reply to aleth [:aleth] from comment #32)
> The effect of delete this.chatRoomFields is to prevent future automatic
> reconnections, because the user has chosen to leave the room (rather than us
> leaving accidentally for some reason). I don't want to assume that whenever
> we leave the room (and [1] is called) that it was intentional. Instead, we
> should handle it case by case where we are sure about the user's intentions.

I also meant to handle all cases in one place like (code 110 means the intentional case).
you are right in case [1] because the user has chosen to leave the room, so it does not matter if he/she gets confirmed about that (just remove connection data).
(Assignee)

Updated

4 years ago
Attachment #8622648 - Flags: feedback+ → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed

Updated

4 years ago
Blocks: 955284
You need to log in before you can comment on or make changes to this bug.