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 <dev@mhibbe.de>

app/src/main/java/com/nextcloud/talk/adapters/messages/UnreadNoticeMessageViewHolder.java

@@ -24,7 +24,7 @@ public class UnreadNoticeMessageViewHolder extends MessageHolders.SystemMessageV
 
     @Override
     public void viewDetached() {
-        messagesListAdapter.deleteById("-1");
+//        messagesListAdapter.deleteById("-1");
     }
 
     @Override

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<ChatMessage>) {
-        val newMessagesAvailable = (adapter?.itemCount ?: 0) > 0 && chatMessageList.isNotEmpty()
-        val insertNewMessagesNotice = shouldInsertNewMessagesNotice(newMessagesAvailable, chatMessageList)
-        val scrollToEndOnUpdate = isScrolledToBottom()
+    private fun processMessagesFromTheFuture(chatMessageList: List<ChatMessage>, 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<ChatMessage>) =
-        if (newMessagesAvailable) {
-            chatMessageList.any { it.actorId != conversationUser!!.userId }
-        } else {
-            false
-        }
-
-    private fun updateUnreadMessageInfos(chatMessageList: List<ChatMessage>, scrollToEndOnUpdate: Boolean) {
+    private fun setUnreadMessageMarker(chatMessageList: List<ChatMessage>) {
         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<ChatMessage>) {
@@ -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 = (

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<ChatMessage>
                 >

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<ChatMessage>
                 >
@@ -62,7 +63,8 @@ class OfflineFirstChatRepository @Inject constructor(
 
     private val _messageFlow:
         MutableSharedFlow<
-            Pair<
+            Triple<
+                Boolean,
                 Boolean,
                 List<ChatMessage>
                 >
@@ -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)
         }
     }