Skip to content

Conversation

@fedrunov
Copy link
Contributor

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

with MSC3771 we have a possibility to display read receipts for messages in a thread

Motivation and context

#6996

Screenshots / GIFs

output

@fedrunov fedrunov requested a review from ericdecanini October 27, 2022 10:49
@fedrunov fedrunov requested review from bmarty and removed request for ericdecanini November 1, 2022 13:13
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

Thanks! The code is not compiling, can you check please? Also I have added some remarks.

* Force the read marker to be set on the latest event.
*/
suspend fun markAsRead(params: MarkAsReadParams = MarkAsReadParams.BOTH)
suspend fun markAsRead(params: MarkAsReadParams = MarkAsReadParams.BOTH, mainTimeLineOnly: Boolean)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add default value to the new parameters to avoid API breaks.
Also can you document API change in a .sdk file for towncrier? Thanks.

*/
internal fun updateNotificationsNew(roomId: String, realm: Realm, currentUserId: String) {
val readReceipt = findMyReadReceipt(realm, roomId, currentUserId) ?: return
val readReceipt = findMyReadReceipt(realm, roomId, currentUserId, ReadService.THREAD_ID_MAIN) ?: return
Copy link
Member

Choose a reason for hiding this comment

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

Maybe use null instead of introducing const ReadService.THREAD_ID_MAIN. Not sure it's better though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

null and main values for threadId in read receipt are different cases so we can't use null here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After looking deeper at this method I've realised that it needs to be changed, so it will use exact threadId instead of main

internal fun ReadReceiptEntity.Companion.where(realm: Realm, roomId: String, userId: String): RealmQuery<ReadReceiptEntity> {
return realm.where<ReadReceiptEntity>()
.equalTo(ReadReceiptEntityFields.PRIMARY_KEY, buildPrimaryKey(roomId, userId))
.equalTo(ReadReceiptEntityFields.USER_ID, userId)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe make this fun private to avoid any mistake. I understand that only the 2 functions above have to be used now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've found some usages of this method, but after some closer look it appeared that these cases should use threadId also, so now there is no usages and I'll rather remove this method completely. Or is there a reason to keep it and make private even if it's not used?

}

private fun buildPrimaryKey(roomId: String, userId: String) = "${roomId}_$userId"
private fun buildPrimaryKey(roomId: String, userId: String, threadId: String?) = "${roomId}_${userId}_${threadId ?: "null"}"
Copy link
Member

Choose a reason for hiding this comment

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

For compatibility I would have written

Suggested change
private fun buildPrimaryKey(roomId: String, userId: String, threadId: String?) = "${roomId}_${userId}_${threadId ?: "null"}"
private fun buildPrimaryKey(roomId: String, userId: String, threadId: String?): String {
return if (threadId == null) {
"${roomId}_${userId}"
} else {
"${roomId}_${userId}_${threadId}"
}

private val readReceiptsSummaryMapper: ReadReceiptsSummaryMapper,
@UserId private val userId: String
@UserId private val userId: String,
private val homeServerCapabilitiesService: HomeServerCapabilitiesService,
Copy link
Member

Choose a reason for hiding this comment

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

Better to inject HomeServerCapabilitiesDataSource here.

private fun ensureReadReceiptAreLoaded(realm: Realm) {
readReceiptHandler.getContentFromInitSync(roomId)
?.also {
Timber.w("INIT_SYNC Insert when opening timeline RR for room $roomId")
Copy link
Member

Choose a reason for hiding this comment

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

Maybe just reduce the log level to d, or delete the also block.

) {
// First check if we have data from init sync to handle
getContentFromInitSync(roomId)?.let {
Timber.w("INIT_SYNC Insert during incremental sync RR for room $roomId")
Copy link
Member

Choose a reason for hiding this comment

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

Same remark about log level.

.readReceipts(readReceiptsData)
.avatarRenderer(avatarRenderer)
.shouldHideReadReceipts(isFromThreadTimeLine)
.shouldHideReadReceipts(false)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe shouldHideReadReceipts can be just removed. Or maybe set to something like isFromThreadTimeLine && !canUseThreadReadReceiptsAndNotifications

@fedrunov fedrunov requested a review from bmarty November 24, 2022 08:42
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the update. Just a last remark about API doc.

* Set the read receipt on the event with provided eventId.
*/
suspend fun setReadReceipt(eventId: String)
suspend fun setReadReceipt(eventId: String, threadId: String)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add Kdoc for parameters, especially tell about the possible value THREAD_ID_MAIN for threadId.

@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

1.5% 1.5% Coverage
0.0% 0.0% Duplication

@fedrunov fedrunov merged commit 18bcc83 into develop Nov 25, 2022
@fedrunov fedrunov deleted the feature/nfe/threads_read_receipts branch November 25, 2022 08:49
fedrunov added a commit that referenced this pull request Nov 25, 2022
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