-
Notifications
You must be signed in to change notification settings - Fork 749
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Load existing DM instead of creating a new one #4157
Conversation
71d551c
to
17bcf90
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@Florian14 do you thing this can close #2893? |
@bmarty I think so, just tested before and after my PR, work fine now. |
@@ -71,10 +71,13 @@ class CreateDirectRoomViewModel @AssistedInject constructor(@Assisted | |||
* If users already have a DM room then navigate to it instead of creating a new room. | |||
*/ | |||
private fun onSubmitInvitees(action: CreateDirectRoomAction.CreateRoomAndInviteSelectedUsers) { | |||
if (action.existingDmRoomId != null) { | |||
val existingRoomId = action.selections.singleOrNull()?.getMxId()?.let { userId -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not look right. If it's a request for a grouped DM this will just open the DM with the first user?
** Edit ** ok single or null would do the trick.
check getMxid it's not always returning mxid.
An improvment could be to check for existing grouped DM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
singleOrNull() ≠ firstOrNull()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, agree to check for existing grouped DM, requires some more changes
fun getMxId(): String { | ||
return when (this) { | ||
is UserPendingSelection -> user.userId | ||
is ThreePidPendingSelection -> threePid.value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this would be a mail or Pn but not a mxid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but I think it will work, until the other user has not joined the room. @Florian14 do you confirm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the user email was used as the mxId until the user has accepted the invite (is mxId != userId ?). This is probably a lack of knowledge of the protocol on my part.
In previous implementation, existing DM was checked only for QR code invites.
This PR moves this check in the ViewModel side (which makes more sense) and applies it for 1:1 in both cases (QR and user invites).