Skip to content
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

Unify the way we decide whether a room is a DM or a group room #3100

Merged
merged 8 commits into from
Jul 10, 2024
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,13 @@ open class AcceptDeclineInviteStateProvider : PreviewParameterProvider<AcceptDec
anAcceptDeclineInviteState(),
anAcceptDeclineInviteState(
invite = Optional.of(
InviteData(RoomId("!room:matrix.org"), isDirect = true, roomName = "Alice"),
InviteData(RoomId("!room:matrix.org"), isDm = true, roomName = "Alice"),
),
declineAction = AsyncAction.Confirming,
),
anAcceptDeclineInviteState(
invite = Optional.of(
InviteData(RoomId("!room:matrix.org"), isDirect = false, roomName = "Some room"),
InviteData(RoomId("!room:matrix.org"), isDm = false, roomName = "Some room"),
),
declineAction = AsyncAction.Confirming,
),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,5 +21,5 @@ import io.element.android.libraries.matrix.api.core.RoomId
data class InviteData(
val roomId: RoomId,
val roomName: String,
val isDirect: Boolean,
val isDm: Boolean,
)
Original file line number Diff line number Diff line change
Expand Up @@ -79,13 +79,13 @@ private fun DeclineConfirmationDialog(
onDismissClick: () -> Unit,
modifier: Modifier = Modifier
) {
val contentResource = if (invite.isDirect) {
val contentResource = if (invite.isDm) {
R.string.screen_invites_decline_direct_chat_message
} else {
R.string.screen_invites_decline_chat_message
}

val titleResource = if (invite.isDirect) {
val titleResource = if (invite.isDm) {
R.string.screen_invites_decline_direct_chat_title
} else {
R.string.screen_invites_decline_chat_title
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,7 @@ class AcceptDeclineInvitePresenterTest {
return InviteData(
roomId = roomId,
roomName = name,
isDirect = isDirect
isDm = isDirect
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe rename also the parameter.

)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ import io.element.android.libraries.matrix.api.core.RoomIdOrAlias
import io.element.android.libraries.matrix.api.room.CurrentUserMembership
import io.element.android.libraries.matrix.api.room.MatrixRoomInfo
import io.element.android.libraries.matrix.api.room.RoomType
import io.element.android.libraries.matrix.api.room.isDm
import io.element.android.libraries.matrix.api.room.join.JoinRoom
import io.element.android.libraries.matrix.api.room.preview.RoomPreview
import io.element.android.libraries.matrix.ui.model.toInviteSender
Expand Down Expand Up @@ -173,7 +174,7 @@ private fun RoomPreview.toContentState(): ContentState {
topic = topic,
alias = canonicalAlias,
numberOfMembers = numberOfJoinedMembers,
isDirect = false,
isDm = false,
roomType = roomType,
roomAvatarUrl = avatarUrl,
joinAuthorisationStatus = when {
Expand All @@ -194,7 +195,7 @@ internal fun RoomDescription.toContentState(): ContentState {
topic = topic,
alias = alias,
numberOfMembers = numberOfMembers,
isDirect = false,
isDm = false,
roomType = RoomType.Room,
roomAvatarUrl = avatarUrl,
joinAuthorisationStatus = when (joinRule) {
Expand All @@ -213,7 +214,7 @@ internal fun MatrixRoomInfo.toContentState(): ContentState {
topic = topic,
alias = canonicalAlias,
numberOfMembers = activeMembersCount,
isDirect = isDirect,
isDm = isDm,
roomType = if (isSpace) RoomType.Space else RoomType.Room,
roomAvatarUrl = avatarUrl,
joinAuthorisationStatus = when {
Expand All @@ -233,7 +234,7 @@ internal fun ContentState.toInviteData(): InviteData? {
roomId = roomId,
// Note: name should not be null at this point, but use Id just in case...
roomName = name ?: roomId.value,
isDirect = isDirect
isDm = isDm
)
else -> null
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ sealed interface ContentState {
val topic: String?,
val alias: RoomAlias?,
val numberOfMembers: Long?,
val isDirect: Boolean,
val isDm: Boolean,
val roomType: RoomType,
val roomAvatarUrl: String?,
val joinAuthorisationStatus: JoinAuthorisationStatus,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import io.element.android.libraries.matrix.api.core.RoomIdOrAlias
import io.element.android.libraries.matrix.api.core.UserId
import io.element.android.libraries.matrix.api.core.toRoomIdOrAlias
import io.element.android.libraries.matrix.api.room.RoomType
import io.element.android.libraries.matrix.api.room.isDm
import io.element.android.libraries.matrix.ui.model.InviteSender

open class JoinRoomStateProvider : PreviewParameterProvider<JoinRoomState> {
Expand Down Expand Up @@ -84,6 +85,12 @@ open class JoinRoomStateProvider : PreviewParameterProvider<JoinRoomState> {
roomType = RoomType.Space,
)
),
aJoinRoomState(
contentState = aLoadedContentState(
name = "A DM",
isDm = true,
)
),
)
}

Expand All @@ -106,7 +113,7 @@ fun aLoadedContentState(
alias: RoomAlias? = RoomAlias("#exa:matrix.org"),
topic: String? = "Element X is a secure, private and decentralized messenger.",
numberOfMembers: Long? = null,
isDirect: Boolean = false,
isDm: Boolean = false,
roomType: RoomType = RoomType.Room,
roomAvatarUrl: String? = null,
joinAuthorisationStatus: JoinAuthorisationStatus = JoinAuthorisationStatus.Unknown
Expand All @@ -116,7 +123,7 @@ fun aLoadedContentState(
alias = alias,
topic = topic,
numberOfMembers = numberOfMembers,
isDirect = isDirect,
isDm = isDm,
roomType = roomType,
roomAvatarUrl = roomAvatarUrl,
joinAuthorisationStatus = joinAuthorisationStatus
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ class JoinRoomPresenterTest {
assertThat(contentState.topic).isEqualTo(roomInfo.topic)
assertThat(contentState.alias).isEqualTo(roomInfo.canonicalAlias)
assertThat(contentState.numberOfMembers).isEqualTo(roomInfo.activeMembersCount)
assertThat(contentState.isDirect).isEqualTo(roomInfo.isDirect)
assertThat(contentState.isDm).isEqualTo(roomInfo.isDirect)
assertThat(contentState.roomAvatarUrl).isEqualTo(roomInfo.avatarUrl)
}
}
Expand Down Expand Up @@ -283,7 +283,7 @@ class JoinRoomPresenterTest {
assertThat(contentState.topic).isEqualTo(roomDescription.topic)
assertThat(contentState.alias).isEqualTo(roomDescription.alias)
assertThat(contentState.numberOfMembers).isEqualTo(roomDescription.numberOfMembers)
assertThat(contentState.isDirect).isFalse()
assertThat(contentState.isDm).isFalse()
assertThat(contentState.roomAvatarUrl).isEqualTo(roomDescription.avatarUrl)
}
}
Expand Down Expand Up @@ -398,7 +398,7 @@ class JoinRoomPresenterTest {
topic = "Room topic",
alias = RoomAlias("#alias:matrix.org"),
numberOfMembers = 2,
isDirect = false,
isDm = false,
roomType = RoomType.Room,
roomAvatarUrl = "avatarUrl",
joinAuthorisationStatus = JoinAuthorisationStatus.CanJoin
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import io.element.android.libraries.di.SessionScope
import io.element.android.libraries.matrix.api.MatrixClient
import io.element.android.libraries.matrix.api.core.RoomId
import io.element.android.libraries.matrix.api.room.RoomMembershipObserver
import io.element.android.libraries.matrix.api.room.isDm
import kotlinx.coroutines.launch
import timber.log.Timber
import javax.inject.Inject
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ class DefaultLeaveRoomPresenterTest {
client = FakeMatrixClient().apply {
givenGetRoomResult(
roomId = A_ROOM_ID,
result = FakeMatrixRoom(activeMemberCount = 2, isDirect = true, isOneToOne = true),
result = FakeMatrixRoom(activeMemberCount = 2, isDirect = true),
)
}
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ import io.element.android.libraries.matrix.api.room.MatrixRoom
import io.element.android.libraries.matrix.api.room.MatrixRoomInfo
import io.element.android.libraries.matrix.api.room.MatrixRoomMembersState
import io.element.android.libraries.matrix.api.room.MessageEventType
import io.element.android.libraries.matrix.api.room.isDm
import io.element.android.libraries.matrix.ui.messages.reply.map
import io.element.android.libraries.matrix.ui.model.getAvatarData
import io.element.android.libraries.matrix.ui.room.canCall
Expand Down Expand Up @@ -162,7 +163,7 @@ class MessagesPresenter @AssistedInject constructor(
var showReinvitePrompt by remember { mutableStateOf(false) }
LaunchedEffect(hasDismissedInviteDialog, composerState.textEditorState.hasFocus(), syncUpdateFlow.value) {
withContext(dispatchers.io) {
showReinvitePrompt = !hasDismissedInviteDialog && composerState.textEditorState.hasFocus() && room.isDirect && room.activeMemberCount == 1L
showReinvitePrompt = !hasDismissedInviteDialog && composerState.textEditorState.hasFocus() && room.isDm && room.activeMemberCount == 1L
}
}
val networkConnectionStatus by networkMonitor.connectivity.collectAsState()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ import io.element.android.libraries.matrix.api.room.MatrixRoom
import io.element.android.libraries.matrix.api.room.Mention
import io.element.android.libraries.matrix.api.room.draft.ComposerDraft
import io.element.android.libraries.matrix.api.room.draft.ComposerDraftType
import io.element.android.libraries.matrix.api.room.isDm
import io.element.android.libraries.matrix.ui.messages.reply.InReplyToDetails
import io.element.android.libraries.matrix.ui.messages.reply.map
import io.element.android.libraries.mediapickers.api.PickerProvider
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ import io.element.android.libraries.core.coroutine.CoroutineDispatchers
import io.element.android.libraries.matrix.api.core.EventId
import io.element.android.libraries.matrix.api.room.MatrixRoom
import io.element.android.libraries.matrix.api.room.MessageEventType
import io.element.android.libraries.matrix.api.room.isDm
import io.element.android.libraries.matrix.api.room.roomMembers
import io.element.android.libraries.matrix.api.timeline.ReceiptType
import io.element.android.libraries.matrix.api.timeline.item.event.TimelineItemEventOrigin
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -839,7 +839,6 @@ class MessageComposerPresenterTest {
val david = aRoomMember(userId = A_USER_ID_4, displayName = "Dave", membership = RoomMembershipState.JOIN)
val room = FakeMatrixRoom(
isDirect = false,
isOneToOne = false,
).apply {
givenRoomMembersState(
MatrixRoomMembersState.Ready(
Expand Down Expand Up @@ -904,7 +903,8 @@ class MessageComposerPresenterTest {
val david = aRoomMember(userId = A_USER_ID_4, displayName = "Dave", membership = RoomMembershipState.JOIN)
val room = FakeMatrixRoom(
isDirect = true,
isOneToOne = true,
activeMemberCount = 2,
isEncrypted = true,
).apply {
givenRoomMembersState(
MatrixRoomMembersState.Ready(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import io.element.android.libraries.matrix.api.room.MatrixRoom
import io.element.android.libraries.matrix.api.room.MatrixRoomMembersState
import io.element.android.libraries.matrix.api.room.RoomMember
import io.element.android.libraries.matrix.api.room.StateEventType
import io.element.android.libraries.matrix.api.room.isDm
import io.element.android.libraries.matrix.api.room.powerlevels.canInvite
import io.element.android.libraries.matrix.api.room.powerlevels.canSendState
import io.element.android.libraries.matrix.api.room.roomNotificationSettings
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import io.element.android.libraries.matrix.api.core.UserId
import io.element.android.libraries.matrix.api.room.MatrixRoom
import io.element.android.libraries.matrix.api.room.RoomMember
import io.element.android.libraries.matrix.api.room.RoomMembershipState
import io.element.android.libraries.matrix.api.room.isDm
import io.element.android.libraries.matrix.api.room.powerlevels.canBan
import io.element.android.libraries.matrix.api.room.powerlevels.canKick
import io.element.android.services.analytics.api.AnalyticsService
Expand All @@ -58,8 +59,7 @@ class DefaultRoomMembersModerationPresenter @Inject constructor(
private suspend fun canKick() = room.canKick().getOrDefault(false)

override suspend fun canDisplayModerationActions(): Boolean {
val isDm = room.isDm && room.isEncrypted
return !isDm && (canBan() || canKick())
return !room.isDm && (canBan() || canKick())
}

@Composable
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ import org.junit.Test
class DefaultRoomMembersModerationPresenterTest {
@Test
fun `canDisplayModerationActions - when room is DM is false`() = runTest {
val room = FakeMatrixRoom(isDirect = true, isPublic = true, isOneToOne = true).apply {
val room = FakeMatrixRoom(isDirect = true, isPublic = true, activeMemberCount = 2).apply {
givenRoomInfo(aRoomInfo(isDirect = true, isPublic = false, activeMembersCount = 2))
}
val presenter = createDefaultRoomMembersModerationPresenter(matrixRoom = room)
Expand All @@ -54,7 +54,7 @@ class DefaultRoomMembersModerationPresenterTest {

@Test
fun `canDisplayModerationActions - when user can kick other users, FF is enabled and room is not a DM returns true`() = runTest {
val room = FakeMatrixRoom(isDirect = false, isOneToOne = false).apply {
val room = FakeMatrixRoom(isDirect = false, activeMemberCount = 10).apply {
givenCanKickResult(Result.success(true))
}
val presenter = createDefaultRoomMembersModerationPresenter(matrixRoom = room)
Expand All @@ -63,7 +63,7 @@ class DefaultRoomMembersModerationPresenterTest {

@Test
fun `canDisplayModerationActions - when user can ban other users, FF is enabled and room is not a DM returns true`() = runTest {
val room = FakeMatrixRoom(isDirect = false, isOneToOne = false).apply {
val room = FakeMatrixRoom(isDirect = false, activeMemberCount = 10).apply {
givenCanBanResult(Result.success(true))
}
val presenter = createDefaultRoomMembersModerationPresenter(matrixRoom = room)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -290,5 +290,5 @@ internal fun RoomListRoomSummary.toInviteData() = InviteData(
roomId = roomId,
// Note: `name` should not be null at this point, but just in case, fallback to the roomId
roomName = name ?: roomId.value,
isDirect = isDirect,
isDm = isDm,
)
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,8 @@ internal fun RoomSummaryRow(
modifier = modifier
) {
InviteNameAndIndicatorRow(name = room.name)
InviteSubtitle(isDirect = room.isDirect, inviteSender = room.inviteSender, canonicalAlias = room.canonicalAlias)
if (!room.isDirect && room.inviteSender != null) {
InviteSubtitle(isDm = room.isDm, inviteSender = room.inviteSender, canonicalAlias = room.canonicalAlias)
if (!room.isDm && room.inviteSender != null) {
Spacer(modifier = Modifier.height(4.dp))
InviteSenderView(
modifier = Modifier.fillMaxWidth(),
Expand Down Expand Up @@ -206,12 +206,12 @@ private fun NameAndTimestampRow(

@Composable
private fun InviteSubtitle(
isDirect: Boolean,
isDm: Boolean,
inviteSender: InviteSender?,
canonicalAlias: RoomAlias?,
modifier: Modifier = Modifier
) {
val subtitle = if (isDirect) {
val subtitle = if (isDm) {
inviteSender?.userId?.value
} else {
canonicalAlias?.value
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ class RoomListRoomSummaryFactory @Inject constructor(
isMarkedUnread = details.isMarkedUnread,
timestamp = lastMessageTimestampFormatter.format(details.lastMessageTimestamp),
lastMessage = details.lastMessage?.let { message ->
roomLastMessageFormatter.format(message.event, details.isDirect)
roomLastMessageFormatter.format(message.event, details.isDm)
}.orEmpty(),
avatarData = avatarData,
userDefinedNotificationMode = details.userDefinedNotificationMode,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ open class RoomListRoomSummaryProvider : PreviewParameterProvider<RoomListRoomSu
userId = UserId("@bob:matrix.org"),
displayName = "Bob",
),
isDirect = true,
isDm = true,
),
aRoomListRoomSummary(
name = null,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ data class NotificationData(
val roomAvatarUrl: String?,
val roomDisplayName: String?,
val isDirect: Boolean,
val isDm: Boolean,
val isEncrypted: Boolean,
val isNoisy: Boolean,
val timestamp: Long,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,6 @@ interface MatrixRoom : Closeable {
val activeMemberCount: Long
val joinedMemberCount: Long

/** Whether the room is a direct message. */
val isDm: Boolean get() = isDirect && isOneToOne

val roomInfoFlow: Flow<MatrixRoomInfo>
val roomTypingMembersFlow: Flow<List<UserId>>

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
/*
* Copyright (c) 2024 New Vector Ltd
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package io.element.android.libraries.matrix.api.room

/**
* Returns whether the room with the provided info is a DM.
* A DM is a room with at most 2 active members (one of them may have left).
*
* @param isDirect true if the room is direct
* @param activeMembersCount the number of active members in the room (joined or invited)
*/
fun isDm(isDirect: Boolean, activeMembersCount: Int): Boolean {
return isDirect && activeMembersCount <= 2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sadly we cannot rely on the number of activeMembersCount since we can have bot like AuditBot or AdminBot in all the DM.

We will have to find a solution to manage such rooms.

But I agree that this is not new with this PR, so we may handle this later, and so this is not a blocker.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we should make explicit changes in the SDK to support bots in DMs.

}

/**
* Returns whether the [MatrixRoom] is a DM.
*/
val MatrixRoom.isDm get() = isDm(isDirect, activeMemberCount.toInt())

/**
* Returns whether the [MatrixRoomInfo] is from a DM.
*/
val MatrixRoomInfo.isDm get() = isDm(isDirect, activeMembersCount.toInt())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for those handy extensions!

Loading
Loading