Skip to content

Commit

Permalink
fix messages not shown + improve unread marker behavior
Browse files Browse the repository at this point in the history 
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 <[email protected]>
  • Loading branch information
@mahibi
mahibi committed Oct 29, 2024
1 parent b290cd2 commit 180dd03
Show file tree
Hide file tree
Showing 4 changed files with 66 additions and 48 deletions.
2 changes: 1 addition & 1 deletion app/src/main/java/com/nextcloud/talk/adapters/messages/UnreadNoticeMessageViewHolder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ public UnreadNoticeMessageViewHolder(View itemView, Object payload) {

@Override
public void viewDetached() {
messagesListAdapter.deleteById("-1");
// messagesListAdapter.deleteById("-1");
}

@Override
Expand Down
77 changes: 37 additions & 40 deletions app/src/main/java/com/nextcloud/talk/chat/ChatActivity.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -756,6 +756,8 @@ class ChatActivity :
is MessageInputViewModel.SendChatMessageSuccessState -> {
myFirstMessage = state.message

removeUnreadMessagesMarker()

if (binding.unreadMessagesPopup.isShown == true) {
binding.unreadMessagesPopup.hide()
}
Expand Down Expand Up @@ -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)

Expand All @@ -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()
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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) {
Expand All @@ -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>) {
Expand Down Expand Up @@ -2754,7 +2751,8 @@ class ChatActivity :
return false
}

if (!message1IsSystem && (
if (!message1IsSystem &&
(
(message1.actorType != message2.actorType) ||
(message2.actorId != message1.actorId)
)
Expand Down Expand Up @@ -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 = (
Expand Down
3 changes: 2 additions & 1 deletion app/src/main/java/com/nextcloud/talk/chat/data/ChatMessageRepository.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ interface ChatMessageRepository : LifecycleAwareManager {
*/
val messageFlow:
Flow<
Pair<
Triple<
Boolean,
Boolean,
List<ChatMessage>
>
Expand Down
32 changes: 26 additions & 6 deletions app/src/main/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepository.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,8 @@ class OfflineFirstChatRepository @Inject constructor(

override val messageFlow:
Flow<
Pair<
Triple<
Boolean,
Boolean,
List<ChatMessage>
>
Expand All @@ -62,7 +63,8 @@ class OfflineFirstChatRepository @Inject constructor(

private val _messageFlow:
MutableSharedFlow<
Pair<
Triple<
Boolean,
Boolean,
List<ChatMessage>
>
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -229,6 +232,7 @@ class OfflineFirstChatRepository @Inject constructor(

val fieldMap = getFieldMap(
lookIntoFuture = false,
timeout = 0,
includeLastKnown = false,
setReadMarker = true,
lastKnown = beforeMessageId.toInt()
Expand All @@ -254,13 +258,18 @@ 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()
)

val networkParams = Bundle()

var showUnreadMessagesMarker = true

while (true) {
if (!monitor.isOnline.first() || itIsPaused) {
Thread.sleep(HALF_SECOND)
Expand All @@ -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()
Expand All @@ -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
}
}
}
Expand Down Expand Up @@ -325,6 +343,7 @@ class OfflineFirstChatRepository @Inject constructor(

private fun getFieldMap(
lookIntoFuture: Boolean,
timeout: Int,
includeLastKnown: Boolean,
setReadMarker: Boolean,
lastKnown: Int?,
Expand All @@ -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
Expand All @@ -357,6 +376,7 @@ class OfflineFirstChatRepository @Inject constructor(
if (loadFromServer) {
val fieldMap = getFieldMap(
lookIntoFuture = false,
timeout = 0,
includeLastKnown = true,
setReadMarker = false,
lastKnown = messageId.toInt(),
Expand Down Expand Up @@ -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)
}
}
Expand All @@ -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)
}
}
Expand Down

0 comments on commit 180dd03

Please sign in to comment.