From e4d23fae6c46ce435a0344a67fefcfd99aa6deaf Mon Sep 17 00:00:00 2001 From: Nerixyz Date: Mon, 28 Oct 2024 22:08:59 +0100 Subject: [PATCH 1/3] fix: take indices to messages as a hint --- src/common/Channel.cpp | 22 ++++- src/common/Channel.hpp | 11 ++- src/messages/LimitedQueue.hpp | 68 +++++++++++++- src/widgets/helper/ChannelView.cpp | 25 ++--- src/widgets/helper/ChannelView.hpp | 3 +- tests/src/LimitedQueue.cpp | 142 ++++++++++++++++++++++++++++- 6 files changed, 244 insertions(+), 27 deletions(-) diff --git a/src/common/Channel.cpp b/src/common/Channel.cpp index ef778bad1..ecedf22a1 100644 --- a/src/common/Channel.cpp +++ b/src/common/Channel.cpp @@ -253,21 +253,33 @@ void Channel::fillInMissingMessages(const std::vector &messages) } } -void Channel::replaceMessage(MessagePtr message, MessagePtr replacement) +void Channel::replaceMessage(const MessagePtr &message, + const MessagePtr &replacement) { int index = this->messages_.replaceItem(message, replacement); if (index >= 0) { - this->messageReplaced.invoke((size_t)index, replacement); + this->messageReplaced.invoke((size_t)index, message, replacement); } } -void Channel::replaceMessage(size_t index, MessagePtr replacement) +void Channel::replaceMessage(size_t index, const MessagePtr &replacement) { - if (this->messages_.replaceItem(index, replacement)) + MessagePtr prev; + if (this->messages_.replaceItem(index, replacement, &prev)) { - this->messageReplaced.invoke(index, replacement); + this->messageReplaced.invoke(index, prev, replacement); + } +} + +void Channel::replaceMessage(size_t hint, const MessagePtr &message, + const MessagePtr &replacement) +{ + auto index = this->messages_.replaceItem(hint, message, replacement); + if (index >= 0) + { + this->messageReplaced.invoke(hint, message, replacement); } } diff --git a/src/common/Channel.hpp b/src/common/Channel.hpp index ac90573ff..0b71bf0ca 100644 --- a/src/common/Channel.hpp +++ b/src/common/Channel.hpp @@ -66,7 +66,9 @@ public: pajlada::Signals::Signal> messageAppended; pajlada::Signals::Signal &> messagesAddedAtStart; - pajlada::Signals::Signal messageReplaced; + /// (index, prev-message, replacement) + pajlada::Signals::Signal + messageReplaced; /// Invoked when some number of messages were filled in using time received pajlada::Signals::Signal &> filledInMessages; pajlada::Signals::NoArgSignal destroyed; @@ -96,8 +98,11 @@ public: void addOrReplaceTimeout(MessagePtr message); void disableAllMessages(); - void replaceMessage(MessagePtr message, MessagePtr replacement); - void replaceMessage(size_t index, MessagePtr replacement); + void replaceMessage(const MessagePtr &message, + const MessagePtr &replacement); + void replaceMessage(size_t index, const MessagePtr &replacement); + void replaceMessage(size_t hint, const MessagePtr &message, + const MessagePtr &replacement); void deleteMessage(QString messageID); /// Removes all messages from this channel and invokes #messagesCleared diff --git a/src/messages/LimitedQueue.hpp b/src/messages/LimitedQueue.hpp index a204c84a9..045ec5826 100644 --- a/src/messages/LimitedQueue.hpp +++ b/src/messages/LimitedQueue.hpp @@ -8,6 +8,7 @@ #include #include #include +#include #include namespace chatterino { @@ -212,9 +213,10 @@ public: * * @param[in] index the index of the item to replace * @param[in] replacement the item to put in place of the item at index + * @param[out] prev (optional) the item located at @a index before replacing * @return true if a replacement took place */ - bool replaceItem(size_t index, const T &replacement) + bool replaceItem(size_t index, const T &replacement, T *prev = nullptr) { std::unique_lock lock(this->mutex_); @@ -223,10 +225,46 @@ public: return false; } - this->buffer_[index] = replacement; + if (prev) + { + *prev = std::exchange(this->buffer_[index], replacement); + } + else + { + this->buffer_[index] = replacement; + } return true; } + /** + * @brief Replace the needle with the given item + * + * @param hint A hint on where the needle _might_ be + * @param[in] needle the item to search for + * @param[in] replacement the item to replace needle with + * @return the index of the replaced item, or -1 if no replacement took place + */ + int replaceItem(size_t hint, const T &needle, const T &replacement) + { + std::unique_lock lock(this->mutex_); + + if (hint < this->buffer_.size() && this->buffer_[hint] == needle) + { + this->buffer_[hint] = replacement; + return static_cast(hint); + } + + for (size_t i = 0; i < this->buffer_.size(); ++i) + { + if (this->buffer_[i] == needle) + { + this->buffer_[i] = replacement; + return static_cast(i); + } + } + return -1; + } + /** * @brief Inserts the given item before another item * @@ -315,6 +353,32 @@ public: return std::nullopt; } + /** + * @brief Find an item with a hint + * + * @param hint A hint on where the needle _might_ be + * @param needle The item to search for + * @return the item and its index or none if it's not found + */ + std::optional> find(size_t hint, auto &&predicate) + { + std::unique_lock lock(this->mutex_); + + if (hint < this->buffer_.size() && predicate(this->buffer_[hint])) + { + return std::pair{hint, this->buffer_[hint]}; + }; + + for (size_t i = 0; i < this->buffer_.size(); i++) + { + if (predicate(this->buffer_[i])) + { + return std::pair{i, this->buffer_[i]}; + } + } + return std::nullopt; + } + /** * @brief Returns the first item matching a predicate, checking in reverse * diff --git a/src/widgets/helper/ChannelView.cpp b/src/widgets/helper/ChannelView.cpp index dc0a98cbd..a5fa62582 100644 --- a/src/widgets/helper/ChannelView.cpp +++ b/src/widgets/helper/ChannelView.cpp @@ -967,10 +967,10 @@ void ChannelView::setChannel(const ChannelPtr &underlyingChannel) this->channelConnections_.managedConnect( underlyingChannel->messageReplaced, - [this](auto index, const auto &replacement) { + [this](auto index, const auto &prev, const auto &replacement) { if (this->shouldIncludeMessage(replacement)) { - this->channel_->replaceMessage(index, replacement); + this->channel_->replaceMessage(index, prev, replacement); } }); @@ -1051,8 +1051,9 @@ void ChannelView::setChannel(const ChannelPtr &underlyingChannel) // on message replaced this->channelConnections_.managedConnect( this->channel_->messageReplaced, - [this](size_t index, MessagePtr replacement) { - this->messageReplaced(index, replacement); + [this](size_t index, const MessagePtr &prev, + const MessagePtr &replacement) { + this->messageReplaced(index, prev, replacement); }); // on messages filled in @@ -1258,19 +1259,21 @@ void ChannelView::messageAddedAtStart(std::vector &messages) this->queueLayout(); } -void ChannelView::messageReplaced(size_t index, MessagePtr &replacement) +void ChannelView::messageReplaced(size_t hint, const MessagePtr &prev, + const MessagePtr &replacement) { - auto oMessage = this->messages_.get(index); - if (!oMessage) + auto optItem = this->messages_.find(hint, [&](const auto &it) { + return it->getMessagePtr() == prev; + }); + if (!optItem) { return; } - - auto message = *oMessage; + const auto &[index, oldItem] = *optItem; auto newItem = std::make_shared(replacement); - if (message->flags.has(MessageLayoutFlag::AlternateBackground)) + if (oldItem->flags.has(MessageLayoutFlag::AlternateBackground)) { newItem->flags.set(MessageLayoutFlag::AlternateBackground); } @@ -1278,7 +1281,7 @@ void ChannelView::messageReplaced(size_t index, MessagePtr &replacement) this->scrollBar_->replaceHighlight(index, replacement->getScrollBarHighlight()); - this->messages_.replaceItem(message, newItem); + this->messages_.replaceItem(index, newItem); this->queueLayout(); } diff --git a/src/widgets/helper/ChannelView.hpp b/src/widgets/helper/ChannelView.hpp index 704210712..40b40444e 100644 --- a/src/widgets/helper/ChannelView.hpp +++ b/src/widgets/helper/ChannelView.hpp @@ -268,7 +268,8 @@ private: std::optional overridingFlags); void messageAddedAtStart(std::vector &messages); void messageRemoveFromStart(MessagePtr &message); - void messageReplaced(size_t index, MessagePtr &replacement); + void messageReplaced(size_t hint, const MessagePtr &prev, + const MessagePtr &replacement); void messagesUpdated(); void performLayout(bool causedByScrollbar = false, diff --git a/tests/src/LimitedQueue.cpp b/tests/src/LimitedQueue.cpp index 0a94ea928..83230a510 100644 --- a/tests/src/LimitedQueue.cpp +++ b/tests/src/LimitedQueue.cpp @@ -114,20 +114,152 @@ TEST(LimitedQueue, PushFront) TEST(LimitedQueue, ReplaceItem) { - LimitedQueue queue(5); + LimitedQueue queue(10); queue.pushBack(1); queue.pushBack(2); queue.pushBack(3); + queue.pushBack(4); + queue.pushBack(5); + queue.pushBack(6); int idex = queue.replaceItem(2, 10); EXPECT_EQ(idex, 1); - idex = queue.replaceItem(5, 11); + idex = queue.replaceItem(7, 11); EXPECT_EQ(idex, -1); - bool res = queue.replaceItem(std::size_t(0), 9); + int prev = -1; + bool res = queue.replaceItem(std::size_t(0), 9, &prev); EXPECT_TRUE(res); - res = queue.replaceItem(std::size_t(5), 4); + EXPECT_EQ(prev, 1); + res = queue.replaceItem(std::size_t(6), 4); EXPECT_FALSE(res); - SNAPSHOT_EQUALS(queue.getSnapshot(), {9, 10, 3}, "first snapshot"); + // correct hint + EXPECT_EQ(queue.replaceItem(3ULL, 4, 11), 3); + // incorrect hints + EXPECT_EQ(queue.replaceItem(5ULL, 11, 12), 3); + EXPECT_EQ(queue.replaceItem(0ULL, 12, 13), 3); + // oob hint + EXPECT_EQ(queue.replaceItem(42ULL, 13, 14), 3); + // bad needle + EXPECT_EQ(queue.replaceItem(0ULL, 15, 16), -1); + + SNAPSHOT_EQUALS(queue.getSnapshot(), {9, 10, 3, 14, 5, 6}, + "first snapshot"); +} + +TEST(LimitedQueue, Find) +{ + LimitedQueue queue(10); + queue.pushBack(1); + queue.pushBack(2); + queue.pushBack(3); + queue.pushBack(4); + queue.pushBack(5); + queue.pushBack(6); + + // without hint + EXPECT_FALSE(queue + .find([](int i) { + return i == 0; + }) + .has_value()); + EXPECT_EQ(queue + .find([](int i) { + return i == 1; + }) + .value(), + 1); + EXPECT_EQ(queue + .find([](int i) { + return i == 2; + }) + .value(), + 2); + EXPECT_EQ(queue + .find([](int i) { + return i == 6; + }) + .value(), + 6); + EXPECT_FALSE(queue + .find([](int i) { + return i == 7; + }) + .has_value()); + EXPECT_FALSE(queue + .find([](int i) { + return i > 6; + }) + .has_value()); + EXPECT_FALSE(queue + .find([](int i) { + return i <= 0; + }) + .has_value()); + + // with hint + EXPECT_FALSE(queue + .find(0, + [](int i) { + return i == 0; + }) + .has_value()); + // correct hint + EXPECT_EQ(queue + .find(0, + [](int i) { + return i == 1; + }) + .value(), + (std::pair{0ULL, 1})); + EXPECT_EQ(queue + .find(1, + [](int i) { + return i == 2; + }) + .value(), + (std::pair{1ULL, 2})); + // incorrect hint + EXPECT_EQ(queue + .find(1, + [](int i) { + return i == 1; + }) + .value(), + (std::pair{0ULL, 1})); + EXPECT_EQ(queue + .find(5, + [](int i) { + return i == 6; + }) + .value(), + (std::pair{5ULL, 6})); + // oob hint + EXPECT_EQ(queue + .find(6, + [](int i) { + return i == 3; + }) + .value(), + (std::pair{2ULL, 3})); + // non-existent items + EXPECT_FALSE(queue + .find(42, + [](int i) { + return i == 7; + }) + .has_value()); + EXPECT_FALSE(queue + .find(0, + [](int i) { + return i > 6; + }) + .has_value()); + EXPECT_FALSE(queue + .find(0, + [](int i) { + return i <= 0; + }) + .has_value()); } From 1d5c8725ae4ef8711e74478cfd04c95f836cbe3f Mon Sep 17 00:00:00 2001 From: Nerixyz Date: Mon, 28 Oct 2024 22:20:37 +0100 Subject: [PATCH 2/3] chore: add changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a811097e2..b95cd7ded 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -63,6 +63,7 @@ - Bugfix: Fixed double-click selection not working when clicking outside a message. (#5617) - Bugfix: Fixed emotes starting with ":" not tab-completing. (#5603) - Bugfix: Fixed 7TV emotes messing with Qt's HTML. (#5677) +- Bugfix: Fixed incorrect messages getting replaced visually. (#5683) - Dev: Update Windows build from Qt 6.5.0 to Qt 6.7.1. (#5420) - Dev: Update vcpkg build Qt from 6.5.0 to 6.7.0, boost from 1.83.0 to 1.85.0, openssl from 3.1.3 to 3.3.0. (#5422) - Dev: Unsingletonize `ISoundController`. (#5462) From 6430682902134de4c7d4adb199daff3ae1e95eb5 Mon Sep 17 00:00:00 2001 From: Nerixyz Date: Mon, 28 Oct 2024 22:40:55 +0100 Subject: [PATCH 3/3] fix: we need C++ 23 for a size_t suffix --- tests/src/LimitedQueue.cpp | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/tests/src/LimitedQueue.cpp b/tests/src/LimitedQueue.cpp index 83230a510..803969789 100644 --- a/tests/src/LimitedQueue.cpp +++ b/tests/src/LimitedQueue.cpp @@ -135,14 +135,14 @@ TEST(LimitedQueue, ReplaceItem) EXPECT_FALSE(res); // correct hint - EXPECT_EQ(queue.replaceItem(3ULL, 4, 11), 3); + EXPECT_EQ(queue.replaceItem(3, 4, 11), 3); // incorrect hints - EXPECT_EQ(queue.replaceItem(5ULL, 11, 12), 3); - EXPECT_EQ(queue.replaceItem(0ULL, 12, 13), 3); + EXPECT_EQ(queue.replaceItem(5, 11, 12), 3); + EXPECT_EQ(queue.replaceItem(0, 12, 13), 3); // oob hint - EXPECT_EQ(queue.replaceItem(42ULL, 13, 14), 3); + EXPECT_EQ(queue.replaceItem(42, 13, 14), 3); // bad needle - EXPECT_EQ(queue.replaceItem(0ULL, 15, 16), -1); + EXPECT_EQ(queue.replaceItem(0, 15, 16), -1); SNAPSHOT_EQUALS(queue.getSnapshot(), {9, 10, 3, 14, 5, 6}, "first snapshot"); @@ -198,6 +198,7 @@ TEST(LimitedQueue, Find) }) .has_value()); + using Pair = std::pair; // with hint EXPECT_FALSE(queue .find(0, @@ -212,14 +213,14 @@ TEST(LimitedQueue, Find) return i == 1; }) .value(), - (std::pair{0ULL, 1})); + (Pair{0, 1})); EXPECT_EQ(queue .find(1, [](int i) { return i == 2; }) .value(), - (std::pair{1ULL, 2})); + (Pair{1, 2})); // incorrect hint EXPECT_EQ(queue .find(1, @@ -227,14 +228,14 @@ TEST(LimitedQueue, Find) return i == 1; }) .value(), - (std::pair{0ULL, 1})); + (Pair{0, 1})); EXPECT_EQ(queue .find(5, [](int i) { return i == 6; }) .value(), - (std::pair{5ULL, 6})); + (Pair{5, 6})); // oob hint EXPECT_EQ(queue .find(6, @@ -242,7 +243,7 @@ TEST(LimitedQueue, Find) return i == 3; }) .value(), - (std::pair{2ULL, 3})); + (Pair{2, 3})); // non-existent items EXPECT_FALSE(queue .find(42,