From 89f4fa74409edc2e4d88353d325671ab5a6e5947 Mon Sep 17 00:00:00 2001 From: Sale Djenic Date: Thu, 25 Aug 2022 15:50:56 +0200 Subject: [PATCH] fix(@desktop/chat): chat - messages in 1-1 chat arrive out of order An explanation why we keep track of `timestamp` and `localTimestamp` here, introduction of those two actually fixes the following issues: - https://github.com/status-im/status-desktop/issues/6004 - https://github.com/status-im/status-desktop/issues/7058 We should always refer to `whisperTimestamp` as it is set for a message by the network in order they are sent (that solves the issue #6004), but another issue #7058 is happening cause `whisperTimestamp` has one second accuracy (which is a very big timeframe for messages). That further means that all messsages sent by user A within 1000ms will be received with the same `whisperTimestamp` value on the side of user B, in that case to differ the order of those message we're using localy set `timestamp` on the sender side which is received unchanged on the receiver side. Now a question why don't we use only locally set `timestamp` may araise... the answer is... because of issue #6004, cause it can be that users A and B send a message in almost the same time in that case message sent by user A will be immediatelly added to the message list, while message sent by user B will arrive like a less then a second later and in that case user A may see user B message before or after his message and the same for user B, depends on local time of those 2 users which is set for `timestamp` time in the moment they sent a message. If we anyhow find a way to have here accutacy higher than 1 second, then we can go on only with `whisperTimestamp` https://github.com/status-im/status-go/blob/3f987cc565091327f017bfe674c08ed01e301d00/protocol/messenger.go#L3726 Fixes: #7058 --- .../modules/main/activity_center/module.nim | 3 +- .../chat_content/messages/module.nim | 8 +++-- .../main/chat_section/chat_content/module.nim | 3 +- .../modules/shared_models/message_item.nim | 35 ++++++++++++++++++- .../modules/shared_models/message_model.nim | 9 +++-- 5 files changed, 50 insertions(+), 8 deletions(-) diff --git a/src/app/modules/main/activity_center/module.nim b/src/app/modules/main/activity_center/module.nim index e1f45caa80..d567b33a59 100644 --- a/src/app/modules/main/activity_center/module.nim +++ b/src/app/modules/main/activity_center/module.nim @@ -85,7 +85,8 @@ proc createMessageItemFromDto(self: Module, message: MessageDto, chatDetails: Ch message.image, message.containsContactMentions(), message.seen, - message.timestamp, + timestamp = message.whisperTimestamp, + localTimestamp = message.timestamp, ContentType(message.contentType), message.messageType, message.sticker.url, diff --git a/src/app/modules/main/chat_section/chat_content/messages/module.nim b/src/app/modules/main/chat_section/chat_content/messages/module.nim index b985a28c29..8cd4080d5d 100644 --- a/src/app/modules/main/chat_section/chat_content/messages/module.nim +++ b/src/app/modules/main/chat_section/chat_content/messages/module.nim @@ -87,6 +87,7 @@ proc createFetchMoreMessagesItem(self: Module): Item = messageContainsMentions = false, seen = true, timestamp = 0, + localTimestamp = 0, ContentType.FetchMoreMessagesButton, messageType = -1, sticker = "", @@ -125,6 +126,7 @@ proc createChatIdentifierItem(self: Module): Item = messageContainsMentions = false, seen = true, timestamp = 0, + localTimestamp = 0, ContentType.ChatIdentifier, messageType = -1, sticker = "", @@ -189,7 +191,8 @@ method newMessagesLoaded*(self: Module, messages: seq[MessageDto], reactions: se m.image, m.containsContactMentions(), m.seen, - m.whisperTimestamp, + timestamp = m.whisperTimestamp, + localTimestamp = m.timestamp, m.contentType.ContentType, m.messageType, sticker = m.sticker.url, @@ -278,7 +281,8 @@ method messageAdded*(self: Module, message: MessageDto) = message.image, message.containsContactMentions(), message.seen, - message.whisperTimestamp, + timestamp = message.whisperTimestamp, + localTimestamp = message.timestamp, message.contentType.ContentType, message.messageType, sticker = message.sticker.url, diff --git a/src/app/modules/main/chat_section/chat_content/module.nim b/src/app/modules/main/chat_section/chat_content/module.nim index 248f25c1d0..65b0d7fcc9 100644 --- a/src/app/modules/main/chat_section/chat_content/module.nim +++ b/src/app/modules/main/chat_section/chat_content/module.nim @@ -177,7 +177,8 @@ proc buildPinnedMessageItem(self: Module, messageId: string, actionInitiatedBy: m.image, m.containsContactMentions(), m.seen, - m.timestamp, + timestamp = m.whisperTimestamp, + localTimestamp = m.timestamp, m.contentType.ContentType, m.messageType, m.sticker.url, diff --git a/src/app/modules/shared_models/message_item.nim b/src/app/modules/shared_models/message_item.nim index fcbef8d0c9..384aae9d88 100644 --- a/src/app/modules/shared_models/message_item.nim +++ b/src/app/modules/shared_models/message_item.nim @@ -5,6 +5,31 @@ import ../../../app_service/service/contacts/dto/contacts export types.ContentType import message_reaction_model, message_reaction_item, message_transaction_parameters_item +## An explanation why we keep track of `timestamp` and `localTimestamp` here +## +## introduction of those two actually fixes the following issues: +## https://github.com/status-im/status-desktop/issues/6004 +## https://github.com/status-im/status-desktop/issues/7058 +## +## We should always refer to `whisperTimestamp` as it is set for a message by the network +## in order they are sent (that solves the issue #6004), but another issue #7058 is happening +## cause `whisperTimestamp` has one second accuracy (which is a very big timeframe for messages). +## That further means that all messsages sent by user A within 1000ms will be received with the +## same `whisperTimestamp` value on the side of user B, in that case to differ the order of +## those message we're using localy set `timestamp` on the sender side which is received unchanged +## on the receiver side. +## Now a question why don't we use only locally set `timestamp` may araise... the answer is... +## because of issue #6004, cause it can be that users A and B send a message in almost the same +## time in that case message sent by user A will be immediatelly added to the message list, while +## message sent by user B will arrive like a less then a second later and in that case user A may +## see user B message before or after his message and the same for user B, depends on local time +## of those 2 users which is set for `timestamp` time in the moment they sent a message. +## +## If we anyhow find a way to have here accutacy higher than 1 second, then we can go on only +## with `whisperTimestamp` +## https://github.com/status-im/status-go/blob/3f987cc565091327f017bfe674c08ed01e301d00/protocol/messenger.go#L3726 + + type Item* = ref object id: string @@ -26,6 +51,7 @@ type gapFrom: int64 gapTo: int64 timestamp: int64 + localTimestamp: int64 contentType: ContentType messageType: int reactionsModel: MessageReactionModel @@ -54,7 +80,8 @@ proc initItem*( image: string, messageContainsMentions, seen: bool, - timestamp: int64, + timestamp: int64, # whisper timestamp, with 1s accuracy (even accuracy looks like 1ms, last 3 digits are always 0) + localTimestamp: int64, # local timestamp obtained when a message is being sent, with 1ms accuracy contentType: ContentType, messageType: int, sticker: string, @@ -81,6 +108,7 @@ proc initItem*( result.messageImage = image result.messageContainsMentions = messageContainsMentions result.timestamp = timestamp + result.localTimestamp = localTimestamp result.contentType = contentType result.messageType = messageType result.pinned = false @@ -112,6 +140,7 @@ proc `$`*(self: Item): string = messageText:{self.messageText}, messageContainsMentions:{self.messageContainsMentions}, timestamp:{$self.timestamp}, + localTimestamp:{$self.localTimestamp} contentType:{$self.contentType.int}, messageType:{$self.messageType}, pinned:{$self.pinned}, @@ -207,6 +236,9 @@ proc seen*(self: Item): bool {.inline.} = proc timestamp*(self: Item): int64 {.inline.} = self.timestamp +proc localTimestamp*(self: Item): int64 {.inline.} = + self.localTimestamp + proc contentType*(self: Item): ContentType {.inline.} = self.contentType @@ -277,6 +309,7 @@ proc toJsonNode*(self: Item): JsonNode = "gapFrom": self.gapFrom, "gapTo": self.gapTo, "timestamp": self.timestamp, + "localTimestamp": self.localTimestamp, "contentType": self.contentType.int, "messageType": self.messageType, "pinned": self.pinned, diff --git a/src/app/modules/shared_models/message_model.nim b/src/app/modules/shared_models/message_model.nim index d6219854d4..a6a8701e6a 100644 --- a/src/app/modules/shared_models/message_model.nim +++ b/src/app/modules/shared_models/message_model.nim @@ -228,10 +228,13 @@ QtObject: if(self.items[i].responseToMessageWithId == messageId): result.add(self.items[i].id) - proc findIndexBasedOnTimestampToInsertTo(self: Model, timestamp: int64): int = + proc findIndexBasedOnTimestampToInsertTo(self: Model, timestamp: int64, localTimestamp: int64): int = for i in 0 ..< self.items.len: - if(timestamp > self.items[i].timestamp): + if timestamp > self.items[i].timestamp: return i + elif timestamp == self.items[i].timestamp: + if localTimestamp > self.items[i].localTimestamp: + return i return 0 proc filterExistingItems(self: Model, items: seq[Item]): seq[Item] = @@ -311,7 +314,7 @@ QtObject: let parentModelIndex = newQModelIndex() defer: parentModelIndex.delete - let position = self.findIndexBasedOnTimestampToInsertTo(item.timestamp) + let position = self.findIndexBasedOnTimestampToInsertTo(item.timestamp, item.localTimestamp) self.beginInsertRows(parentModelIndex, position, position) self.items.insert(item, position)