Skip to content

Commit 590f081

Browse files
AndyScherzingerAndyScherzinger
authored
Merge pull request #6498 from nextcloud/feat/noid/convoListUpdate
🔄️ Improve conversation list performance
2 parents 20ea8c8 + 3d3dafb commit 590f081

21 files changed

Lines changed: 1403 additions & 164 deletions

‎app/src/main/java/com/nextcloud/talk/chat/ChatActivity.kt‎

Lines changed: 3 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -2790,20 +2790,7 @@ class ChatActivity :
27902790
}
27912791

27922792
private fun updateRemoteLastReadMessageIfNeeded() {
2793-
if (this::spreedCapabilities.isInitialized) {
2794-
spreedCapabilities?.let {
2795-
val url = ApiUtils.getUrlForChatReadMarker(
2796-
ApiUtils.getChatApiVersion(it, intArrayOf(ApiUtils.API_V1)),
2797-
conversationUser.baseUrl!!,
2798-
roomToken
2799-
)
2800-
2801-
chatViewModel.updateRemoteLastReadMessageIfNeeded(
2802-
credentials = credentials!!,
2803-
url = url
2804-
)
2805-
}
2806-
}
2793+
chatViewModel.updateRemoteLastReadMessageIfNeeded()
28072794
}
28082795

28092796
private fun isActivityNotChangingConfigurations(): Boolean = !isChangingConfigurations
@@ -3487,15 +3474,7 @@ class ChatActivity :
34873474
}
34883475

34893476
private fun markAsRead(messageId: Int) {
3490-
chatViewModel.setChatReadMessage(
3491-
credentials!!,
3492-
ApiUtils.getUrlForChatReadMarker(
3493-
ApiUtils.getChatApiVersion(spreedCapabilities, intArrayOf(ApiUtils.API_V1)),
3494-
conversationUser?.baseUrl!!,
3495-
roomToken
3496-
),
3497-
messageId
3498-
)
3477+
chatViewModel.setChatReadMessage(messageId)
34993478
}
35003479

35013480
fun markAsUnread(chatMessage: ChatMessage) {
@@ -3510,15 +3489,7 @@ class ChatActivity :
35103489
} else {
35113490
0
35123491
}
3513-
chatViewModel.setChatReadMessage(
3514-
credentials!!,
3515-
ApiUtils.getUrlForChatReadMarker(
3516-
ApiUtils.getChatApiVersion(spreedCapabilities, intArrayOf(ApiUtils.API_V1)),
3517-
conversationUser.baseUrl!!,
3518-
roomToken
3519-
),
3520-
lastReadMessage
3521-
)
3492+
chatViewModel.setChatReadMessage(lastReadMessage)
35223493
}
35233494

35243495
fun copyMessage(message: ChatMessage?) {

‎app/src/main/java/com/nextcloud/talk/chat/data/ChatMessageRepository.kt‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,24 @@ interface ChatMessageRepository : LifecycleAwareManager {
7272
*/
7373
suspend fun fetchNewMessages(): Boolean
7474

75+
/**
76+
* Optimistically writes the user's read state into the local conversation entry, so the
77+
* conversation list reflects it immediately. Sending the read marker to the server is the
78+
* caller's concern; the next room list sync re-asserts the server state either way.
79+
*/
80+
suspend fun updateLocalReadState(lastReadMessage: Int)
81+
82+
/**
83+
* Registers [lastReadMessage] as a pending read marker synchronously, without going through
84+
* [updateLocalReadState]'s suspending database reads first. Call this before launching the
85+
* actual local write: leaving the chat can race a room list sync triggered by the conversation
86+
* list resuming at (almost) the same time, and that sync's response must find the marker
87+
* already pending to guard against a server state it computed before the marker was sent -
88+
* a gap of even a couple hundred milliseconds while registration waits on database reads is
89+
* enough for that race to lose.
90+
*/
91+
fun markPendingReadMarker(lastReadMessage: Int)
92+
7593
/**
7694
* Loads messages from local storage. If the messages are not found, then it
7795
* synchronizes the database with the server, before retrying exactly once. Only

‎app/src/main/java/com/nextcloud/talk/chat/data/network/ChatMessageSyncer.kt‎

Lines changed: 30 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import android.os.SystemClock
1212
import android.util.Log
1313
import com.nextcloud.talk.chat.data.model.ChatMessage
1414
import com.nextcloud.talk.chat.domain.ChatPullResult
15+
import com.nextcloud.talk.conversationlist.data.network.ConversationListUpdater
1516
import com.nextcloud.talk.data.database.dao.ChatBlocksDao
1617
import com.nextcloud.talk.data.database.dao.ChatMessagesDao
1718
import com.nextcloud.talk.data.database.mappers.asEntity
@@ -43,12 +44,13 @@ import javax.inject.Inject
4344
* per-room coalescing bookkeeping of [catchUpRoom], which collapses bursts of catch-up requests
4445
* (e.g. one push notification per incoming message) into few actual fetches.
4546
*/
46-
@Suppress("TooManyFunctions")
47+
@Suppress("TooManyFunctions", "LargeClass")
4748
class ChatMessageSyncer @Inject constructor(
4849
private val chatDao: ChatMessagesDao,
4950
private val chatBlocksDao: ChatBlocksDao,
5051
private val network: ChatNetworkDataSource,
51-
private val networkMonitor: NetworkMonitor
52+
private val networkMonitor: NetworkMonitor,
53+
private val conversationListUpdater: ConversationListUpdater
5254
) {
5355

5456
/**
@@ -91,13 +93,18 @@ class ChatMessageSyncer @Inject constructor(
9193
* [syncFailed] is true when the sync ended in a transient error (offline, failed request), so
9294
* callers like a background worker can retry later. It stays false for skips that retrying
9395
* would not change, e.g. a missing server capability.
96+
*
97+
* [newestPersistedMessage] is the newest persisted message that qualifies as a conversation's
98+
* last message (system messages that never change a conversation's preview are skipped). It
99+
* feeds the conversation list update after a background catch-up.
94100
*/
95101
data class SyncOutcome(
96102
val persistedNewMessages: Boolean,
97103
val newestPersistedMessageId: Long?,
98104
val oldestPersistedMessageId: Long? = null,
99105
val persistedMessageCount: Int = 0,
100-
val syncFailed: Boolean = false
106+
val syncFailed: Boolean = false,
107+
val newestPersistedMessage: ChatMessageJson? = null
101108
)
102109

103110
/**
@@ -357,6 +364,8 @@ class ChatMessageSyncer @Inject constructor(
357364
Log.d(TAG, "Background catch-up for room ${target.roomToken}: no new messages")
358365
}
359366

367+
conversationListUpdater.updateConversationFromCatchUp(target, outcome)
368+
360369
return outcome
361370
}
362371

@@ -467,7 +476,8 @@ class ChatMessageSyncer @Inject constructor(
467476
newestPersistedMessageId = backlogOutcome.newestPersistedMessageId ?: nextAnchor,
468477
oldestPersistedMessageId = anchorOutcome.oldestPersistedMessageId,
469478
persistedMessageCount = anchorOutcome.persistedMessageCount + backlogOutcome.persistedMessageCount,
470-
syncFailed = backlogOutcome.syncFailed
479+
syncFailed = backlogOutcome.syncFailed,
480+
newestPersistedMessage = backlogOutcome.newestPersistedMessage ?: anchorOutcome.newestPersistedMessage
471481
)
472482
}
473483

@@ -497,6 +507,7 @@ class ChatMessageSyncer @Inject constructor(
497507
var totalCount = 0
498508
var oldestPersisted: Long? = null
499509
var newestPersisted: Long? = null
510+
var newestPersistedMessage: ChatMessageJson? = null
500511

501512
repeat(MAX_BACKLOG_ROUNDS) {
502513
val fieldMap = buildFieldMap(
@@ -515,6 +526,7 @@ class ChatMessageSyncer @Inject constructor(
515526
totalCount += roundOutcome.persistedMessageCount
516527
oldestPersisted = oldestPersisted ?: roundOutcome.oldestPersistedMessageId
517528
newestPersisted = roundOutcome.newestPersistedMessageId ?: newestPersisted
529+
newestPersistedMessage = roundOutcome.newestPersistedMessage ?: newestPersistedMessage
518530
}
519531

520532
val caughtUp = !roundOutcome.persistedNewMessages || roundOutcome.persistedMessageCount < limit
@@ -525,7 +537,8 @@ class ChatMessageSyncer @Inject constructor(
525537
newestPersistedMessageId = newestPersisted,
526538
oldestPersistedMessageId = oldestPersisted,
527539
persistedMessageCount = totalCount,
528-
syncFailed = roundOutcome.syncFailed
540+
syncFailed = roundOutcome.syncFailed,
541+
newestPersistedMessage = newestPersistedMessage
529542
)
530543
}
531544
anchor = nextAnchor
@@ -556,7 +569,8 @@ class ChatMessageSyncer @Inject constructor(
556569
newestPersistedMessageId = fallbackOutcome.newestPersistedMessageId,
557570
oldestPersistedMessageId = fallbackOutcome.oldestPersistedMessageId,
558571
persistedMessageCount = fallbackOutcome.persistedMessageCount,
559-
syncFailed = fallbackOutcome.syncFailed
572+
syncFailed = fallbackOutcome.syncFailed,
573+
newestPersistedMessage = fallbackOutcome.newestPersistedMessage ?: newestPersistedMessage
560574
)
561575
}
562576

@@ -681,11 +695,19 @@ class ChatMessageSyncer @Inject constructor(
681695
events
682696
)
683697
persistedMessages.maxOfOrNull { it.id }?.let { recordHttpSyncedMessageId(target, it) }
698+
val newestPersistedMessage = if (persistedMessages.isNotEmpty()) {
699+
result.messages
700+
.filter { it.systemMessageType !in ConversationListUpdater.LAST_MESSAGE_HIDDEN_SYSTEM_TYPES }
701+
.maxByOrNull { it.id }
702+
} else {
703+
null
704+
}
684705
SyncOutcome(
685706
persistedNewMessages = persistedMessages.isNotEmpty(),
686707
newestPersistedMessageId = persistedMessages.maxOfOrNull { it.id },
687708
oldestPersistedMessageId = persistedMessages.minOfOrNull { it.id },
688-
persistedMessageCount = persistedMessages.size
709+
persistedMessageCount = persistedMessages.size,
710+
newestPersistedMessage = newestPersistedMessage
689711
)
690712
} else {
691713
Log.d(TAG, "No new messages to update")
@@ -967,6 +989,7 @@ class ChatMessageSyncer @Inject constructor(
967989
SyncOutcome(persistedNewMessages = false, newestPersistedMessageId = null, syncFailed = true)
968990

969991
private const val DEFAULT_MESSAGES_LIMIT = 100
992+
970993
private const val MILLIS_PER_SECOND = 1000L
971994
private const val ROOM_REFRESH_MAX_AGE_MILLIS = 3 * 60 * 60 * 1000L // 3 hours
972995
private const val CATCH_UP_COOLDOWN_MILLIS = 5_000L

‎app/src/main/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepository.kt‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import android.os.Bundle
1212
import android.util.Log
1313
import com.nextcloud.talk.chat.data.ChatMessageRepository
1414
import com.nextcloud.talk.chat.data.model.ChatMessage
15+
import com.nextcloud.talk.conversationlist.data.network.ConversationListUpdater
1516
import com.nextcloud.talk.data.database.dao.ChatBlocksDao
1617
import com.nextcloud.talk.data.database.dao.ChatMessagesDao
1718
import com.nextcloud.talk.data.database.mappers.asEntity
@@ -56,7 +57,8 @@ class OfflineFirstChatRepository @Inject constructor(
5657
private val chatBlocksDao: ChatBlocksDao,
5758
private val network: ChatNetworkDataSource,
5859
private val networkMonitor: NetworkMonitor,
59-
private val syncer: ChatMessageSyncer
60+
private val syncer: ChatMessageSyncer,
61+
private val conversationListUpdater: ConversationListUpdater
6062
) : ChatMessageRepository {
6163

6264
lateinit var currentUser: User
@@ -363,6 +365,14 @@ class OfflineFirstChatRepository @Inject constructor(
363365
return outcome.persistedNewMessages
364366
}
365367

368+
override suspend fun updateLocalReadState(lastReadMessage: Int) {
369+
conversationListUpdater.updateLocalReadState(syncTarget, lastReadMessage)
370+
}
371+
372+
override fun markPendingReadMarker(lastReadMessage: Int) {
373+
conversationListUpdater.markPendingReadMarker(syncTarget.internalConversationId, lastReadMessage)
374+
}
375+
366376
override suspend fun loadMoreMessages(
367377
anchorMessageId: Long,
368378
direction: ChatMessageRepository.LoadMoreDirection,

‎app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt‎

Lines changed: 34 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ import com.nextcloud.talk.data.database.mappers.toDomainModel
4040
import com.nextcloud.talk.data.database.model.ChatMessageEntity
4141
import com.nextcloud.talk.data.user.model.User
4242
import com.nextcloud.talk.extensions.toIntOrZero
43+
import com.nextcloud.talk.jobs.ReadMarkerSyncWorker
4344
import com.nextcloud.talk.jobs.ShareOperationWorker
4445
import com.nextcloud.talk.jobs.UploadAndShareFilesWorker
4546
import com.nextcloud.talk.logger.Logger
@@ -1799,45 +1800,48 @@ class ChatViewModel @AssistedInject constructor(
17991800
/**
18001801
* Please use with caution to not spam the server
18011802
*/
1802-
fun updateRemoteLastReadMessageIfNeeded(credentials: String, url: String) {
1803+
fun updateRemoteLastReadMessageIfNeeded() {
18031804
Log.d(TAG, "updateRemoteLastReadMessageIfNeeded, localLastReadMessage: $localLastReadMessage")
1804-
Log.d(
1805-
TAG,
1806-
"updateRemoteLastReadMessageIfNeeded, _uiState.value.conversation!!.lastReadMessage: " +
1807-
_uiState.value.conversation!!.lastReadMessage
1808-
)
1805+
val conversationLastReadMessage = _uiState.value.conversation?.lastReadMessage ?: return
1806+
Log.d(TAG, "updateRemoteLastReadMessageIfNeeded, conversation.lastReadMessage: $conversationLastReadMessage")
18091807

1810-
if (localLastReadMessage > _uiState.value.conversation!!.lastReadMessage) {
1808+
if (localLastReadMessage > conversationLastReadMessage) {
18111809
Log.d(TAG, "updateRemoteLastReadMessageIfNeeded, setChatReadMessage...")
18121810

1813-
setChatReadMessage(credentials, url, localLastReadMessage)
1811+
setChatReadMessage(localLastReadMessage)
18141812
}
18151813
}
18161814

18171815
/**
1818-
* Please use with caution to not spam the server
1816+
* Marks the chat as read up to [lastReadMessage]: the local conversation entry is updated
1817+
* immediately (optimistic, so the conversation list reflects it right away) while sending the
1818+
* marker to the server is delegated to [ReadMarkerSyncWorker], which retries transient
1819+
* failures with backoff. The server stays the authority — every room list sync re-asserts its
1820+
* read state, so a marker that ultimately could not be sent falls back to the server state
1821+
* instead of leaving the client diverged.
1822+
*
1823+
* [markPendingReadMarker] runs synchronously, before anything is launched, so the marker is
1824+
* armed the instant this returns: leaving the chat commonly races the conversation list's own
1825+
* resume-triggered sync, and that race is only guarded correctly if the pending marker already
1826+
* exists by the time the sync's response is merged. [updateLocalReadState] itself does two
1827+
* suspending database reads before writing - registering the marker only there, inside a
1828+
* launched coroutine, previously left a real gap (observed at ~250ms, more under main-thread
1829+
* contention) during which such a sync could see no pending marker yet and apply unguarded.
18191830
*/
1820-
fun setChatReadMessage(credentials: String, url: String, lastReadMessage: Int) {
1821-
chatNetworkDataSource.setChatReadMarker(credentials, url, lastReadMessage)
1822-
.subscribeOn(Schedulers.io())
1823-
.observeOn(AndroidSchedulers.mainThread())
1824-
.subscribe(object : Observer<GenericOverall> {
1825-
override fun onSubscribe(d: Disposable) {
1826-
disposableSet.add(d)
1827-
}
1828-
1829-
override fun onError(e: Throwable) {
1830-
Log.e(TAG, e.message, e)
1831-
}
1832-
1833-
override fun onComplete() {
1834-
// unused atm
1835-
}
1836-
1837-
override fun onNext(t: GenericOverall) {
1838-
// unused atm
1839-
}
1840-
})
1831+
fun setChatReadMessage(lastReadMessage: Int) {
1832+
if (!this::currentUser.isInitialized) {
1833+
return
1834+
}
1835+
chatRepository.markPendingReadMarker(lastReadMessage)
1836+
viewModelScope.launch {
1837+
chatRepository.updateLocalReadState(lastReadMessage)
1838+
}
1839+
ReadMarkerSyncWorker.enqueue(
1840+
context = NextcloudTalkApplication.sharedApplication!!.applicationContext,
1841+
userId = currentUser.id!!,
1842+
roomToken = chatRoomToken,
1843+
lastReadMessage = lastReadMessage
1844+
)
18411845
}
18421846

18431847
fun shareToNotes(credentials: String, url: String, message: String, displayName: String) {

0 commit comments

Comments
 (0)