Skip to content

Read receipt in wrong order #5532

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

Merged
merged 2 commits into from
Mar 14, 2022
Merged

Read receipt in wrong order #5532

merged 2 commits into from
Mar 14, 2022

Conversation

Claire1817
Copy link
Contributor

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

Update read receipt order in the factory.

Motivation and context

#5514

Screenshots / GIFs

Tests

Open ready receipt details on a message (especially if there is some events hidden before it)

Tested devices

  • Physical
  • Emulator
  • OS version(s):

Checklist

@Claire1817 Claire1817 marked this pull request as draft March 14, 2022 09:34
@github-actions
Copy link

github-actions bot commented Mar 14, 2022

Unit Test Results

  98 files  ±  0    98 suites  ±0   1m 9s ⏱️ -5s
178 tests +  4  178 ✔️ +  4  0 💤 ±0  0 ±0 
584 runs  +16  584 ✔️ +16  0 💤 ±0  0 ±0 

Results for commit 6c7b4d4. ± Comparison against base commit acfeb7f.

This pull request removes 1 and adds 5 tests. Note that renamed tests count towards both.
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ when handling display name update then updates upstream user display name
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given changing profile picture is not supported when updating display name then updates upstream user display name and completes personalization
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given changing profile picture is supported when updating display name then updates upstream user display name and moves to choose profile picture
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given homeserver does not support personalisation when registering account then updates state and emits account created event
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given only supports changing profile picture when handling PersonalizeProfile then emits contents choose profile picture
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given supports changing display name when handling PersonalizeProfile then emits contents choose display name

♻️ This comment has been updated with latest results.

@Claire1817 Claire1817 marked this pull request as ready for review March 14, 2022 10:19
@Claire1817 Claire1817 requested review from a team, ganfra and ouchadam and removed request for a team and ganfra March 14, 2022 10:28
.toList()

}.toList()
val readReceiptsDataSorted = sortItem(readReceiptsData)
Copy link
Contributor

@ouchadam ouchadam Mar 14, 2022

Choose a reason for hiding this comment

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

do we know why this extra sorting is needed? it looks like we're already sorting by originServerTs https://github.com./vector-im/element-android/blob/develop/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/database/mapper/TimelineEventMapper.kt#L53 (assuming this mapper is used)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's necessary when we have to concatenate the read receipts which come from different events (not displayed by default in android application but displayed in web application). The complete list is not ordered but each small list is.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 thanks for the explanation

@@ -37,17 +37,22 @@ class ReadReceiptsItemFactory @Inject constructor(private val avatarRenderer: Av
val readReceiptsData = readReceipts
.map {
ReadReceiptData(it.roomMember.userId, it.roomMember.avatarUrl, it.roomMember.displayName, it.originServerTs)
}
.toList()
Copy link
Contributor

Choose a reason for hiding this comment

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

we can avoid the extra function by applying the sorting here

.map { ... }
.sortedByDescending { it.timestamp }

also looks like the toList isn't needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, you're right :)

Copy link
Contributor

Choose a reason for hiding this comment

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

💯 thanks!

Copy link
Member

Choose a reason for hiding this comment

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

The fix is OK, but I think the sorting should be rather done SDK side.

@Claire1817 Claire1817 requested a review from ouchadam March 14, 2022 16:11
@Claire1817 Claire1817 enabled auto-merge March 14, 2022 16:27
@Claire1817 Claire1817 merged commit 5de7873 into develop Mar 14, 2022
@Claire1817 Claire1817 deleted the cgizard/ISSSUE-5514 branch March 14, 2022 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants