From 7ede3389ff247b81a1d08a97f69127e4be65b466 Mon Sep 17 00:00:00 2001 From: Patryk Osmaczko Date: Mon, 19 Dec 2022 19:23:46 +0100 Subject: [PATCH] fix(chat): ensure messages ordering on model append/prepend fixes: #8466 --- .../modules/shared_models/message_model.nim | 63 +++----- test/nim/message_model_test.nim | 148 ++++++++++++++++++ 2 files changed, 171 insertions(+), 40 deletions(-) create mode 100644 test/nim/message_model_test.nim diff --git a/src/app/modules/shared_models/message_model.nim b/src/app/modules/shared_models/message_model.nim index 4b18bb32c6..73e8622d2d 100644 --- a/src/app/modules/shared_models/message_model.nim +++ b/src/app/modules/shared_models/message_model.nim @@ -84,7 +84,7 @@ QtObject: read = getCount notify = countChanged - method rowCount(self: Model, index: QModelIndex = nil): int = + method rowCount*(self: Model, index: QModelIndex = nil): int = return self.items.len method roleNames(self: Model): Table[int, string] = @@ -252,45 +252,47 @@ QtObject: elif clock == self.items[i].clock: # break ties by message id if id > self.items[i].id: return i - return 0 + return self.items.len proc filterExistingItems(self: Model, items: seq[Item]): seq[Item] = for item in items: if(self.findIndexForMessageId(item.id) < 0): result &= item - proc prependItems*(self: Model, items: seq[Item]) = - let itemsToAppend = self.filterExistingItems(items) - if(itemsToAppend.len == 0): + proc insertItemBasedOnClock*(self: Model, item: Item) = + if(self.findIndexForMessageId(item.id) != -1): return let parentModelIndex = newQModelIndex() defer: parentModelIndex.delete - let first = 0 - let last = itemsToAppend.len - 1 - self.beginInsertRows(parentModelIndex, first, last) - self.items = itemsToAppend & self.items + let position = self.findIndexBasedOnClockToInsertTo(item.clock, item.id) + + self.beginInsertRows(parentModelIndex, position, position) + self.items.insert(item, position) self.endInsertRows() + + if position > 0: + self.updateItemAtIndex(position - 1) + if position + 1 < self.items.len: + self.updateItemAtIndex(position + 1) self.countChanged() + proc prependItems*(self: Model, items: seq[Item]) = + let itemsToAppend = self.filterExistingItems(items) + if(itemsToAppend.len == 0): + return + + for item in items: + self.insertItemBasedOnClock(item) + proc appendItems*(self: Model, items: seq[Item]) = let itemsToAppend = self.filterExistingItems(items) if(itemsToAppend.len == 0): return - let parentModelIndex = newQModelIndex() - defer: parentModelIndex.delete - - let first = self.items.len - let last = first + itemsToAppend.len - 1 - self.beginInsertRows(parentModelIndex, first, last) - self.items.add(itemsToAppend) - self.endInsertRows() - - if first > 0: - self.updateItemAtIndex(first - 1) - self.countChanged() + for item in items: + self.insertItemBasedOnClock(item) proc appendItem*(self: Model, item: Item) = if(self.findIndexForMessageId(item.id) != -1): @@ -324,25 +326,6 @@ QtObject: self.updateItemAtIndex(1) self.countChanged() - proc insertItemBasedOnClock*(self: Model, item: Item) = - if(self.findIndexForMessageId(item.id) != -1): - return - - let parentModelIndex = newQModelIndex() - defer: parentModelIndex.delete - - let position = self.findIndexBasedOnClockToInsertTo(item.clock, item.id) - - self.beginInsertRows(parentModelIndex, position, position) - self.items.insert(item, position) - self.endInsertRows() - - if position > 0: - self.updateItemAtIndex(position - 1) - if position + 1 < self.items.len: - self.updateItemAtIndex(position + 1) - self.countChanged() - proc replyDeleted*(self: Model, messageIndex: int) {.signal.} proc updateMessagesWithResponseTo(self: Model, messageId: string) = diff --git a/test/nim/message_model_test.nim b/test/nim/message_model_test.nim new file mode 100644 index 0000000000..058bc3b87c --- /dev/null +++ b/test/nim/message_model_test.nim @@ -0,0 +1,148 @@ +import unittest + +import ../../../src/app_service/common/types +import ../../../src/app_service/service/contacts/dto/contacts +import ../../../src/app_service/service/message/dto/message + +import ../../../src/app/modules/shared_models/message_model +import ../../../src/app/modules/shared_models/message_item +import ../../../src/app/modules/shared_models/message_transaction_parameters_item + +proc createTestMessageItem(id: string, clock: int64): Item = + return initItem( + id = id, + communityId = "", + responseToMessageWithId = "", + senderId = "", + senderDisplayName = "", + senderOptionalName = "", + senderIcon = "", + amISender = false, + senderIsAdded = false, + outgoingStatus = "", + text = "", + image = "", + messageContainsMentions = false, + seen = true, + timestamp = 0, + clock = clock, + ContentType.NewMessagesMarker, + messageType = -1, + contactRequestState = 0, + sticker = "", + stickerPack = -1, + links = @[], + transactionParameters = newTransactionParametersItem("","","","","","",-1,""), + mentionedUsersPks = @[], + senderTrustStatus = TrustStatus.Unknown, + senderEnsVerified = false, + discordMessage = DiscordMessage(), + resendError = "" + ) + +let message1 = createTestMessageItem("0xa", 1) +let message2 = createTestMessageItem("0xb", 2) +let message3 = createTestMessageItem("0xc", 3) +let message4 = createTestMessageItem("0xd", 3) +let message5 = createTestMessageItem("0xe", 4) + +template checkOrder(model: Model) = + require(model.items.len == 5) + check(model.items[0].id == message5.id) + check(model.items[1].id == message4.id) + check(model.items[2].id == message3.id) + check(model.items[3].id == message2.id) + check(model.items[4].id == message1.id) + +suite "empty model": + let model = newModel() + + test "initial size": + require(model.rowCount() == 0) + +# newest messages should be first, break ties by message id +suite "inserting new messages": + setup: + let model = newModel() + + test "insert same message twice": + model.insertItemBasedOnClock(message1) + check(model.rowCount() == 1) + model.insertItemBasedOnClock(message1) + check(model.rowCount() == 1) + + test "insert in order": + model.insertItemBasedOnClock(message1) + check(model.rowCount() == 1) + model.insertItemBasedOnClock(message2) + check(model.rowCount() == 2) + model.insertItemBasedOnClock(message3) + check(model.rowCount() == 3) + model.insertItemBasedOnClock(message4) + check(model.rowCount() == 4) + model.insertItemBasedOnClock(message5) + check(model.rowCount() == 5) + checkOrder(model) + + test "insert out of order": + model.insertItemBasedOnClock(message5) + check(model.rowCount() == 1) + model.insertItemBasedOnClock(message4) + check(model.rowCount() == 2) + model.insertItemBasedOnClock(message3) + check(model.rowCount() == 3) + model.insertItemBasedOnClock(message2) + check(model.rowCount() == 4) + model.insertItemBasedOnClock(message1) + check(model.rowCount() == 5) + checkOrder(model) + + test "insert out of order (randomly)": + model.insertItemBasedOnClock(message3) + check(model.rowCount() == 1) + model.insertItemBasedOnClock(message1) + check(model.rowCount() == 2) + model.insertItemBasedOnClock(message4) + check(model.rowCount() == 3) + model.insertItemBasedOnClock(message2) + check(model.rowCount() == 4) + model.insertItemBasedOnClock(message5) + check(model.rowCount() == 5) + checkOrder(model) + +# assumption: each append sequence is already sorted +suite "appending new messages": + setup: + let model = newModel() + + test "append empty model": + model.appendItems(@[message5, + message4, + message3, + message2, + message1]) + checkOrder(model) + + test "append to model with only newer messages": + model.insertItemBasedOnClock(message5) + model.insertItemBasedOnClock(message4) + model.appendItems(@[message3, + message2, + message1]) + checkOrder(model) + + test "append to model with newer and older messages": + model.insertItemBasedOnClock(message5) + model.insertItemBasedOnClock(message1) + model.appendItems(@[message4, + message3, + message2]) + checkOrder(model) + + test "append to model with newer and older messages and some in between": + model.insertItemBasedOnClock(message5) + model.insertItemBasedOnClock(message1) + model.insertItemBasedOnClock(message3) # in between + model.appendItems(@[message4, + message2]) + checkOrder(model)