Open Bug 1348064 Opened 3 years ago Updated 1 day ago

Implement Direct Messages

Categories

(Chat Core :: Matrix, enhancement)

enhancement
Not set

Tracking

(Not tracked)

People

(Reporter: matrixisreal, Assigned: khushil324)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Assignee: nobody → pavankarthikboddeda
Status: NEW → ASSIGNED
Attached patch DM_V0.patchSplinter Review
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.
Attachment #8880571 - Flags: review?(clokep)
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?
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
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
(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!
> 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 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.
Attachment #8880571 - Flags: review?(clokep) → feedback+
(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?
Attached patch V2.patchSplinter Review
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).
Attachment #8886877 - Flags: feedback?(fred.wang)
Attachment #8886877 - Flags: feedback?(clokep)
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
Attachment #8886877 - Flags: feedback?(fred.wang) → feedback+
Depends on: 1394397
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.
Attachment #8886877 - Flags: feedback?(clokep)
Assignee: pavankarthikboddeda → nobody
Status: ASSIGNED → NEW
Blocks: 1572636

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.

Assignee: nobody → khushil324
You need to log in before you can comment on or make changes to this bug.