Closed Bug 1347202 Opened 5 years ago Closed 3 years ago

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

Categories

(Chat Core :: Matrix, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED INVALID

People

(Reporter: matrixisreal, Assigned: matrixisreal)

References

Details

Attachments

(2 files)

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.
Assignee: nobody → pavankarthikboddeda
Can you add some steps to reproduce this issue so I'll be able to test this when you fix it? :) Thanks!
Attached patch testSplinter Review
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.

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.
Blocks: 1346519
Attached patch new.patchSplinter Review
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
Attachment #8847562 - Flags: review?(clokep)
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.
Blocks: 1346441
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?
Attachment #8847562 - Flags: review?(clokep) → review-
(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..
Blocks: 1394397

The implementation of Matrix needs to be updated in general.

Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.