1347202 - Joining a new Chat causes some issues with current implementation of joinChat

Status: RESOLVED INVALID

(Chat Core :: Matrix, enhancement)

Description

[Pavan Karthik [:matrixisreal]]

In joinRoom.then( )'s callback getRoom is called
 
Apparently the issue is that the room information isn't available until it gets pulled in from the sync,So getRoom returns null even when the joinRoom request completes.

Comment 1

[Patrick Cloke [:clokep]]

Can you add some steps to reproduce this issue so I'll be able to test this when you fix it? :) Thanks!

Comment 2

[Pavan Karthik [:matrixisreal]]

Attachment: test

STR :  
1. Apply this patch.
2. use /join <channel address or ID> to join a new channel

Example : 
/join #mozilla_#instantbird 
         (Note: The channel ,in this case instantbird shouldn't be in the rooms                 list of the account, make sure to check using diff client.)

Expected Result : NewChannel is opened in the new tab.

Actual Result: New tab Opens, but the conversation is not built.
and an error this.room is null is thrown in the join Room

The call stack is : 
joinChat -> joinRoom -> callback ->  initListOfParticipants ->this.room.getJoinedmems()
which is where error is triggered.

Hope the STR is'nt too complicated.

Comment 3

[Pavan Karthik [:matrixisreal]]

You might also do 
  account.joinChat(channel address or ID)
in   https://dxr.mozilla.org/comm-central/source/chat/protocols/matrix/matrix.js#130
if you dont want to use /join.

Comment 4

[Pavan Karthik [:matrixisreal]]

Attachment: new.patch

Had to make some changes above the scope of the bug, as they were required and necessary for the fix.

As discussed earlier, getRooms() returns null after the client.joinRoom, 
so UI has to be updated iff when Room data can be fetched.

The "Room" Event is fired whenever the client is ready with room data.
    // "Room" event is fired for every new room Object ever created. i.e:
    // 1. For every Room the account was already in.(After the client is created).
    // 2. For every new Room the user Joins.
    // 3. For every Room the user gets a "invite" request.

So UI must be updated in the Room event handler.

As (1) this also fixes the bug 1346519

Comment 5

[Pavan Karthik [:matrixisreal]]

Apart from this there are some changes which I find that had to be made regarding how Rooms/convs are maintained

1. a better way to identify Rooms.-  
       (I propose every conv should have name,roomId, completeAlias )
2. I added a conversations list which is necessary which makes the _roomList redundant, Shall we remove _roomList?

3. a better way to handle user names. (Instead of userId we should probably display nicks and allow the user to hover when they need the ID )

4. Handle Join messages.

I dont know if each of these need a bug report.

Comment 6

[Patrick Cloke [:clokep]]

Comment on attachment 8847562 [details] [diff] [review]
new.patch

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

::: chat/protocols/matrix/matrix.js
@@ +165,5 @@
>          account._roomList[room.roomId].
>            writeMessage(event.getSender(), body, { incoming: true });
>        }
>      });
> +    // "Room" event is fired for every new room Object ever created. i.e:

Are you able to "leave" a room then?

@@ +173,5 @@
> +    this._client.on("Room", function(room) {
> +      // TODO: Handle invites.
> +      let conv = account.getConversation(room.roomId);
> +      conv._roomId = room.roomId;
> +      account._roomList[room.roomId] = conv;

Now we have `conversations` and `_roomList`, what's the difference?

@@ +247,5 @@
> +  createConversation: function(aName) { return this.getConversation(aName); },
> +
> +  // Returns a conversation.
> +  // Creates it if it is not in the conversations list.
> +  getConversation: function(aName) {

Is this copied from IRC? Do like all of our protocols use this same design?

Comment 7

[Pavan Karthik [:matrixisreal]]

(In reply to Patrick Cloke [:clokep] from comment #6)
> Comment on attachment 8847562 [details] [diff] [review]
> new.patch
> 
> Review of attachment 8847562 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: chat/protocols/matrix/matrix.js
> @@ +165,5 @@
> >          account._roomList[room.roomId].
> >            writeMessage(event.getSender(), body, { incoming: true });
> >        }
> >      });
> > +    // "Room" event is fired for every new room Object ever created. i.e:
> 
> Are you able to "leave" a room then?

  I don't clearly understand what you are asking, but leaving rooms is not yet implemented.


> 
> @@ +173,5 @@
> > +    this._client.on("Room", function(room) {
> > +      // TODO: Handle invites.
> > +      let conv = account.getConversation(room.roomId);
> > +      conv._roomId = room.roomId;
> > +      account._roomList[room.roomId] = conv;
> 
> Now we have `conversations` and `_roomList`, what's the difference?

conversations map is necessary and sufficient to maintain a record of all rooms.
We should remove _roomList and replace it with conversations

> 
> @@ +247,5 @@
> > +  createConversation: function(aName) { return this.getConversation(aName); },
> > +
> > +  // Returns a conversation.
> > +  // Creates it if it is not in the conversations list.
> > +  getConversation: function(aName) {
> 
> Is this copied from IRC? Do like all of our protocols use this same design?

Yes the design is pretty much what all protocols use..

Comment 8

[Patrick Cloke [:clokep]]

The implementation of Matrix needs to be updated in general.