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.

Sign up for GitHub

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

Jump to bottom

Update timeline items read receipts when the room members are loaded #2194

Merged
merged 4 commits into from
Jan 24, 2024
Merged

Update timeline items read receipts when the room members are loaded #2194

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
daf056a
Update timeline items' sender info and read receipts when the room me…
jmartinesp Jan 8, 2024
c4c4870
Only update this info if we have loaded the room members
jmartinesp Jan 8, 2024
aa73112
Remove inaccurate 'update sender info' text from the changelog
jmartinesp Jan 8, 2024
765cb97
Merge remote-tracking branch 'origin/develop' into fix/jme/2176-updat…
jmartinesp Jan 23, 2024
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
1 change: 1 addition & 0 deletions changelog.d/2176.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Update timeline items' read receipts when the room members info is loaded.
10 changes: 4 additions & 6 deletions ...l/src/main/kotlin/io/element/android/features/messages/impl/timeline/TimelinePresenter.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ import io.element.android.libraries.matrix.api.verification.SessionVerifiedStatu
import io.element.android.libraries.matrix.ui.room.canSendMessageAsState
import kotlinx.collections.immutable.ImmutableList
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.flow.combine
import kotlinx.coroutines.flow.launchIn
import kotlinx.coroutines.flow.onEach
import kotlinx.coroutines.launch
Expand Down Expand Up @@ -111,8 +112,6 @@ class TimelinePresenter @AssistedInject constructor(
}
}

val membersState by room.membersStateFlow.collectAsState()

fun handleEvents(event: TimelineEvents) {
when (event) {
TimelineEvents.LoadMore -> localScope.paginateBackwards()
Expand Down Expand Up @@ -149,13 +148,12 @@ class TimelinePresenter @AssistedInject constructor(
}

LaunchedEffect(Unit) {
timeline
.timelineItems
.onEach {
combine(timeline.timelineItems, room.membersStateFlow) { items, membersState ->
timelineItemsFactory.replaceWith(
timelineItems = it,
timelineItems = items,
roomMembers = membersState.roomMembers().orEmpty()
)
items
}
.onEach { timelineItems ->
if (timelineItems.isEmpty()) {
Expand Down
11 changes: 10 additions & 1 deletion ...tlin/io/element/android/features/messages/impl/timeline/factories/TimelineItemsFactory.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,16 @@ class TimelineItemsFactory @Inject constructor(
newTimelineItemStates.add(timelineItemState)
}
} else {
newTimelineItemStates.add(cacheItem)
val updatedItem = if (cacheItem is TimelineItem.Event && roomMembers.isNotEmpty()) {
eventItemFactory.update(
timelineItem = cacheItem,
receivedMatrixTimelineItem = timelineItems[index] as MatrixTimelineItem.Event,
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure that timelineItems[index] can always be cast to MatrixTimelineItem.Event?

Copy link
Member Author

Choose a reason for hiding this comment

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

It uses the unique id to identify cached items, so if the cached item is an event I expect the updated item with that id to also be an event. Maybe that's not always the case?

Copy link
Member

Choose a reason for hiding this comment

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

Yes the cached value is obviously the same, otherwise there is a big issue :D

roomMembers = roomMembers
)
} else {
cacheItem
}
newTimelineItemStates.add(updatedItem)
}
}
val result = timelineItemGrouper.group(newTimelineItemStates).toPersistentList()
Expand Down
46 changes: 31 additions & 15 deletions ...ement/android/features/messages/impl/timeline/factories/event/TimelineItemEventFactory.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,21 +53,7 @@ class TimelineItemEventFactory @Inject constructor(
val currentSender = currentTimelineItem.event.sender
val groupPosition =
computeGroupPosition(currentTimelineItem, timelineItems, index)
val senderDisplayName: String?
val senderAvatarUrl: String?

when (val senderProfile = currentTimelineItem.event.senderProfile) {
ProfileTimelineDetails.Unavailable,
ProfileTimelineDetails.Pending,
is ProfileTimelineDetails.Error -> {
senderDisplayName = null
senderAvatarUrl = null
}
is ProfileTimelineDetails.Ready -> {
senderDisplayName = senderProfile.getDisambiguatedDisplayName(currentSender)
senderAvatarUrl = senderProfile.avatarUrl
}
}
val (senderDisplayName, senderAvatarUrl) = currentTimelineItem.getSenderInfo()

val timeFormatter = DateFormat.getTimeInstance(DateFormat.SHORT)
val sentTime = timeFormatter.format(Date(currentTimelineItem.event.timestamp))
Expand Down Expand Up @@ -101,6 +87,36 @@ class TimelineItemEventFactory @Inject constructor(
)
}

fun update(
timelineItem: TimelineItem.Event,
receivedMatrixTimelineItem: MatrixTimelineItem.Event,
roomMembers: List<RoomMember>,
): TimelineItem.Event {
return timelineItem.copy(
readReceiptState = receivedMatrixTimelineItem.computeReadReceiptState(roomMembers)
)
}

private fun MatrixTimelineItem.Event.getSenderInfo(): Pair<String?, String?> {
val senderDisplayName: String?
val senderAvatarUrl: String?

when (val senderProfile = event.senderProfile) {
ProfileTimelineDetails.Unavailable,
ProfileTimelineDetails.Pending,
is ProfileTimelineDetails.Error -> {
senderDisplayName = null
senderAvatarUrl = null
}
is ProfileTimelineDetails.Ready -> {
senderDisplayName = senderProfile.getDisambiguatedDisplayName(event.sender)
senderAvatarUrl = senderProfile.avatarUrl
}
}

return senderDisplayName to senderAvatarUrl
}

private fun MatrixTimelineItem.Event.computeReactionsState(): TimelineItemReactions {
val timeFormatter = DateFormat.getTimeInstance(DateFormat.SHORT)
var aggregatedReactions = event.reactions.map { reaction ->
Expand Down
52 changes: 51 additions & 1 deletion ...c/test/kotlin/io/element/android/features/messages/impl/timeline/TimelinePresenterTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,14 +35,18 @@ import io.element.android.features.poll.api.actions.SendPollResponseAction
import io.element.android.features.poll.test.actions.FakeEndPollAction
import io.element.android.features.poll.test.actions.FakeSendPollResponseAction
import io.element.android.libraries.featureflag.api.FeatureFlags
import io.element.android.libraries.matrix.api.room.MatrixRoomMembersState
import io.element.android.libraries.matrix.api.timeline.MatrixTimeline
import io.element.android.libraries.matrix.api.timeline.MatrixTimelineItem
import io.element.android.libraries.matrix.api.timeline.item.event.EventReaction
import io.element.android.libraries.matrix.api.timeline.item.event.ReactionSender
import io.element.android.libraries.matrix.api.timeline.item.event.Receipt
import io.element.android.libraries.matrix.api.timeline.item.virtual.VirtualTimelineItem
import io.element.android.libraries.matrix.test.AN_EVENT_ID
import io.element.android.libraries.matrix.test.A_USER_ID
import io.element.android.libraries.matrix.test.encryption.FakeEncryptionService
import io.element.android.libraries.matrix.test.room.FakeMatrixRoom
import io.element.android.libraries.matrix.test.room.aRoomMember
import io.element.android.libraries.matrix.test.timeline.FakeMatrixTimeline
import io.element.android.libraries.matrix.test.timeline.aMessageContent
import io.element.android.libraries.matrix.test.timeline.anEventTimelineItem
Expand All @@ -60,6 +64,7 @@ import kotlinx.coroutines.test.runTest
import org.junit.Rule
import org.junit.Test
import java.util.Date
import kotlin.time.Duration.Companion.seconds

private const val FAKE_UNIQUE_ID = "FAKE_UNIQUE_ID"

Expand Down Expand Up @@ -353,6 +358,50 @@ class TimelinePresenterTest {
}
}

@Test
fun `present - when room member info is loaded, read receipts info should be updated`() = runTest {
val timeline = FakeMatrixTimeline(
listOf(
MatrixTimelineItem.Event(
FAKE_UNIQUE_ID,
anEventTimelineItem(
sender = A_USER_ID,
receipts = persistentListOf(
Receipt(
userId = A_USER_ID,
timestamp = 0L,
)
)
)
)
)
)
val room = FakeMatrixRoom(matrixTimeline = timeline).apply {
givenRoomMembersState(MatrixRoomMembersState.Unknown)
}

val avatarUrl = "https://domain.com/avatar.jpg"

val presenter = createTimelinePresenter(timeline, room)
moleculeFlow(RecompositionMode.Immediate) {
presenter.present()
}.test {
val initialState = consumeItemsUntilPredicate(30.seconds) { it.timelineItems.isNotEmpty() }.last()
val event = initialState.timelineItems.first() as TimelineItem.Event
assertThat(event.senderAvatar.url).isNull()
assertThat(event.readReceiptState.receipts.first().avatarData.url).isNull()

room.givenRoomMembersState(
MatrixRoomMembersState.Ready(
persistentListOf(aRoomMember(userId = A_USER_ID, avatarUrl = avatarUrl))
)
)

val updatedEvent = awaitItem().timelineItems.first() as TimelineItem.Event
assertThat(updatedEvent.readReceiptState.receipts.first().avatarData.url).isEqualTo(avatarUrl)
}
}

private suspend fun <T> ReceiveTurbine<T>.awaitFirstItem(): T {
// Skip 1 item if Mentions feature is enabled
if (FeatureFlags.Mentions.defaultValue) {
Expand All @@ -363,6 +412,7 @@ class TimelinePresenterTest {

private fun TestScope.createTimelinePresenter(
timeline: MatrixTimeline = FakeMatrixTimeline(),
room: FakeMatrixRoom = FakeMatrixRoom(matrixTimeline = timeline),
timelineItemsFactory: TimelineItemsFactory = aTimelineItemsFactory(),
redactedVoiceMessageManager: RedactedVoiceMessageManager = FakeRedactedVoiceMessageManager(),
messagesNavigator: FakeMessagesNavigator = FakeMessagesNavigator(),
Expand All @@ -371,7 +421,7 @@ class TimelinePresenterTest {
): TimelinePresenter {
return TimelinePresenter(
timelineItemsFactory = timelineItemsFactory,
room = FakeMatrixRoom(matrixTimeline = timeline),
room = room,
dispatchers = testCoroutineDispatchers(),
appScope = this,
navigator = messagesNavigator,
Expand Down
Loading