1348064 - Implement Direct Messages for Matrix

Status: RESOLVED FIXED

(Chat Core :: Matrix, enhancement)

Description

[Pavan Karthik [:matrixisreal]]

See: https://bugzilla.mozilla.org/show_bug.cgi?id=1199855#c33

Comment 1

[Pavan Karthik [:matrixisreal]]

Attachment: DM_V0.patch

Tried to handle as many as edge cases as possible.
This patch contains handling DMs, invites, autojoins at startup.
But I am sure this will need some refactoring though,

Thanks.

Comment 2

[Frédéric Wang (:fredw)]

Comment on attachment 8880571 [details] [diff] [review]
DM_V0.patch

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

Just some quick comments...

::: chat/chat-prefs.js
@@ +86,5 @@
>  pref("chat.prpls.prpl-skype.disable", true);
>  // Disable Facebook as the XMPP gateway no longer exists.
>  pref("chat.prpls.prpl-facebook.disable", true);
>  // Disable experimental Matrix support.
> +pref("chat.prpls.prpl-matrix.disable", false);

Are you enabling matrix on purpose or was it just for testing your patch?

::: chat/protocols/matrix/matrix.js
@@ +166,5 @@
>        }
>      });
>      this._client.on("Room.timeline", function(event, room, toStartOfTimeline) {
>        // TODO: Better handle messages!
> +

nit: whitespace added?

@@ +181,5 @@
>            writeMessage(event.getSender(), body, { incoming: true });
>        }
>      });
> +    // "Room" event is fired,
> +    // 1. For every Room the account was already in.(After the client is synced).

nit: weird spacing?

@@ +186,5 @@
> +    // 2. For every new Room the user Joins newly.
> +    // 3. For every Room the user gets an "invite" request.
> +    this._client.on("Room", function(room) {
> +      const me = room.getMember(account.userId);
> +      // RIght now just auto accept the invites by joining the room,

nit: Right

@@ +524,5 @@
> +        let conv = getConv(aConv);
> +        console.log(account._client.getRoom(aMsg));
> +        return true;
> +      }
> +    },

Is it supposed to be in the final patch, or did you just use that while testing it?

Comment 3

[Pavan Karthik [:matrixisreal]]

e);
> >  // Disable experimental Matrix support.
> > +pref("chat.prpls.prpl-matrix.disable", false);
> 
> Are you enabling matrix on purpose or was it just for testing your patch?
> 

just for testing



> 
> @@ +524,5 @@
> > +        let conv = getConv(aConv);
> > +        console.log(account._client.getRoom(aMsg));
> > +        return true;
> > +      }
> > +    },
> 
> Is it supposed to be in the final patch, or did you just use that while
> testing it?

Yeah, I this was just for testing purposes too. Left it because I thought It would be useful for anyone else to test it too. :P

Comment 4

[Pavan Karthik [:matrixisreal]]

Btw, I have updated my blog with my secong blog post, most of which is about this patch.
Check this out. http://pavankarthik.tk :D

Comment 5

[Frédéric Wang (:fredw)]

(In reply to Pavan Karthik [:matrixisreal] from comment #3)
> just for testing
> 
> Yeah, I this was just for testing purposes too. Left it because I thought It
> would be useful for anyone else to test it too. :P

OK, I think in Mozilla the feedback? flag if you just want some feedback on a patch. Using review? assumes that you have a version that you consider ready, which means that debug stuff should not be present.(In reply to Pavan Karthik [:matrixisreal] from comment #4)

> Btw, I have updated my blog with my secong blog post, most of which is about
> this patch.
> Check this out. http://pavankarthik.tk :D

Looks great. Keep up the good work!

Comment 6

[Pavan Karthik [:matrixisreal]]

> OK, I think in Mozilla the feedback? flag if you just want some feedback on
> a patch. Using review? assumes that you have a version that you consider
> ready, which means that debug stuff should not be present.(In 
> 
Oh..  Didn't know :p .Noted. 

> > Btw, I have updated my blog with my secong blog post, most of which is about
> > this patch.
> > Check this out. http://pavankarthik.tk :D
> 
> Looks great. Keep up the good work!

Thanks :D

Comment 7

[Patrick Cloke [:clokep]]

Comment on attachment 8880571 [details] [diff] [review]
DM_V0.patch

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

I gave a bunch of feedback, overall it looks like the code would work, but is extremely confusing. I'm finding it really difficult to keep track of what's our objects vs. what's given to us via the Matrix SDK, etc. This probably means we need more comments describing functionality / storage. I'm also a bit concerned about race conditions in some of the promises. Did you run into any weird errors, etc. while testing this?

::: chat/protocols/matrix/matrix.js
@@ +77,5 @@
> +  __proto__: GenericConvIMPrototype,
> +  sendMsg: function(aMsg) {
> +    this._account._client.sendTextMessage(this._roomId, aMsg);
> +  },
> +  get room() { return this._account._client.getRoom(this._roomId); },

Where's this._roomId get set?

@@ +185,5 @@
> +    // 1. For every Room the account was already in.(After the client is synced).
> +    // 2. For every new Room the user Joins newly.
> +    // 3. For every Room the user gets an "invite" request.
> +    this._client.on("Room", function(room) {
> +      const me = room.getMember(account.userId);

Can this fail? What if you're not in the room at all? Can that happen?

@@ +192,5 @@
> +        // If the 'direct' hint is present, that this is a DM room for
> +        // whoever invited us.
> +        if (me.events.member.getContent().is_direct) {
> +          let roomMembers = room.getJoinedMembers();
> +          if (roomMembers.length == 1) {

What if is_direct is set, but there's more than one member? It looks like in that case we won't even join the room?

@@ +259,5 @@
>      this._client.startClient();
>    },
> +
> +  // A Map from userIds to list of roomIds, which is just the object dmRoomMap from accountData
> +  // which keeps track of DM rooms.

What is this used for?

@@ +266,5 @@
> +  // A Map from roomIds to userIds
> +  // Used to check if a room is DM or not.
> +  // NOTE : Not particularly sure if this is needed,
> +  // we might just have just DMUserToRooms map and have functions to
> +  // check if the room is in the map. But I felt this was more comfortable.

I don't understand why this would be necessary, you have the map of rooms, can't you just check if those are a DM?

@@ +272,5 @@
> +
> +  // when createRoom is called, 'sometimes' the "Room" event is fired
> +  // before it returns a promise. So we won't know at the event handler that
> +  // the room corresponds to the one which is being created unless we
> +  // keep a track whether createRoomReturned or not.

Have you asked anyone about this? This would seem like a bug in the SDK if it's true.

@@ +276,5 @@
> +  // keep a track whether createRoomReturned or not.
> +  createRoomReturned: true,
> +
> +  // Updates DMRoomToUser, Call whenever DMUserToRooms is changed.
> +  _fillDMRoomToUser: function() {

This seems very inefficient to rebuild the entire DMRoomToUser map every time DMUserToRooms changes.

@@ +284,5 @@
> +        this.DMRoomToUser[roomId] = user;
> +      }
> +    }
> +  },
> +  getDMUserIdForRoomId: function(aRoomId) {

This is unused.

@@ +294,5 @@
> +  // Used when checking for existing rooms while creating a DirectConversation.
> +  getDMRoomIdforUserId: function(aUserId) {
> +    let roomList = []; // list of rooms the user is in.
> +
> +    // Add the rooms in the user's(other guy) list if any.

"Add the other user's rooms to a list."

@@ +309,5 @@
> +      let room = this._client.getRoom(roomId);
> +      if (room) {
> +        let user = room.getMember(aUserId);
> +        if (user) {
> +          roomList.push(roomId);

Didn't we already add a bunch of stuff to roomList? I'm confused at what it's representing.

@@ +323,5 @@
> +      // most recent event in the room's timeline.
> +      var mostRecentRoom = null;
> +      let mostRecentTimeStamp = 0;
> +      for (const roomId of roomList) {
> +        let room = this._client.getRoom(roomId);

Above we already iterated over rooms and got rooms, why are we doing this twice? Don't we already know the room exists?

@@ +326,5 @@
> +      for (const roomId of roomList) {
> +        let room = this._client.getRoom(roomId);
> +        if (room) {
> +          let latestEvent = room.timeline[room.timeline.length - 1];
> +          if (latestEvent) {

Can latestEvent not exist here? Is it possible for room.timeline to be an empty list?

@@ +338,5 @@
> +      }
> +    }
> +
> +    if (mostRecentRoom)
> +      return mostRecentRoom.roomId;

If you move this return inside of the roomList.length if-statement then you don't need to check if mostRecentRoom before returning, it *has* to exist.

@@ +372,5 @@
> +      roomList.push(roomId);
> +    }
> +    dmRoomMap[userId] = roomList;
> +    this._fillDMRoomToUser();
> +    this._client.setAccountData('m.direct', dmRoomMap);

I think I've asked you this before, but is this data shared across multiple clients?

@@ -217,5 @@
>      conv.joining = true;
>      let account = this;
>      this._client.joinRoom(roomIdOrAlias).then(function(room) {
>        conv._roomId = room.roomId;
> -      account._roomList[room.roomId] = conv;

So _roomList is replaced with conversations?

@@ +428,5 @@
> +
> +
> +  // Params : a roomOrUserId
> +  // Returns : a conversation.
> +  getConversation: function(roomOrUserId) {

Please have parameters start with 'a' like all of our methods, e.g. aRoomOrUserId.

@@ +430,5 @@
> +  // Params : a roomOrUserId
> +  // Returns : a conversation.
> +  getConversation: function(roomOrUserId) {
> +    if (roomOrUserId.startsWith("!") || roomOrUserId.startsWith("#")) {
> +      // getConversation is called on roomId.

It'd be nice to have a larger comment at the top saying what valid inputs are and what happens in each case.

@@ +434,5 @@
> +      // getConversation is called on roomId.
> +      let roomId = roomOrUserId;
> +      if (!this.conversations.has(roomId)) {
> +        let convClass = (this.DMRoomToUser[roomId]) ?
> +                         MatrixDirectConversation : MatrixConversation;

I find the DMRoomToUser usage here confusing, if the conversation doesn't exist, how could it be a user ID if we already checked above that it's a room ID?

@@ +448,5 @@
> +    } else {
> +      // when getConversation is called on userId.
> +      let userId = roomOrUserId;
> +      let user = this._client.getUser(userId);
> +      if (!user) return;

Is this an error scenaro? Should we be logging this? Should it happen?

Nit: Put a line break before 'return;'.

@@ +458,5 @@
> +        return this.conversations.get(roomId);
> +      }
> +
> +      let account = this;
> +      let conv = new MatrixDirectConversation(account, user.displayName);

Nit: No reason to save account as a separate variable.

@@ +464,5 @@
> +
> +      // If a DM Room for the user already exists
> +      // in the DM Map from Account Data, then joinRoom
> +      // and return the conversation.
> +      if (roomId && account._client.getRoom(roomId).getMember(userId)) {

How could roomId be false-y here?

@@ +468,5 @@
> +      if (roomId && account._client.getRoom(roomId).getMember(userId)) {
> +        account._client.joinRoom(roomId).then(function(room) {
> +          conv.joining = false;
> +          conv._roomId = room.roomId;
> +          account.conversations.set(room.roomId, conv);

Shouldn't we be setting this *outside* of the callback? What happens if you try to join and immediately try to join again?

@@ +478,5 @@
> +      }
> +
> +      // As there is no room we shall createRoom and
> +      // update the dmRoomMap in accountData.
> +      account.createRoomReturned = false;

This has a global variable createRoomReturned, what happens if you're trying to create multiple room sat once?

@@ +486,5 @@
> +      }).then(function(res) {
> +        account.createRoomReturned = true;
> +        conv.joining = false;
> +        let roomId = res.room_id;
> +        conv._roomId = roomId;

I don't love how we're just modifying a property like this and then depending on it in the object.

Comment 8

[Pavan Karthik [:matrixisreal]]

(In reply to Patrick Cloke [:clokep] from comment #7)
> 
> 
> ::: chat/protocols/matrix/matrix.js
> @@ +77,5 @@
> > +  __proto__: GenericConvIMPrototype,
> > +  sendMsg: function(aMsg) {
> > +    this._account._client.sendTextMessage(this._roomId, aMsg);
> > +  },
> > +  get room() { return this._account._client.getRoom(this._roomId); },
> 
> Where's this._roomId get set?
> 
Hmm.. I should probably add this, 

> @@ +185,5 @@
> > +    // 1. For every Room the account was already in.(After the client is synced).
> > +    // 2. For every new Room the user Joins newly.
> > +    // 3. For every Room the user gets an "invite" request.
> > +    this._client.on("Room", function(room) {
> > +      const me = room.getMember(account.userId);
> 
> Can this fail? What if you're not in the room at all? Can that happen?

I haven't checked if or not the room event is fired for the rooms the user has left. I'll do it.

> 
> @@ +192,5 @@
> > +        // If the 'direct' hint is present, that this is a DM room for
> > +        // whoever invited us.
> > +        if (me.events.member.getContent().is_direct) {
> > +          let roomMembers = room.getJoinedMembers();
> > +          if (roomMembers.length == 1) {
> 
> What if is_direct is set, but there's more than one member? It looks like in
> that case we won't even join the room?
Hmm.. Yeah, I missed the case. then we should probably treat the room as a group chat. 

> 
> @@ +259,5 @@
> >      this._client.startClient();
> >    },
> > +
> > +  // A Map from userIds to list of roomIds, which is just the object dmRoomMap from accountData
> > +  // which keeps track of DM rooms.
> 
> What is this used for?

To clear things up abt dmMap, 
AccountData is Matrix SDK object, which has a dmRoomMap(userIds -> [roomList]). This object is modified by clients and is accessible to "all" the clients.

Our account.DMUserToRoom is just a reference to the dmRoomMap object.

We need this map to differentiate the DM rooms from non DMs.

I added account.DMRoomToUser "just" to check if a rooms is DM or not[if DMRoomToUser returns null then it is a non DM Room].
I did this  as I thought(initially) that it was a better design and might be used in future but it turned out to be confusing.
We should probably just replace this with a function checking if the roomId is in DMUserToRoom.

> 
> @@ +266,5 @@
> > +  // A Map from roomIds to userIds
> > +  // Used to check if a room is DM or not.
> > +  // NOTE : Not particularly sure if this is needed,
> > +  // we might just have just DMUserToRooms map and have functions to
> > +  // check if the room is in the map. But I felt this was more comfortable.
> 
> I don't understand why this would be necessary, you have the map of rooms,
> can't you just check if those are a DM?
> 


> @@ +272,5 @@
> > +
> > +  // when createRoom is called, 'sometimes' the "Room" event is fired
> > +  // before it returns a promise. So we won't know at the event handler that
> > +  // the room corresponds to the one which is being created unless we
> > +  // keep a track whether createRoomReturned or not.
> 
> Have you asked anyone about this? This would seem like a bug in the SDK if
> it's true.
> 
Yes it is a bug. I have talked to Dave(who wrote this DM stuff) and few other matrix devs on matrix and confirmed abt this. But they could not find a way out apart from this :/ . 


> @@ +294,5 @@
> > +  // Used when checking for existing rooms while creating a DirectConversation.
> > +  getDMRoomIdforUserId: function(aUserId) {
> > +    let roomList = []; // list of rooms the user is in.
> > +
> > +    // Add the rooms in the user's(other guy) list if any.
> 
> "Add the other user's rooms to a list."
>
 
> @@ +309,5 @@
> > +      let room = this._client.getRoom(roomId);
> > +      if (room) {
> > +        let user = room.getMember(aUserId);
> > +        if (user) {
> > +          roomList.push(roomId);
> 
> Didn't we already add a bunch of stuff to roomList? I'm confused at what
> it's representing.
> 
I guess my comments were a bit unclear.

As discussed before dmRoomMap in AccountData is "only" modified by clients. And this stuff isn't documented and there are "no" strict rules on how to map the users to rooms.
Let A,B be to users. and X be the client.
The function returns a roomId of a DMroom between A and B, if given userID of B.
So, If 1) A is on a client X, and opens a DMRoom with B, then the clients might add the items, (A -> [room]) or (B -> [room]) to the dmRoomMap.

And now if 2)  B invites A, then again the same two possibilites adding (A -> [room]) or (B -> [room]) to the dmRoomMap.
So to the roomList we need to add two types of rooms(not just B -> [room]),
The ones in B -> [room], (the bunch of stuff which I already added, (referecing ur comment))
The ones in A -> [room], Here we need to check for each and every room whether that B is a member of that room, and add the room to roomList if yes.
Now from the roomList we need to select the mostRecentRoom.

I hope its clear why we are adding new rooms to RoomList again while iterating  

Just for the record : riot adds B -> [roomId] in case 1) and A -> [roomId] in case 2.

> @@ +323,5 @@
> > +      // most recent event in the room's timeline.
> > +      var mostRecentRoom = null;
> > +      let mostRecentTimeStamp = 0;
> > +      for (const roomId of roomList) {
> > +        let room = this._client.getRoom(roomId);
> 
> Above we already iterated over rooms and got rooms, why are we doing this
> twice? Don't we already know the room exists?
> 

Point. yes we need not check it again.
> @@ +326,5 @@
> > +      for (const roomId of roomList) {
> > +        let room = this._client.getRoom(roomId);
> > +        if (room) {
> > +          let latestEvent = room.timeline[room.timeline.length - 1];
> > +          if (latestEvent) {
> 
> Can latestEvent not exist here? Is it possible for room.timeline to be an
> empty list?
> 

Yeah. when the user gets an invite(not sure if it is only for newly create rooms), and is still unaccepted, the timeline is null.

> @@ +338,5 @@
> > +      }
> > +    }
> > +
> > +    if (mostRecentRoom)
> > +      return mostRecentRoom.roomId;
> 
> If you move this return inside of the roomList.length if-statement then you
> don't need to check if mostRecentRoom before returning, it *has* to exist.
> 

yeah, better. :)

> @@ +372,5 @@
> > +      roomList.push(roomId);
> > +    }
> > +    dmRoomMap[userId] = roomList;
> > +    this._fillDMRoomToUser();
> > +    this._client.setAccountData('m.direct', dmRoomMap);
> 
> I think I've asked you this before, but is this data shared across multiple
> clients?
> 


> @@ -217,5 @@
> >      conv.joining = true;
> >      let account = this;
> >      this._client.joinRoom(roomIdOrAlias).then(function(room) {
> >        conv._roomId = room.roomId;
> > -      account._roomList[room.roomId] = conv;
> 
> So _roomList is replaced with conversations?
>

Yeah, Just to be constitent with how things are done with other protocols.
 
> @@ +428,5 @@
> > +
> > +
> > +  // Params : a roomOrUserId
> > +  // Returns : a conversation.
> > +  getConversation: function(roomOrUserId) {
> 
> Please have parameters start with 'a' like all of our methods, e.g.
> aRoomOrUserId.

noted.

> 
> @@ +430,5 @@
> > +  // Params : a roomOrUserId
> > +  // Returns : a conversation.
> > +  getConversation: function(roomOrUserId) {
> > +    if (roomOrUserId.startsWith("!") || roomOrUserId.startsWith("#")) {
> > +      // getConversation is called on roomId.
> 
> It'd be nice to have a larger comment at the top saying what valid inputs
> are and what happens in each case.
> 

Sure.

> @@ +434,5 @@
> > +      // getConversation is called on roomId.
> > +      let roomId = roomOrUserId;
> > +      if (!this.conversations.has(roomId)) {
> > +        let convClass = (this.DMRoomToUser[roomId]) ?
> > +                         MatrixDirectConversation : MatrixConversation;
> 
> I find the DMRoomToUser usage here confusing, if the conversation doesn't
> exist, how could it be a user ID if we already checked above that it's a
> room ID?
>

I didn't undersatand the question but I as we should probably remove DMRoomToUser, this question must be redundant ?
 
> @@ +448,5 @@
> > +    } else {
> > +      // when getConversation is called on userId.
> > +      let userId = roomOrUserId;
> > +      let user = this._client.getUser(userId);
> > +      if (!user) return;
> 
> Is this an error scenaro? Should we be logging this? Should it happen?
> 
I think so.

> Nit: Put a line break before 'return;'.
> 
> @@ +458,5 @@
> > +        return this.conversations.get(roomId);
> > +      }
> > +
> > +      let account = this;
> > +      let conv = new MatrixDirectConversation(account, user.displayName);
> 
> Nit: No reason to save account as a separate variable.
> 

> @@ +464,5 @@
> > +
> > +      // If a DM Room for the user already exists
> > +      // in the DM Map from Account Data, then joinRoom
> > +      // and return the conversation.
> > +      if (roomId && account._client.getRoom(roomId).getMember(userId)) {
> 
> How could roomId be false-y here?
> 
But roomId could be a null, right?
      roomId is  account.getDMRoomIdforUserId(userId);
and getDMRoomIdforUserId returns null if no DMroom exists. 


> @@ +468,5 @@
> > +      if (roomId && account._client.getRoom(roomId).getMember(userId)) {
> > +        account._client.joinRoom(roomId).then(function(room) {
> > +          conv.joining = false;
> > +          conv._roomId = room.roomId;
> > +          account.conversations.set(room.roomId, conv);
> 
> Shouldn't we be setting this *outside* of the callback? 
> What happens if you
> try to join and immediately try to join again?

Doing it outside the callback is a safer option. Yeah


> 
> @@ +478,5 @@
> > +      }
> > +
> > +      // As there is no room we shall createRoom and
> > +      // update the dmRoomMap in accountData.
> > +      account.createRoomReturned = false;
> 
> This has a global variable createRoomReturned, what happens if you're trying
> to create multiple room sat once?
>

At this moment, I dont have a solution for this, Do you have anything in mind?

 
> @@ +486,5 @@
> > +      }).then(function(res) {
> > +        account.createRoomReturned = true;
> > +        conv.joining = false;
> > +        let roomId = res.room_id;
> > +        conv._roomId = roomId;
> 
> I don't love how we're just modifying a property like this and then
> depending on it in the object.

I'll try to change this, any suggestions?

Comment 9

[Pavan Karthik [:matrixisreal]]

Attachment: V2.patch

Hi,
It has been a while, Sorry for merging a lot of things in a single patch.
This is still in a draft change, but most of the things are pretty much done.
The major changes and additions are:
1. Some changes in DM after feedback from clokep. But still there are some unsolved problems like if the way I am handling the createRoom reace condition is a good way or not(or if it breaks in any case.).
2. Handling messages for different events: Handled most of the important messages/system messages,
This turned out to be longer that I expected. I have mostly tried to use all the riot strings for all of the events. Shall we add this handling messages part to a new file? 
3. showing typing notifications.
4. Commands. I have added all of the riot supported commands.

With this most of the important things in Part 1,2,3 mentioned in my schedule are completed(ish! needs review, testing and maybe some refactoring). But things like presence and making a UI for handling Invites are still untouched :/
I'll try to find time to finish these off while I start working on Part4 (send Receipts).

Comment 10

[Frédéric Wang (:fredw)]

Comment on attachment 8886877 [details] [diff] [review]
V2.patch

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

> It has been a while, Sorry for merging a lot of things in a single patch.

Indeed, the patch is big and hard to review. Maybe you could split it into smaller pieces? (and open corresponding dependent bugs)

::: chat/chat-prefs.js
@@ +86,5 @@
>  pref("chat.prpls.prpl-skype.disable", true);
>  // Disable Facebook as the XMPP gateway no longer exists.
>  pref("chat.prpls.prpl-facebook.disable", true);
>  // Disable experimental Matrix support.
> +pref("chat.prpls.prpl-matrix.disable", false);

This should not be modified.

::: chat/locales/en-US/matrix.properties
@@ +18,5 @@
> +
> +# LOCALIZATION NOTE (message.*):
> +#    These are shown as system messages in the conversation.
> +#    %S is the reason string for the particular action.
> +#    Used within context of ban, kick and withdrew invite.

withdraw?

@@ +19,5 @@
> +# LOCALIZATION NOTE (message.*):
> +#    These are shown as system messages in the conversation.
> +#    %S is the reason string for the particular action.
> +#    Used within context of ban, kick and withdrew invite.
> +message.reason= Reason: %S.

extra space after the equal sign

@@ +80,5 @@
> +
> +# LOCALIZATION NOTE (error.*):
> +#    These are the error strings shown as system messages.
> +#    %S is the room Alias which the user has inputted.
> +error.unrecognisedRoomAlias=Unrecognised room alias: %S

I think it's Unrecognised with US spelling.

@@ +82,5 @@
> +#    These are the error strings shown as system messages.
> +#    %S is the room Alias which the user has inputted.
> +error.unrecognisedRoomAlias=Unrecognised room alias: %S
> +
> +# LOCALZIATION NOTE (command.*):

s/LOCALZIATION/LOCALIZATION/

::: chat/protocols/matrix/matrix.js
@@ +7,5 @@
>  Cu.import("resource:///modules/imXPCOMUtils.jsm");
>  Cu.import("resource:///modules/jsProtoHelper.jsm");
> +Cu.import("resource:///modules/NormalizedMap.jsm");
> +Cu.import("resource:///modules/imServices.jsm");
> +Cu.import("resource://gre/modules/Console.jsm");

Is Console.jsm necessary?

@@ +379,2 @@
>          }
> +        console.log(event);

I think this should not be in the final patch (then you can remove the Cu.import)

@@ +598,4 @@
>    disconnect: function() {
>      if (this._client)
>        this._client.stopClient();
>    },

unnecessary whitespace change

Comment 11

[Patrick Cloke [:clokep]]

Comment on attachment 8886877 [details] [diff] [review]
V2.patch

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

Clearing review. The Matrix SDK needs to be updated and re-integrated before we do more work on it.

Comment 12

[Mayeul C]

Note that a rework is pending to implement canonical DMs.

I haven't read the MSC in detail, but I am pretty sure that multiple DMs per-user will still be supported.

Comment 13

[Khushil Mistry [:khushil324]]

Attachment: Bug-1614025_implement-direct-messages-0.patch

Comment 14

[Khushil Mistry [:khushil324]]

Attachment: Bug-1614025_implement-direct-messages-1.patch

Comment 15

[Khushil Mistry [:khushil324]]

Attachment: Bug-1614025_implement-direct-messages-2.patch

Comment 16

[Khushil Mistry [:khushil324]]

Attachment: Bug-1348064_MatrixParticipant-and-MatrixConversation-0.patch

Comment 17

[Khushil Mistry [:khushil324]]

Attachment: Bug-1348064_implement-direct-messages-MatrixDirectConversation-0.patch

Two different patches as you have asked.

Comment 18

[Patrick Cloke [:clokep]]

Comment on attachment 9145624 [details] [diff] [review]
Bug-1348064_MatrixParticipant-and-MatrixConversation-0.patch

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

::: chat/protocols/matrix/matrix.jsm
@@ +15,5 @@
>    GenericAccountPrototype,
>    GenericConvChatPrototype,
>    GenericConvChatBuddyPrototype,
>    GenericProtocolPrototype,
> +  GenericConversationPrototype,

This is going to be unused and should be removed from this patch.

@@ +59,5 @@
> +  },
> +
> +  get buddyIconFilename() {
> +    let avatarUrl = this._roomMember.getAvatarUrl(this._account._baseURL);
> +    if (this._roomMember.user && this._roomMember.user.avatarUrl && avatarUrl) {

I think this logic is a little funky, it would be clearer as:

if (this._roomMember.user && this._roomMember.user.avatarUrl) {
    return this._roomMember.getAvatarUrl(this._account._baseURL);
}
return "";

@@ +227,5 @@
>   *  setPresence
>   */
>  function MatrixAccount(aProtocol, aImAccount) {
>    this._init(aProtocol, aImAccount);
> +  this._baseURL = this.getString("server");

We should use this below in `connect` (and anywhere else we call `getString("server")`).

Comment 19

[Khushil Mistry [:khushil324]]

Attachment: Bug-1348064_MatrixParticipant-and-MatrixConversation-1.patch

Comment 20

[Patrick Cloke [:clokep]]

Comment on attachment 9146575 [details] [diff] [review]
Bug-1348064_MatrixParticipant-and-MatrixConversation-1.patch

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

::: chat/protocols/matrix/matrix.jsm
@@ +58,5 @@
> +  },
> +
> +  get buddyIconFilename() {
> +    if (this._roomMember.user && this._roomMember.user.avatarUrl) {
> +      return this._roomMember.getAvatarUrl(this._account._baseURL) || "";

Why the additional `|| ""` here? Do we expect `getAvatarUrl` to return false?

@@ +243,5 @@
>    connect() {
>      this.reportConnecting();
>      let dbName = "chat:matrix:" + this.imAccount.id;
>      let baseURL = this.getString("server") + ":" + this.getInt("port");
> +    this._baseURL = this.getString("server");

I think I wasn't clear with my previous comment. Setting `this._baseURL` in the constructor makes sense, but then we should USE It everywhere else.

Additionally, do we need the port on the baseURL or is just the server name enough? Why do these two values differ?

Comment 21

[Khushil Mistry [:khushil324]]

(In reply to Patrick Cloke [:clokep] from comment #20)

Why the additional || "" here? Do we expect getAvatarUrl to return false?

getAvatarUrl can return null. And if we return null, it will not show anything. But we want to show the default user icon.

I think I wasn't clear with my previous comment. Setting this._baseURL in
the constructor makes sense, but then we should USE It everywhere else.

Additionally, do we need the port on the baseURL or is just the server name
enough? Why do these two values differ?

BaseURL is used only in createClient and getAvatarUrl. createClient needs a baseURL with the correct port. We can remove the let baseURL and add port number in this._baseURL.

Comment 22

[Patrick Cloke [:clokep]]

(In reply to Khushil Mistry [:khushil324] from comment #21)

(In reply to Patrick Cloke [:clokep] from comment #20)

Why the additional || "" here? Do we expect getAvatarUrl to return false?

getAvatarUrl can return null. And if we return null, it will not show anything. But we want to show the default user icon.

Got it, I didn't realize that could ever return null.

I think I wasn't clear with my previous comment. Setting this._baseURL in
the constructor makes sense, but then we should USE It everywhere else.

Additionally, do we need the port on the baseURL or is just the server name
enough? Why do these two values differ?

BaseURL is used only in createClient and getAvatarUrl. createClient needs a baseURL with the correct port. We can remove the let baseURL and add port number in this._baseURL.

Let's add the port to this._baseURL. I think we want to use the same one everywhere.

Comment 23

[Khushil Mistry [:khushil324]]

I have a question related to GenericConversationPrototype.
In this patch, I am adding functionality that if you close any conversation, we will also leave that room.

close() {
    this._account._client.leave(this._roomId);
    this._account._roomList.delete(this._roomId);
    GenericConvChatPrototype.close.call(this);
 },

But when we remove the account, we just need to remove all the conversations. At that time, we don't want to leave the group/direct messaging. At that time, we need GenericConversationPrototype. So I think GenericConversationPrototype is needed.

Comment 24

[Khushil Mistry [:khushil324]]

Attachment: Bug-1348064_MatrixParticipant-and-MatrixConversation-2.patch

Comment 25

[Khushil Mistry [:khushil324]]

Attachment: Bug-1348064_implement-direct-messages-MatrixDirectConversation-1.patch

Comment 26

[Patrick Cloke [:clokep]]

Comment on attachment 9146604 [details] [diff] [review]
Bug-1348064_MatrixParticipant-and-MatrixConversation-2.patch

Looks good!

Comment 27

[Khushil Mistry [:khushil324]]

Checkin-needed-tb for Bug-1348064_MatrixParticipant-and-MatrixConversation-2.patch

Comment 28

[Patrick Cloke [:clokep]]

Comment on attachment 9146798 [details] [diff] [review]
Bug-1348064_implement-direct-messages-MatrixDirectConversation-1.patch

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

I'm having a lot of trouble following this patch. It seems to do many things all at once and it is unclear if they're all inter-related or only "kind of" related.

A couple of things I find confusing:
* We seem to be tracking the state of whether a conversation is a chat or an IM in multiple places. Each conversation should already know what it is, I don't think we need account level methods to check that.
* This patch might be a lot simpler if we left out things like typing and dealt with that in the future.
* It looks like some variables/properties are just renamed. If we did this as a separate patch it would make the changes in here much easier to read.

::: chat/protocols/matrix/matrix.jsm
@@ +88,5 @@
> +}
> +MatrixDirectConversation.prototype = {
> +  __proto__: GenericConvIMPrototype,
> +  _typingTimer: null,
> +  supportChatStateNotifications: true,

This flag looks unnecessary for Matrix? I think it might have been copied from XMPP, but not necessary here.

@@ +97,5 @@
> +   */
> +  close() {
> +    this._account._client.leave(this._roomId);
> +    this._account.roomList.delete(this._roomId);
> +    GenericConversationPrototype.close.call(this);

This should be `GenericConvIMPrototype`.

@@ +130,5 @@
> +
> +    this._setTypingState("paused");
> +  },
> +
> +  _setTypingState(newState) {

Unlike the XMPP implementation it looks like the state here is just a boolean, I think this would be clear as "_setTyping(isTyping)".

Additionally it would be great if a bunch of these methods had comment strings!

@@ +178,5 @@
> +   * Initialize the room after the response from the Matrix client.
> +   *
> +   * @param {Object} room - associated room with the conversation.
> +   */
> +  initRoom(room) {

It would be nice to have any shared implementation between the two conversations in a base-class. We do this for IRC, although it is a little messy. Check GenericIRCConversation.

@@ +352,5 @@
>    __proto__: GenericAccountPrototype,
>    observe(aSubject, aTopic, aData) {},
>    remove() {
> +    for (let room of this.roomList.values()) {
> +      GenericConversationPrototype.close.call(room);

Shouldn't this call `room.close()` directly?

@@ +436,5 @@
> +        } else if (
> +          member.membership === "join" ||
> +          member.membership === "leave"
> +        ) {
> +          this.checkRoomForUpdate(conv);

Should this only happen if the conv is currently a direct conversation? Right now this will also run if `conv.isChat` is true.

@@ +446,5 @@
> +     * Get the map of direct messaging rooms.
> +     */
> +    this._client.on("accountData", event => {
> +      if (event.getType() == "m.direct") {
> +        this.DMUserToRooms = event.getContent();

This variable name doesn't really follow the format we usually use, maybe _userToRoom would be clearer? I believe you need to set a default value for this somewhere on the account so that if this event never occurs the code won't break.

@@ +519,5 @@
>  
> +    this._client.on("RoomMember.typing", (event, member) => {
> +      if (this.isDMRoom(member.roomId) && member.userId != this.userId) {
> +        let conv = this.roomList.get(member.roomId);
> +        if (!conv.isChat) {

We seem to check this twice here, which confuses me. Why do we have to check isDMRoom + conv.isChat, shouldn't those always be in sync?

@@ +554,5 @@
> +          this.getGroupConversation(room.roomId);
> +        }
> +      } else if (me && me.membership == "join") {
> +        // To avoid the race condition. Whenever we will create the room,
> +        // this will also be fired. So we want to avoid makingo of multiple

There seems to be a typo in this comment.

@@ +556,5 @@
> +      } else if (me && me.membership == "join") {
> +        // To avoid the race condition. Whenever we will create the room,
> +        // this will also be fired. So we want to avoid makingo of multiple
> +        // conversations with the same room.
> +        if (!this.createRoomReturned || this.roomList.has(room.roomId)) {

I'm a bit confused at why this flag is necessary. I would think knowing it is in the room list is enough?

@@ +657,5 @@
> +    if (this.roomList.has(roomId)) {
> +      return this.roomList.get(roomId);
> +    }
> +
> +    let account = this;

Why can't we just use `this` throughout? It should work with the arrow functions.

@@ +825,2 @@
>    updateRoomMember(event, member) {
> +    if (this.roomList && this.roomList.has(member.roomId)) {

Why would roomList not exist? We should initialize it in the constructor.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/caeedaa95357
Implement Direct Messages: MatrixParticipant and MatrixConversation. r=clokep

Comment 30

[Khushil Mistry [:khushil324]]

(In reply to Patrick Cloke [:clokep] from comment #28)

I'm having a lot of trouble following this patch. It seems to do many things
all at once and it is unclear if they're all inter-related or only "kind of"
related.

A couple of things I find confusing:

• We seem to be tracking the state of whether a conversation is a chat or an
  IM in multiple places. Each conversation should already know what it is, I
  don't think we need account level methods to check that.
• This patch might be a lot simpler if we left out things like typing and
  dealt with that in the future.
• It looks like some variables/properties are just renamed. If we did this
  as a separate patch it would make the changes in here much easier to read.

I will split typing part in the another patch to make it more readable.

This should be GenericConvIMPrototype.

Yes, but now we will have a common class sharing common methods. So this method will go there. There, it should be GenericConvIMPrototype.

Additionally it would be great if a bunch of these methods had comment
strings!

Sure, I will add the comments.

It would be nice to have any shared implementation between the two
conversations in a base-class. We do this for IRC, although it is a little
messy. Check GenericIRCConversation.

Agree.

Shouldn't this call room.close() directly?

This will be called when you are deleting the account. So you just want to close the conversation, don't want to leave the room. close function has a functionality that we added: you will also leave the room if you click on close button.

Should this only happen if the conv is currently a direct conversation?
Right now this will also run if conv.isChat is true.

No, it should happen for both types of conversation. Suppose if some conversation is direct conversation according to the matrix and it has 3 participants. So it will be the group conversation for us. If one leaves, then that conversation should be the direct conversation for us also so we want to update it from group conversation to direct conversations.

This variable name doesn't really follow the format we usually use, maybe
_userToRoom would be clearer? I believe you need to set a default value for
this somewhere on the account so that if this event never occurs the code
won't break.

Whenever we connect to the server, "accountdata" will get fired at the part of the startClient.

We seem to check this twice here, which confuses me. Why do we have to check
isDMRoom + conv.isChat, shouldn't those always be in sync?

Yes, here we can remove isDMRoom(). But we are using this.isDMRoom() at three other places elsewhere we are using conv.isChat. isDMRoom and conv.isChat will not be the same in the following cases: If we have a matrix direct conversation of 3 people. It will be a group conversation for us. But if one person leaves, we should make it a direct conversation. At this time, isDMRoom() will be true as there are 2 people only in the room but the conversation would be group conversation. isDMRoom() is used at places where we want to convert the conversation from group to direct or vise versa.

There seems to be a typo in this comment.

Yes.

I'm a bit confused at why this flag is necessary. I would think knowing it
is in the room list is enough?

When you create a new room, this will also be fired: this._client.on("Room"). But we are creating a room in the joinChat Function.

Why can't we just use this throughout? It should work with the arrow
functions.

Agree.

Why would roomList not exist? We should initialize it in the constructor.

roomList will be new Map() at the starting. We are initializing it. But if it has not any element, it will be null and this.roomList.has is showing an error. This will happen when "RoomMember.name" and "RoomMember.powerLevel" is being fired before "Room". At those times, we will be updating the participants in init function of the conversation.

Comment 31

[Khushil Mistry [:khushil324]]

Attachment: Bug-1348064_implement-direct-messages-MatrixDirectConversation-2.patch

Comment 32

[Khushil Mistry [:khushil324]]

Attachment: Bug-1348064_Implement-Direct-Messages-typing-0.patch

Comment 33

[Patrick Cloke [:clokep]]

Unfortunately I'm still having a lot of trouble looking at this, there's a lot of pieces and I think many of them deserve comments. I'd propose doing the following:

1. There's a few pieces of this patch that rename parameters from aFoo to foo. Let's just do this in a separate bug and do the entire file at once. This should make subsequent diffs much easier.
2. It seems there's a separate bug that this is fixing with joining rooms on federated servers, let's put that in a separate bug.
3. There are some changes around _roomList -> roomList (and defining it in the init and such). It is unclear to me why this is necessary, but it feels like a separate patch.
4. The pieces to create the MatrixDirectConversation object (and using the GenericMatrixConversation and such) can be a separate patch, even though the object is unused that's fine (note that code that does this looks good in the current patch, minus that it seems like the room getter should be moved to GenericMatrixConversation).
5. The remaining bits at this point are creating the MatrixDirectConversation and the upgrade/downgrade between conversation types. I'm not sure if this is one or two more patches, but hopefully the above removes a lot of the surrounding diff and makes it clearer what is happening.

I'd also recommend that we move typing notifications to a completely separate bug after all this is done. I think the current patch looks good, but it seems like we should be abstracting some of the code with the other protocols.

Comment 34

[Khushil Mistry [:khushil324]]

Attachment: Bug-1348064_roomlist-changes-0.patch

Part 1

Comment 35

[Khushil Mistry [:khushil324]]

Attachment: Bug-1348064_GenericMatrixConversation-MatrixDirectConversation-0.patch

Part 2.

get room() {
  return this._account._client.getRoom(this._roomId);
},

Facing some issues while moving above getter to GenericMatrixConversation. Showing an error that this._account is null. This error comes before initialization of any function in the file. So it can be the problem that we can not have such getters in the Javascript variable.

Comment 36

[Khushil Mistry [:khushil324]]

Attachment: Bug-1348064_direct-messaging-0.patch

Part 3.
Joining the Chat and Typing part will be uploaded in the new bugs.

Comment 37

[Khushil Mistry [:khushil324]]

Attachment: Bug-1348064_roomlist-changes-1.patch

Comment 38

[Patrick Cloke [:clokep]]

Comment on attachment 9149943 [details] [diff] [review]
Bug-1348064_roomlist-changes-1.patch

Looks good! Thanks for making those changes.

For posterity's sake -- we had a conversation over Matrix about this patch an why the check in `updateRoomMember` was necessary (for `this.roomList` -- which should always exist!) and we realized there were some missing `bind(this)` calls. The other changes I asked for were all cosmetic in nature.

Comment 39

[Khushil Mistry [:khushil324]]

Checkin needed for the following patches in order:

Bug-1348064_roomlist-changes-1.patch
Bug-1348064_GenericMatrixConversation-MatrixDirectConversation-0.patch

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/57cd946f5c02
update the usage of the _roomlist in the Matrix Chat. r=clokep
https://hg.mozilla.org/comm-central/rev/35574aea0982
MatrixDirectConversation and GenericMatrixConversation added in the Matrix chat. r=clokep

Comment 41

[Patrick Cloke [:clokep]]

Comment on attachment 9149761 [details] [diff] [review]
Bug-1348064_direct-messaging-0.patch

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

This is looking a lot better than last time I looked at it, but there's still a bit of work to do. I don't think there's anything overall wrong with the approach, but there's some places we could be more consistent.

::: chat/protocols/matrix/matrix.jsm
@@ +367,5 @@
> +     * Get the map of direct messaging rooms.
> +     */
> +    this._client.on("accountData", event => {
> +      if (event.getType() == "m.direct") {
> +        this._userToRoom = event.getContent();

We should probably define `_userToRoom` on initialization to an empty object or something.

@@ +444,5 @@
>  
> +    /*
> +     * We auto join all the rooms in which we are invited. This will also be
> +     * fired for all the rooms we have joined earlier when SDK gets connected.
> +     * We will use that part to to make conversations, direct or group.

I'm surprised that this isn't checking _userToRoom at all? Why don't we need to see that? Is the is_direct property the same thing?

@@ +468,5 @@
> +      } else if (me && me.membership == "join") {
> +        // To avoid the race condition. Whenever we will create the room,
> +        // this will also be fired. So we want to avoid making of multiple
> +        // conversations with the same room.
> +        if (!this.createRoomReturned || this.roomList.has(room.roomId)) {

The roomList should immediately have the room with the flag set to joining. Once we join we should be setting the joined flag.

@@ +473,3 @@
>            return;
>          }
> +        if (this.isDMRoom(room.roomId)) {

I prefer longer names like isDirectRoom (same goes for setDMRoom -> setDirectRoom).

@@ +473,5 @@
>            return;
>          }
> +        if (this.isDMRoom(room.roomId)) {
> +          let interlocutorId;
> +          for (let roomMember of room.getJoinedMembers()) {

Why can't we just grab the first room member like we do above?

@@ +497,5 @@
> +   * @return {Boolean} - If room is direct direct messaging room or not.
> +   */
> +  isDMRoom(checkRoomId) {
> +    for (let user of Object.keys(this._userToRoom)) {
> +      for (let roomId of this._userToRoom[user]) {

It might make sense to transform _userToRoom into a Set object, which contains the IDs of the DM rooms. It would avoid this double loop here. I know you need a Map of user -> conv ID too, but it is probably worth storing both.

@@ +516,5 @@
> +   * @param {Object} groupConv - the group conversation which needs to be
> +   *                             converted.
> +   */
> +  convertToDM(groupConv) {
> +    GenericConversationPrototype.close.call(groupConv);

Please add a comment about why we're using the generic call here.

@@ +582,5 @@
> +        })
> +        .catch(error => {
> +          this.ERROR(error);
> +          conv.joining = false;
> +          conv.close();

If we close the conversation here then the user will just see it disappear without any info. I think we usually set the conversation to left and add some info for the user to see.

@@ +590,5 @@
> +      return conv;
> +    }
> +
> +    if (
> +      this.createRoomReturned &&

Having this global state isn't good, it means we can only be joining a single room at once. I said it above, but I think you can replace it with the joining flag.

@@ +597,5 @@
> +      this.createRoomReturned = false;
> +      let conv = new MatrixConversation(this, roomId, this.userId);
> +      conv.joining = true;
> +      let indexOf = roomId.indexOf(":");
> +      let name = roomId.slice(1, indexOf);

I think you can do this all in one go: roomId.split(':', 1)[0]

@@ +600,5 @@
> +      let indexOf = roomId.indexOf(":");
> +      let name = roomId.slice(1, indexOf);
> +      this._client
> +        .createRoom({
> +          room_alias_name: name,

Must all these properties be provided? Is there not sane defaults? How did you decide on them?

@@ +615,5 @@
> +          this.createRoomReturned = true;
> +          let newRoomId = res.room_id;
> +          let room = this._client.getRoom(newRoomId);
> +          conv.initRoom(room);
> +          this.roomList.set(newRoomId, conv);

We should be adding this to the roomList right after we make the conversation.

@@ +643,5 @@
> +   *
> +   * @return {String} - ID of the room.
> +   */
> +  getDMRoomIdForUserId(userId) {
> +    let roomList = [];

Instead of keeping track of a list and then calculating things after the fact I'd propose that we track two variables: our potential room ID to return and the timestamp of the most recent event for that room. For each location where we push onto roomList we can compare the timestamps and either: leave room ID as is, or replace it with the updated one.

Then after we check the two sets of room lists we either return what we have or null (which can be the default value for the roomId).

@@ +645,5 @@
> +   */
> +  getDMRoomIdForUserId(userId) {
> +    let roomList = [];
> +
> +    // Check in the 'other' user's roomList and add to our list.

I'm not 100% sure what this is doing, but I think it is trying to find the DM conversation that both you and the other user are still in?

@@ +646,5 @@
> +  getDMRoomIdForUserId(userId) {
> +    let roomList = [];
> +
> +    // Check in the 'other' user's roomList and add to our list.
> +    if (this._userToRoom[userId]) {

Will this cause any issues if userId is __proto__ or something weird? We should be consistent with below here: this._userToRoom[userId] || []

@@ +696,5 @@
> +    return null;
> +  },
> +
> +  /*
> +   * Sets the room ID for for corresponsing user ID for direct messaging.

This comment needs to say something about propagating back to the server.

@@ +710,5 @@
> +      dmRoomMap = mDirectEvent.getContent();
> +    }
> +
> +    // remove it from the lists of any others users
> +    // (it can only be a DM room for one person)

Please use full sentences with punctuation (capitalize the first character and put a period at the end). There's a few other places that this should happen too.

@@ +724,5 @@
> +    }
> +
> +    // now add it, if it's not already there.
> +    let roomList = dmRoomMap[userId] || [];
> +    if (!roomList.includes(roomId)) {

If the room list is here already we should:
1. Check that before iterating all the other entries in this map.
2. Not call setAccountData since we shouldn't be modifying anything in this case?

@@ +811,5 @@
>        });
>      return conv;
>    },
> +
> +  createConversation(userId, roomId) {

This is part of the interface, I don't know what will happen by just adding additional parameters here like this. Check the users of it: https://searchfox.org/comm-central/search?q=createConversation&redirect=false

@@ +815,5 @@
> +  createConversation(userId, roomId) {
> +    if (userId == this.userId) {
> +      return null;
> +    }
> +    return this.getDirectConversation(userId, roomId);

If getDirectConversation is only called in one place can we just inline it?

@@ +832,5 @@
> +   * @param {String} roomId - ID of the room if we know the exact room.
> +   *
> +   * @return {Object} - The resulted conversation.
> +   */
> +  getDirectConversation(userId, roomId) {

An awful lot of the logic here is duplicated from getGroupConversation. It might be nice to combine them if possible, there could be a getConversation method which takes the properties as a parameter or something maybe. I'm not 100% sure what differs between them.

Comment 42

[Khushil Mistry [:khushil324]]

(In reply to Patrick Cloke [:clokep] from comment #41)

We should probably define _userToRoom on initialization to an empty object
or something.

Agree.

I'm surprised that this isn't checking _userToRoom at all? Why don't we need
to see that? Is the is_direct property the same thing?

Here, we are identifying with: me.events.member.getContent().is_direct. When one gets invited, this flag will be there to identify if the user is invited to the direct room or not.

The roomList should immediately have the room with the flag set to joining.
Once we join we should be setting the joined flag.

Here, the problem is when we create a new room, we will not have the room id. So we can not put the conversation in the roomList Map. Once the room is created, then only we will get the roomID but "Room" listener will be fired simultaneously. So it's hard to synchronize between both.

I prefer longer names like isDirectRoom (same goes for setDMRoom ->
setDirectRoom).

Done.

Why can't we just grab the first room member like we do above?

We can be first or second in the list. It's not defined.

It might make sense to transform _userToRoom into a Set object, which
contains the IDs of the DM rooms. It would avoid this double loop here. I
know you need a Map of user -> conv ID too, but it is probably worth storing
both.

Please add a comment about why we're using the generic call here.

Done.

If we close the conversation here then the user will just see it disappear
without any info. I think we usually set the conversation to left and add
some info for the user to see.

If we left the conversation, the user will still think the conversation is working and they will carry on with their work which will cause a lot of problems to Matrix SDK. We need to have a sperate error-showing mechanism in the Chat.

Having this global state isn't good, it means we can only be joining a
single room at once. I said it above, but I think you can replace it with
the joining flag.

The problem mentioned in the above comment.

I think you can do this all in one go: roomId.split(':', 1)[0]

Done.

Must all these properties be provided? Is there not sane defaults? How did
you decide on them?

I have tried to copy them from the Riot Chat Repository.

We should be adding this to the roomList right after we make the
conversation.

Then if it fails, we need to remove it from the roomlist. So it's better to add them when it's in the right form. Also, when you create a room, we will not have a roomID to insert.

Instead of keeping track of a list and then calculating things after the
fact I'd propose that we track two variables: our potential room ID to
return and the timestamp of the most recent event for that room. For each
location where we push onto roomList we can compare the timestamps and
either: leave room ID as is, or replace it with the updated one.

Then after we check the two sets of room lists we either return what we have
or null (which can be the default value for the roomId).

Yes, I have changed getDMRoomIdForUserId and setDirectRoom.

Will this cause any issues if userId is proto or something weird? We
should be consistent with below here: this._userToRoom[userId] || []

userId will be a string always. So don't need to worry about this.

This is part of the interface, I don't know what will happen by just adding
additional parameters here like this. Check the users of it:
https://searchfox.org/comm-central/search?q=createConversation&redirect=false

Yes, I have changed the usage. Now using getDirectConversation at all the places.

An awful lot of the logic here is duplicated from getGroupConversation. It
might be nice to combine them if possible, there could be a getConversation
method which takes the properties as a parameter or something maybe. I'm not
100% sure what differs between them.

I don't think so. The combined function will be really large and complex to understand. So it's better to have 2 different functions to work with. We pass different parameters when we create a new room. If the room already exists and room joining logic can be combined but it will also result in if and else blocks and copy-pasting the similar code in these if-else blocks

Comment 43

[Khushil Mistry [:khushil324]]

Attachment: Bug-1348064_direct-messaging-1.patch

Comment 44

[Patrick Cloke [:clokep]]

Comment on attachment 9153125 [details] [diff] [review]
Bug-1348064_direct-messaging-1.patch

Sorry for the delay on this. I think the changes made look OK. I think a LOT of this code needs better comments (and tests), but we can improve it in the future.

Comment 45

[Patrick Cloke [:clokep]]

Comment on attachment 9153125 [details] [diff] [review]
Bug-1348064_direct-messaging-1.patch

[Approval Request Comment]
Regression caused by (bug #): N/A
User impact if declined: Really none -- Matrix isn't yet enabled, but we're hoping to enable it for 78, so uplifting this early would help.
Testing completed (on c-c, etc.): None
Risk to taking this patch (and alternatives if risky): Not really any risk since this code isn't enabled yet.

Comment 46

[Wayne Mery (:wsmwk)]

Comment on attachment 9153125 [details] [diff] [review]
Bug-1348064_direct-messaging-1.patch

Approved for beta

Comment 47

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9a899d0baaa0
Implement Direct Messages for Matrix. r=clokep

Comment 48

[Patrick Cloke [:clokep]]

This bug dragged on a bit, so just to be clear, most of the patches landed in the 78 cycle. The last one landed in the 79 cycle, but is being uplifted to 78.

Comment 49

[Rob Lemley [:rjl]]

Thunderbird 78.0b4:
https://hg.mozilla.org/releases/comm-beta/rev/9f7fb887944f