Fetch messages in background #4475

rapterjet2004 · 2024-11-22T16:16:49Z

fixes Fetch messages in background when unread messages are detected in conversation list #4381

Fetches messages and saves them when refreshing the conversation list. Only refreshes what is needed, to save battery.

🏁 Checklist

⛑️ Tests (unit and/or integration) are included or not needed
🔖 Capability is checked or not needed
🔙 Backport requests are created or not needed: /backport to stable-xx.x
📅 Milestone is set
🌸 PR title is meaningful (if it should be in the changelog: is it meaningful to users?)

mahibi · 2024-11-26T21:04:31Z

not to be merged for 20.1.0, might review only afterwards

mahibi

something is wrong with the chatBlocks, they don't get merged correctly (i guess because "lastKnown" in the fieldMap is not set up correctly). In the UI this leads to the fact that the newest loaded message is shown first, and then the older messages appear. With correctly merged chatBlocks this should not happen.

There is a high danger to introduce serious bugs when something is wrong with the chatBlocks, so i might be quite nitpicky with this PR ;)

mahibi · 2024-12-03T11:05:37Z

app/src/main/java/com/nextcloud/talk/conversationlist/ConversationsListActivity.kt

@@ -446,6 +449,34 @@ class ConversationsListActivity :
        updateFilterConversationButtonColor()
    }

+    private fun List<ConversationModel>.refreshMessages() {


As we already have a ConversationListViewModel we should use this, so as the code is just about data handling it should not be in the Activity.

mahibi · 2024-12-03T11:41:32Z

fyi @rapterjet2004 i rebased on master...

mahibi · 2025-01-20T15:00:35Z

app/src/main/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModel.kt

+
+    private suspend fun updateRoomMessage(model: ConversationModel, limit: Int, credentials: String, baseUrl: String) {
+        val urlForChatting = ApiUtils.getUrlForChat(1, baseUrl, model.token) // FIXME v1?
+        chatRepository.setData(model, credentials, urlForChatting)


i don't have a better idea yet,
but using the same chatRepository and only setData etc in a loop feels risky.

Some question that come to my mind:
Is the way we use dependency injection fine with this loop approach?
Is async / synchronized handling correct?

Dependency Injection is a bit confusing, as even though the provider is annotated as a @singleton, it's not an actual Singleton. Each time we inject, we create a separate instance.

Async handling should be fine, the suspend function runs in ViewModelScope, so it shouldn't be attached the activity lifecycle.

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

mahibi · 2025-01-22T13:44:38Z

will do some testing tomorrow, then it should be fine to merge

mahibi · 2025-01-23T13:08:23Z

sorry Julius i won't merge this PR.

There were just too many things wrong when i tested (messages that i expected to be there were missing / Messages were loaded from when conversation was created / ...)

Also the calculation with the previous/current/result lists doesn't make sense to me.
Why is only the difference to the last state counted? If the unread messages are missing, we want to have them all.

Tbh i would like to close the PR for now and we should have a fresh look at it later.

mahibi

see comment in pr

rapterjet2004 · 2025-02-14T19:45:40Z

Rewrote and simplified the updateRoomMessages, couldn't replicate the other errors. Can you point me in the right direction?

mahibi · 2025-03-06T13:30:29Z

Rewrote and simplified the updateRoomMessages, couldn't replicate the other errors. Can you point me in the right direction?

hope to find time for this one soon! can you rebase and solve the merge conflicts?

Signed-off-by: rapterjet2004 <[email protected]>

github-actions · 2025-03-06T21:26:21Z

Codacy

Lint

Type	master	PR
Warnings	100	100
Errors	9	9

SpotBugs

Category	Base	New
Bad practice	6	6
Correctness	222	222
Dodgy code	71	71
Internationalization	3	3
Malicious code vulnerability	3	3
Performance	4	4
Security	1	1
Total	310	310

github-actions · 2025-03-06T21:30:59Z

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/4475-talk.apk

To test this change/fix you can simply download above APK file and install and test it in parallel to your existing Nextcloud Talk app.

mahibi

The current state doesn't work for me because credentials are not set:

2025-03-19 15:26:34.201 17596-17874 OfflineFir...Repository com.nextcloud.talk2                  E  Something went wrong when pulling chat messages (attempt: 4)
                                                                                                    kotlin.UninitializedPropertyAccessException: lateinit property credentials has not been initialized
                                                                                                    	at com.nextcloud.talk.chat.data.network.OfflineFirstChatRepository.getMessagesFromServer(OfflineFirstChatRepository.kt:512)
                                                                                                    	at com.nextcloud.talk.chat.data.network.OfflineFirstChatRepository.sync(OfflineFirstChatRepository.kt:579)
                                                                                                    	at com.nextcloud.talk.chat.data.network.OfflineFirstChatRepository.updateRoomMessages(OfflineFirstChatRepository.kt:313)

mahibi · 2025-03-19T13:44:11Z

app/src/main/java/com/nextcloud/talk/conversationlist/ConversationsListActivity.kt

@@ -2077,7 +2080,8 @@ class ConversationsListActivity :

    private fun onMessageSearchError(throwable: Throwable) {
        handleHttpExceptions(throwable)
-        binding.swipeRefreshLayoutView?.isRefreshing = false
+        binding.swipeRefreshLayoutView.isRefreshing = false
+        showErrorDialog()


showErrorDialog is already called in handleHttpExceptions

mahibi · 2025-03-19T14:02:34Z

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

+
+        Log.d(TAG, "---- updateRoomMessages ------------ with lastKnown: $lastKnown")
+        val fieldMap = getFieldMap(
+            lookIntoFuture = true,


lookIntoFuture = true
is wrong. It would trigger the long polling which is not wanted in this case.

mahibi · 2025-03-19T14:31:36Z

app/src/main/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModel.kt

+            }
+        }
+    }
+


Could be simplified to this?:

fun updateRoomMessages(list: List<ConversationModel>) { viewModelScope.launch(Dispatchers.IO) { list.filter { it.unreadMessages > 0 } .forEach { chatRepository.updateRoomMessages(it.internalId, it.unreadMessages) } } }

rapterjet2004 self-assigned this Nov 22, 2024

rapterjet2004 marked this pull request as ready for review November 25, 2024 15:18

rapterjet2004 requested a review from mahibi November 25, 2024 15:50

mahibi requested changes Dec 3, 2024

View reviewed changes

mahibi added this to the 21.0.0 milestone Dec 3, 2024

mahibi force-pushed the issue-4381-fetch-messages-in-background branch 2 times, most recently from 9bf582b to 7dfc82c Compare December 3, 2024 11:40

rapterjet2004 force-pushed the issue-4381-fetch-messages-in-background branch from 7dfc82c to 9b756d4 Compare December 10, 2024 17:16

rapterjet2004 requested a review from mahibi December 12, 2024 15:21

mahibi force-pushed the issue-4381-fetch-messages-in-background branch from 6ed9910 to 096c0aa Compare January 20, 2025 12:51

mahibi requested changes Jan 20, 2025

View reviewed changes

rapterjet2004 requested a review from mahibi January 21, 2025 17:51

mahibi force-pushed the issue-4381-fetch-messages-in-background branch from 48dc925 to 3cba747 Compare January 23, 2025 13:07

mahibi requested changes Jan 23, 2025

View reviewed changes

mahibi removed this from the 21.0.0 milestone Jan 23, 2025

mahibi added the don't merge label Jan 23, 2025

rapterjet2004 force-pushed the issue-4381-fetch-messages-in-background branch from 3cba747 to e7b456a Compare February 14, 2025 18:33

rapterjet2004 requested a review from mahibi February 14, 2025 19:42

Background fetch

7035fb0

Signed-off-by: rapterjet2004 <[email protected]>

rapterjet2004 force-pushed the issue-4381-fetch-messages-in-background branch from 4bf21fc to 7035fb0 Compare March 6, 2025 21:05

refactoring

e8ac538

Signed-off-by: rapterjet2004 <[email protected]>

rapterjet2004 removed the don't merge label Mar 13, 2025

rapterjet2004 added the 3. to review Waiting for reviews label Mar 13, 2025

mahibi requested changes Mar 19, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fetch messages in background #4475

Fetch messages in background #4475

rapterjet2004 commented Nov 22, 2024 •

edited

Loading

mahibi commented Nov 26, 2024

mahibi left a comment

mahibi Dec 3, 2024

mahibi commented Dec 3, 2024

mahibi Jan 20, 2025

rapterjet2004 Jan 21, 2025

mahibi commented Jan 22, 2025

mahibi commented Jan 23, 2025

mahibi left a comment

rapterjet2004 commented Feb 14, 2025

mahibi commented Mar 6, 2025

github-actions bot commented Mar 6, 2025

github-actions bot commented Mar 6, 2025

mahibi left a comment

mahibi Mar 19, 2025

mahibi Mar 19, 2025

mahibi Mar 19, 2025 •

edited

Loading

Fetch messages in background #4475

Are you sure you want to change the base?

Fetch messages in background #4475

Conversation

rapterjet2004 commented Nov 22, 2024 • edited Loading

🏁 Checklist

mahibi commented Nov 26, 2024

mahibi left a comment

Choose a reason for hiding this comment

mahibi Dec 3, 2024

Choose a reason for hiding this comment

mahibi commented Dec 3, 2024

mahibi Jan 20, 2025

Choose a reason for hiding this comment

rapterjet2004 Jan 21, 2025

Choose a reason for hiding this comment

mahibi commented Jan 22, 2025

mahibi commented Jan 23, 2025

mahibi left a comment

Choose a reason for hiding this comment

rapterjet2004 commented Feb 14, 2025

mahibi commented Mar 6, 2025

github-actions bot commented Mar 6, 2025

Codacy

Lint

SpotBugs

github-actions bot commented Mar 6, 2025

mahibi left a comment

Choose a reason for hiding this comment

mahibi Mar 19, 2025

Choose a reason for hiding this comment

mahibi Mar 19, 2025

Choose a reason for hiding this comment

mahibi Mar 19, 2025 • edited Loading

Choose a reason for hiding this comment

rapterjet2004 commented Nov 22, 2024 •

edited

Loading

mahibi Mar 19, 2025 •

edited

Loading