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

Room list room summary cleanup #2273

Merged
merged 10 commits into from
Jan 22, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ package io.element.android.features.roomlist.impl
import androidx.compose.ui.tooling.preview.PreviewParameterProvider
import io.element.android.features.leaveroom.api.aLeaveRoomState
import io.element.android.features.roomlist.impl.model.RoomListRoomSummary
import io.element.android.features.roomlist.impl.model.RoomListRoomSummaryPlaceholders
import io.element.android.features.roomlist.impl.model.aRoomListRoomSummary
import io.element.android.libraries.designsystem.components.avatar.AvatarData
import io.element.android.libraries.designsystem.components.avatar.AvatarSize
import io.element.android.libraries.designsystem.utils.snackbar.SnackbarMessage
Expand Down Expand Up @@ -71,25 +71,29 @@ internal fun aRoomListState() = RoomListState(

internal fun aRoomListRoomSummaryList(): ImmutableList<RoomListRoomSummary> {
return persistentListOf(
RoomListRoomSummary(
aRoomListRoomSummary(
name = "Room",
hasUnread = true,
timestamp = "14:18",
lastMessage = "A very very very very long message which suites on two lines",
avatarData = AvatarData("!id", "R", size = AvatarSize.RoomListItem),
id = "!roomId:domain",
roomId = RoomId("!roomId:domain")
),
RoomListRoomSummary(
aRoomListRoomSummary(
name = "Room#2",
hasUnread = false,
timestamp = "14:16",
lastMessage = "A short message",
avatarData = AvatarData("!id", "Z", size = AvatarSize.RoomListItem),
id = "!roomId2:domain",
roomId = RoomId("!roomId2:domain")
),
RoomListRoomSummaryPlaceholders.create("!roomId2:domain"),
RoomListRoomSummaryPlaceholders.create("!roomId3:domain"),
aRoomListRoomSummary(
id = "!roomId3:domain",
isPlaceholder = true,
),
aRoomListRoomSummary(
id = "!roomId4:domain",
isPlaceholder = true,
),
)
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,9 @@
package io.element.android.features.roomlist.impl.datasource

import io.element.android.features.roomlist.impl.model.RoomListRoomSummary
import io.element.android.features.roomlist.impl.model.RoomListRoomSummaryPlaceholders
import io.element.android.libraries.androidutils.diff.DiffCacheUpdater
import io.element.android.libraries.androidutils.diff.MutableListDiffCache
import io.element.android.libraries.core.coroutine.CoroutineDispatchers
import io.element.android.libraries.core.extensions.orEmpty
import io.element.android.libraries.dateformatter.api.LastMessageTimestampFormatter
import io.element.android.libraries.designsystem.components.avatar.AvatarData
import io.element.android.libraries.designsystem.components.avatar.AvatarSize
import io.element.android.libraries.eventformatter.api.RoomLastMessageFormatter
import io.element.android.libraries.matrix.api.core.RoomId
import io.element.android.libraries.matrix.api.notificationsettings.NotificationSettingsService
import io.element.android.libraries.matrix.api.roomlist.RoomListService
import io.element.android.libraries.matrix.api.roomlist.RoomSummary
Expand All @@ -49,8 +42,7 @@ import kotlin.time.Duration.Companion.seconds

class RoomListDataSource @Inject constructor(
private val roomListService: RoomListService,
private val lastMessageTimestampFormatter: LastMessageTimestampFormatter,
private val roomLastMessageFormatter: RoomLastMessageFormatter,
private val roomListRoomSummaryFactory: RoomListRoomSummaryFactory,
private val coroutineDispatchers: CoroutineDispatchers,
private val notificationSettingsService: NotificationSettingsService,
private val appScope: CoroutineScope,
Expand Down Expand Up @@ -121,7 +113,7 @@ class RoomListDataSource @Inject constructor(
private suspend fun buildAndEmitAllRooms(roomSummaries: List<RoomSummary>) {
if (diffCache.isEmpty()) {
_allRooms.emit(
RoomListRoomSummaryPlaceholders.createFakeList(16).toImmutableList()
roomListRoomSummaryFactory.createFakeList()
)
} else {
val roomListRoomSummaries = ArrayList<RoomListRoomSummary>()
Expand All @@ -141,33 +133,10 @@ class RoomListDataSource @Inject constructor(

private fun buildAndCacheItem(roomSummaries: List<RoomSummary>, index: Int): RoomListRoomSummary? {
val roomListRoomSummary = when (val roomSummary = roomSummaries.getOrNull(index)) {
is RoomSummary.Empty -> RoomListRoomSummaryPlaceholders.create(roomSummary.identifier)
is RoomSummary.Filled -> {
val avatarData = AvatarData(
id = roomSummary.identifier(),
name = roomSummary.details.name,
url = roomSummary.details.avatarUrl,
size = AvatarSize.RoomListItem,
)
val roomIdentifier = roomSummary.identifier()
RoomListRoomSummary(
id = roomSummary.identifier(),
roomId = RoomId(roomIdentifier),
name = roomSummary.details.name,
hasUnread = roomSummary.details.unreadNotificationCount > 0,
timestamp = lastMessageTimestampFormatter.format(roomSummary.details.lastMessageTimestamp),
lastMessage = roomSummary.details.lastMessage?.let { message ->
roomLastMessageFormatter.format(message.event, roomSummary.details.isDirect)
}.orEmpty(),
avatarData = avatarData,
userDefinedNotificationMode = roomSummary.details.userDefinedNotificationMode,
hasRoomCall = roomSummary.details.hasRoomCall,
isDm = roomSummary.details.isDm,
)
}
is RoomSummary.Empty -> roomListRoomSummaryFactory.createPlaceholder(roomSummary.identifier)
is RoomSummary.Filled -> roomListRoomSummaryFactory.create(roomSummary)
null -> null
}

diffCache[index] = roomListRoomSummary
return roomListRoomSummary
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
/*
* 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.features.roomlist.impl.datasource

import io.element.android.features.roomlist.impl.model.RoomListRoomSummary
import io.element.android.libraries.core.extensions.orEmpty
import io.element.android.libraries.dateformatter.api.LastMessageTimestampFormatter
import io.element.android.libraries.designsystem.components.avatar.AvatarData
import io.element.android.libraries.designsystem.components.avatar.AvatarSize
import io.element.android.libraries.eventformatter.api.RoomLastMessageFormatter
import io.element.android.libraries.matrix.api.core.RoomId
import io.element.android.libraries.matrix.api.roomlist.RoomSummary
import kotlinx.collections.immutable.ImmutableList
import kotlinx.collections.immutable.toImmutableList
import javax.inject.Inject

class RoomListRoomSummaryFactory @Inject constructor(
private val lastMessageTimestampFormatter: LastMessageTimestampFormatter,
private val roomLastMessageFormatter: RoomLastMessageFormatter,
) {
fun createPlaceholder(id: String): RoomListRoomSummary {
return RoomListRoomSummary(
id = id,
roomId = RoomId("!aRoom:domain"),
isPlaceholder = true,
name = "Short name",
timestamp = "hh:mm",
lastMessage = "Last message for placeholder",
avatarData = AvatarData(id, "S", size = AvatarSize.RoomListItem),
hasUnread = false,
userDefinedNotificationMode = null,
hasRoomCall = false,
isDm = false,
)
}

fun createFakeList(): ImmutableList<RoomListRoomSummary> {
return List(16) {
createPlaceholder("!fakeRoom$it:domain")
}.toImmutableList()
}

fun create(roomSummary: RoomSummary.Filled): RoomListRoomSummary {
val roomIdentifier = roomSummary.identifier()
val avatarData = AvatarData(
id = roomIdentifier,
name = roomSummary.details.name,
url = roomSummary.details.avatarUrl,
size = AvatarSize.RoomListItem,
)
return RoomListRoomSummary(
id = roomIdentifier,
roomId = RoomId(roomIdentifier),
name = roomSummary.details.name,
hasUnread = roomSummary.details.unreadNotificationCount > 0,
timestamp = lastMessageTimestampFormatter.format(roomSummary.details.lastMessageTimestamp),
lastMessage = roomSummary.details.lastMessage?.let { message ->
roomLastMessageFormatter.format(message.event, roomSummary.details.isDirect)
}.orEmpty(),
avatarData = avatarData,
isPlaceholder = false,
userDefinedNotificationMode = roomSummary.details.userDefinedNotificationMode,
hasRoomCall = roomSummary.details.hasRoomCall,
isDm = roomSummary.details.isDm,
)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,21 +18,20 @@ package io.element.android.features.roomlist.impl.model

import androidx.compose.runtime.Immutable
import io.element.android.libraries.designsystem.components.avatar.AvatarData
import io.element.android.libraries.designsystem.components.avatar.AvatarSize
import io.element.android.libraries.matrix.api.core.RoomId
import io.element.android.libraries.matrix.api.room.RoomNotificationMode

@Immutable
data class RoomListRoomSummary(
val id: String,
val roomId: RoomId,
val name: String = "",
val hasUnread: Boolean = false,
val timestamp: String? = null,
val lastMessage: CharSequence? = null,
val avatarData: AvatarData = AvatarData(id, name, size = AvatarSize.RoomListItem),
val isPlaceholder: Boolean = false,
val userDefinedNotificationMode: RoomNotificationMode? = null,
val hasRoomCall: Boolean = false,
val isDm: Boolean = false,
val name: String,
val hasUnread: Boolean,
val timestamp: String?,
val lastMessage: CharSequence?,
val avatarData: AvatarData,
val isPlaceholder: Boolean,
val userDefinedNotificationMode: RoomNotificationMode?,
val hasRoomCall: Boolean,
val isDm: Boolean,
)

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,10 @@ open class RoomListRoomSummaryProvider : PreviewParameterProvider<RoomListRoomSu
aRoomListRoomSummary(),
aRoomListRoomSummary(lastMessage = null),
aRoomListRoomSummary(hasUnread = true, notificationMode = RoomNotificationMode.MENTIONS_AND_KEYWORDS_ONLY),
aRoomListRoomSummary(timestamp = "88:88", notificationMode = RoomNotificationMode.MENTIONS_AND_KEYWORDS_ONLY),
aRoomListRoomSummary(timestamp = "88:88", notificationMode = RoomNotificationMode.MUTE),
aRoomListRoomSummary(timestamp = "88:88", hasUnread = true),
aRoomListRoomSummary(isPlaceholder = true, timestamp = "88:88"),
aRoomListRoomSummary(notificationMode = RoomNotificationMode.MENTIONS_AND_KEYWORDS_ONLY),
aRoomListRoomSummary(notificationMode = RoomNotificationMode.MUTE),
aRoomListRoomSummary(hasUnread = true),
aRoomListRoomSummary(isPlaceholder = true),
aRoomListRoomSummary(
name = "A very long room name that should be truncated",
lastMessage = "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt" +
Expand All @@ -44,23 +44,27 @@ open class RoomListRoomSummaryProvider : PreviewParameterProvider<RoomListRoomSu
)
}

fun aRoomListRoomSummary(
internal fun aRoomListRoomSummary(
id: String = "!roomId:domain",
name: String = "Room name",
hasUnread: Boolean = false,
lastMessage: String? = "Last message",
timestamp: String? = if (lastMessage != null) "88:88" else null,
isPlaceholder: Boolean = false,
notificationMode: RoomNotificationMode? = null,
hasUnread: Boolean = false,
timestamp: String? = "88:88",
hasRoomCall: Boolean = false,
isPlaceholder: Boolean = false,
name: String = "Room name",
avatarData: AvatarData = AvatarData(id, name, size = AvatarSize.RoomListItem),
isDm: Boolean = false,
) = RoomListRoomSummary(
id = "!roomId",
roomId = RoomId("!roomId:domain"),
id = id,
roomId = RoomId(id),
name = name,
hasUnread = hasUnread,
timestamp = timestamp,
lastMessage = lastMessage,
avatarData = AvatarData("!roomId", "Room name", size = AvatarSize.RoomListItem),
avatarData = avatarData,
isPlaceholder = isPlaceholder,
userDefinedNotificationMode = notificationMode,
hasRoomCall = hasRoomCall,
isDm = isDm,
)
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ import io.element.android.features.networkmonitor.test.FakeNetworkMonitor
import io.element.android.features.roomlist.impl.datasource.FakeInviteDataSource
import io.element.android.features.roomlist.impl.datasource.InviteStateDataSource
import io.element.android.features.roomlist.impl.datasource.RoomListDataSource
import io.element.android.features.roomlist.impl.datasource.RoomListRoomSummaryFactory
import io.element.android.features.roomlist.impl.model.RoomListRoomSummary
import io.element.android.features.roomlist.impl.model.aRoomListRoomSummary
import io.element.android.libraries.dateformatter.api.LastMessageTimestampFormatter
import io.element.android.libraries.dateformatter.test.FakeLastMessageTimestampFormatter
import io.element.android.libraries.designsystem.components.avatar.AvatarData
Expand Down Expand Up @@ -316,7 +316,7 @@ class RoomListPresenterTests {
skipItems(1)

val initialState = awaitItem()
val summary = aRoomListRoomSummary()
val summary = aRoomListRoomSummary
initialState.eventSink(RoomListEvents.ShowContextMenu(summary))

val shownState = awaitItem()
Expand All @@ -336,7 +336,7 @@ class RoomListPresenterTests {
skipItems(1)

val initialState = awaitItem()
val summary = aRoomListRoomSummary()
val summary = aRoomListRoomSummary
initialState.eventSink(RoomListEvents.ShowContextMenu(summary))

val shownState = awaitItem()
Expand Down Expand Up @@ -415,9 +415,11 @@ class RoomListPresenterTests {
inviteStateDataSource = inviteStateDataSource,
leaveRoomPresenter = leaveRoomPresenter,
roomListDataSource = RoomListDataSource(
client.roomListService,
lastMessageTimestampFormatter,
roomLastMessageFormatter,
roomListService = client.roomListService,
roomListRoomSummaryFactory = RoomListRoomSummaryFactory(
lastMessageTimestampFormatter = lastMessageTimestampFormatter,
roomLastMessageFormatter = roomLastMessageFormatter,
),
coroutineDispatchers = testCoroutineDispatchers(),
notificationSettingsService = client.notificationSettingsService(),
appScope = coroutineScope
Expand All @@ -443,4 +445,7 @@ private val aRoomListRoomSummary = RoomListRoomSummary(
lastMessage = "",
avatarData = AvatarData(id = A_ROOM_ID.value, name = A_ROOM_NAME, size = AvatarSize.RoomListItem),
isPlaceholder = false,
userDefinedNotificationMode = null,
hasRoomCall = false,
isDm = false,
)
Copy link
Member Author

Choose a reason for hiding this comment

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

id has changed, and so the avatar color.

Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Copy link
Member Author

Choose a reason for hiding this comment

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

no timestamp if no message.

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with not displaying the timestamp if there's no last message, but DefaultLastMessageFormatter has a CharSequence? return type that could return null even if there is a last message formatted, so you'd see the room item at the top of the list when it's updated, but you wouldn't see a timestamp if I'm not mistaken, which would look weird.

Could we change this return type to CharSequence and make sure we always return a String, even an empty one, if there is a last message for the room? So if there is a last event we always return something (maybe empty, but something) and if there really is nothing we return null.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm fine with not displaying the timestamp if there's no last message

I did not change any logic in the existing codebase, just wanted to have more coherent previews. But you're right, since DefaultLastMessageFormatter can return null, we can have a room list item with no message and a timestamp. So the previous screenshot may reflect something that can happen.

if we want to change this, it should be done in a separate PR, as this one is only rework with no logical change.

Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Loading