Bugzilla

(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.

> @@ +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
> 

Yes, 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

(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