From 180dd03d1fb6b6058cc245c068f02416b1945e0c Mon Sep 17 00:00:00 2001 From: Marcel Hibbe Date: Mon, 28 Oct 2024 13:41:49 +0100 Subject: [PATCH] fix messages not shown + improve unread marker behavior this commit will avoid to fail to show messages in adapter. This was caused by the usage of messagesListAdapter.deleteById("-1"); in UnreadNoticeMessageViewHolder. The bug seems to exist in the past already but was never reported (Sometimes, when receiving a lot of messages it could happen that some message in between is not shown in UI). However with recent changes after release 20.0.2 the bug appeared more often. The root cause was not analyzed, but the handling was modified in general as the unread marker behavior was never really good. By not using deleteById but replace it with new unread marker logic, the bug of disappearing messages is solved and the unread messages marker behavior is improved. Signed-off-by: Marcel Hibbe --- .../UnreadNoticeMessageViewHolder.java | 2 +- .../com/nextcloud/talk/chat/ChatActivity.kt | 77 +++++++++---------- .../talk/chat/data/ChatMessageRepository.kt | 3 +- .../network/OfflineFirstChatRepository.kt | 32 ++++++-- 4 files changed, 66 insertions(+), 48 deletions(-) diff --git a/app/src/main/java/com/nextcloud/talk/adapters/messages/UnreadNoticeMessageViewHolder.java b/app/src/main/java/com/nextcloud/talk/adapters/messages/UnreadNoticeMessageViewHolder.java index 6615b4f0d9..6ae3d5003f 100644 --- a/app/src/main/java/com/nextcloud/talk/adapters/messages/UnreadNoticeMessageViewHolder.java +++ b/app/src/main/java/com/nextcloud/talk/adapters/messages/UnreadNoticeMessageViewHolder.java @@ -24,7 +24,7 @@ public UnreadNoticeMessageViewHolder(View itemView, Object payload) { @Override public void viewDetached() { - messagesListAdapter.deleteById("-1"); +// messagesListAdapter.deleteById("-1"); } @Override 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 df0adc0ecc..552a14c8de 100644 --- a/app/src/main/java/com/nextcloud/talk/chat/ChatActivity.kt +++ b/app/src/main/java/com/nextcloud/talk/chat/ChatActivity.kt @@ -756,6 +756,8 @@ class ChatActivity : is MessageInputViewModel.SendChatMessageSuccessState -> { myFirstMessage = state.message + removeUnreadMessagesMarker() + if (binding.unreadMessagesPopup.isShown == true) { binding.unreadMessagesPopup.hide() } @@ -853,9 +855,10 @@ class ChatActivity : this.lifecycleScope.launch { chatViewModel.getMessageFlow - .onEach { pair -> - val lookIntoFuture = pair.first - var chatMessageList = pair.second + .onEach { triple -> + val lookIntoFuture = triple.first + val setUnreadMessagesMarker = triple.second + var chatMessageList = triple.third chatMessageList = handleSystemMessages(chatMessageList) @@ -871,7 +874,8 @@ class ChatActivity : } if (lookIntoFuture) { - processMessagesFromTheFuture(chatMessageList) + Log.d(TAG, "chatMessageList.size in getMessageFlow:" + chatMessageList.size) + processMessagesFromTheFuture(chatMessageList, setUnreadMessagesMarker) } else { processMessagesNotFromTheFuture(chatMessageList) collapseSystemMessages() @@ -1011,6 +1015,13 @@ class ChatActivity : } } + private fun removeUnreadMessagesMarker() { + val index = adapter?.getMessagePositionById("-1") + if (index != null && index != -1) { + adapter?.items?.removeAt(index) + } + } + @Suppress("Detekt.TooGenericExceptionCaught") override fun onResume() { super.onResume() @@ -2653,13 +2664,22 @@ class ChatActivity : } } - private fun processMessagesFromTheFuture(chatMessageList: List) { - val newMessagesAvailable = (adapter?.itemCount ?: 0) > 0 && chatMessageList.isNotEmpty() - val insertNewMessagesNotice = shouldInsertNewMessagesNotice(newMessagesAvailable, chatMessageList) - val scrollToEndOnUpdate = isScrolledToBottom() + private fun processMessagesFromTheFuture(chatMessageList: List, setUnreadMessagesMarker: Boolean) { + binding.scrollDownButton.visibility = View.GONE + + val scrollToBottom: Boolean - if (insertNewMessagesNotice) { - updateUnreadMessageInfos(chatMessageList, scrollToEndOnUpdate) + if (setUnreadMessagesMarker) { + scrollToBottom = false + setUnreadMessageMarker(chatMessageList) + } else { + if (isScrolledToBottom()) { + scrollToBottom = true + } else { + scrollToBottom = false + binding.unreadMessagesPopup.show() + // here we have the problem that the chat jumps for every update + } } for (chatMessage in chatMessageList) { @@ -2674,44 +2694,21 @@ class ChatActivity : (currentConversation?.type == ConversationEnums.ConversationType.ROOM_TYPE_ONE_TO_ONE_CALL) chatMessage.isFormerOneToOneConversation = (currentConversation?.type == ConversationEnums.ConversationType.FORMER_ONE_TO_ONE) - it.addToStart(chatMessage, scrollToEndOnUpdate) + Log.d(TAG, "chatMessage to add:" + chatMessage.message) + it.addToStart(chatMessage, scrollToBottom) } } - - if (insertNewMessagesNotice && scrollToEndOnUpdate && adapter != null) { - scrollToFirstUnreadMessage() - } } private fun isScrolledToBottom() = layoutManager?.findFirstVisibleItemPosition() == 0 - private fun shouldInsertNewMessagesNotice(newMessagesAvailable: Boolean, chatMessageList: List) = - if (newMessagesAvailable) { - chatMessageList.any { it.actorId != conversationUser!!.userId } - } else { - false - } - - private fun updateUnreadMessageInfos(chatMessageList: List, scrollToEndOnUpdate: Boolean) { + private fun setUnreadMessageMarker(chatMessageList: List) { val unreadChatMessage = ChatMessage() unreadChatMessage.jsonMessageId = -1 unreadChatMessage.actorId = "-1" unreadChatMessage.timestamp = chatMessageList[0].timestamp unreadChatMessage.message = context.getString(R.string.nc_new_messages) adapter?.addToStart(unreadChatMessage, false) - - if (scrollToEndOnUpdate) { - binding.scrollDownButton.visibility = View.GONE - newMessagesCount = 0 - } else { - if (binding.unreadMessagesPopup.isShown) { - newMessagesCount++ - } else { - newMessagesCount = 1 - binding.scrollDownButton.visibility = View.GONE - binding.unreadMessagesPopup.show() - } - } } private fun processMessagesNotFromTheFuture(chatMessageList: List) { @@ -2754,7 +2751,8 @@ class ChatActivity : return false } - if (!message1IsSystem && ( + if (!message1IsSystem && + ( (message1.actorType != message2.actorType) || (message2.actorId != message1.actorId) ) @@ -2863,9 +2861,8 @@ class ChatActivity : TextUtils.isEmpty(messageRight.systemMessage) && DateFormatter.isSameDay(messageLeft.createdAt, messageRight.createdAt) - private fun isSameDayMessages(message1: ChatMessage, message2: ChatMessage): Boolean { - return DateFormatter.isSameDay(message1.createdAt, message2.createdAt) - } + private fun isSameDayMessages(message1: ChatMessage, message2: ChatMessage): Boolean = + DateFormatter.isSameDay(message1.createdAt, message2.createdAt) override fun onLoadMore(page: Int, totalItemsCount: Int) { val id = ( diff --git a/app/src/main/java/com/nextcloud/talk/chat/data/ChatMessageRepository.kt b/app/src/main/java/com/nextcloud/talk/chat/data/ChatMessageRepository.kt index a55a458031..860da6c21f 100644 --- a/app/src/main/java/com/nextcloud/talk/chat/data/ChatMessageRepository.kt +++ b/app/src/main/java/com/nextcloud/talk/chat/data/ChatMessageRepository.kt @@ -22,7 +22,8 @@ interface ChatMessageRepository : LifecycleAwareManager { */ val messageFlow: Flow< - Pair< + Triple< + Boolean, Boolean, List > diff --git a/app/src/main/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepository.kt b/app/src/main/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepository.kt index 4c52f7f315..b0fc8d0812 100644 --- a/app/src/main/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepository.kt +++ b/app/src/main/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepository.kt @@ -53,7 +53,8 @@ class OfflineFirstChatRepository @Inject constructor( override val messageFlow: Flow< - Pair< + Triple< + Boolean, Boolean, List > @@ -62,7 +63,8 @@ class OfflineFirstChatRepository @Inject constructor( private val _messageFlow: MutableSharedFlow< - Pair< + Triple< + Boolean, Boolean, List > @@ -142,6 +144,7 @@ class OfflineFirstChatRepository @Inject constructor( // set up field map to load the newest messages val fieldMap = getFieldMap( lookIntoFuture = false, + timeout = 0, includeLastKnown = true, setReadMarker = true, lastKnown = null @@ -229,6 +232,7 @@ class OfflineFirstChatRepository @Inject constructor( val fieldMap = getFieldMap( lookIntoFuture = false, + timeout = 0, includeLastKnown = false, setReadMarker = true, lastKnown = beforeMessageId.toInt() @@ -254,6 +258,9 @@ class OfflineFirstChatRepository @Inject constructor( var fieldMap = getFieldMap( lookIntoFuture = true, + // timeout for first longpoll is 0, so "unread message" info is not shown if there were + // initially no messages but someone writes us in the first 30 seconds. + timeout = 0, includeLastKnown = false, setReadMarker = true, lastKnown = initialMessageId.toInt() @@ -261,6 +268,8 @@ class OfflineFirstChatRepository @Inject constructor( val networkParams = Bundle() + var showUnreadMessagesMarker = true + while (true) { if (!monitor.isOnline.first() || itIsPaused) { Thread.sleep(HALF_SECOND) @@ -272,8 +281,14 @@ class OfflineFirstChatRepository @Inject constructor( val resultsFromSync = sync(networkParams) if (!resultsFromSync.isNullOrEmpty()) { val chatMessages = resultsFromSync.map(ChatMessageEntity::asModel) - val pair = Pair(true, chatMessages) + + val weHaveMessagesFromOurself = chatMessages.any { it.actorId == currentUser.userId } + showUnreadMessagesMarker = showUnreadMessagesMarker && !weHaveMessagesFromOurself + + val pair = Triple(true, showUnreadMessagesMarker, chatMessages) _messageFlow.emit(pair) + } else { + Log.d(TAG, "resultsFromSync are null or empty") } updateUiForLastCommonRead() @@ -283,10 +298,13 @@ class OfflineFirstChatRepository @Inject constructor( // update field map vars for next cycle fieldMap = getFieldMap( lookIntoFuture = true, + timeout = 30, includeLastKnown = false, setReadMarker = true, lastKnown = newestMessage ) + + showUnreadMessagesMarker = false } } } @@ -325,6 +343,7 @@ class OfflineFirstChatRepository @Inject constructor( private fun getFieldMap( lookIntoFuture: Boolean, + timeout: Int, includeLastKnown: Boolean, setReadMarker: Boolean, lastKnown: Int?, @@ -342,7 +361,7 @@ class OfflineFirstChatRepository @Inject constructor( fieldMap["lastCommonReadId"] = it } - fieldMap["timeout"] = if (lookIntoFuture) 30 else 0 + fieldMap["timeout"] = timeout fieldMap["limit"] = limit fieldMap["lookIntoFuture"] = if (lookIntoFuture) 1 else 0 fieldMap["setReadMarker"] = if (setReadMarker) 1 else 0 @@ -357,6 +376,7 @@ class OfflineFirstChatRepository @Inject constructor( if (loadFromServer) { val fieldMap = getFieldMap( lookIntoFuture = false, + timeout = 0, includeLastKnown = true, setReadMarker = false, lastKnown = messageId.toInt(), @@ -664,7 +684,7 @@ class OfflineFirstChatRepository @Inject constructor( ) if (list.isNotEmpty()) { - val pair = Pair(false, list) + val pair = Triple(false, false, list) _messageFlow.emit(pair) } } @@ -690,7 +710,7 @@ class OfflineFirstChatRepository @Inject constructor( ) if (list.isNotEmpty()) { - val pair = Pair(false, list) + val pair = Triple(false, false, list) _messageFlow.emit(pair) } }