From 1b64ef0c91620a374072397f67585509fb04445d Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Fri, 18 Sep 2026 09:20:00 +0200 Subject: [PATCH 1/2] refactor(chat): move reminders and sharing to notes off RxJava Both used an Observable with a disposable that the chat throws away when it is paused, so backgrounding the app while the request was in flight dropped it silently. They now run in the view model scope like every other request that has to finish. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Andy Scherzinger --- .../com/nextcloud/talk/api/NcApiCoroutines.kt | 25 +++ .../data/network/ChatNetworkDataSource.kt | 20 +-- .../chat/data/network/RetrofitChatNetwork.kt | 65 +++----- .../talk/chat/viewmodels/ChatViewModel.kt | 153 ++++++------------ 4 files changed, 106 insertions(+), 157 deletions(-) diff --git a/app/src/main/java/com/nextcloud/talk/api/NcApiCoroutines.kt b/app/src/main/java/com/nextcloud/talk/api/NcApiCoroutines.kt index 0dd3f2bfa9..2c14a2169b 100644 --- a/app/src/main/java/com/nextcloud/talk/api/NcApiCoroutines.kt +++ b/app/src/main/java/com/nextcloud/talk/api/NcApiCoroutines.kt @@ -30,6 +30,7 @@ import com.nextcloud.talk.models.json.participants.TalkBanOverall import com.nextcloud.talk.models.json.passwordResult.PasswordResultOverall import com.nextcloud.talk.models.json.profile.ProfileOverall import com.nextcloud.talk.models.json.reactions.ReactionsOverall +import com.nextcloud.talk.models.json.reminder.ReminderOverall import com.nextcloud.talk.models.json.status.StatusOverall import com.nextcloud.talk.models.json.status.predefined.PredefinedStatusOverall import com.nextcloud.talk.models.json.tags.AssignConversationTagsRequestDto @@ -625,4 +626,28 @@ interface NcApiCoroutines { @GET suspend fun getUserProfile(@Header("Authorization") authorization: String, @Url url: String): UserProfileOverall + + @GET + suspend fun getReminder(@Header("Authorization") authorization: String, @Url url: String): ReminderOverall + + @DELETE + suspend fun deleteReminder(@Header("Authorization") authorization: String, @Url url: String): GenericOverall + + @FormUrlEncoded + @POST + suspend fun setReminder( + @Header("Authorization") authorization: String, + @Url url: String, + @Field("timestamp") timestamp: Int + ): ReminderOverall + + @FormUrlEncoded + @POST + suspend fun sendLocation( + @Header("Authorization") authorization: String, + @Url url: String, + @Field("objectType") objectType: String, + @Field("objectId") objectId: String, + @Field("metaData") metaData: String + ): GenericOverall } diff --git a/app/src/main/java/com/nextcloud/talk/chat/data/network/ChatNetworkDataSource.kt b/app/src/main/java/com/nextcloud/talk/chat/data/network/ChatNetworkDataSource.kt index 2b96e46943..c230d3c5fb 100644 --- a/app/src/main/java/com/nextcloud/talk/chat/data/network/ChatNetworkDataSource.kt +++ b/app/src/main/java/com/nextcloud/talk/chat/data/network/ChatNetworkDataSource.kt @@ -9,7 +9,7 @@ package com.nextcloud.talk.chat.data.network import com.nextcloud.talk.data.user.model.User import com.nextcloud.talk.models.domain.ConversationModel import com.nextcloud.talk.models.json.capabilities.SpreedCapabilityDto -import com.nextcloud.talk.models.json.chat.ChatMessageDto +import com.nextcloud.talk.models.json.chat.ChatMessageJson import com.nextcloud.talk.models.json.chat.ChatOverall import com.nextcloud.talk.models.json.chat.ChatOverallSingleMessage import com.nextcloud.talk.models.json.conversations.RoomOverall @@ -26,32 +26,32 @@ interface ChatNetworkDataSource { suspend fun getRoom(user: User, roomToken: String): ConversationModel fun getCapabilities(user: User, roomToken: String): Observable fun joinRoom(user: User, roomToken: String, roomPassword: String): Observable - fun setReminder( + suspend fun setReminder( user: User, roomToken: String, messageId: String, timeStamp: Int, chatApiVersion: Int - ): Observable + ): ReminderDto - fun getReminder(user: User, roomToken: String, messageId: String, apiVersion: Int): Observable - fun deleteReminder(user: User, roomToken: String, messageId: String, apiVersion: Int): Observable - fun shareToNotes( + suspend fun getReminder(user: User, roomToken: String, messageId: String, apiVersion: Int): ReminderDto + suspend fun deleteReminder(user: User, roomToken: String, messageId: String, apiVersion: Int): GenericOverall + suspend fun shareToNotes( credentials: String, url: String, message: String, displayName: String - ): Observable + ): ChatOverallSingleMessage suspend fun checkForNoteToSelf(credentials: String, url: String): RoomOverall - fun shareLocationToNotes( + suspend fun shareLocationToNotes( credentials: String, url: String, objectType: String, objectId: String, metadata: String - ): Observable + ): GenericOverall suspend fun leaveRoom(credentials: String, url: String): GenericOverall suspend fun sendChatMessage( @@ -84,7 +84,7 @@ interface ChatNetworkDataSource { messageId: String, limit: Int, threadId: Int? - ): List + ): List suspend fun getOpenGraph(credentials: String, baseUrl: String, extractedLinkToPreview: String): ReferenceDto? suspend fun unbindRoom(credentials: String, baseUrl: String, roomToken: String): GenericOverall diff --git a/app/src/main/java/com/nextcloud/talk/chat/data/network/RetrofitChatNetwork.kt b/app/src/main/java/com/nextcloud/talk/chat/data/network/RetrofitChatNetwork.kt index 9392602aaa..ace191336b 100644 --- a/app/src/main/java/com/nextcloud/talk/chat/data/network/RetrofitChatNetwork.kt +++ b/app/src/main/java/com/nextcloud/talk/chat/data/network/RetrofitChatNetwork.kt @@ -11,7 +11,7 @@ import com.nextcloud.talk.api.NcApiCoroutines import com.nextcloud.talk.data.user.model.User import com.nextcloud.talk.models.domain.ConversationModel import com.nextcloud.talk.models.json.capabilities.SpreedCapabilityDto -import com.nextcloud.talk.models.json.chat.ChatMessageDto +import com.nextcloud.talk.models.json.chat.ChatMessageJson import com.nextcloud.talk.models.json.chat.ChatOverall import com.nextcloud.talk.models.json.chat.ChatOverallSingleMessage import com.nextcloud.talk.models.json.conversations.RoomOverall @@ -59,81 +59,68 @@ class RetrofitChatNetwork(private val ncApi: NcApi, private val ncApiCoroutines: ).map { ConversationModel.mapToConversationModel(it.ocs?.data!!, user) } } - override fun setReminder( + override suspend fun setReminder( user: User, roomToken: String, messageId: String, timeStamp: Int, chatApiVersion: Int - ): Observable { - val credentials: String = ApiUtils.getCredentials(user.username, user.token)!! - return ncApi.setReminder( - credentials, + ): ReminderDto = + ncApiCoroutines.setReminder( + ApiUtils.getCredentials(user.username, user.token)!!, ApiUtils.getUrlForReminder(user, roomToken, messageId, chatApiVersion), timeStamp - ).map { - it.ocs!!.data - } - } + ).ocs!!.data!! - override fun getReminder( + override suspend fun getReminder( user: User, roomToken: String, messageId: String, chatApiVersion: Int - ): Observable { - val credentials: String = ApiUtils.getCredentials(user.username, user.token)!! - return ncApi.getReminder( - credentials, + ): ReminderDto = + ncApiCoroutines.getReminder( + ApiUtils.getCredentials(user.username, user.token)!!, ApiUtils.getUrlForReminder(user, roomToken, messageId, chatApiVersion) - ).map { - it.ocs!!.data - } - } + ).ocs!!.data!! - override fun deleteReminder( + override suspend fun deleteReminder( user: User, roomToken: String, messageId: String, chatApiVersion: Int - ): Observable { - val credentials: String = ApiUtils.getCredentials(user.username, user.token)!! - return ncApi.deleteReminder( - credentials, + ): GenericOverall = + ncApiCoroutines.deleteReminder( + ApiUtils.getCredentials(user.username, user.token)!!, ApiUtils.getUrlForReminder(user, roomToken, messageId, chatApiVersion) - ).map { - it - } - } + ) - override fun shareToNotes( + override suspend fun shareToNotes( credentials: String, url: String, message: String, displayName: String - ): Observable = - ncApi.sendChatMessage( + ): ChatOverallSingleMessage = + ncApiCoroutines.sendChatMessage( credentials, url, message, displayName, - null, + 0, false, - SendMessageUtils().generateReferenceId() - ).map { - it - } + SendMessageUtils().generateReferenceId(), + null + ) override suspend fun checkForNoteToSelf(credentials: String, url: String): RoomOverall = ncApiCoroutines.getNoteToSelfRoom(credentials, url) - override fun shareLocationToNotes( + override suspend fun shareLocationToNotes( credentials: String, url: String, objectType: String, objectId: String, metadata: String - ): Observable = ncApi.sendLocation(credentials, url, objectType, objectId, metadata).map { it } + ): GenericOverall = ncApiCoroutines.sendLocation(credentials, url, objectType, objectId, metadata) override suspend fun leaveRoom(credentials: String, url: String): GenericOverall = ncApiCoroutines.leaveRoom(credentials, url) @@ -209,7 +196,7 @@ class RetrofitChatNetwork(private val ncApi: NcApi, private val ncApiCoroutines: messageId: String, limit: Int, threadId: Int? - ): List { + ): List { val url = ApiUtils.getUrlForChatMessageContext(baseUrl, token, messageId) return ncApiCoroutines.getContextOfChatMessage(credentials, url, limit, threadId).ocs?.data ?: listOf() } diff --git a/app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt b/app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt index 5a3e643797..88d1d9528b 100644 --- a/app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt +++ b/app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt @@ -56,11 +56,10 @@ import com.nextcloud.talk.models.MessageDraft import com.nextcloud.talk.models.domain.ConversationModel import com.nextcloud.talk.models.domain.SearchMessageEntry import com.nextcloud.talk.models.json.capabilities.SpreedCapabilityDto -import com.nextcloud.talk.models.json.chat.ChatMessageDto +import com.nextcloud.talk.models.json.chat.ChatMessageJson import com.nextcloud.talk.models.json.chat.ChatOverallSingleMessage import com.nextcloud.talk.models.json.conversations.ConversationEnums import com.nextcloud.talk.models.json.conversations.RoomOverall -import com.nextcloud.talk.models.json.generic.GenericOverall import com.nextcloud.talk.models.json.opengraph.OpenGraphObjectDto import com.nextcloud.talk.models.json.reminder.ReminderDto import com.nextcloud.talk.models.json.threads.ThreadInfoDto @@ -479,7 +478,7 @@ class ChatViewModel @AssistedInject constructor( mediaPlayerManager.handleOnStop() } - fun onSignalingChatMessageReceived(chatMessages: List) { + fun onSignalingChatMessageReceived(chatMessages: List) { viewModelScope.launch { chatRepository.onSignalingChatMessageReceived(chatMessages) } @@ -1842,45 +1841,44 @@ class ChatViewModel @AssistedInject constructor( ?.subscribe(JoinRoomObserver()) } + @Suppress("Detekt.TooGenericExceptionCaught") fun setReminder(user: User, roomToken: String, messageId: String, timestamp: Int, chatApiVersion: Int) { - chatNetworkDataSource.setReminder(user, roomToken, messageId, timestamp, chatApiVersion) - .subscribeOn(Schedulers.io()) - ?.observeOn(AndroidSchedulers.mainThread()) - ?.subscribe(SetReminderObserver()) + viewModelScope.launch { + try { + chatNetworkDataSource.setReminder(user, roomToken, messageId, timestamp, chatApiVersion) + Log.d(TAG, "reminder set successfully") + } catch (e: Exception) { + Log.e(TAG, "Error when sending reminder", e) + } + } } + @Suppress("Detekt.TooGenericExceptionCaught") fun getReminder(user: User, roomToken: String, messageId: String, chatApiVersion: Int) { - chatNetworkDataSource.getReminder(user, roomToken, messageId, chatApiVersion) - .subscribeOn(Schedulers.io()) - ?.observeOn(AndroidSchedulers.mainThread()) - ?.subscribe(GetReminderObserver()) + viewModelScope.launch { + _getReminderExistState.value = try { + GetReminderExistState(chatNetworkDataSource.getReminder(user, roomToken, messageId, chatApiVersion)) + } catch (e: Exception) { + logger.d(TAG, "Error when getting reminder", e) + GetReminderStartState + } + } } fun overrideReminderState() { _getReminderExistState.value = GetReminderStateSet } + @Suppress("Detekt.TooGenericExceptionCaught") fun deleteReminder(user: User, roomToken: String, messageId: String, chatApiVersion: Int) { - chatNetworkDataSource.deleteReminder(user, roomToken, messageId, chatApiVersion) - .subscribeOn(Schedulers.io()) - ?.observeOn(AndroidSchedulers.mainThread()) - ?.subscribe(object : Observer { - override fun onSubscribe(d: Disposable) { - disposableSet.add(d) - } - - override fun onNext(genericOverall: GenericOverall) { - _getReminderExistState.value = GetReminderStartState - } - - override fun onError(e: Throwable) { - Log.d(TAG, "Error when deleting reminder", e) - } - - override fun onComplete() { - // unused atm - } - }) + viewModelScope.launch { + try { + chatNetworkDataSource.deleteReminder(user, roomToken, messageId, chatApiVersion) + _getReminderExistState.value = GetReminderStartState + } catch (e: Exception) { + Log.d(TAG, "Error when deleting reminder", e) + } + } } fun leaveRoom(credentials: String, url: String, functionToCallAfterLeave: (() -> Unit)?) { @@ -2139,27 +2137,15 @@ class ChatViewModel @AssistedInject constructor( ) } + @Suppress("Detekt.TooGenericExceptionCaught") fun shareToNotes(credentials: String, url: String, message: String, displayName: String) { - chatNetworkDataSource.shareToNotes(credentials, url, message, displayName) - .subscribeOn(Schedulers.io()) - ?.observeOn(AndroidSchedulers.mainThread()) - ?.subscribe(object : Observer { - override fun onSubscribe(d: Disposable) { - disposableSet.add(d) - } - - override fun onNext(genericOverall: ChatOverallSingleMessage) { - // unused atm - } - - override fun onError(e: Throwable) { - Log.d(TAG, "Error when sharing to notes $e") - } - - override fun onComplete() { - // unused atm - } - }) + viewModelScope.launch { + try { + chatNetworkDataSource.shareToNotes(credentials, url, message, displayName) + } catch (e: Exception) { + Log.d(TAG, "Error when sharing to notes", e) + } + } } suspend fun checkForNoteToSelf(credentials: String, baseUrl: String): ConversationModel? = @@ -2181,27 +2167,15 @@ class ChatViewModel @AssistedInject constructor( null } + @Suppress("Detekt.TooGenericExceptionCaught") fun shareLocationToNotes(credentials: String, url: String, objectType: String, objectId: String, metadata: String) { - chatNetworkDataSource.shareLocationToNotes(credentials, url, objectType, objectId, metadata) - .subscribeOn(Schedulers.io()) - ?.observeOn(AndroidSchedulers.mainThread()) - ?.subscribe(object : Observer { - override fun onSubscribe(d: Disposable) { - disposableSet.add(d) - } - - override fun onNext(genericOverall: GenericOverall) { - // unused atm - } - - override fun onError(e: Throwable) { - Log.e(TAG, "Error when sharing location to notes $e") - } - - override fun onComplete() { - // unused atm - } - }) + viewModelScope.launch { + try { + chatNetworkDataSource.shareLocationToNotes(credentials, url, objectType, objectId, metadata) + } catch (e: Exception) { + Log.e(TAG, "Error when sharing location to notes", e) + } + } } fun deleteReaction(roomToken: String, chatMessage: ChatMessage, emoji: String) { @@ -2489,43 +2463,6 @@ class ChatViewModel @AssistedInject constructor( } } - inner class SetReminderObserver : Observer { - override fun onSubscribe(d: Disposable) { - disposableSet.add(d) - } - - override fun onNext(reminder: ReminderDto) { - Log.d(TAG, "reminder set successfully") - } - - override fun onError(e: Throwable) { - Log.e(TAG, "Error when sending reminder, $e") - } - - override fun onComplete() { - // unused atm - } - } - - inner class GetReminderObserver : Observer { - override fun onSubscribe(d: Disposable) { - disposableSet.add(d) - } - - override fun onNext(reminder: ReminderDto) { - _getReminderExistState.value = GetReminderExistState(reminder) - } - - override fun onError(e: Throwable) { - Log.d(TAG, "Error when getting reminder $e") - _getReminderExistState.value = GetReminderStartState - } - - override fun onComplete() { - // unused atm - } - } - @Suppress("Detekt.TooGenericExceptionCaught") fun outOfOfficeStatusOfUser(credentials: String, baseUrl: String, userId: String) { viewModelScope.launch { From bf571a9b333a4ff8a1db2735977d18b15e807b1b Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Fri, 18 Sep 2026 09:20:10 +0200 Subject: [PATCH 2/2] fix(chat): tell the user when a reminder or a share to notes failed A reminder the server refused was only logged, and the chat says "Message sent" the moment a share to notes starts, before the answer arrives - so a share that failed was reported as a success. Both now show what happened. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Andy Scherzinger --- .../com/nextcloud/talk/api/NcApiCoroutines.kt | 4 +- .../com/nextcloud/talk/chat/ChatActivity.kt | 6 + .../data/network/ChatNetworkDataSource.kt | 13 +- .../chat/data/network/RetrofitChatNetwork.kt | 24 +-- .../talk/chat/viewmodels/ChatViewModel.kt | 143 ++++++++++++++---- app/src/main/res/values/strings.xml | 1 + .../talk/chat/viewmodels/ChatViewModelTest.kt | 32 ++++ 7 files changed, 174 insertions(+), 49 deletions(-) diff --git a/app/src/main/java/com/nextcloud/talk/api/NcApiCoroutines.kt b/app/src/main/java/com/nextcloud/talk/api/NcApiCoroutines.kt index 2c14a2169b..6a51f00de5 100644 --- a/app/src/main/java/com/nextcloud/talk/api/NcApiCoroutines.kt +++ b/app/src/main/java/com/nextcloud/talk/api/NcApiCoroutines.kt @@ -641,6 +641,7 @@ interface NcApiCoroutines { @Field("timestamp") timestamp: Int ): ReminderOverall + @Suppress("LongParameterList") @FormUrlEncoded @POST suspend fun sendLocation( @@ -648,6 +649,7 @@ interface NcApiCoroutines { @Url url: String, @Field("objectType") objectType: String, @Field("objectId") objectId: String, - @Field("metaData") metaData: String + @Field("metaData") metaData: String, + @Field("referenceId") referenceId: String ): GenericOverall } diff --git a/app/src/main/java/com/nextcloud/talk/chat/ChatActivity.kt b/app/src/main/java/com/nextcloud/talk/chat/ChatActivity.kt index 164916cc03..f908072178 100644 --- a/app/src/main/java/com/nextcloud/talk/chat/ChatActivity.kt +++ b/app/src/main/java/com/nextcloud/talk/chat/ChatActivity.kt @@ -1540,6 +1540,12 @@ class ChatActivity : } } + lifecycleScope.launch { + chatViewModel.actionFailures.collect { message -> + Snackbar.make(binding.root, message, Snackbar.LENGTH_LONG).show() + } + } + lifecycleScope.launch { chatViewModel.noMoreSearchResults.collect { val inSearchMode = chatViewModel.chatMode.value == ChatViewModel.ChatMode.SEARCH_MODE diff --git a/app/src/main/java/com/nextcloud/talk/chat/data/network/ChatNetworkDataSource.kt b/app/src/main/java/com/nextcloud/talk/chat/data/network/ChatNetworkDataSource.kt index c230d3c5fb..553f1e1aea 100644 --- a/app/src/main/java/com/nextcloud/talk/chat/data/network/ChatNetworkDataSource.kt +++ b/app/src/main/java/com/nextcloud/talk/chat/data/network/ChatNetworkDataSource.kt @@ -9,7 +9,7 @@ package com.nextcloud.talk.chat.data.network import com.nextcloud.talk.data.user.model.User import com.nextcloud.talk.models.domain.ConversationModel import com.nextcloud.talk.models.json.capabilities.SpreedCapabilityDto -import com.nextcloud.talk.models.json.chat.ChatMessageJson +import com.nextcloud.talk.models.json.chat.ChatMessageDto import com.nextcloud.talk.models.json.chat.ChatOverall import com.nextcloud.talk.models.json.chat.ChatOverallSingleMessage import com.nextcloud.talk.models.json.conversations.RoomOverall @@ -36,21 +36,26 @@ interface ChatNetworkDataSource { suspend fun getReminder(user: User, roomToken: String, messageId: String, apiVersion: Int): ReminderDto suspend fun deleteReminder(user: User, roomToken: String, messageId: String, apiVersion: Int): GenericOverall + + @Suppress("LongParameterList") suspend fun shareToNotes( credentials: String, url: String, message: String, - displayName: String + displayName: String, + referenceId: String ): ChatOverallSingleMessage suspend fun checkForNoteToSelf(credentials: String, url: String): RoomOverall + @Suppress("LongParameterList") suspend fun shareLocationToNotes( credentials: String, url: String, objectType: String, objectId: String, - metadata: String + metadata: String, + referenceId: String ): GenericOverall suspend fun leaveRoom(credentials: String, url: String): GenericOverall @@ -84,7 +89,7 @@ interface ChatNetworkDataSource { messageId: String, limit: Int, threadId: Int? - ): List + ): List suspend fun getOpenGraph(credentials: String, baseUrl: String, extractedLinkToPreview: String): ReferenceDto? suspend fun unbindRoom(credentials: String, baseUrl: String, roomToken: String): GenericOverall diff --git a/app/src/main/java/com/nextcloud/talk/chat/data/network/RetrofitChatNetwork.kt b/app/src/main/java/com/nextcloud/talk/chat/data/network/RetrofitChatNetwork.kt index ace191336b..606642563c 100644 --- a/app/src/main/java/com/nextcloud/talk/chat/data/network/RetrofitChatNetwork.kt +++ b/app/src/main/java/com/nextcloud/talk/chat/data/network/RetrofitChatNetwork.kt @@ -11,7 +11,7 @@ import com.nextcloud.talk.api.NcApiCoroutines import com.nextcloud.talk.data.user.model.User import com.nextcloud.talk.models.domain.ConversationModel import com.nextcloud.talk.models.json.capabilities.SpreedCapabilityDto -import com.nextcloud.talk.models.json.chat.ChatMessageJson +import com.nextcloud.talk.models.json.chat.ChatMessageDto import com.nextcloud.talk.models.json.chat.ChatOverall import com.nextcloud.talk.models.json.chat.ChatOverallSingleMessage import com.nextcloud.talk.models.json.conversations.RoomOverall @@ -21,7 +21,6 @@ import com.nextcloud.talk.models.json.reminder.ReminderDto import com.nextcloud.talk.models.json.upcomingEvents.UpcomingEventsOverall import com.nextcloud.talk.models.json.userAbsence.UserAbsenceOverall import com.nextcloud.talk.utils.ApiUtils -import com.nextcloud.talk.utils.message.SendMessageUtils import io.reactivex.Observable import retrofit2.Response @@ -98,18 +97,10 @@ class RetrofitChatNetwork(private val ncApi: NcApi, private val ncApiCoroutines: credentials: String, url: String, message: String, - displayName: String + displayName: String, + referenceId: String ): ChatOverallSingleMessage = - ncApiCoroutines.sendChatMessage( - credentials, - url, - message, - displayName, - 0, - false, - SendMessageUtils().generateReferenceId(), - null - ) + ncApiCoroutines.sendChatMessage(credentials, url, message, displayName, 0, false, referenceId, null) override suspend fun checkForNoteToSelf(credentials: String, url: String): RoomOverall = ncApiCoroutines.getNoteToSelfRoom(credentials, url) @@ -119,8 +110,9 @@ class RetrofitChatNetwork(private val ncApi: NcApi, private val ncApiCoroutines: url: String, objectType: String, objectId: String, - metadata: String - ): GenericOverall = ncApiCoroutines.sendLocation(credentials, url, objectType, objectId, metadata) + metadata: String, + referenceId: String + ): GenericOverall = ncApiCoroutines.sendLocation(credentials, url, objectType, objectId, metadata, referenceId) override suspend fun leaveRoom(credentials: String, url: String): GenericOverall = ncApiCoroutines.leaveRoom(credentials, url) @@ -196,7 +188,7 @@ class RetrofitChatNetwork(private val ncApi: NcApi, private val ncApiCoroutines: messageId: String, limit: Int, threadId: Int? - ): List { + ): List { val url = ApiUtils.getUrlForChatMessageContext(baseUrl, token, messageId) return ncApiCoroutines.getContextOfChatMessage(credentials, url, limit, threadId).ocs?.data ?: listOf() } diff --git a/app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt b/app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt index 88d1d9528b..276a406d3c 100644 --- a/app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt +++ b/app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt @@ -12,6 +12,8 @@ import android.net.Uri import android.os.Bundle import android.provider.OpenableColumns import android.util.Log +import androidx.annotation.StringRes +import androidx.annotation.VisibleForTesting import androidx.lifecycle.DefaultLifecycleObserver import androidx.lifecycle.LifecycleOwner import androidx.lifecycle.LiveData @@ -56,7 +58,7 @@ import com.nextcloud.talk.models.MessageDraft import com.nextcloud.talk.models.domain.ConversationModel import com.nextcloud.talk.models.domain.SearchMessageEntry import com.nextcloud.talk.models.json.capabilities.SpreedCapabilityDto -import com.nextcloud.talk.models.json.chat.ChatMessageJson +import com.nextcloud.talk.models.json.chat.ChatMessageDto import com.nextcloud.talk.models.json.chat.ChatOverallSingleMessage import com.nextcloud.talk.models.json.conversations.ConversationEnums import com.nextcloud.talk.models.json.conversations.RoomOverall @@ -81,6 +83,7 @@ import com.nextcloud.talk.utils.UserIdUtils import com.nextcloud.talk.utils.throttleLatest import com.nextcloud.talk.utils.bundle.BundleKeys import com.nextcloud.talk.utils.database.user.CurrentUserProvider +import com.nextcloud.talk.utils.isTransientFailure import com.nextcloud.talk.utils.message.SendMessageUtils import com.nextcloud.talk.utils.message.groupHashOf import com.nextcloud.talk.utils.preferences.AppPreferences @@ -478,7 +481,7 @@ class ChatViewModel @AssistedInject constructor( mediaPlayerManager.handleOnStop() } - fun onSignalingChatMessageReceived(chatMessages: List) { + fun onSignalingChatMessageReceived(chatMessages: List) { viewModelScope.launch { chatRepository.onSignalingChatMessageReceived(chatMessages) } @@ -636,6 +639,10 @@ class ChatViewModel @AssistedInject constructor( private val _reactionFailures = MutableSharedFlow(extraBufferCapacity = 1) val reactionFailures: SharedFlow = _reactionFailures + /** A reminder or a share to notes that the server refused, for the chat to report. */ + private val _actionFailures = MutableSharedFlow(extraBufferCapacity = 1) + val actionFailures: SharedFlow = _actionFailures + private val reactionLocks = mutableMapOf() @Volatile private var firstUnreadMessageId: Int? = null @@ -1841,15 +1848,9 @@ class ChatViewModel @AssistedInject constructor( ?.subscribe(JoinRoomObserver()) } - @Suppress("Detekt.TooGenericExceptionCaught") fun setReminder(user: User, roomToken: String, messageId: String, timestamp: Int, chatApiVersion: Int) { - viewModelScope.launch { - try { - chatNetworkDataSource.setReminder(user, roomToken, messageId, timestamp, chatApiVersion) - Log.d(TAG, "reminder set successfully") - } catch (e: Exception) { - Log.e(TAG, "Error when sending reminder", e) - } + requestWithFeedback("set the reminder", R.string.nc_common_error_sorry) { + chatNetworkDataSource.setReminder(user, roomToken, messageId, timestamp, chatApiVersion) } } @@ -1869,15 +1870,10 @@ class ChatViewModel @AssistedInject constructor( _getReminderExistState.value = GetReminderStateSet } - @Suppress("Detekt.TooGenericExceptionCaught") fun deleteReminder(user: User, roomToken: String, messageId: String, chatApiVersion: Int) { - viewModelScope.launch { - try { - chatNetworkDataSource.deleteReminder(user, roomToken, messageId, chatApiVersion) - _getReminderExistState.value = GetReminderStartState - } catch (e: Exception) { - Log.d(TAG, "Error when deleting reminder", e) - } + requestWithFeedback("delete the reminder", R.string.nc_common_error_sorry) { + chatNetworkDataSource.deleteReminder(user, roomToken, messageId, chatApiVersion) + _getReminderExistState.value = GetReminderStartState } } @@ -2137,14 +2133,9 @@ class ChatViewModel @AssistedInject constructor( ) } - @Suppress("Detekt.TooGenericExceptionCaught") fun shareToNotes(credentials: String, url: String, message: String, displayName: String) { - viewModelScope.launch { - try { - chatNetworkDataSource.shareToNotes(credentials, url, message, displayName) - } catch (e: Exception) { - Log.d(TAG, "Error when sharing to notes", e) - } + shareToNotes("share the message to notes", credentials, url) { referenceId -> + chatNetworkDataSource.shareToNotes(credentials, url, message, displayName, referenceId) } } @@ -2167,13 +2158,97 @@ class ChatViewModel @AssistedInject constructor( null } - @Suppress("Detekt.TooGenericExceptionCaught") fun shareLocationToNotes(credentials: String, url: String, objectType: String, objectId: String, metadata: String) { + shareToNotes("share the location to notes", credentials, url) { referenceId -> + chatNetworkDataSource.shareLocationToNotes(credentials, url, objectType, objectId, metadata, referenceId) + } + } + + /** + * Shares to the note to self and, when the share failed for a transient reason, asks the note to + * self whether the message arrived anyway before sending it a second time. The server stores the + * reference id of a message without ever looking at it again, so it deduplicates nothing: a blind + * second attempt after a timeout the server did accept posts the note twice. + */ + private fun shareToNotes( + description: String, + credentials: String, + notesUrl: String, + share: suspend (String) -> Unit + ) { + val referenceId = SendMessageUtils().generateReferenceId() + + viewModelScope.launch { + val failure = attemptShare(share, referenceId) ?: return@launch + + if (!isTransientFailure(failure)) { + reportShareFailure(description, failure) + return@launch + } + + if (!alreadyShared(credentials, notesUrl, referenceId)) { + attemptShare(share, referenceId)?.let { reportShareFailure(description, it) } + } + } + } + + /** Runs the share once and reports what went wrong, or null when it worked. */ + @Suppress("Detekt.TooGenericExceptionCaught") + private suspend fun attemptShare(share: suspend (String) -> Unit, referenceId: String): Exception? = + try { + share(referenceId) + null + } catch (e: CancellationException) { + throw e + } catch (e: Exception) { + e + } + + /** + * Whether the note to self already holds the message of this share. A lookup that cannot be + * answered counts as "it might", because posting a second copy is worse than not retrying. + */ + @Suppress("Detekt.TooGenericExceptionCaught") + private suspend fun alreadyShared(credentials: String, notesUrl: String, referenceId: String): Boolean = + try { + val fieldMap = hashMapOf( + "lookIntoFuture" to 0, + "includeLastKnown" to 1, + "setReadMarker" to 0, + "markNotificationsAsRead" to 0, + "timeout" to 0, + "limit" to SHARE_LOOKUP_LIMIT + ) + val recent = chatNetworkDataSource.pullChatMessages(credentials, notesUrl, fieldMap) + holdsShare(recent.body()?.ocs?.data, referenceId) + } catch (e: CancellationException) { + throw e + } catch (e: Exception) { + logger.w(TAG, "Could not check whether the share arrived, so it is not sent again", e) + true + } + + private fun reportShareFailure(description: String, throwable: Throwable) { + logger.e(TAG, "Failed to $description", throwable) + _actionFailures.tryEmit(R.string.nc_message_not_added_to_notes) + } + + /** + * Runs an action whose only visible outcome is whether it worked. The reminder dialog is already + * dismissed and the chat has already said the message was sent by the time the answer arrives, so + * a failure that is not reported here is never reported at all. + * + * Deliberately without a retry: sharing to notes posts a message with a reference id generated per + * call, so a second attempt after a timeout the server did accept would post it twice. + */ + @Suppress("Detekt.TooGenericExceptionCaught") + private fun requestWithFeedback(description: String, @StringRes failureMessage: Int, request: suspend () -> Unit) { viewModelScope.launch { try { - chatNetworkDataSource.shareLocationToNotes(credentials, url, objectType, objectId, metadata) + request() } catch (e: Exception) { - Log.e(TAG, "Error when sharing location to notes", e) + logger.e(TAG, "Failed to $description", e) + _actionFailures.tryEmit(failureMessage) } } } @@ -2630,6 +2705,18 @@ class ChatViewModel @AssistedInject constructor( companion object { private val TAG = ChatViewModel::class.java.simpleName + /** Enough of the note to self to cover what a share of ours could have landed behind. */ + private const val SHARE_LOOKUP_LIMIT = 20 + + /** + * Whether [messages] already hold the share that was sent with [referenceId]. Messages the + * server did not return - because the lookup failed or answered "not modified" - are read as + * "it may well be there", since sending a second copy is worse than not sending one at all. + */ + @VisibleForTesting + fun holdsShare(messages: List?, referenceId: String): Boolean = + messages?.any { it.referenceId == referenceId } ?: true + /** * Returns the read marker that makes [messageId] the first unread message: the id of the * message right before it, or 0 when it is the oldest message there is, which marks the whole diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 45bcaed003..b4f776e943 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -558,6 +558,7 @@ How to translate with transifex: Scheduled thread Reply to thread: %1$s Message added to notes + Could not add the message to notes Failed to send message Failed to send message: Add attachment diff --git a/app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelTest.kt b/app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelTest.kt index 39838d7eb2..e67dbec3da 100644 --- a/app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelTest.kt +++ b/app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelTest.kt @@ -10,8 +10,10 @@ package com.nextcloud.talk.chat.viewmodels import com.nextcloud.talk.chat.ui.model.ChatMessageUi import com.nextcloud.talk.chat.ui.model.MessageStatusIcon import com.nextcloud.talk.chat.ui.model.MessageTypeContent +import com.nextcloud.talk.models.json.chat.ChatMessageDto import com.nextcloud.talk.utils.message.SendMessageUtils import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse import org.junit.Assert.assertNull import org.junit.Assert.assertTrue import org.junit.Test @@ -62,6 +64,36 @@ class ChatViewModelTest { assertNull(ChatViewModel.findFirstUnreadMessageId(messages, lastReadMessage = 40)) } + // holdsShare(): whether a share to the note to self that failed transiently already arrived. + // The server stores a reference id without ever looking at it again, so it deduplicates nothing + // and a second attempt that is not needed posts the message twice. + + @Test + fun `a share the note to self already holds is not sent again`() { + val messages = listOf(messageWithReference("other-1"), messageWithReference("ours")) + + assertTrue(ChatViewModel.holdsShare(messages, "ours")) + } + + @Test + fun `a share that did not arrive is sent again`() { + val messages = listOf(messageWithReference("other-1"), messageWithReference("other-2")) + + assertFalse(ChatViewModel.holdsShare(messages, "ours")) + } + + @Test + fun `a lookup that could not be answered does not send the share again`() { + assertTrue(ChatViewModel.holdsShare(null, "ours")) + } + + @Test + fun `messages without a reference id of their own are no proof either way`() { + assertFalse(ChatViewModel.holdsShare(listOf(messageWithReference(null)), "ours")) + } + + private fun messageWithReference(referenceId: String?) = ChatMessageDto(referenceId = referenceId) + // combineFileShareGroups(): combining batch-uploaded file shares into grouped "album" bubbles. // Mirrors web's combineFileMessages.ts tests - see https://github.com/nextcloud/spreed/pull/19040